feat(executorch): copy-back for non-KV mutable buffers in the TensorRT delegate - #4459
Conversation
|
#4459 widens the #4445 ordering blocker into a three-way reversal. lift_mutated_buffers appends copy-back values to the graph output: and the interpreter appends aliased KV outputs after that. So the engine binding order becomes [user…, copyback…, kv_aliased…]. But _declare_aliased_kv_mutations_on_ep sets the graph output to kv_getitems + copyback_getitems + out_args, giving delegate args [kv_aliased…, copyback…, user…] — the three groups in reverse. Since preprocess still passes output names through in engine order, a model with all three kinds gets a full permutation mismatch rather than the two-way swap in #4445. |
| # index_put KV write correctly falls through to copy-back rather than being | ||
| # dropped in the false expectation of aliasing. (A dedicated index_put -> | ||
| # IKVCacheUpdateLayer converter would let it use zero-copy aliasing instead.) | ||
| _KV_WRITE_TARGETS = { |
There was a problem hiding this comment.
This classification is op-level, but aliasing isn't an op-level property — so this can still drop a write-back in exactly the way the PR sets out to fix.
The comment above says the set must "stay in sync with the ops the converters actually turn into an IKVCacheUpdateLayer." The problem is that no set of ops can stay in sync with that, because whether an IKVCacheUpdateLayer gets emitted depends on shapes and network position, not just the target. In slice_scatter.py, _kv_eligible requires:
- a static
s_max - a 4-D
[b, d, s_max, h]cache dim == 2- a non-dynamic batch dim
and on top of that emit_kv_cache_update_layer bails when the cache isn't a direct network input (input_binding_name returns None) or when add_kv_cache_update returns None. index_copy.py has the same eligible/index_copy_fallback split.
When any of those fail, the converter emits a plain scatter with no aliasing — but _is_kv_cache_write already returned True, so the copy_ is erased with no copy-back appended and the write-back is silently lost. A 3-D cache or dim != 2 is enough to hit it.
Before this PR the erase was unconditional, so this failure mode existed for every mutated buffer; this PR narrows it to these two ops but keeps the same "trust the op" assumption for them.
Could the classification be derived from what was actually emitted rather than predicted? The engine's aliased_io map is the ground truth, and _declare_aliased_kv_mutations_on_ep already reads it. Deriving copy-back as "mutated buffer not present in aliased_io" post-conversion would make the two sides agree by construction.
If you'd rather keep the pre-conversion classification to avoid restructuring, the minimum would be a post-conversion assertion: every buffer classified as KV must appear in the engine's aliased_io, otherwise error (or fall back to copy-back) rather than silently dropping the write.
There was a problem hiding this comment.
By the time aliased_io is generated, the mutation write-backs have already been removed by DCE. Deriving copy-back from it would require re-attaching the write-back for ALL buffers and removing the ones present in aliased_io afterward.
Applied your suggestion to the classification and added the post-conversion assertion.
| self.assertEqual(len(lifted), 2) | ||
| self.assertEqual(new_gm.meta["_copyback_mutation_buffers"], ["state"]) | ||
|
|
||
|
|
There was a problem hiding this comment.
The KV cases here use slice_scatter / index_copy shapes that are KV-eligible, so they only cover the happy path of the classification.
Could you add a case where the op is slice_scatter but the converter's fast path would not fire — e.g. a 3-D cache, or dim != 2, or a dynamic s_max? Today that lands in the KV bucket and loses its write-back. Whatever behavior you settle on (copy-back, or a hard error), a test pinning it would keep the two sides from drifting as _kv_eligible evolves.
Same with #4445, I was not able to reproduce this. |
6a88214 to
a8b0d61
Compare
|
I am not 100% sure, but it seems like, in the decode step the KV cache is written in place by the engine, then copied again by ExecuTorch here: https://github.com/pytorch/executorch/blob/7811c69f41db6cc84abfd0b4a32f2e49393958a5/exir/passes/insert_write_back_for_buffers_pass.py#L163 because it is also a BUFFER_MUTATION output node. Please correct me if I am wrong |
|
This fixes the bug for executorch but leaves Torch-TensorRT runtime wrong. @narendasan Should we use the same mechanism to support in-place operation for Torch-TensorRT runtime by appending a copy node at the end of graph and run it in python as graph break? If we want to do that, we should move that part from export.py to compiler or interpreter |
You are correct, there is a redundant copy added by ET. Unfortunately, removing it would cause ExecuTorch to stop tracking and persisting the mutation. Fixing this properly would require delegate-boundary aliasing support on the ExecuTorch side. |
a8b0d61 to
da17e5b
Compare
|
@shoumikhin Thanks for taking a look and the thorough writeup. I have taken your suggestion regarding KV caching in submodules and applied the fix to problem 2 here. Problem 1 is a pre-existing bug on main and can be landed separately (#4472). Regarding excluded buffer writes from TRT, I believe those are out of scope for this PR and can remain in yours (#4470). |
da17e5b to
718d0c8
Compare
bd6e855 to
5e7b37d
Compare
5e7b37d to
0143022
Compare
|
I had left too many long comments here, several of them wrong or already fixed, so I deleted them and replaced them with this one list. Anything below that I did not personally reproduce is marked as such. Inline threads with your replies are untouched, since your answers there are worth keeping. Needs fixing before merge1. Saving with
The user output is not just dropped, it is relabeled as the new buffer contents. Saving succeeds silently and the saved program then fails on load with 2. Copy-back is skipped entirely when the optional
Fix is to move that import below the copy-back handling, or narrow the 3. It calls Worth a look, but I could not confirm these myself4. A buffer with a trained initial value may be wrong from the first call. Declaring a buffer 5. Two things I got wrong, so you do not chase them
NotesThe strided The design is right. The classifier's split between engine-aliased KV writes and copy-back writes is the correct call, and |
|
One more, and this one is on merged When elif module_type == _ModuleType.ep:
...
if output_format == "exported_program":
module = _declare_aliased_kv_mutations_on_ep(module) # declares
...
elif output_format == "executorch":
_save_as_executorch( # does not
module,That is Measured by instrumenting the call, same model and same input both times: I ran that against merged Effect: for I think it is a two-line fix, adding the same call the sibling branches make right before Separately, and not yours: |
…r path
save(retrace=True) threaded `_copyback_mutation_buffers` into
`_declare_aliased_kv_mutations_on_ep` on every branch, but the legacy exporter
already declares copy-back and moves each mutation ahead of the user outputs, so
the pass's "drop the trailing N outputs" step sliced a real user output and
relabeled it as the buffer's new contents. The saved program then failed on load
(`treespec.unflatten ... leaves has length 0`).
Gate the threading on the exporter, mirroring the retrace=False branch which
already passes no copy-back buffers: `_copyback_bufs = [] if _use_legacy else
module.meta.get("_copyback_mutation_buffers", [])`. This leaves the exporter's own
pairing logic untouched (it stays correct for the torch.export path it was written
for, where the declared value is still trailing).
Test: `test_saved_copyback_program_reloads_and_keeps_its_user_output` (parametrized
legacy True/False) drives the real `save()` -> `torch.export.load()` round trip with
no mocks, asserting the mutation is declared, the user output survives, and the
buffer updates on the reloaded module. Reverting the gate fails the legacy arm with
the `treespec.unflatten` error above.
…ch] extra `_declare_aliased_kv_mutations_on_ep` guarded `from torch_tensorrt.executorch.backend import _get_engine_info_for_node` with `except ImportError: return exp_program` -- a hundred lines before any copy-back work, which needs nothing from executorch. So a plain install (the extra is optional) silently dropped every copy-back mutation and the buffer never updated. Narrow the guard so only the engine loop is skipped. Whether the extra is importable is a property of the process rather than of a node, so it selects the iterable (`engine_nodes`) instead of being re-tested on every node; the copy-back path below runs either way. Test: `test_declare_copyback_runs_without_executorch_extra` hides `torch_tensorrt.executorch.backend` from `sys.modules` and asserts the copy-back BUFFER_MUTATION is still declared and the user output survives. Its graph carries an `execute_engine` node so the engine loop is reachable -- without one, dropping the guard still passes, while a real model raises `TypeError: 'NoneType' object is not callable`.
…rter
`convert_exported_program_to_serialized_trt_engine` calls `lift_mutated_buffers`
but acted on neither of its two results, where `compile()` acts on both. Two ways
a buffer could come back silently stale:
* a write classified as copy-back appends its new value as an extra engine
output, and this entry point reports neither which output carries which
buffer nor performs the update;
* a write predicted to be engine-aliased has its `copy_` dropped up front, so
if the converter emits no aliasing layer the write-back is simply gone. A
full-extent `slice_scatter` does exactly that -- it is predicted eligible,
and the converter returns `src` before reaching `try_emit_kv_cache_update`.
Reject the first right after `lift`, and cross-check the second against the
engine's own `aliased_io` before returning.
`assert_predicted_kv_aliased` now takes the aliased-input set rather than a
GraphModule, since the two callers hold ground truth in different shapes:
`compile()` reads it off the compiled submodules, the engine converter off
`interpreter_result.aliased_io`. Sharing the set-building via
`aliased_input_bindings` keeps one copy of the diagnostic. Predicted bindings are
read before lowering, as `compile()` does -- `gm` is replaced and the meta does
not follow it.
Test: `test_serialized_engine_rejects_copyback_buffers` pins the copy-back
rejection and that the message names the buffer. The predicted-KV cross-check is
covered by `TestPredictedKvAssertion`, now driving the assembly both callers use.
`_declare_aliased_kv_mutations_on_ep` claimed idempotency on the strength of its `already_exposed` check, which holds for the KV half only. The copy-back half detaches the trailing N outputs positionally before consulting that set, so a second run slices whatever trails the graph -- by then a genuine user output -- and the saved program fails to load with `treespec.unflatten ... leaves has length 0`. Per-buffer state cannot distinguish the two cases: torch.export declares a mutation it recognises and *leaves* the trailing value, while a previous run of this pass declares it and *consumes* the value. Both look "already declared". So record the consuming step on the GraphModule, in `gm.meta["_copyback_mutations_declared"]`, and skip the slice when it is set. `create_trt_exp_program` records it too, since it declares copy-back itself at transform time. Latent today -- nothing double-applies through the current entry points -- but the docstring asserted the property, which is how a future caller would get it wrong. Test: `test_declare_copyback_is_idempotent` runs the pass twice and asserts the second call returns the program untouched with its user output intact. Disabling the marker check fails it.
`_get_engine_info_for_node` ended in a hand-rolled `engine_obj.__getstate__()`, the pickling hook that re-serializes the whole ICudaEngine and base64-encodes it. `get_engine_info_from_state(..., metadata_only=True)` exists for callers that read only metadata and takes the record from `serialize_metadata_only` instead. Two callers are exactly that. The aliased-mutation declaration pass reads ALIASED_IO, INPUT_BINDING_NAMES and OUTPUT_BINDING_NAMES and nothing else, and it runs the engine loop before it can know whether any engine has aliased I/O, so a program with no aliased KV paid a full serialization per engine node -- measured at 0.4 s and a 67 MB transient string for one ~50 MB engine -- and then returned unchanged. `TensorRTPartitioner._resolve_target_device_for_partition` reads a single field, `DEVICE_IDX`, once per partition, on every export that does not pin `target_device` -- the default -- and behind no early-out at all: 967.6 ms against 3.1 ms on a ~100 MB engine, plus a 134 MB base64 string built and dropped. `metadata_only` defaults to False; only those two callers opt in. The accessor's caveat that ENGINE_IDX is unreliable under `metadata_only` applies to neither, since neither reads that slot. Test: `test_declare_aliased_kv_mutations_reads_engine_metadata_only` spies on `_get_engine_info_for_node` and asserts the declaration pass calls it with `metadata_only=True`; `test_resolve_target_device_uses_partition_engine` asserts the same for the partitioner. Dropping the kwarg fails each. `_resolve_target_device_for_partition` wraps its extraction in a broad `except Exception` that this new keyword passes through. A `_get_engine_info_for_node` that does not accept `metadata_only` raises `TypeError` there, and the partition's device label degrades to `cuda:0` behind a WARNING rather than failing the export -- so on a multi-GPU model every delegate would be labeled with GPU 0. The broad except and the fallback both predate this commit and are left as they are; it is disclosed here because this is the commit that widens the signature that except wraps.
Nothing under `py/torch_tensorrt/executorch/` ran the mutation-declaration pass, so of the three sources `export()` accepts only one arrived with its engines' aliased KV writes and its copy-back mutations declared: a GraphModule with retrace=False, which the legacy exporter declares while building the program. A GraphModule with retrace=True goes to torch.export, which truncates the aliased KV outputs and leaves the copy-back values as trailing user outputs, and an ExportedProgram or a method mapping carries whatever the caller exported it with. Undeclared, ExecuTorch freezes those buffers and they never update. Run `_declare_aliased_kv_mutations_on_ep` over `program_map` in `export()`, once `_prepare_programs` has normalized all three shapes into it, threading each method's `_copyback_mutation_buffers` from the source that method came from (`_copyback_buffers_by_method`). The pass is idempotent, so a program that already carries the declaration -- the legacy exporter's, or a caller's -- comes through unchanged. `save()` reaches this from `output_format="executorch"`, which makes one of its own warnings false: the `retrace=False` / `use_legacy_exporter=False` combination it warns about no longer leaves copy-back undeclared on that format, so the warning is now gated on the format. `output_format="exported_program"` still declares only the KV half of a saved ExportedProgram, so `save()` is not uniform across its two serializing formats and this does not make it so. Test: `test_executorch_export_declares_copyback_for_every_source_shape` runs a real copy-back program through `torch_tensorrt.executorch.export` once per source shape -- both GraphModule retrace modes, an ExportedProgram, and a method mapping -- and checks the Edge program's output specs. The legacy-exporter shapes (a GraphModule at the retrace default, and the mapping's `decode`) arrive already declared, so they also cover coming through a second declaration attempt unharmed. `test_save_warns_when_copyback_cannot_be_declared` gains an `output_format` parameter, so the branch that must warn and the branch that must not are pinned together. `FakeExportedProgram` in `test_export.py` gains a graph module and a signature, because the pass now runs on every program the option-forwarding tests hand to `export()`.
Declaring a copy-back write as an ExecuTorch BUFFER_MUTATION puts the buffer under
ExecuTorch's rule that a mutated buffer has a meaningless initial state: only its
shape and dtype are serialized, unless a pass marks it `et_init_buffer`. A buffer
read before it is written -- a GDN conv_state or recurrent_state, an EMA, a running
statistic -- therefore starts from whatever the allocation held.
ExecuTorch warns about this itself, but generically: it names no buffer and gives no
indication that lifting the buffer to an engine binding is what made it mutable.
Name the buffers and state the rule they fall under, with its condition attached, so
the message carries what a reader needs without the source in view. The condition is
not decoration: with `InitializedMutableBufferPass` supplied and the buffer on CPU,
the value is in the `.pte` -- `struct.pack("<32f", *([7.0] * 32))` is findable in the
saved bytes -- so a warning that the buffer loads uninitialized would be false there.
The message also names `InitializedMutableBufferPass`, and bounds it. Withholding the
name does not withhold the advice: ExecuTorch's emitter warns about the same buffer
later in the same export and names the pass itself, saying nothing about where the
buffer has to live. On a default-settings CUDA save of the shape this project
produces, the two land ten lines apart on one stderr. What only torch-tensorrt is
placed to add is the bound. The pass sets `et_init_buffer` on placeholders matching a
pattern; the emitter then marks the spec const and serializes the buffer by reading
it host-side with `ctypes.cast(spec.storage.data_ptr(), ...)`. Whether that read is
safe is decided by where the buffer lives, and nothing else: one model saves cleanly
with the buffer on CPU and dies in `_tensor_spec_to_evalue` with the same buffer on
CUDA, a segfault rather than an exception the caller can catch. Exporting a model on
CUDA is the ordinary way to reach a TensorRT engine, so the fatal case is the common
one -- which is why the caveat travels with the name rather than the name being left
out.
Test: `test_export_warns_that_copyback_buffers_load_uninitialized` asserts the buffers
are named, that the serialization rule is stated with its `et_init_buffer` condition,
and that `InitializedMutableBufferPass` appears only in a message that also carries
the segfault caveat. That last assertion fails against both earlier drafts of this
string, which named the pass as a bare instruction, and against a draft that keeps
this prose and appends the name with the caveat stripped.
`test_export_is_quiet_without_copyback_buffers` pins that an export with nothing to
copy back stays silent. Both key on one constant, `COPYBACK_WARNING_ANCHOR`, so
rewording the warning cannot leave the quiet test keyed on text no message contains:
rewording the headline and warning unconditionally together leaves the pair red,
where keying the two on different substrings left it green.
`lift_mutated_buffers` classifies a write as engine-aliased before partitioning and erases its `copy_`; `assert_predicted_kv_aliased` confirms after conversion that the engine really did alias it. Under `compile(dryrun=True)` no engine is built, so `aliased_io` is empty and every prediction looks unfulfilled -- the check failed the run outright, which is the opposite of what a dryrun is for. Skip it there. The skip is keyed on an explicit `engines_built`, not on `settings.dryrun`, because the two entry points do not mean the same thing by that flag. `convert_exported_program_to_serialized_trt_engine` accepts `dryrun`, puts it in `CompilationSettings` and then never consults it: `interpret_module_to_result` runs and an engine comes out. Reading `dryrun` inside the shared helper would therefore hand that entry point an opt-out kwarg for a check it is meant to have none for. Only `compile()` passes `engines_built=False`, and only where it returns before conversion. When it does fire, say why. The classification runs before partitioning, so the usual cause is the partitioner rejecting the subgraph the write landed in for having fewer than `min_block_size` supported ops -- the default, 5, is enough to trigger it on a model that compiles at 1. The message now names the value in force and the remedy. This does not fix the mis-prediction itself: a write kept out of an engine for any reason other than `torch_executed_ops` still loses its write-back and still raises. Routing those to copy-back the way `_write_op_is_torch_executed` already does needs the prediction to stop being destructive, which is a larger change. Test: `test_no_engines_built_does_not_raise`, `test_dryrun_alone_does_not_skip` -- which pins that the converter's `dryrun` is not an opt-out -- and `test_message_names_min_block_size`. Each fails with its half of the change disabled.
… cross-check
Every existing test drives `lift_mutated_buffers` and `assert_predicted_kv_aliased`
in isolation, so the lines in `_compiler.py` that connect them were unobserved:
replacing any of them with `pass` left the suite green.
`TestCompileSeam` goes through `compile()` and pins
* the copy-back list reaching `trt_gm.meta['_copyback_mutation_buffers']`, that
the recorded name still resolves to a buffer after inlining, and that the
trailing output carries the post-write value;
* `assert_predicted_kv_aliased` being called with the predictions lift made;
* the cross-check being load-bearing -- a predicted-KV write the partitioner keeps
out of every engine fails the compile rather than returning a module whose
buffer never updates;
* `compile()` telling the check that a dryrun built no engines. The check cannot
work that out for itself, since the engine converter also takes `dryrun` and
builds regardless, so `compile()` is the only thing that knows -- and until now
nothing observed that it says so.
Verified by mutation: neutering the `trt_gm.meta` propagation fails the first, the
third fails if the cross-check call is removed, and the fourth fails if
`engines_built` is dropped from the `compile()` call site.
…e predictor `_kv_write_will_alias` reuses `_kv_eligible`, the converter's own eligibility predicate, but computes the arguments it is handed independently of `impl/slice_scatter.py`, so the predicate agrees while the inputs do not: * `update_len` is taken from the source's extent along `dim` rather than `end - start`, so an open-ended `cache[:, :, 3:, :]` (which lowers with `end == INT64_MAX`) looks eligible to the predictor and fails the bound check in the converter; * a non-`int` `start` is silently read as `0` where the converter raises; * a negative `start` is passed through unnormalised; * the converter's full-overwrite shortcut -- which returns `src` and emits no KV layer at all -- has no counterpart, so a full-extent `slice_scatter` is predicted to alias and then hits the cross-check: ``` RuntimeError: lift_mutated_buffers classified these buffer writes as KV-cache (engine-aliased) and dropped their copy_, but the compiled engine did not alias them (absent from aliased_io): ['buf_cache']. ``` Factor that derivation into `resolve_slice_scatter_write` and have both sides call it, so the docstring's claim of reusing the converter's own predicates holds for the arguments too. The helper also reports the cases in which the converter returns or raises before reaching `_kv_eligible`, which is what lets the predictor rule out the full overwrite. It returns `Tuple[Optional[int], Optional[int], Optional[int], KVWriteStatus]` -- the bounds are `None` under the two statuses that resolve nothing -- so each caller's guard on the status is also what gives it three `int`s. `step` behaviour is deliberately untouched. `_kv_eligible` does not consult it, and the helper consults it only where the converter does (the full-overwrite shortcut), so a strided write with concrete bounds still takes the KV path; `test_strided_write_still_takes_the_kv_path` guards that. Two changes ride along, both from the dim check moving into the shared helper so that the predictor and the converter apply one rule to `dim`: an out-of-range `dim` raises `IndexError` from the converter rather than out of `input.shape[dim]` with a TensorRT message, and `dim` must be an `int`, so a `numpy.int64` `dim` is now read as a bad dim where it used to work. That `IndexError` leads with the offending value and its type and puts the rule after them, because `BAD_DIM` covers two causes and opening with the rule reads as an accusation the input answers: an in-range `numpy.int64` is refused on type while the value printed is in range, and an out-of-range Python `int` is refused on range while the type printed is the required one. Naming the type at all is what separates the two -- the value alone would read as an out-of-range complaint about an in-range number -- and the type name states that distinction the same way on every numpy version, which `repr` does not: `repr(np.int64(2))` is `'2'` before numpy 2.0. Both sides dispatch on `status` rather than on the bounds coming back `None`. Both forms behave the same, but only the status is a claim the helper makes; `None` is a consequence of it that the return type does not tie down. The `assert` each side puts under its status guard is a narrowing device rather than a check: `OK` is returned only after `isinstance` has passed on all three bounds, so neither assert can observe a violation, and both vanish under `python -O`. They are there to give mypy, and a reader, the `int`s the following line needs -- and to state the one invariant the same way on both sides of the split. Test: `TestSliceScatterEarlyExits` drives both raising exits through the converter. The dynamic-bounds exit had no coverage anywhere, so restoring the helper's bounds pass-through left that guard unreachable with every test still green. The `numpy.int64` case pins the printed type, which is what distinguishes it from the out-of-range case. Both pin the whole sentence, anchored at both ends, so neither cause can take on the other's wording unnoticed.
The value a copy-back write re-attaches as a graph output is the whole post-write buffer, not the slice that changed, so a decode step pays a full-buffer materialization of that output plus a full-buffer copy from ExecuTorch's own write-back pass -- per copy-back buffer, per call. For a single-position write into a large KV cache that dominates the step. It is the right correctness tradeoff, but nothing says so, and someone debugging a slow model has no way to tell whether they are hitting it. Record it once, in the `_buffer_lifting.py` module docstring, with `gm.meta['_copyback_mutation_buffers']` named as the way to find out which buffers are paying. `lift_mutated_buffers`, the function that routes a write into copy-back, points at that docstring rather than restating it.
`save()` reaches the mutation declaration two different ways. For
`output_format="exported_program"` it calls `_declare_aliased_kv_mutations_on_ep`
itself. For `output_format="executorch"` an ExportedProgram source gets it only
because `_save_as_executorch` hands the program to
`torch_tensorrt.executorch.export`, which declares for every source shape it
accepts; nothing in `save()` does it on that branch.
Only the first route was pinned. A rework of `_save_as_executorch` that lowered to
Edge directly would drop the declaration for the second, and the loss is silent at
save time: torch.export drops the engines' aliased KV outputs at the fx boundary, so
the delegate ends up carrying fewer outputs than the engine has output bindings, and
copy-back buffers load frozen.
Test: `test_save_declares_aliased_mutations_on_every_serializing_branch` spies on the
pass -- delegating to the real one, so the pipeline underneath stays real -- across
(module type x serializing format) = {ep, fx_retrace, fx_no_retrace} x
{exported_program, executorch}. aot_inductor is excluded: it warns instead of
declaring. Removing the declaration from `torch_tensorrt.executorch.export()` fails
the `(ep, executorch)` case and leaves the other five passing, which is the case this
adds.
`origin/main` is mypy-clean on both `py/torch_tensorrt/dynamo/_exporter.py` and `py/torch_tensorrt/dynamo/lowering/_buffer_lifting.py`; with this PR applied, neither is. Four errors across the two, all from commits already pushed, so they are corrected here rather than amended. The pre-commit hook runs mypy over the files a commit changes, so a PR-shaped run fails on files this PR touches. `_exporter.py`, `output_nodes`: declared a list and then rebound to a `tuple(...)` when the mutation outputs are moved ahead of the user outputs. Keep it a list and build the tuple only where the output node's args require one. `_exporter.py`, the `[executorch]` import guard: it assigns `None` to a name mypy has typed as the imported function. Give the name its own `Optional[Callable[..., List[Any]]]` annotation, bind the import to it, and `assert` it inside the scan loop, which is where the narrowing has to hold -- the loop body cannot inherit it from a `nodes_to_scan` expression. A `# type: ignore[assignment]` was the shorter fix and the wrong one: under `follow_imports = "skip"` it is reported as unused whenever `_exporter.py` is checked without `torch_tensorrt/executorch/backend.py` alongside it, which is exactly the file set a commit touching only `_exporter.py` produces. The price of the annotation is `...` for the arguments, which the keyword call forces. `_buffer_lifting.py`, two `no-any-return`s in `_kv_write_will_alias`: both returned values come from functions imported under `follow_imports = "skip"`, so mypy reads them as `Any` however their own file annotates them -- `_index_copy_kv_eligible` is already declared `-> bool` and that makes no difference here. Convert at the point of return instead. Verified with `mypy --python-version 3.12 --cache-dir=/dev/null` on each changed file alone and on all eight together, at this commit and at the base: no error the base does not already have, and `_buffer_lifting.py` goes from two errors to none. The single error left in the combined run, `_compile.py:571 Redundant cast to "bytes"`, reproduces on `origin/main`. Run under mypy 1.14.1 and under 1.15.0, the version `.pre-commit-config.yaml` pins for the hook, since the argument above is about what that hook reports. Both versions give identical output on all six runs -- individual and combined, at this commit, at the base and at `origin/main`.
|
Changes since the last push, mapped against your numbered items. 1. Cross-check turns working compiles into hard failures at the default
|
deleted from _compiler.py |
fails |
|---|---|
trt_gm.meta["_copyback_mutation_buffers"] = _copyback_mutation_buffers |
test_compile_threads_copyback_buffers_into_trt_gm_meta |
the assert_predicted_kv_aliased(...) call |
test_compile_raises_when_a_predicted_kv_write_is_not_aliased, and test_compile_runs_the_predicted_kv_cross_check, which patches the symbol and asserts it was called with ["buf_cache"] |
the engines_built=not settings.dryrun argument at that call site |
test_compile_does_not_cross_check_a_dryrun |
The model in test_compile_raises_when_a_predicted_kv_write_is_not_aliased is your repro — zeros(2, 4, 16, 8) cache, cache[:, :, 3:4, :] = x, min_block_size=5 — so the raise is pinned as behaviour rather than left to the assertion being present. The meta test also checks that the recorded name still resolves to a buffer after inlining and that the trailing output carries the post-write value, not a stale read.
3. The classifier re-derives _kv_eligible's arguments
Factored into resolve_slice_scatter_write in dynamo/conversion/impl/slice_scatter.py, returning (start, end, step, KVWriteStatus). The converter and _kv_write_will_alias both call it; the converter's argument derivation is now literally the same code the predictor runs. Your four:
update_lenfrom the source shape. Both sides now takeend - start.test_open_ended_slice_is_not_kvdrivescache[:, :, 3:, :]withend == INT64_MAX, which failsstart + update_len <= s_maxin the predictor exactly as it does in the converter.- A non-
intstarttreated as0. NowKVWriteStatus.DYNAMIC_BOUNDS: the predictor returnsFalse, the converter raisesNotImplementedError.test_non_int_start_is_not_kv. - No negative-index normalisation. The helper counts a negative
startorendfromdim_sizebefore anything reads it.test_negative_start_is_normalisedcovers both sides of the bound —-4in a 16-slot cache normalises to 12, so a 4-slot write is eligible and an 8-slot write is not. - No full-overwrite shortcut. Now
KVWriteStatus.FULL_OVERWRITE, which the predictor treats as "nothing aliases the cache" and routes to copy-back.test_full_overwrite_is_not_kv, both with explicit0, 16bounds and with the bounds omitted.
step is deliberately unchanged. _kv_eligible does not consult it, and the helper consults it only where the converter does — the full-overwrite shortcut, which requires step == 1 — so a strided write with concrete bounds still takes the KV path. That matches your note that the case predates this PR. test_strided_write_still_takes_the_kv_path pins the current answer so the shared derivation cannot change it by accident, without claiming it is the right one.
Two behaviour changes ride along, both from the dim check moving into the shared helper so predictor and converter apply one rule to dim: an out-of-range dim raises IndexError from the converter rather than surfacing out of input.shape[dim] with a TensorRT message, and dim must be a Python int, so a numpy.int64 dim is read as a bad dim where it used to work. The IndexError prints the offending value and its type name, because those two causes share one status and an in-range numpy.int64 would otherwise read as an out-of-range complaint about an in-range number. TestSliceScatterEarlyExits in tests/py/dynamo/conversion/test_slice_scatter_aten.py drives both raising exits through the converter and matches the type name rather than the rendered value, which numpy 2.0 changed; the dynamic-bounds exit had no coverage anywhere, so restoring the helper's bounds pass-through left that guard unreachable with every test still green.
4. The copy-back cost is not written down
Now in the _buffer_lifting.py module docstring, under "What copy-back costs": only an engine-aliased write is free; a copy-back write re-attaches the whole post-write buffer as a graph output, because a BUFFER_MUTATION output is defined as the buffer's new contents rather than the slice that changed. Each call therefore pays, per copy-back buffer, the graph materializing the full buffer as that output plus ExecuTorch's write-back pass copying it into the caller-owned buffer after the delegate returns. For a decode step writing one position into a multi-megabyte cache that is a full-cache-sized materialization plus a full-cache-sized copy to record a single-slot update, and easily dominates the step. It states that this is the right correctness tradeoff, that it is why the classifier routes a cache write to engine aliasing instead, and that a model landing unexpectedly in copy-back comes out slow rather than wrong — with gm.meta['_copyback_mutation_buffers'] named as the way to find out which buffers are paying. lift_mutated_buffers points at the docstring rather than restating it.
Your 15:12 comment: save(ep, output_format="executorch") never declares the mutation
Fixed, though not with the two lines in _compile.py. The declaration now runs inside torch_tensorrt.executorch.export(), over program_map once _prepare_programs has normalized all three source shapes into it, threading each method's _copyback_mutation_buffers from the source that method came from. save(ep, output_format="executorch") reaches it through _save_as_executorch. Doing it there also covers the two source shapes save() cannot reach at all — an ExportedProgram or a method mapping handed straight to export(), neither of which was declared either. The pass is idempotent, so a program that already carries the declaration (the legacy exporter's, or a caller's) comes through unchanged, and it runs on the staged copy so a caller's own program is left intact.
Regression test, since you asked: test_save_declares_aliased_mutations_on_every_serializing_branch sweeps (module type ∈ {ep, fx_retrace, fx_no_retrace}) × (format ∈ {exported_program, executorch}), spying on the pass while delegating to the real one so the pipeline underneath stays real. aot_inductor is excluded — it warns instead of declaring. Removing the declaration from torch_tensorrt.executorch.export() fails the (ep, executorch) case and leaves the other five passing, which is the case that was missing. test_executorch_export_declares_copyback_for_every_source_shape checks the resulting Edge program's output specs once per source shape export() accepts: both GraphModule retrace modes, an ExportedProgram, and a method mapping.
Knock-on: save()'s retrace=False / use_legacy_exporter=False warning became false for output_format="executorch", since that branch now declares, so the warning is gated on the format. test_save_warns_when_copyback_cannot_be_declared takes an output_format parameter and pins all four combinations together.
Still asymmetric in one place: save(exported_program, output_format="exported_program") declares only the KV half, because no copy-back buffer list is threaded on that branch. This does not make save() uniform across its two serializing formats.
Changes you did not ask for
Idempotency marker. _declare_aliased_kv_mutations_on_ep claimed idempotency on the strength of its already_exposed check, which holds for the KV half only. The copy-back half detaches the trailing N outputs positionally before consulting that set, so a second run sliced whatever trailed the graph — by then a genuine user output — and the saved program failed to load with treespec.unflatten ... leaves has length 0. Per-buffer state cannot separate the two cases: torch.export declares a mutation it recognises and leaves the trailing value, while a previous run of this pass declares it and consumes the value. So the consuming step records gm.meta["_copyback_mutations_declared"] and the slice is skipped when it is set; create_trt_exp_program records it too. Latent through today's entry points, but the docstring asserted the property, which is how a future caller would get it wrong.
Metadata-only engine read. _get_engine_info_for_node ended in a hand-rolled engine_obj.__getstate__() — the pickling hook, which re-serializes the whole ICudaEngine and base64-encodes it. Two callers read metadata and nothing else, and both now pass metadata_only=True: the declaration pass, which reads ALIASED_IO and the binding names and runs its engine loop before it can know whether any engine has aliased I/O, so a program with no aliased KV paid a full serialization per engine node and then returned unchanged (0.4 s and a 67 MB transient string for one ~50 MB engine); and TensorRTPartitioner._resolve_target_device_for_partition, which reads DEVICE_IDX once per partition on every export that does not pin target_device, the default, behind no early-out at all (967.6 ms against 3.1 ms on a ~100 MB engine, plus a 134 MB base64 string built and dropped).
Uninitialized copy-back buffers. Declaring a copy-back write a BUFFER_MUTATION puts the buffer under ExecuTorch's rule that a mutated buffer has a meaningless initial state — only shape and dtype are serialized — so a buffer read before it is written starts from whatever the allocation held. ExecuTorch warns generically and names no buffer; export() now names them and states the rule they fall under. It offers no remedy, and must not: ExecuTorch's own InitializedMutableBufferPass sets et_init_buffer, after which the emitter marks the spec const and serializes the buffer by reading its storage host-side through ctypes.cast(spec.storage.data_ptr(), ...) — which segfaults the process, rather than raising, whenever the buffer is CUDA-resident, and exporting a model on CUDA is the ordinary way to reach a TensorRT engine. test_export_warns_that_copyback_buffers_load_uninitialized asserts the pass stays unnamed, and fails against both earlier drafts of this string. Engine-aliased KV buffers are excluded on purpose: an aliased write is a cache-position write on the sequence axis, such a model is assumed to read the cache under that same position, and initializing it would write the model's largest tensor whole into the .pte for nothing.
Three things worth knowing
Two tests fail on main today, and both are test defects rather than production defects. tests/py/dynamo/executorch/test_api.py::test_public_save_forwards_lowering_kwargs_graphmodule_no_retrace stubs _exporter.export to return a bare object() sentinel; #4445 added a _declare_aliased_kv_mutations_on_ep(exp_program) call on that same branch, and the sentinel flows straight into it. tests/py/dynamo/executorch/test_edge_cases.py::test_save_as_executorch_uses_public_lowering_and_persists_data asserts export.assert_called_once_with(source, partitioners=..., compile_specs=...), while _save_as_executorch forwards eight — #4433 added transform_passes, compile_config, constant_methods and generate_etrecord, #4336 added weight_streaming_budget_per_engine. Both reproduce at this head unchanged. A fix is prepared and will be sent as its own PR rather than folded in here.
origin/main does not pass its own isort pre-commit hook. With the pinned isort 6.0.0 and the repo's profile = "black", py/torch_tensorrt/executorch/_export.py and tests/py/dynamo/executorch/test_weight_streaming_budget.py both fail --check-only, for the same import: WEIGHT_STREAMING_BUDGET_COMPILE_SPEC_KEY is ordered after normalize_weight_streaming_budget_per_engine in a from torch_tensorrt.executorch.partitioner import (...) block, and isort wants the constant first. Traced to #4336. Not fixed here, but this PR touches both files, so a PR-shaped hook run reports them.
CI is not checking any of this. Noted from your comment: executorch-runtime-test is gated on executorch-runtime-build, which is failing on main, so the ExecuTorch tests are skipped rather than run, here and on main. Nothing above changes that.
|
Re-reviewed at Five new things, ordered by what I would fix first. Everything below I ran on this head and on the merge base 1. A buffer can be classified copy-back and still be engine-aliased, and the model then raises on every call
When the write reads the buffer through a clone, classification sees a class M(torch.nn.Module):
def __init__(self):
super().__init__()
self.register_buffer("k", torch.zeros(1, 2, 16, 4).cuda())
def forward(self, x):
k = self.k.clone()
k[:, :, 3:4, :] = x
self.k.copy_(k)
return k.sum(dim=-1)Same failure on an 8 head decoder step with a 128 slot cache at the default
Two suggestions, and I would take both:
Either way the guard on line 115 needs a test. I removed it and the suite still passed. 2.
|
`lift_mutated_buffers` appends each copy-back buffer's new value to the graph's output node so it survives dead-code elimination and reaches the exporters, which reclassify it as a `BUFFER_MUTATION`. Nothing between `compile()` and the ExecuTorch runtime writes it into the buffer, so returning it to the caller advertised a mutation that did not happen: a model with one non-KV mutable buffer went from one return value on the merge base to two, with the module's buffer stale either way. Hide it at the call boundary instead of dropping it from the graph. A `CodeGen` subclass generates a `forward` that stops at the user outputs while the output node keeps everything, so the exporters, dead-code elimination, and the `torch.fx.Interpreter` that a non-strict `torch.export` runs a GraphModule through all still see the value -- a retrace carries it into the program exactly as before. Non-strict is what both retrace paths get: `_compile.save` passes `strict=False` and `dynamo._exporter.export` takes the default, which is `False` in torch 2.14.0.dev20260713. Measured on a 2-output GraphModule with one value hidden: ``` plain call arity 1 torch.export(strict=False) 2 outputs torch.export(strict=True) 1 output <- hidden value lost fx.Interpreter 2 outputs ``` so the docstring says "non-strict" rather than "torch.export", and a caller who exports the compiled module themselves with `strict=True` is told what they lose. That call currently dies first on a pre-existing `NotImplementedError: '__eq__' is not implemented for __torch__.torch.classes.tensorrt.Engine`, so the hazard is latent rather than live. `_TorchTensorRTModule.forward` already does the same with the aliased outputs the interpreter appends. `torch_tensorrt.dynamo.compile` and `torch_tensorrt.compile(ir="dynamo")` now return what the merge base returns; `save()` still declares the `BUFFER_MUTATION` under both `retrace=False` and `retrace=True`; `torch.compile(backend="tensorrt")` never reached this path and is unaffected. Test Plan: `tests/py/dynamo/lowering/test_buffer_lifting.py`: 42 passed (was 35). `TestHiddenCopybackOutputs` covers the mechanism on CPU -- `forward` truncates, the output node does not, `eliminate_dead_code` cannot reach the hidden value, and `fx.Interpreter` still yields it. Through `compile()`: `test_compile_hides_the_copyback_value_from_the_return`, `test_saved_program_declares_the_hidden_copyback_mutation` (both retrace modes, and it pins the surviving `USER_OUTPUT` so an exporter that lost the value cannot pass by declaring the mutation out of the user output), and `test_retracing_exporter_sees_the_hidden_copyback_value`. The truncation takes the *last* N values, so it is right only while the copy-back values really are the trailing ones. Two mechanisms append outputs, and `test_compile_hides_copyback_beside_an_engine_aliased_kv_buffer` is the case where both run on one module: an engine-aliased KV cache and a copy-back buffer together. It pins that the aliased output the interpreter appends inside the submodule is truncated at that boundary and never reaches the outer graph, so the value the count leaves behind is the copy-back buffer's and not a KV output. Discrimination: deleting the `hide_copyback_outputs` call gives 3 failed, 39 passed -- `test_compile_hides_the_copyback_value_from_the_return`, `test_compile_hides_copyback_beside_an_engine_aliased_kv_buffer` and `test_saved_program_declares_the_hidden_copyback_mutation`. Set A (`test_buffer_lifting.py` + `tests/py/dynamo/executorch/`): 4 failed, 255 passed -- the same four failures as before this commit. Set B: 33 passed. `black`, `ruff`, mypy per-file all clean.
…h directions
`remove_input_alias_fixing_clones`, an ATEN lowering pass inside `post_lowering`,
erases a `clone` whose placeholder has no other user. `lift_mutated_buffers` runs
before that, saw a `call_function` at `args[0]`, and filed the write copy-back --
after which the converter saw a direct network input and emitted the KV layer
anyway. The runtime truncates the aliased output as an engine side effect while the
outer graph still reads it, so the compiled module raised
`IndexError: index 1 is out of bounds for dimension 0 with size 1` on every call.
Clone, scatter, write back is the ordinary shape when the post-write cache is also
read in the same forward.
`_reads_the_cache_as_a_network_input` now peels such a clone, reproducing the
lowering pass's own sole-user condition so a cache another node also reads keeps its
clone and stays copy-back. Classification moved to after the `copy_` is erased,
because the `copy_` is the last reader of the placeholder in the ordinary write-only
shape and its presence is what decides whether the clone survives.
`assert_predicted_kv_aliased` gains the other direction: a buffer filed copy-back
that turns up in the engine's `aliased_io` now raises with a message that names the
consequence, instead of handing back a module that cannot run.
`predicted_kv_bindings` is empty in that case, so the existing check returned at its
early exit and saw nothing. `lift_mutated_buffers` records the copy-back input
bindings in `gm.meta["_copyback_bindings"]` for it.
The prediction is now coupled to a lowering pass that is documented as deletable
("TODO: Delete this lowering pass once aot_export_joint_simple is patched") and
whose index in `post_lowering_pass_list` is reachable through the public
`_remove_lowering_pass`. That coupling was written down on the classifier's side
only, so `remove_input_alias_fixing_clones` now carries the back-reference.
Verified against the review's repro and against an 8-head decoder step with a
128-slot cache
at the default `min_block_size`. Head now matches the merge base on both:
```
merge base head 697cb65 here
copyback [] ['k_cache','v_cache'] []
aliased_io both caches both caches both caches
call 1 output IndexError 1 output
matches eager True -- True
buffer matches eager True -- True
```
**Behaviour change worth stating, because it lands at the default setting.** A
cloned KV-shaped write whose subgraph no engine claims now raises the cross-check
error rather than compiling. Same model, three revisions:
```
merge base 08276e7 head 697cb65 here
CLONE-slice_scatter mbs=5 compiles, buffer stale compiles, stale RuntimeError
CLONE-index_copy mbs=5 compiles, buffer stale compiles, stale RuntimeError
same at mbs=1 correct IndexError on call correct
```
So this converts a *silently stale* compile into a hard failure at `min_block_size`'s
default of 5, not only at 1. That is the same answer the identical write without the
clone already gets, and a loud failure beats a buffer the caller believes is being
updated -- but a user with a small clone-shaped cache model sees a new error where
their model used to compile. `_reads_the_cache_as_a_network_input`'s docstring says
so at the point the decision is made.
Test Plan:
`tests/py/dynamo/lowering/test_buffer_lifting.py`: 53 passed (was 42).
`TestCacheMustReachTheConverterAsANetworkInput` covers the guard that had no test:
a direct placeholder, a clone of a sole-use placeholder, a clone of a shared
placeholder, a clone of a non-placeholder, a cache read through another op, and a
cache read from a `get_attr`. `TestPredictedKvAssertion` gains the copy-back
direction, and `TestCompileSeam` drives the review's repro model end to end plus the
cross-check with the classifier stubbed to mis-predict.
Discrimination, each mutation applied alone:
| mutation | fails |
|---|---|
| drop the clone-peel branch | `test_clone_of_a_sole_use_placeholder_is_kv`, `test_compile_runs_a_cloned_cache_write_and_writes_it_back` |
| delete the network-input guard | the four `..._is_not_kv` cases |
| drop the copy-back-aliased raise | `test_raises_when_a_copyback_write_is_aliased`, `test_compile_raises_when_a_copyback_write_is_aliased` |
| drop `copyback_bindings` at the `compile()` call site | `test_compile_raises_when_a_copyback_write_is_aliased` |
| classify before erasing the `copy_` | `test_compile_runs_a_cloned_cache_write_and_writes_it_back` |
Set A: 4 failed, 266 passed -- the same four failures as before. Set B: 33 passed.
`black`, `ruff`, mypy per-file all clean.
The cross-check's message named the value in force and then recommended it. At `min_block_size=1` it read `min_block_size (1) rejected the subgraph it landed in; min_block_size=1 rules that out` -- a self-contradiction, and no remedy the reader can act on. Split the sentence: above 1, name the value and the remedy as before; at 1, leave `min_block_size` out of the sentence entirely and name the cause -- a converter or a capability validator rejected the op. This is the reachable half of review item 4. The other half -- the message blaming `min_block_size` for a write that is dead rather than partitioned out -- is fixed by routing that write to copy-back so it no longer raises at all, in a later commit in this stack. Test Plan: `tests/py/dynamo/lowering/test_buffer_lifting.py`: 54 passed (was 53). `test_message_does_not_offer_min_block_size_1_when_it_is_already_1`. Discrimination: collapsing the branch back to the single unconditional sentence fails it, and `test_message_names_min_block_size` pins that the above-1 message is unchanged. Set A: 4 failed, 267 passed -- the same four failures as before. Set B: 33 passed. `black`, `ruff`, mypy clean on every changed file.
…r recommends
The error at the end of `convert_exported_program_to_serialized_trt_engine` said
"Use torch_tensorrt.dynamo.compile, which performs the write-back itself."
`compile()` does not perform the write-back: it leaves the value on the graph for an
exporter to reclassify, so a caller who follows the advice literally gets a module
whose buffer is as stale as the one this error refused to hand back. The raise itself
is right.
Adopted the review's wording after checking both halves of it against the code:
```
copyback: ['state']
specs: [('BUFFER_MUTATION', 'state'), ('USER_OUTPUT', None)]
ep.module() buffer before: [0.0, 0.0, 0.0]
ep.module() buffer after : [0.6054, 0.5456, -0.0038]
buffer updated by running the loaded program: True
```
So `compile()` plus `save()` does declare the mutation, and something that loads the
saved program does apply it -- which is why the sentence says "the runtime" rather
than naming ExecuTorch: `ep.module()` re-lifts the declaration into a `copy_` and
applies it in PyTorch too.
The comment above the raise claimed "the buffer is updated afterwards" without
saying by what, and said this entry point does not "perform that update", implying
some other entry point does. Corrected to name the serialization step.
The existing test's buffer-name assertion is anchored to the rendered list
(`"['state']"`) rather than the bare word, which also matches `stateless`,
`state_dict` and `statement`. It does not assert the remedy's wording: what makes
that wording true is that following it works, so it is held to account by the two
tests that do follow it end to end
(`test_executorch_export_declares_copyback_for_every_source_shape` and
`test_saved_program_declares_the_hidden_copyback_mutation`) rather than by a
substring pin that a correct reword would break and a differently wrong remedy would
pass.
Test Plan:
`tests/py/dynamo/executorch/test_kv_cache_export.py -k serialized_engine_rejects_copyback`:
1 passed. Set A: 4 failed, 267 passed. `black`, mypy clean.
…pected behaviour
`test_strided_write_still_takes_the_kv_path` asserted that
`cache[:, :, 0:8:2, :]` is classified KV-eligible. That routing is wrong -- the write
lands in slots 0, 1, 2, 3 rather than 0, 2, 4, 6 -- so the test asserted the wrong
answer is the expected answer, and the eventual fix would have arrived looking like a
regression with a passing test to delete.
Measured on both trees, same model, same input:
```
tip merge base 08276e7
eager slots [1, 0, 2, 0, 3, 0, 4, 0, 0, ...] [1, 0, 2, 0, 3, 0, 4, 0, 0, ...]
trt slots [1, 2, 3, 4, 0, 0, 0, 0, 0, ...] [1, 2, 3, 4, 0, 0, 0, 0, 0, ...]
```
Identical, so the bug predates this PR and the decision that `step != 1` stays
KV-eligible stands. Re-pointed at what the test was actually there to freeze:
`resolve_slice_scatter_write` returns `step` unchanged, which is the shared
derivation's contract, without blessing the routing built on top of it.
The `OK` return carries a comment saying so, scoped to the path that is actually
wrong. `try_emit_kv_cache_update` never reads `step`, so a strided write the KV fast
path accepts is lowered as if it were contiguous; the scatter fallback below does
read it (`np.arange(start, end, step)`) and lowers the same write correctly, which
`test_fallback_step_two` pins. Writing the claim unscoped -- "nothing downstream
reads `step`" -- would send the next reader at code that is correct and invite a
"fix" at the `OK` return that breaks that test. The comment does not record that the
same behaviour was measured on the merge base: that provenance is
true of this branch and stops being true the moment it lands, at which point the
comment would be telling its reader the opposite of the facts. The measurement
belongs here, in the commit that made it. The docstring paragraph that called the
strided case "a separate question this does not settle" points at the note instead.
Behaviour is unchanged: the two production edits are a docstring and a comment, and
filtering the `py/` diff for non-comment lines returns nothing.
Test Plan:
`tests/py/dynamo/lowering/test_buffer_lifting.py` + `test_slice_scatter_aten.py`:
unchanged from before this commit.
Discrimination: returning a normalized `1` in place of `step` at the `OK` exit fails
`test_step_is_returned_unchanged`.
mypy on `slice_scatter.py` is byte-identical to `697cb65756` (both stop on the same
pre-existing numpy-stub syntax error). `black` clean.
…e write
Comments only; no behaviour change.
`_kv_write_will_alias` said returning `False` "keeps the `copy_`". It does not: the
`copy_` is erased on both branches, and what `False` buys is the new value being
re-attached as a graph output instead. A reader taking the comment at face value
would look for a `copy_` in the lowered graph and find nothing.
Two docstrings said an ineligible `slice_scatter` / `index_copy` "is lowered to a
non-aliasing scatter", which is one of four outcomes. Against the converter:
| status | what the converter does | write-back to preserve? |
|---|---|---|
| `OK`, not `_kv_eligible` | falls back to a non-aliasing scatter | yes |
| `FULL_OVERWRITE` | returns `src`; no scatter at all | yes -- `src` is the buffer's new contents |
| `DYNAMIC_BOUNDS` | raises `NotImplementedError` | never reached |
| `BAD_DIM` | raises `IndexError` | never reached |
`index_copy` adds a fifth: an ineligible one reaches `index_copy_fallback`, which is
a scatter but itself raises `NotImplementedError` for a dynamic index or an
unsupported dim. Corrected in `_kv_write_will_alias`, in `lift_mutated_buffers`, and
in the `TestCopyBackClassification` class docstring that repeated the claim.
The last column matters, because the first draft of this docstring said only the
scatter case has a write-back to preserve -- which contradicts the comment forty
lines below it in the same function ("A full overwrite needs that: it returns the
source, emits no KV layer, and its write still has to be recorded"). The comment was
right and the docstring was wrong, so the docstring now names both.
The two per-test docstrings in `test_ineligible_index_copy_is_copyback` and
`test_ineligible_slice_scatter_is_copyback` are left alone: each describes one
specific model, and both of those models really do take the scatter fallback.
Test Plan:
`tests/py/dynamo/lowering/test_buffer_lifting.py`: unchanged.
`black` and mypy clean.
Completes review item 1. Peeling the clone in `_kv_write_will_alias` fixed
`slice_scatter`, whose aliasing turns on the cache having an input binding name at
conversion time, but not `index_copy`: `_index_copy_kv_eligible` applies its own
`args[0].op == "placeholder"` test, and at classification time `args[0]` is still the
clone. So the `index_copy` decode write -- the per-step cache-position write, which
is the shape this whole path exists for -- was still filed copy-back while the
converter aliased it, and still raised on every call. Found while building the
end-to-end run for item 1; the review's repro used `slice_scatter`, which is
why one fix
looked like enough.
```python
def forward(self, x, pos):
k = self.k.clone()
k = k.index_copy(2, pos, x)
self.k.copy_(k)
return k.sum(dim=-1)
```
```
merge base 08276e7 1 output, matches eager, buffer matches eager
head 697cb65 copyback ['k'], aliased_io {'output1': ('buf_k', ...)}
IndexError: index 1 is out of bounds for dimension 0 with size 1
before this commit RuntimeError from the new copy-back cross-check
here 1 output, matches eager, buffer matches eager
```
`_reads_the_cache_as_a_network_input` becomes `_effective_cache_input`, returning the
node the converter will be handed rather than a bool, and `_index_copy_kv_eligible`
takes a keyword-only `input_node` override so the classifier can point it at that
node. The partitioner passes nothing and reads `args[0]` exactly as before -- by the
time it runs as a capability validator, lowering has settled what the input is. The
back-reference in `remove_input_alias_fixing_clones` follows the rename.
Test Plan:
`tests/py/dynamo/lowering/test_buffer_lifting.py`: 58 passed (was 54).
`test_index_copy_through_a_sole_use_clone_is_kv`,
`test_index_copy_through_a_shared_clone_is_not_kv`,
`test_index_copy_validator_still_reads_args0_by_default` (which pins that the
override changes nothing for the partitioner), and
`TestCompileSeam.test_compile_runs_a_cloned_index_copy_write_and_writes_it_back`.
Discrimination: dropping the `input_node=cache_input` argument fails
`test_index_copy_through_a_sole_use_clone_is_kv` and
`test_compile_runs_a_cloned_index_copy_write_and_writes_it_back`.
`black`, `ruff` clean. mypy on `_buffer_lifting.py` clean, and on
`aten_ops_converters.py` byte-identical to `697cb65756` (both stop on the same
pre-existing numpy-stub syntax error).
…raising
Review item 4 offered two remedies for a KV-shaped write nothing else consumes. The
first -- say honestly why the write vanished rather than blaming `min_block_size` --
is a commit earlier in this stack. This is the second, "route the dead case to
copy-back so it does not raise at all", which the previous round declared
impossible. It is not.
Why it looked impossible. Erasing the `copy_` leaves such a write dead, so it is
eliminated before any engine can alias it and the cross-check fails a compile the
merge base ran (with the buffer silently never updated). Copy-back would revive it,
because its value is re-attached as a graph output -- and then the converter sees a
live KV-eligible `slice_scatter` on a network input and aliases the cache, so the
buffer is both copy-back and engine-aliased and the module raises `IndexError` on
every call. Measured, with the copy-back cross-check disabled so the compile could
get that far:
```
copyback=['cache'] aliased_io={'output1': ('buf_cache', 'kv_cache_update')}
graph output arity=2
CALL RAISED IndexError: index 1 is out of bounds for dimension 0 with size 1
```
That is real, and it is why routing to copy-back and nothing else does not work.
What was wrong was the conclusion drawn from it -- that nothing can say "live, but
do not alias" between classification and conversion. `ConversionContext` already
carries `current_node` for exactly this: "annotations set by lowering passes".
So: `lift_mutated_buffers` marks such a write `node.meta["_trt_no_kv_alias"]` when
it re-routes it, and both eligibility checks honour the mark -- `impl.slice_scatter`
off `ctx.current_node`, `_index_copy_kv_eligible` off the node it is handed. They
are separate checks, so both are needed, and the `index_copy` one is the decode
write. The key is a bare string literal at each site, as `.meta["val"]` and this
stack's other meta keys already are: a named constant would have to live in
`conversion/` and be imported into `lowering/_buffer_lifting.py`, which keeps its
conversion imports function-local precisely to avoid that dependency, and a typo at
a read site is caught loudly by the copy-back cross-check.
One constraint is recorded rather than guarded: the mark goes on the write *node*
while the classification is per *buffer*, so a value node written back into two
buffers would put the two in conflict. `torch.export` rejects that shape with
`SpecViolationError` before the classifier sees it, so there is nothing reachable to
guard.
The mark is deliberately narrow: only a write the classifier knows the converter
*would* have aliased and re-routed anyway. A write that is copy-back because
nothing could alias it is left unmarked, so `assert_predicted_kv_aliased`'s
copy-back direction still has a real mis-prediction to catch. Marking every
copy-back write instead disarms it -- measured, it is what makes
`test_compile_raises_when_a_copyback_write_is_aliased` stop firing.
Surviving lowering is a risk rather than an assumption: the mark rides on
`node.meta` through every pass in `post_lowering`, and a pass that rebuilt the node
without its meta would drop it silently. `compile()` reads the marks back off the
lowered graph before conversion and raises naming the lost node, so that failure is
diagnosed at its cause rather than as a module that raises on every call.
Measured on the review's repro and on the same shape through `index_copy`, at
`min_block_size` 1 and the default 5:
```
DEAD-slice_scatter mbs=1 copyback=['cache'] aliased_io={} arity=1 matches eager
DEAD-slice_scatter mbs=5 copyback=['cache'] aliased_io={} arity=1 matches eager
DEAD-index_copy mbs=1 copyback=['k'] aliased_io={} arity=1 matches eager
DEAD-second-reader mbs=1 copyback=['cache'] aliased_io={} arity=1 matches eager
CLONE-slice_scatter mbs=1 copyback=[] aliased_io={'output1': ('buf_k','kv_cache_update')}
CLONE-index_copy mbs=1 copyback=[] aliased_io={'output1': ('buf_k','kv_cache_update')}
LIVE-unaliased mbs=1 still raises the KV cross-check, as it should
```
and the write reaches the runtime rather than merely compiling: the saved program
declares `BUFFER_MUTATION cache`, and the `.pte` write-back is observed per step
end to end. `DEAD-second-reader` is the shape that motivated the earlier message
split -- the buffer is read before the write, so the *placeholder* has a reader
while the *write* is dead -- and it now compiles too.
A dead write therefore never reaches the cross-check, so the cross-check carries no
branch explaining one. `run_decompositions` runs before `lift_mutated_buffers` and
has already
eliminated dead code, so a write that is dead is dead at classification time, where
this now catches it. Two attempts to construct the residual case -- a write live at
classification whose last consumer a later pass removes -- were eliminated before
classification (a dead consumer, and a no-op `.to()`) or rejected by export
(`torch._assert` on tensor data, `GuardOnDataDependentSymNode`).
Test Plan:
`tests/py/dynamo/lowering/test_buffer_lifting.py`: 72 passed (was 58).
`TestDeadKvWriteRouting` covers the classifier: the dead write goes to copy-back and
not to `predicted_kv_bindings`, the write node carries the mark, a live KV write and
a non-KV write do not, and the mark turns `_index_copy_kv_eligible` from True to
False on the same node. `TestNoKvAliasMarkerSurvival` covers the read-back.
`TestCompileSeam` drives it end to end at both `min_block_size` values on
`slice_scatter`, on `index_copy`, on the second-reader shape and on the
clone-shaped-*and*-dead intersection of this commit with the clone peel earlier in
the stack, plus `test_saved_program_declares_the_dead_write_mutation` and
`test_compile_reads_the_markers_back_after_lowering`, which pins where the read-back
runs as well as that it runs: `post_lowering` rewrites the module in place and hands
back the same object, so the graph the read-back sees is the same object from either
position and only the order distinguishes them.
Discrimination, each mutation applied alone and reverted:
| mutation | tests killed |
|---|---|
| drop the `not new_value.users` re-route | 8: both classifier cases, all five compile-level cases, the read-back call site |
| re-route but do not set the mark | 7: `test_the_rerouted_write_is_marked` + all six compile-level |
| `impl.slice_scatter` stops honouring the mark | 4 compile-level (`index_copy` unaffected -- separate check) |
| `_index_copy_kv_eligible` stops honouring the mark | `test_the_marker_blocks_index_copy_eligibility`, `test_compile_routes_a_dead_index_copy_write_to_copyback` |
| mark every copy-back write | `test_a_non_kv_write_is_not_marked`, `test_compile_raises_when_a_copyback_write_is_aliased` |
| mark and re-route every aliasable write | 12, including `test_a_live_kv_write_is_not_marked` |
| drop the clone peel in `_effective_cache_input` | `test_compile_routes_a_dead_cloned_cache_write_to_copyback` ("aliased them in place anyway") |
| drop the read-back call in `compile()` | `test_compile_reads_the_markers_back_after_lowering` |
| move the read-back above `post_lowering` | `test_compile_reads_the_markers_back_after_lowering` (1 failed, 71 passed) |
| read-back ignores a stripped mark | `test_stripped_marker_raises` |
| read-back ignores a replaced node | `test_replaced_node_raises` |
| read-back reports every mark lost | `test_surviving_marker_passes` + all five compile-level |
Set A: 4 failed, 285 passed -- the same four failures as at `697cb65756`. Set B: 33
passed. `tests/py/dynamo/runtime`: 1 failed (pre-existing functorch), 154 passed, 63
skipped. `black`, `ruff`, `isort`, mypy per-file and combined all identical to
`697cb65756`.
… lowering `assert_no_kv_alias_markers_survived` checked only that every marker the classifier set was still on its node. Reporting that half alone misdiagnoses the case that actually reaches it: every pass in `post_lowering` that carries a node's `meta` onto a replacement erases the original in the same block, so a carried marker leaves the recorded name missing *and* an unrecorded node holding the marker. Reported as a missing marker, the message says the meta was not carried over when it was, and predicts that the engine will alias the write when the marker's presence on the replacement is exactly what stops it. Compute both directions before raising and name which of the three states holds: the marker dropped, the marker copied onto a node the classifier chose to alias, or the node replaced with the marker travelling along. The state is the whole message -- what each one costs at runtime is a comment above the raise, matching the sibling cross-check. The empty-expected early return goes too, so a marker appearing where none was recorded is caught rather than skipped in the common case. Test Plan: `tests/py/dynamo/lowering/test_buffer_lifting.py`: 74 passed. `TestNoKvAliasMarkerSurvival` covers all three states; each fails when its own branch is removed, and `test_replacement_carrying_the_marker_raises` fails when the both-sets branch is dropped and the case falls back to the missing-marker wording. Set A: 4 failed, 287 passed -- the four known pre-existing failures, no fifth.
|
Changes since 1. Copy-back classification that the engine aliases anywayBoth suggestions are in. The cross-check is bidirectional. A buffer in The clone is peeled. On the item-1 repro at The guard has tests. The item-1 repro used 2.
|
…T delegate (#4459) Co-authored-by: Anthony Shoumikhin <shoumikhin@meta.com> Co-authored-by: guac e2e <guac@localhost> Co-authored-by: x <x>
Why this PR is needed
Caller-owned KV cache support (#4445) lets a mutable buffer live above the delegate and be updated in place by the engine. It handles this by lifting each mutated buffer to a delegate input and erasing the trailing
copy_, relying on TensorRT'sIKVCacheUpdateLayeraliasing to write the new value back to the caller-owned storage (zero-copy).That assumption only holds for KV-cache writes. The
slice_scatterandindex_copyconverters have a fast path that emits anIKVCacheUpdateLayerwhose output is aliased in-place to the cache input. Any other in-place mutable buffer has no such aliasing — for example theconv_state/recurrent_statering-buffers of a Gated DeltaNet (GDN) layer. For those, erasing thecopy_dropped the write-back entirely: the update became dead code and was eliminated, so the engine received fewer args than expected at runtime and the buffer never updated (silently wrong output).The fix
Distinguish the two kinds of mutation in
lift_mutated_buffers:slice_scatter/index_copy) keep the existing zero-copy aliasing path — thecopy_is erased and the write-back is handled by the engine'sIKVCacheUpdateLayer.BUFFER_MUTATIONof its caller-owned buffer, so ExecuTorch copies it back after the delegate runs.This uses the standard mutable-buffer representation rather than the engine-enforced aliased-I/O path (which is reserved for zero-copy KV aliasing).
Classification reuses the converters' own eligibility predicates rather than the op target alone, so a
slice_scatter/index_copythe converter cannot turn into anIKVCacheUpdateLayerfalls to copy-back instead of being dropped.assert_predicted_kv_aliasedthen cross-checks the classification against the engines that were actually built, in both directions: every write predicted as KV must appear in an engine'saliased_io, and no write filed copy-back may appear there. The first direction catches a silently lost write-back. The second catches a module that raises on every call, because the runtime treats an aliased output as an engine side effect and does not return it, while the surrounding graph still reads the trailing output copy-back added.One derivation for
slice_scattereligibilityA predicate is only as good as its arguments: deriving
_kv_eligible's inputs independently on the two sides mis-predicts as effectively as a different predicate would.resolve_slice_scatter_writeindynamo/conversion/impl/slice_scatter.pyis the single derivation both sides call — the converter derives its arguments from the TRT tensors, the predictor from the fx node, and both go through this helper. It fills in the op's defaults, counts negative indices from the end, and reports which of the converter's early exits a write hits through aKVWriteStatus:FULL_OVERWRITE(the converter returns the source and emits no KV layer),DYNAMIC_BOUNDS(a non-intbound, which the converter raisesNotImplementedErroron),BAD_DIM(adimthat is not a Pythonintor does not index the cache, which raisesIndexError), orOK. OnlyOKreaches_kv_eligible, withupdate_lentaken asend - start. Every other status is a case in which the converter returns or raises before it can emit an aliasing layer, so none of them can be predicted to alias: a full overwrite keeps its copy-back, and the two raising statuses abort the compile, so nothing is copied back at all._kv_eligibleitself does not takestep. The shared derivation readsstepfor the full-overwrite shortcut, which requiresstep == 1, and otherwise passes it through untouched. On the converter side only the scatter fallback reads it:try_emit_kv_cache_updatetakes nostepat all and writesupdate_lenslots consecutively fromstart. So a strided write the KV fast path accepts is lowered as if it were contiguous —cache[:, :, 0:8:2, :]lands in slots 0, 1, 2, 3 rather than 0, 2, 4, 6, silently. That is confined to the KV fast path: a strided write that falls through to the scatter fallback is lowered correctly, because the fallback builds its scatter indices withstepin hand, andtest_fallback_step_twodrives exactly that shape and compares against eager. The KV-path miscompile behaves the same way onmain, and this change neither introduces nor fixes it; theOKreturn carries a comment recording it as known and unfixed and scoping it to the KV path, so the next reader is not sent at a fallback that is correct.test_step_is_returned_unchangedpins only what the shared derivation owes its two callers — thatstepcomes back as it went in — so freezing the derivation does not freeze the routing built on top of it.Two converter behaviours differ from
main, because thedimcheck moved into the shared helper so that predictor and converter apply one rule todim: an out-of-rangedimraisesIndexErrorfrom the converter instead of surfacing as a TensorRT message out ofinput.shape[dim], anddimmust be a Pythonint, so anumpy.int64dimis read as a bad dim where onmainit works.The cache has to reach the converter as a network input
emit_kv_cache_update_layeraliases the cache only when the argument it is handed is a direct network input — anything else has no input binding name, and the converter falls back to a plain scatter. After lifting, a mutated buffer is a placeholder, so the ordinary write-only shape satisfies that directly. Aclonein between does not, and that is the shape a forward takes when it also reads the post-write cache: the write's cache argument is acall_function, butremove_input_alias_fixing_clones— an ATEN lowering pass insidepost_lowering— then erases the clone, and the converter does see a network input after all._effective_cache_inputtherefore predicts what the converter will be handed rather than reading what is in the graph at classification time, reproducing that pass's own condition exactly: the clone is peeled only when its input is a placeholder and that placeholder's sole user is the clone. A placeholder some other node also reads keeps its clone, the converter sees acall_function, and nothing aliases. For the same reason classification runs after the trailingcopy_is erased — thecopy_is the last reader of the buffer placeholder in the write-only shape, so whether it is still there decides whether the clone survives. The peel has to reach both write ops:_index_copy_kv_eligibleapplies a placeholder check of its own, so it takes aninput_nodekeyword the classifier passes and the partitioner does not; running as a capability validator, after lowering has settled what the input is, it readsnode.args[0]itself.Predicting this wrong in the copy-back direction is worse than a lost write-back: the write is filed copy-back, its new value re-attached as a trailing graph output, the engine aliases the cache anyway, and every call raises. That is what the second direction of the cross-check exists for.
remove_input_alias_fixing_clonescarries a note back to the classifier, so that loosening the pass's condition, or deleting the pass as its own TODO invites, is done knowing a prediction depends on it.A KV write nothing else reads
Erasing the trailing
copy_can leave a KV-eligible write with no consumer at all — a forward that updates a cache and returns something unrelated. Dead-code elimination then takes the write before any engine can see it, so a prediction of engine aliasing could never be fulfilled. Such a write is routed to copy-back instead: re-attaching its new value as a graph output makes it live again, so the model compiles at the defaultmin_block_sizeand the ExecuTorch runtime applies the buffer, rather than the compile failing on a prediction nothing could fulfil.Copy-back and engine aliasing are mutually exclusive for one buffer, so a re-routed write also has to be kept out of
aliased_io. It carriesnode.meta["_trt_no_kv_alias"], and both eligibility checks honour it:impl.slice_scatterreads it offctx.current_node, and_index_copy_kv_eligibleoff the node it is handed. The key is a bare string literal at every site, which is how every other custom meta key indynamo/is written —_fp8_softmax_scale, set by a lowering pass and read offctx.current_nodein a converter, is the same shape — and it keeps_buffer_lifting.pyfree of a module-level import fromconversion/.The marker is deliberately narrow. It goes only on a write the classifier knows the converter would otherwise have aliased and whose result nothing consumes, never on copy-back writes at large: marking every copy-back write would leave the copy-back direction of the cross-check nothing to catch, and catching a genuine mis-prediction there is the whole point of it. The marker rides on
node.metathrough every pass inpost_lowering, where a pass that rebuilds a node without its meta would drop it silently, soassert_no_kv_alias_markers_survivedreconciles the names recorded before lowering against the lowered graph before conversion. That reconciliation runs in both directions and reports which of three faults it found: a recorded name that kept its node but lost the marker, an unrecorded node that gained one, or a recorded name gone with an unrecorded node carrying the marker instead.The marker sits on the write node while the classification is per buffer, so one value node written back into two buffers would put the two in conflict.
torch.exportrejects that shape withSpecViolationErrorbefore the classifier sees it, which makes it a constraint on what can arrive rather than a case to guard against.Two further cases
get_attrtarget is fully qualified (layers.0.self_attn.kv_cache.k_cache), andhasattr/getattrdo not walk a dotted path, so nested caches were silently skipped and frozen as constants.lift_mutated_buffersresolves through submodules withget_buffer;register_bufferrejects dots, so the buffer is renamed to a flat attribute (lifted_buf_*) and the recorded copy-back targets are remapped through the same renaming (otherwise the verifier rejects the program).settings.torch_executed_opsnever reaches a converter, so it cannot emit anIKVCacheUpdateLayerand the engine will not alias that buffer. The classifier honours the exclusion list (matched the way the partitioner matches it, so the two cannot disagree) and leaves such a write on the copy-back path — which is what lets a program split across the TensorRT and CUDA delegates export.How it works
The classification happens once, in
lift_mutated_buffers, and is threaded to both save paths:dynamo/lowering/_buffer_lifting.py— for each lifted buffer, a KV write is left to aliasing and its input-binding name recorded ingm.meta["_predicted_kv_bindings"]; a non-KV write has its new value appended as a trailing graph output (so it survives DCE now that thecopy_is gone), its buffer name recorded in output order ingm.meta["_copyback_mutation_buffers"], and its input-binding name recorded ingm.meta["_copyback_bindings"]for the other direction of the cross-check. A KV-eligible write re-routed for want of a consumer is marked, and the marked node names recorded ingm.meta["_no_kv_alias_writes"]. Nested buffers are resolved throughget_bufferand renamed; excluded writes are left on the copy-back path.dynamo/_compiler.py— reads the markers back against the lowered graph (assert_no_kv_alias_markers_survived) afterpost_loweringand before conversion, forwards the copy-back list onto the compiled module's meta so it reaches the exporters, runsassert_predicted_kv_aliasedagainst thealiased_ioof the compiled submodules with both binding sets, and hides the copy-back values from what the compiled module returns (hide_copyback_outputs).dynamo/_exporter.pycreate_trt_exp_program(retrace=False, legacy exporter): tags the trailing outputs with their buffer target and moves all mutation outputs ahead of the user outputs (verifier requirement). A write-only buffer has no survivingget_attrafter DCE andliftderives BUFFER input specs fromget_attrnodes alone, so an unused one is re-added for it._declare_aliased_kv_mutations_on_ep(run on the resultingExportedProgramby everysave()branch that serializes a signature, and bytorch_tensorrt.executorch.export()for every source shape it accepts): detaches the full trailing run of copy-back values, pairs each positionally with its buffer (incopyback_buffersorder), and declares asBUFFER_MUTATIONonly the bufferstorch.exportdid not already declare itself — skipping the rest — then rebuilds the top-levelout_specsoto_edge's unflatten sees the right leaf count. Pairing across the full run before filtering is deliberate: dropping already-declared buffers first would shift the run and pair a value with the wrong buffer, which the runtime would then copy into a buffer of a different shape.executorch/_export.py—torch_tensorrt.executorch.export()runs the declaration pass overprogram_maponce_prepare_programshas normalized all three accepted source shapes into it, threading each method's_copyback_mutation_buffersfrom the source that method came from. It runs on the staged copy rather than the caller's program, so a caller who hands in their ownExportedProgramkeeps it usable afterwards.The KV half of
_declare_aliased_kv_mutations_on_epneeds the optional[executorch]extra to read an engine's binding names; on a plain install that import fails, the engine scan is skipped, and the copy-back half still runs.What
compile()returnsA copy-back value is only ever consumed by whatever serializes the module. Nothing between
compile()and the ExecuTorch runtime writes it anywhere, so a compiled module called directly from PyTorch leaves its buffer at the value it was compiled with, exactly as onmain. Handing that value back as an extra return value would report a mutation the caller cannot act on, sohide_copyback_outputsinstalls aCodeGenwhose generatedforwardstops short of the trailing copy-back values. The compiled module keeps the arity of the model it was compiled from: a non-KV mutable buffer does not turn one return value into two.Only the generated
forwardstops early. The values stay on the graph's output node, so everything that reads the graph rather than calling the module still finds them — the exporters, dead-code elimination, and thetorch.fx.Interpreterthat a non-stricttorch.exportruns a GraphModule through, which is why a retrace keeps them too. Non-strict is what both retrace paths get:_compile.savepassesstrict=Falseanddynamo._exporter.exporttakes the default. A caller who exports the compiled module themselves withstrict=Truegets the module called rather than interpreted, and the hidden values do not reach the program; the_HiddenCopybackOutputsdocstring records that at the point of decision._TorchTensorRTModule.forwarddoes the same with the aliased outputs the interpreter appends: the engine produces them, the caller never sees them.Where copy-back is declared
exported_programexecutorchaot_inductorExportedProgramGraphModule,retrace=TrueGraphModule,retrace=FalseGraphModule,retrace=Falseuse_legacy_exporter=Falsetorch_tensorrt.executorch.export()declares for every source shape it accepts, andsave(output_format="executorch")reaches it on every branch, so that column is uniform.save(output_format="exported_program")declares only the KV half of an already-exported program, since no copy-back buffer list is threaded on that branch;save()is therefore not uniform across its two serializing formats.aot_inductoris left undeclared on thetorch.exportpaths — whether an aliased in-place mutation survives functionalization under inductor is unverified — matching #4445's KV scoping. It warns when the module carries engines with aliased I/O, and, underretrace=Falsewithuse_legacy_exporter=False, when a copy-back buffer would go undeclared.Among the
GraphModulepaths,retrace=Falsewithuse_legacy_exporter=Falseandoutput_format="exported_program"is the one combination that leaves a copy-back mutation undeclared: on theretrace=Falsebranch the legacy exporter is what declares copy-back, andtorch_tensorrt.executorch.export()covers theexecutorchformat, so this combination falls between the two. It warns rather than silently saving a signature that omits the update.Idempotency
The declaration pass is safe to call more than once, by two separate mechanisms. The KV half skips any buffer already carrying a
BUFFER_MUTATIONspec, which is also how the legacy exporter's transform-time declaration survives a later run. The copy-back half cannot skip one buffer at a time, because it detaches its outputs by position rather than by name, and per-buffer state cannot tell the two "already declared" cases apart:torch.exportdeclares a mutation it recognises and leaves the trailing value, while a previous run of this pass declares it and consumes the value. So the consuming step recordsgm.meta["_copyback_mutations_declared"]on the GraphModule and the slice is skipped when it is set.create_trt_exp_programsets the same marker, since it declares copy-back itself at transform time. Without the marker a second run slices whatever trails the graph — by then a genuine user output — and the saved program fails to load withtreespec.unflatten ... leaves has length 0.Uninitialized copy-back buffers
Declaring a write as an ExecuTorch
BUFFER_MUTATIONputs the buffer under ExecuTorch's rule that a mutated buffer has a meaningless initial state: only its shape and dtype are serialized. A buffer read before it is written — a GDNconv_stateorrecurrent_state, an EMA, a running statistic — therefore starts from whatever the allocation held. ExecuTorch warns about this itself, but generically, naming no buffer.torch_tensorrt.executorch.export()names the copy-back buffers and states the rule they fall under, with the condition attached rather than stated flat, since a caller who has already supplied the pass below does have the value in the.pte: they "are declared mutated, so ExecuTorch serializes only their shape and dtype, not their value, unless a pass marks them meta["et_init_buffer"]; without that, a buffer read before it is written starts from whatever the allocation held."The remedy is named, but only together with the one thing upstream does not say about it. ExecuTorch's own way to put an initial value back is
InitializedMutableBufferPass, which setset_init_bufferon the placeholders its patterns match; the emitter then marks the spec const and serializes the buffer by reading its storage host-side throughctypes.cast(spec.storage.data_ptr(), ...). Whether that read is safe is decided by where the buffer lives: it works while the buffer is on CPU, and it takes the export down with a segfault, rather than raising, once the buffer is CUDA-resident — which is what exporting a model on CUDA leaves behind, and exporting on CUDA is the ordinary way to reach a TensorRT engine. ExecuTorch's emitter names the pass to the same user later in the same export and says nothing about where the buffer has to live, so withholding the name here would withhold the caveat and not the advice. The warning carries both, and the test that covers it requires the two to travel together.Engine-aliased KV buffers lose their serialized initial value the same way and are deliberately excluded from the warning. An aliased write is a cache-position write on the sequence axis — the only form
IKVCacheUpdateLayerexpresses — and such a model is assumed to read the cache under that same position, through a bounded slice or a position mask, so slots no step has written should not reach the output. Nothing verifies that; it is a property of the model. On that assumption initializing a cache buys nothing and would cost what is typically the model's largest tensor, written whole into the.pte.What copy-back costs
Only an engine-aliased (KV) write is free. A
BUFFER_MUTATIONoutput is defined as the buffer's new contents rather than the slice that changed, so a copy-back write re-attaches the whole post-write buffer as a graph output. Each call therefore pays, per copy-back buffer, a full-buffer materialization of that output plus a full-buffer copy from ExecuTorch's own write-back pass after the delegate returns. For a decode step writing one position into a multi-megabyte KV cache, that easily dominates the step. It is the right correctness tradeoff — the alternative is silently losing the write — but it is why the classifier routes a cache write to engine aliasing instead, and why a model that unexpectedly lands in copy-back comes out slow rather than wrong. This is recorded in the_buffer_lifting.pymodule docstring, withgm.meta['_copyback_mutation_buffers']named as the way to find out which buffers are paying it.The engine-converter entry point
convert_exported_program_to_serialized_trt_engine(..., lift_mutable_buffers=True)hands the caller raw engine bindings and performs no write-back of its own. A copy-back write there would append an extra engine output that the entry point reports nothing about, so the buffer would come back stale; it is rejected right after lifting, with the offending buffers named and the working route pointed at:torch_tensorrt.dynamo.compileand saving the result, since it is the save that declares the buffer mutation for the runtime to apply —compile()on its own performs no write-back either. The cross-check runs there too, againstinterpreter_result.aliased_io.Diagnosing a failed cross-check
assert_predicted_kv_aliasedtakes the aliased-input binding set rather than a module, because the two callers hold ground truth in different shapes —compile()reads it off the compiled submodules, the engine converter off the interpreter result — andaliased_input_bindingsassembles it for both. Both directions are keyed on thebuf_*input-binding name, which is stable across the buffer rename that inlining does later and is exactly whataliased_iorecords on the input side.Classification runs before lowering and partitioning, so an unfulfilled KV prediction means something in between kept the write out of the engine it was predicted into. The error names the buffers and then names a likely cause, citing
min_block_sizeonly where that setting can be the reason: above 1 it reports the value in force and thatmin_block_size=1rules the setting out, and at 1 it points at a converter or a capability validator rejecting the op instead, since naming the setting there would blame the value it would have to recommend.Under
compile(dryrun=True)no engine is built, soaliased_iois empty for reasons that say nothing about the predictions and every one of them would look unfulfilled. The skip is keyed on an explicitengines_builtkeyword rather than onsettings.dryrun, because the two entry points do not mean the same thing by that flag:convert_exported_program_to_serialized_trt_engineacceptsdryrun, puts it inCompilationSettingsand then never consults it, so an engine comes out regardless. Readingdryruninside the shared helper would hand that entry point an opt-out kwarg for a check it is meant to have none for. Onlycompile()passesengines_built=False, and only where it returns before conversion.Reading engine metadata without re-serializing the engine
__getstate__on a TensorRT engine is the pickling hook, not a metadata accessor: it re-serializes the wholeICudaEngineand base64-encodes it.get_engine_info_from_state(..., metadata_only=True)takes the record fromserialize_metadata_onlyinstead, and_get_engine_info_for_nodeforwards the flag. Two callers read metadata only and opt in. The aliased-mutation declaration pass readsALIASED_IO,INPUT_BINDING_NAMESandOUTPUT_BINDING_NAMESand runs its engine loop before it can know whether any engine has aliased I/O at all, so a program with no aliased KV paid a full serialization per engine node and then returned unchanged — measured at 0.4 s and a 67 MB transient string for one ~50 MB engine.TensorRTPartitioner._resolve_target_device_for_partitionreads a single field,DEVICE_IDX, once per partition, on every export that does not pintarget_device, which is the default: 967.6 ms against 3.1 ms on a ~100 MB engine, plus a 134 MB base64 string built and dropped. The accessor's caveat thatENGINE_IDXis unreliable undermetadata_onlyapplies to neither, since neither reads that slot.Scope
The classification is a no-op for any model with no non-KV in-place mutable buffer and no KV write whose result nothing consumes — including all standard models, and any KV-cache model whose writes the converter turns into
IKVCacheUpdateLayers. Thedimhandling above is the one converter behaviour that differs for programs with no mutable buffer at all.Testing
Unit tests, CPU-only unless noted, and gated on
executorch.exirwhere they touch it:tests/py/dynamo/lowering/test_buffer_lifting.py— classification: KVslice_scatter/index_copywrites stay aliased with no copy-back; a non-KV mutation is recorded and its new value re-attached as the trailing output (verified numerically equal to the updated buffer);index_putfalls to copy-back; a mixed KV + non-KV graph records only the non-KV buffer; an ineligibleindex_copy(2-D cache) and an ineligibleslice_scatter(wrong dim) fall to copy-back rather than being dropped; a write excluded throughtorch_executed_opsis left on the copy-back path; nested buffers are lifted, and inlining remaps the recorded copy-back targets to their flattened attribute names.TestSliceScatterDerivationIsSharedpins the corners where an independent derivation would part company with the converter — full overwrite, open-endedend == INT64_MAX, negativestart, non-intstart— plus eachKVWriteStatus, the bounds its contract promises, andstepcoming back as it went in.TestCacheMustReachTheConverterAsANetworkInputcovers what the converter will be handed: a direct placeholder and a sole-use clone of one classify as KV, forslice_scatterand forindex_copy, while a shared clone, a clone of a non-placeholder, a cache read through another op and a cache still read from aget_attrdo not — and theindex_copyvalidator still readsnode.args[0]when nobody overrides it, so the partitioner's view of the same node is unchanged.TestDeadKvWriteRoutingcovers the write nothing else reads: it is filed copy-back rather than predicted KV, the re-routed write carries the marker, a live KV write and a non-KV write do not, and the marker turns_index_copy_kv_eligiblefrom true to false on the same node.TestNoKvAliasMarkerSurvivaldrives the reconciliation: a surviving marker passes, while a stripped marker, a marker copied onto an unmarked node, a marker appearing where none was recorded, and a marked node replaced by another carrying the marker each raise.TestPredictedKvAssertioncovers the backstop in both directions: it passes when a predicted-KV write is aliased and raises when it is not, raises when a copy-back write is aliased and passes when it is not, aggregatesaliased_ioacross multiple engines, no-ops with no prediction, skips underengines_built=Falsefor either direction, does not skip ondryrunalone, and namesmin_block_sizeonly where that setting can be the cause.TestHiddenCopybackOutputspins that hiding a value leaves the graph's output node untouched, that hiding nothing is a no-op, and that a graph consumer still sees the hidden value.TestCompileSeam(CUDA) drivescompile()end to end and pins the wire between the two halves: the copy-back list reachingtrt_gm.metaand still resolving to a live buffer, the cross-check being called with the predictions lifting made, each direction of it failing a compile,compile()telling the check that a dryrun built no engines, a clonedslice_scatterand a clonedindex_copycache write each compiling and writing back, a deadslice_scatterwrite compiling with an emptyaliased_ioatmin_block_size1 and at the default 5 and its mutation arriving in a saved program, the same for a deadindex_copywrite, for a dead cloned write where both mechanisms meet, and for a dead write whose buffer still has another reader, the marker read-back running on the lowered graph rather than before lowering, and the copy-back value staying out of the compiled module's return — beside an engine-aliased KV buffer in the same module, and while a retrace and a saved program still find it.tests/py/dynamo/executorch/test_kv_cache_export.py— both exporter passes reclassify a trailing copy-back output toBUFFER_MUTATIONahead of the user outputs; a buffertorch.exportalready declared is skipped rather than declared twice, and with a mix of declared and undeclared buffers each remaining copy-back value is paired with the buffer it was appended for; a second run of the pass is a no-op; the copy-back half still runs when the[executorch]extra is unimportable; the declaration pass reads engine metadata only; a write-only copy-back buffer still reachesliftas aget_attr(driving the realliftandExportedProgramconstructor so a regression reproduces the verifier error rather than passing vacuously), and the re-addedget_attrneither duplicates an existing one nor invents a dangling one; a saved copy-back program reloads, returns exactly the outputs the source module returned, and updates its buffer;torch_tensorrt.executorch.export()declares copy-back for each source shape it accepts (both GraphModule retrace modes, an ExportedProgram, and a method mapping) and leaves the caller's own program intact; everysave()branch that serializes a signature reaches the declaration pass, swept over module type by serializing format;save()warns on exactly the one combination that cannot declare; and the engine-converter entry point rejects a copy-back buffer by name, matched in its rendered form so a substring of an unrelated word cannot satisfy the assertion.tests/py/dynamo/executorch/test_export.py— the uninitialized-buffer warning names the buffer, states what ExecuTorch keeps of it and under what condition, and namesInitializedMutableBufferPassonly in a message that also carries the CUDA caveat; an export with nothing to copy back stays silent.tests/py/dynamo/conversion/test_slice_scatter_aten.py—TestSliceScatterFallbackdrives the scatter fallback against eager across ranks and dims, including astep=2write and a dynamic-shape case;TestSliceScatterEarlyExitsdrives the converter's raising exits: dynamic bounds, and both forms of baddim.tests/py/dynamo/executorch/test_api.py— the partitioner asks for the engine record withmetadata_only=True.The copy-back path was also exercised outside the test suite, by running
.ptes on a real ExecuTorch runner: a synthetic copy-back model; an L=2 Gemma4 MoE with an added non-KV buffer, which puts copy-back and engine-aliased KV in one hybrid TensorRT + CUDA model; a Qwen3.5 GDN decode whose four per-layerconv_*/rec_*state buffers are all copy-back and whose recurrent state has no KV aliasing anywhere; and a clone / cache-position-write / write-back decode step. In each, a step observes the previous step's write. Those runs are not part of this PR's automated tests.Stacking
Not stacked. #4446 (retrace=False legacy-exporter fix) and #4445 (caller-owned KV cache) have both landed, and this branch is based on
mainabove them.Follow-ups
output_format="executorch"now lives intorch_tensorrt.executorch.export(), where it runs per method. Theoutput_format="exported_program"branches still declare fromsave()in_compile.py, andsave(exported_program, output_format="exported_program")still declares only the KV half; makingsave()uniform across its two serializing formats is a follow-up._write_op_is_torch_executedand the dead-write re-route are the two reasons a write can miss an engine that the classifier can see coming, and both file the write as copy-back up front. The others —min_block_size, a missing converter, a failed capability validator — are not knowable before partitioning, so the cross-check catches them after it instead. Making the prediction non-destructive, by always keeping the copy-back output and dropping it after conversion oncealiased_iohas settled which buffers the engine aliased, would remove the need for the cross-check entirely; that is a larger change and belongs in its own PR.