You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
[TRTLLM-17659][refactor] Pin down the encoder-to-decoder handoff in PyExecutor - #21768
EncoderStageConfig is removed. EncoderStage and EncoderDecoderRunner now accept EncoderConfig, and ModelEngine creates it directly. Verify external callers do not depend on the removed class.
_do_terminate_request now clears an unconsumed py_encoder_output and its ready event. This releases the request’s reference to packed encoder storage when termination occurs before the first context step.
The handoff comments describe submission, publication, admission, consumption, recomputation, and failure handling. LlmRequest comments clarify the handoff fields; they do not change runtime behavior.
No configuration or integration test-list files changed. Current-head build, test, and CI results are not available in the inspected diff.
QA Engineer Review
test_py_executor.py adds tests for token and feature inputs, async and synchronous execution, TP1 and TP2, cancellation, launch and publication failures, recomputation, cross-KV reservation, and packed-storage release.
test_encoder.py updates its helper to use EncoderConfig for encoder-decoder cases.
No integration tests or test-list files changed. The changed tests are unit tests, so integration-list membership does not apply.
Coverage verdict: sufficient for the changed handoff, failure, cancellation, recomputation, and termination paths. Test results are not available.
Per-File QA Perspective
tensorrt_llm/_torch/pyexecutor/engine/runners/encoder_decoder.py: Verify callers pass EncoderConfig now that EncoderStageConfig has been removed.
tensorrt_llm/_torch/pyexecutor/llm_request.py: Comments clarify handoff-state timing and use. No runtime behavior changed.
tensorrt_llm/_torch/pyexecutor/model_engine.py: Verify encoder-decoder runner construction creates EncoderConfig.
tensorrt_llm/_torch/pyexecutor/py_executor.py: Verify output publication, stream attachment, cross-KV consumption, and termination cleanup across the handoff lifecycle.
tests/unittest/_torch/pyexecutor/engine/runners/test_encoder.py: Updates encoder-decoder test setup to use EncoderConfig. This unit test does not require integration-list membership.
tests/unittest/_torch/pyexecutor/test_py_executor.py: Covers handoff lifecycle, cancellation, failures, recomputation, cross-KV behavior, and packed-storage release. This unit test does not require integration-list membership.
Description
Encoder-decoder models (BART, T5, Whisper) hand the encoder output to the first decoder context step through the request state and three request fields, which several PyExecutor methods set and clear. This PR writes that handoff down and pins it with tests: submit, publish, readiness, the single cross-KV projection, recompute, and the transitions around them (cancellation before submission, while the encoder step is pending and after publish; encoder failure; late publish). The lifecycle is the block comment above PyExecutor's encoder methods and the field comment in LlmRequest.
The review found one gap, fixed here: a request terminated after its encoder output was published but before its first context step kept its view of the packed output, and with it the storage of its whole encoder batch. _do_terminate_request now drops an output that was never consumed, next to the existing multimodal release. Nothing else changes behaviour; no new state or record is introduced.
A separate commit removes EncoderStageConfig, an empty subclass of EncoderConfig, so EncoderStage and EncoderDecoderRunner take EncoderConfig directly. It overlapped #21428, which removed EncoderRunnerConfig the same way; this branch is rebased onto it.
Test Coverage
tests/unittest/_torch/pyexecutor/test_py_executor.py: the handoff tests next to the existing encoder-decoder cases — each publish path, each cancellation point, encoder failure, recompute through V1 pause and V2 reset, and the release of an unconsumed output on termination.
BART, T5 and Whisper end-to-end tests in l0_h100, run on H100; a forced-recompute check on T5-small and BART-large-cnn (greedy, fp32 and bf16) gave token-identical outputs for the recomputed requests.
Known: test_bart_pytorch_continuous_admission_replays_encoder_and_mixed_cuda_graphs[tp1] failed once (all-zero tokens on the streaming request) while running the whole list; it passed on every isolated rerun, and the change in this PR is a no-op in that test. The t5-large/flan-t5-{base,large,xxl} cases were not run for lack of weights on the test cluster.
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
If PR introduces API changes, an appropriate PR label is added - either api-compatible or api-breaking. For api-breaking, include BREAKING in the PR title.
Any new dependencies have been scanned for license and vulnerabilities
NVIDIA/TensorRT-LLM#20996: It changes the inheritance of EncoderConfig and EncoderStageConfig, which this PR consolidates.
NVIDIA/TensorRT-LLM#20270: It changes the EncoderConfig factory contract used by this PR's encoder configuration creation.
NVIDIA/TensorRT-LLM#21428: It removes EncoderRunnerConfig in favor of EncoderConfig, a related configuration consolidation.
Merge Risk: Minimal · up to df96e
The change releases unconsumed encoder output on termination. No code-related merge blocker was established; the reported CI failure should be resolved before merging.
Security architecture risk: Low · up to df96e
The change releases encoder output that cancelled or failed requests no longer need. The inspected paths preserve request ownership and readiness controls, with no demonstrated expansion of access or privilege. Confidence remains limited by incomplete security coverage and the absence of live GPU validation.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
inferred — The demonstrated exposure is shared GPU capacity within an executor: one unconsumed request view can retain its encoder batch's packed storage. The PR reduces that retention after termination; the inspected change does not establish a new cross-tenant data-access or privilege path.
Trust Boundaries and Controls
observed — Request state is the publication ownership guard. Scatter advances offsets for every original batch member but assigns output only to requests still in ENCODER_INIT, preventing a late completion from reviving a terminal request or shifting a sibling's slice.
Resilience and Maintainability Implications
inferred — The inspected cancellation, late-publication and encoder-failure paths preserve terminal request ownership. Normal consumption and terminal cleanup both release temporary views; repeating the added cleanup after the view is gone performs no further handoff mutation.
Pre-merge checks: 4 · 1
Failed checks: 1 inconclusive
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Title check
The title is concise and clearly identifies the encoder-to-decoder handoff as the main change. It also uses a specific refactor tag.
Description check
The description explains the problem, the fix, and the related configuration refactor. It includes detailed test coverage, known test results, and the repository checklist.
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
Explanation
Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 8 functions across 5 files. (1 skipped: 1 too large.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Reviewing files that changed from the base of the PR and between 91cc8fe and df96e84.
Files selected for processing: 2
tensorrt_llm/_torch/pyexecutor/model_engine.py
tensorrt_llm/_torch/pyexecutor/py_executor.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
…terminates
A request terminated before its first context kept its view of the packed encoder output, and with it the whole batch's storage.
Signed-off-by: Lori Ren <lorir@nvidia.com>
Automatically added "ci: full pre-merge approved" because this PR has satisfied the required GitHub review approvals. Unresolved review conversations and other required checks remain independent merge requirements.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Dev Engineer Review
EncoderStageConfigis removed.EncoderStageandEncoderDecoderRunnernow acceptEncoderConfig, andModelEnginecreates it directly. Verify external callers do not depend on the removed class._do_terminate_requestnow clears an unconsumedpy_encoder_outputand its ready event. This releases the request’s reference to packed encoder storage when termination occurs before the first context step.LlmRequestcomments clarify the handoff fields; they do not change runtime behavior.QA Engineer Review
test_py_executor.pyadds tests for token and feature inputs, async and synchronous execution, TP1 and TP2, cancellation, launch and publication failures, recomputation, cross-KV reservation, and packed-storage release.test_encoder.pyupdates its helper to useEncoderConfigfor encoder-decoder cases.Per-File QA Perspective
tensorrt_llm/_torch/pyexecutor/engine/runners/encoder_decoder.py: Verify callers passEncoderConfignow thatEncoderStageConfighas been removed.tensorrt_llm/_torch/pyexecutor/llm_request.py: Comments clarify handoff-state timing and use. No runtime behavior changed.tensorrt_llm/_torch/pyexecutor/model_engine.py: Verify encoder-decoder runner construction createsEncoderConfig.tensorrt_llm/_torch/pyexecutor/py_executor.py: Verify output publication, stream attachment, cross-KV consumption, and termination cleanup across the handoff lifecycle.tests/unittest/_torch/pyexecutor/engine/runners/test_encoder.py: Updates encoder-decoder test setup to useEncoderConfig. This unit test does not require integration-list membership.tests/unittest/_torch/pyexecutor/test_py_executor.py: Covers handoff lifecycle, cancellation, failures, recomputation, cross-KV behavior, and packed-storage release. This unit test does not require integration-list membership.Description
Encoder-decoder models (BART, T5, Whisper) hand the encoder output to the first decoder context step through the request state and three request fields, which several
PyExecutormethods set and clear. This PR writes that handoff down and pins it with tests: submit, publish, readiness, the single cross-KV projection, recompute, and the transitions around them (cancellation before submission, while the encoder step is pending and after publish; encoder failure; late publish). The lifecycle is the block comment abovePyExecutor's encoder methods and the field comment inLlmRequest.The review found one gap, fixed here: a request terminated after its encoder output was published but before its first context step kept its view of the packed output, and with it the storage of its whole encoder batch.
_do_terminate_requestnow drops an output that was never consumed, next to the existing multimodal release. Nothing else changes behaviour; no new state or record is introduced.A separate commit removes
EncoderStageConfig, an empty subclass ofEncoderConfig, soEncoderStageandEncoderDecoderRunnertakeEncoderConfigdirectly. It overlapped #21428, which removedEncoderRunnerConfigthe same way; this branch is rebased onto it.Test Coverage
tests/unittest/_torch/pyexecutor/test_py_executor.py: the handoff tests next to the existing encoder-decoder cases — each publish path, each cancellation point, encoder failure, recompute through V1 pause and V2 reset, and the release of an unconsumed output on termination.tests/unittest/_torch/pyexecutor/engine/runners/test_encoder_decoder.py,engine/runners/test_encoder.py,kv_cache/test_kv_cache_v2_scheduler.py,kv_cache/test_dual_pool_kv_cache.py.l0_h100, run on H100; a forced-recompute check on T5-small and BART-large-cnn (greedy, fp32 and bf16) gave token-identical outputs for the recomputed requests.Known:
test_bart_pytorch_continuous_admission_replays_encoder_and_mixed_cuda_graphs[tp1]failed once (all-zero tokens on the streaming request) while running the whole list; it passed on every isolated rerun, and the change in this PR is a no-op in that test. Thet5-large/flan-t5-{base,large,xxl}cases were not run for lack of weights on the test cluster.PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.