refactor(executorch): use shared caller stream - #4454
Conversation
main pins the ExecuTorch dependency to the release/1.3 branch head (6118688a), which predates the shared extension_cuda caller-stream work. Bump the pin to the release/1.4 branch head (cd380e7aefd18c171271cc228d3a155455095219), the first ExecuTorch release line that contains both pytorch/executorch#20158 (shared extension_cuda caller-stream library) and pytorch/executorch#20498 (caller CUDA stream for H2D/D2H copies). This follows the existing convention of pinning a release-branch head commit; the new_git_repository rule takes a commit, not a branch name. The four Bazel files that pin ExecuTorch are updated identically: MODULE.bazel, docker/MODULE.bazel.docker, docker/MODULE.bazel.ngc, and toolchains/ci_workspaces/MODULE.bazel.tmpl. Swap the branch-head commit for the immutable v1.4.0 tag once ExecuTorch publishes it.
1832276 to
bba42e2
Compare
|
CI caught a real mistake in my previous push, now fixed. I had removed the The underlying reason is that Bazel's So the test step now locates the versioned library Bazel materialized and passes it tensorrt_lib_dir="$(
find -L "$(bazel info output_base)/external" \
-path '*tensorrt*/lib/libnvinfer.so.*' -printf '%h\n' 2>/dev/null |
sort -u | head -n1
)"
bazel test //tests/cpp/executorch:executorch_backend_tests \
--compilation_mode opt --config=linux --test_output=errors \
--test_env=LD_LIBRARY_PATH="${tensorrt_lib_dir}"This is still simpler than before. It keeps the sandbox enabled, since Worth noting from the same run: |
Replace the TensorRT ExecuTorch backend private caller-stream TLS with ExecuTorch CallerStreamGuard/getCallerStream so CUDA-capable delegates share one process-wide selection. Link and package one shared extension_cuda instance, add ordinary caller-stream inference coverage in the reference runner, and verify both CMake-built and packaged runners consume the shared TLS without libtorch. The ExecuTorch release/1.4 pin is intentionally owned by the preceding version-bump commit.
bba42e2 to
42bf7d4
Compare
|
Second attempt at the test-runtime fix. My previous one was wrong in a way the logs I set Both libraries have the same root cause. Bazel's The step now locates the directory holding each versioned soname and appends to bazel_external="$(bazel info output_base)/external"
for _soname in libnvinfer.so libcudart.so; do
_dir="$(
find -L "${bazel_external}" -path "*/lib*/${_soname}.*" -printf '%h\n' 2>/dev/null |
sort -u | head -n1
)"
if [[ -z "${_dir}" ]]; then
echo "Could not locate a versioned ${_soname} under ${bazel_external}" >&2
exit 1
fi
export LD_LIBRARY_PATH="${_dir}${LD_LIBRARY_PATH:+:${LD_LIBRARY_PATH}}"
doneI verified this locally by reproducing both failure modes rather than reasoning about The middle line is the failure this run hit. The guard also names the specific library This is still simpler than the original version of the step: the sandbox stays enabled, |
|
CI is green on the ExecuTorch gate. All three C++ tests pass: The same job also exercised the rest of this change end to end:
Remaining failures on this commit are pre-existing on
The dynamo jobs stop before running any test with Note that a large number of |
| cudaStream_t stream = g_user_stream_set ? g_user_stream : cudaStreamPerThread; | ||
| const auto caller_stream = ::executorch::extension::cuda::getCallerStream(); | ||
| const bool caller_stream_set = caller_stream.has_value(); | ||
| cudaStream_t stream = caller_stream.value_or(cudaStreamPerThread); |
There was a problem hiding this comment.
This means that eventually enqueueV3 is happening on the caller stream, right? Is it going to be a problem @narendasan
There was a problem hiding this comment.
Good question, and the short answer is yes, but that is not new here.
enqueueV3 already ran on the caller-selected stream before this change. On the base
commit the same function had:
cudaStream_t stream = g_user_stream_set ? g_user_stream : cudaStreamPerThread; // line 382
...
if (!ctx->enqueueV3(stream)) { // line 571This change only swaps where that stream value is read from. It was a thread-local
private to this delegate, and it is now ExecuTorch's shared one. The
enqueueV3 call, the staging copies, the synchronize, and the completion event are all
untouched.
What happens with no guard active
Nothing changes. getCallerStream() returns empty, so the stream is
cudaStreamPerThread and execute() still synchronizes before returning, exactly as
before. Existing callers see identical behavior.
When execute() can return with work still in flight
This is the part worth being precise about. The decision is:
const bool must_sync = output_staged_to_host || input_staged_from_host || !caller_stream_set;execute() skips the end-of-call synchronize only when all of these hold:
- the caller scoped a
CallerStreamGuard, and - every input is already GPU-accessible, and
- every output is already GPU-accessible.
If any tensor is host-backed, the backend staged it through a temporary device buffer,
so it synchronizes before returning to keep the "results are ready on return" contract.
And with no guard at all it always synchronizes. So the asynchronous path only happens
when the caller explicitly asked for stream semantics and no host memory is involved.
Ordering and lifetime safety
Within execute(), stream is assigned once and never reassigned, and every CUDA
operation uses that same variable: the host-to-device copy, enqueueV3, the
device-to-host copy, the synchronize, and the event record. There is no mixing of the
caller's stream with the default stream.
For the asynchronous path, the backend records a completion event on that stream and
waits on it in two places before it could disturb TensorRT state:
- at the start of the next
execute(), before anysetInputShapeor
setTensorAddress, since those are host-side calls on the execution context; - in the handle destructor, before freeing staging buffers and releasing the execution
context.
It waits on the event rather than the stream, so teardown stays correct even if the
caller has already destroyed their stream.
This is the same pattern ExecuTorch's CUDA backend uses
The caller-stream primitive exists precisely so several CUDA delegates can share one
caller-provided stream. ExecuTorch's own CUDA/AOTI backend reads the same value and
installs it for its whole execution:
// backends/cuda/runtime/cuda_backend.cpp
const std::optional<cudaStream_t> caller_stream =
executorch::extension::cuda::getCallerStream();
...
setCurrentCUDAStream(caller_stream.value_or(handle->get_cuda_stream()), 0);If the two delegates read different stream values, a program that mixes them cannot
order their work, which is the problem this change fixes.
Known limits, stated plainly
- The selected stream must be on the TensorRT engine's device. That is not validated in
code today; a mismatch surfaces as anenqueueV3failure. Pre-existing, and this
change makes the error message name the likely cause. - CUDA green-context streams need context-aware completion-event handling and are not
claimed as supported. This change narrows the previous claim rather than widening it. - The device-resident asynchronous path is not covered end to end by CI, because the
reference runner's tensors are host-backed and therefore take the synchronized path.
Happy to gate the asynchronous return behind an explicit backend option instead, if you
would prefer that a backend-neutral stream selection never changes when execute()
returns. That would be a small follow-up and would leave the default fully synchronous.
Stacked on #4434
This change needs the ExecuTorch 1.4 caller-stream APIs, which #4434 provides by
bumping the pinned ExecuTorch commit. That bump is a separate concern and is reviewed
separately.
Both pull requests come from a fork, and GitHub only accepts a base branch that lives
in the upstream repository, so this cannot be expressed as a GitHub-level stack. The
commits are stacked correctly in git instead:
Because the base here has to be
main, this diff also shows the four pin files ownedby #4434:
Please review only the caller-stream changes here, and #4434 for the pin.
The two commits touch no files in common, so there is nothing to reconcile between
them. Once #4434 lands I will rebase, and those four files will drop out of this diff
with no change to the caller-stream code. Reviewing this as 17 files rather than 21 is
equivalent to reviewing the post-rebase state.
The problem
An ExecuTorch program can mix delegates. One subgraph may run on TensorRT while
another runs on the CUDA/AOTI backend. If the application wants both to run on a
CUDA stream it owns, both delegates have to agree on which stream that is.
Today they cannot agree. Torch-TensorRT keeps its own private "caller stream"
value, and ExecuTorch's CUDA backend keeps a different one. Selecting a stream for
one backend leaves the other running somewhere else, which breaks ordering.
The fix
Delete Torch-TensorRT's private caller-stream state and read ExecuTorch's shared
one instead.
Before:
After:
TensorRT and other CUDA-capable ExecuTorch delegates now read the same selection.
Why one shared library matters
The selection lives in a
thread_localvariable inside a single shared library,libextension_cuda.so. A shared library is a.sofile that a program loads atrun time, and every part of the program that loads the same one sees the same
variable.
If a second copy of that variable ends up in the process, for example because
something linked a static archive instead, then the runner writes to one copy and
the delegate reads the other. Nothing crashes. The delegate silently falls back to
a different stream:
So the build must guarantee exactly one shared library. This PR enforces that in
three ways:
confirming it is a shared object. Checking the file name is not enough, because
the linker happily links a static archive that has been renamed to
.so, whichproduces exactly the duplicate above.
add_subdirectoryon ExecuTorch's ownextension/cudainstead of re-declaringthe target. This keeps the two builds identical, and if something else also
declares the target, CMake fails loudly rather than producing two libraries.
DT_NEEDEDentry forlibextension_cuda.soplus dynamic imports of bothgetCallerStreamandCallerStreamGuard. A private copy satisfies thosereferences at link time and leaves no import, so its absence is the signal.
What else changes
cudaStreamPerThreadwhen no guard is active.return with work still in flight. Two separate reads could drift apart.
libextension_cuda.so.a
CallerStreamGuard.Compatibility
Removing
torch_tensorrt::executorch_backend::CudaStreamGuardis an intentionalsource-level C++ API change. Native callers switch to
CallerStreamGuardas shownabove. The replacement is behavior-preserving: an explicitly selected null stream
still counts as a caller selection, matching the old two-field encoding.
This PR validates ordinary CUDA streams. On the discrete-GPU CI configuration the
reference runner exercises the synchronized host-staging path. The device-resident
asynchronous path is not covered end to end by that runner. CUDA green-context
streams need context-aware completion-event handling and are not claimed as
supported here.
Python export-only usage is unchanged.
Testing
Verified on Linux x86_64 with an NVIDIA H100, CUDA 12.8, and TensorRT 11.0,
against the pinned ExecuTorch source checkout:
empty override, missing override, and rejection of a static archive, a linker
script, and an executable renamed to
.so. A shared object with no.soextension is correctly accepted, since the header decides, not the name.
embeds a private copy. The gate rejects it, including after stripping, and
accepts a correctly linked one.
explicit
cudaStreamPerThread, nesting, and per-thread isolation.the executable, and the reverse case confirming a duplicate copy is detectable.
ET_CHECK_MSGsurvives-O2 -DNDEBUG, so release-build checks are notcompiled out.
are clean.