[TRTLLM-15947][refactor] BREAKING: Remove C++ state left dead by the TRTLLMSampler removal by zhaoyangwang-nvidia · Pull Request #18532 · NVIDIA/TensorRT-LLM

zhaoyangwang-nvidia · 2026-09-01T13:16:00Z

Dev Engineer Review

Removes obsolete C++ and Python configuration plumbing.
Replaces runtime::SamplingConfig with executor::SamplingConfig.
Removes ExecutorConfig, obsolete speculative-decoding classes, debug utilities, and related bindings and serialization.
Updates LlmRequest, sampling scalar fields, seed names, API references, telemetry, benchmarks, and documentation.
Preserves the top-level SamplingConfig binding as an alias.
Review constructor changes, API consistency, scalar sampling values, serialization compatibility, seed handling, and removed request state.
Full build and test validation was not completed. Regression risk remains because public APIs and pickle compatibility change.

QA Engineer Review

Test code changed across C++, Python, bindings, executor, batch-manager, runtime, integration, and API-stability tests.
Added test_SamplingConfig_is_executor_alias.
Updated tests for KV-cache behavior, request serialization, scalar sampling values, worker construction, RPC proxy setup, API references, and sampling-parameter capture.
Removed tests for Medusa, ExecutorConfig, ExternalDraftTokensConfig, Eagle and speculative-decoding configuration, runtime SamplingConfig, and related serialization and pickle behavior.
No test-list files were identified in the supplied changes. CI and manual QA coverage cannot be confirmed.
Verdict: needs follow-up.

Description

Follow-up cleanup to #18232 / #18233 (TRTLLM-15405), which removed TRTLLMSampler and the C++ decoder stack behind it. That removal left a large amount of C++ state that exists only to be constructed, stored, and marshalled across the nanobind boundary — with no remaining consumer on either side.

This PR is a dead-code sweep, not a behavior change. Net ~4500 lines removed. It is split into reviewable commits, each independently justified. The first five cover the original sweep:

Remove C++ state left dead by the TRTLLMSampler removal — LlmRequest::mBadWordsList / mStopWordsList (Python already owns the authoritative copies as py_bad_words / py_stop_words_list, so the bindings only cost a torch.tensor() allocation per request on the submit path), ExternalDraftTokensConfig, SpeculativeDecodingConfig, plus runtime::SamplingConfig's batch-fusing constructor and the fields whose only consumer it was.
Move the return-logprobs flag off runtime::SamplingConfig — outputLogProbs / cumLogProbs were not sampling parameters but a single boolean stowed on the decoder's parameter struct. Backed by LlmRequest::mReturnLogProbs instead. This also fixes an uninitialized bool that was being marshalled to Python.
Delete runtime::SamplingConfig — it existed to fuse per-request scalars into optional<vector<T>> columns for the batched decoder. With that decoder gone, every reader was unwrapping the singletons again (_unwrap_singleton asserted exactly one element, and sampler_common.py carried a TODO to drop the plumbing). LlmRequest now holds an executor::SamplingConfig directly.
Remove executor config classes orphaned by the decoder removal — EagleConfig, OrchestratorConfig, ParallelConfig, DebugConfig (+ the orphaned batch_manager/utils/debugUtils.{h,cpp}), LogitsPostProcessorConfig.
Remove ExecutorConfig and its vestigial plumbing — ExecutorConfig configured executor::Executor, which no longer exists; its getters were read only by Serialization, forming a closed loop. Python never constructed one, and TorchLlmArgs.get_executor_config was already broken (it called a super() method that had been deleted).

Later commits address review feedback:

Remove Medusa config plumbing — MedusaDecodingConfig.supports_backend() rejected both surviving backends, so the public LLM-API class was unreachable. Removes the DecodingConfig member/accessors, the executor::MedusaChoices typedef, serialization, the binding, the llmapi class and trtllm-bench latency --medusa_choices.
Collapse MedusaModule / EagleModule / LookaheadModule into SpeculativeDecodingModule — all three had degenerated into shells whose only additions (default trees, getNumTransformerLayers(), getExecutionConfig(), …) lost their last caller with the decoder. gptJsonConfig constructs the base class directly with identical arguments.
Remove the Python lookahead decoding API — see below.
Assorted review fixes: a beam-width constraint message on the deprecated numReturnSequences path, tle:: alias usage, and a new test_SamplingConfig_is_executor_alias.

API-breaking changes

request.sampling_config.temperature is now 0.7 rather than [0.7]. tensorrt_llm.bindings.SamplingConfig is kept as an alias of tensorrt_llm.bindings.executor.SamplingConfig, and a copy constructor was added to the executor binding so the existing SamplingConfig(params._get_sampling_config()) spelling keeps working.
Two fields renamed to the executor spelling: min_length → min_tokens, random_seed → seed.
Pickle compatibility is broken for tensorrt_llm.bindings.SamplingConfig and for the removed config classes. Pickle tuple indices and size guards for Request, ExecutorConfig, DecodingConfig and SamplingConfig are renumbered accordingly.
ExecutorConfig is removed from the llmapi.serialization pickle allowlist and from the llm_utils re-export.
Lookahead decoding is removed from the Python API. LookaheadDecodingConfig (llmapi) already raised ValueError on both surviving backends. SamplingParams.lookahead_config was not gated that way — it could be set, silently ignored, and forwarded into a C++ field that nothing has read since [TRTLLM-15405][refactor] Remove the C++ decoder stack behind TRTLLMSampler #18233. Both are gone, along with the base_worker forwarding. The C++ executor::LookaheadDecodingConfig and its Request/DecodingConfig fields stay for now; they will be removed in stages.
The C++ serialization wire format changed. Request and DecodingConfig lose fields, and serialization.cpp carries no version tag (it never has on main), so peers built before and after this commit will mis-parse each other rather than fail cleanly. Matched builds across all ranks — in particular context and generation processes in disaggregated serving — are a hard requirement.

Notes for reviewers

Three same-name classes were deliberately kept, since a naive grep conflates them with the removed ones:

CacheState::ParallelConfig (dataTransceiverState.h) — live, used by the disagg cache transceiver.
tensorrt_llm::DebugConfig (common/assert.h) — unrelated to executor::DebugConfig, still used by assert.cpp and kv_cache_manager_v2.
EagleConfig in _torch/auto_deploy/models/custom/modeling_eagle.py — an unrelated HuggingFace-style config class.

This PR also drops tests/unittest/llmapi/test_llm_args.py::test_executor_config_consistency, a governance test asserting BaseLlmArgs mirrored every ExecutorConfig option. It guarded against C++ gaining an option the Python API forgot to expose; with ExecutorConfig gone it has nothing left to guard.

DecodingConfig and ExtendedRuntimePerfKnobConfig are intentionally not removed here: bench builds them as kwargs for LLM(), not for ExecutorConfig, so they remain reachable via llm_args.

Test Coverage

This change removes code paths rather than adding them, so it is guarded by the existing suites that exercise the touched surfaces:

tests/unittest/bindings/test_bindings_ut.py
tests/unittest/bindings/test_executor_bindings.py
tests/unittest/_torch/sampler/test_torch_sampler.py
tests/unittest/_torch/sampler/test_logits_logprobs.py
tests/unittest/_torch/sampler/test_beam_search.py
tests/unittest/_torch/executor/test_resource_manager.py
tests/unittest/_torch/executor/test_pytorch_model_engine.py
tests/unittest/_torch/speculative/test_capture_sampling_params.py
tests/unittest/executor/test_base_worker.py
tests/unittest/executor/test_rpc_proxy.py
tests/unittest/llmapi/test_llm_args.py
C++: cpp/tests/unit_tests/batch_manager/llmRequestTest.cpp, cpp/tests/unit_tests/executor/serializeUtilsTest.cpp, cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp

Not yet validated: this branch has only been syntax-checked per-file against the devel container; a full CMake build and the suites above have not been run locally. /bot run results are the first real validation — please do not merge before they are green.

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-compatible or api-breaking. For api-breaking, include BREAKING in 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.

Funatiq

defaultDecodingParams.h can also be removed now.

Its only consumer was runtime::SamplingConfig, deleted earlier in this PR. NVIDIA#18233 had moved the header from layers/ to runtime/ to empty out layers/, which kept it alive one step longer than the code that used it. Nothing else includes it and no CMake list names it; an incremental build after deleting it has no work to do. Signed-off-by: ZhaoyangWang <zhaoyangw@nvidia.com>

…decoder Both are compatibility shims for representations only the removed decoder needed, which the surviving readers immediately undid. beam_width_array: LlmRequest.get_beam_width_by_iter, PyExecutor._validate_request and _get_max_beam_width each handled a nested [[...]] form. That nesting came from runtime::SamplingConfig's one-vector-per-request storage; executor::SamplingConfig::getBeamWidthArray returns a flat OptVec<SizeType32> and always has, so the branch is unreachable now that LlmRequest holds the executor type. test_beam_width_array_max_accepts_nested_shape asserted that shape specifically and goes with it; the flat-list test alongside it stays. embedding_bias: GenericLlmRequest unsqueezed it to [1, vocab] because, per the comment, "that's what IFB code expects". The only remaining reader is sampler_features, via _py_embedding_bias_1d, which squeezed the dimension back off. Store it 1-D and drop both steps; the executor-side tensor was already 1-D (requestTest.cpp:55), so the two sides now agree. Verified locally: full C++ rebuild clean, 113 gtests pass including llmRequestTest whose shape assertions this changes, and the sampler and bindings Python tests pass. Signed-off-by: ZhaoyangWang <zhaoyangw@nvidia.com>

I had edited the manifest by hand because the generator needs torch, which is unavailable on my host. That missed three entries belonging to the removed LookaheadDecodingConfig: speculative_config.max_ngram_size, max_verification_set_size and max_window_size. Regenerated with scripts/generate_llm_args_golden_manifest.py inside the build container; the only difference from the hand-edited file is those three stale entries. Signed-off-by: ZhaoyangWang <zhaoyangw@nvidia.com>

…indings The previous commit removed the unsqueeze in the executor::Request-based GenericLlmRequest constructor, but the nanobind LlmRequest constructor has its own copy: makeOptionalTensor was called with unsqueeze=true for embedding_bias. That is the constructor production uses -- llm_request.py builds LlmRequest(embedding_bias=...) directly -- so the bias stayed 2-D there while the Python squeeze that used to undo it was gone. embedding_bias was the only caller passing the flag, so drop the parameter rather than leave an unused knob. The bias is now 1-D end to end. Adds test_llm_request_embedding_bias_stays_1d, which goes through the nanobind constructor. The existing coverage went through the executor Request path and the sampler tests never set embedding_bias, which is why neither caught this. Signed-off-by: ZhaoyangWang <zhaoyangw@nvidia.com>

Per review: nothing in C++ read it. The bias was stored on the request, marshalled back to torch through the bindings, and only ever consumed by the TorchSampler via the Python-side copy. Keeping it in two places is also what let the previous commit fix one unsqueeze and miss the other. LlmRequest now takes no embedding_bias parameter; llm_request.py pops the kwarg and owns the tensor as py_embedding_bias (renamed from the private _py_embedding_bias_1d, which no longer needs to distinguish itself from a C++ copy). executor::Request::mEmbeddingBias is untouched — base_worker passes it in and it is part of the Request pickle. Also drops the argument from the 34 positional LlmRequest constructions that would otherwise shift, and the llmRequestTest assertions on the removed getter. Verified locally: full rebuild clean, llmRequestTest/requestTest/ blockKeyTest/serializeUtilsTest all pass. The 9 kvCacheManagerTest PartialCopy failures are a local sm90-build-on-B200 mismatch, unrelated to this change. Signed-off-by: ZhaoyangWang <zhaoyangw@nvidia.com>

…t binding test_llm_request constructs the binding directly and asserted the value back off it. The parameter is gone now that the tensor is owned on the Python side, so drop both the kwarg and the assertion. Signed-off-by: ZhaoyangWang <zhaoyangw@nvidia.com>

…pler tests These build SimpleNamespace stubs by hand rather than going through the bindings, so they kept wrapping sampling parameters in singleton lists the way runtime::SamplingConfig used to. The production readers now treat them as scalars, giving TypeError: '<' not supported between 'int' and 'list' in penalties.py and token_ban.py, and 50 != [50] in the capture test. Fix the stubs; production code is already correct (llm_request.py assigns py_min_length from sampling_config.min_tokens, a scalar). Swept the rest of the tests for other singleton-list sampling params; none left. test_penalties and test_token_ban pass locally (65 tests). Signed-off-by: ZhaoyangWang <zhaoyangw@nvidia.com>

zhaoyangwang-nvidia · 2026-09-03T02:16:51Z

/bot run --disable-fail-fast

tensorrt-cicd · 2026-09-03T02:25:00Z

PR_Github #71124 [ run ] triggered by Bot. Commit: 757d943 Link to invocation

QiJune

LGTM

Deleting _first_or_none from _scan_one_model_sampling means the stubs no longer get their singleton lists unwrapped, so they have to hand over scalars like the real sampling config does. test_capture_sampling_params: my earlier pass only fixed top_k, leaving temperature and top_p wrapped -- hence [1.0] != 1.0 and [([0.7], 50, [0.9])] != [(0.7, 50, 0.9)] on x86 single-GPU. test_group_all_greedy_sync: same stub shape, found by grepping the tests for singleton-wrapped sampling params rather than waiting for CI to reach it. Note this file has a separate pre-existing failure on main (SimpleNamespace lacks _scan_one_model_sampling, which update_is_all_greedy_sample calls); it is not in any CI test list, so CI never runs it. Out of scope here. A repo-wide sweep for the pattern now comes back empty. Signed-off-by: ZhaoyangWang <zhaoyangw@nvidia.com>

zhaoyangwang-nvidia · 2026-09-03T05:55:49Z

/bot run --disable-fail-fast

tensorrt-cicd · 2026-09-03T06:03:28Z

PR_Github #71188 [ run ] triggered by Bot. Commit: 04c65de Link to invocation

tensorrt-cicd · 2026-09-03T06:06:35Z

PR_Github #71124 [ run ] completed with state ABORTED. Commit: 757d943

Link to invocation

tensorrt-cicd · 2026-09-03T13:07:18Z

PR_Github #71188 [ run ] completed with state SUCCESS. Commit: 04c65de
/LLM/main/L0_MergeRequest_PR pipeline #58324 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

Please check the failed tests and fix your PR
If you cannot view the failures, ask the CI triggerer to share details
Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

zhaoyangwang-nvidia · 2026-09-04T01:21:35Z

/bot run --disable-fail-fast

tensorrt-cicd · 2026-09-04T01:27:29Z

PR_Github #71376 [ run ] triggered by Bot. Commit: 04c65de Link to invocation

tensorrt-cicd · 2026-09-04T06:07:53Z

PR_Github #71376 [ run ] completed with state FAILURE. Commit: 04c65de
/LLM/main/L0_MergeRequest_PR pipeline #58494 completed with status: 'UNSTABLE'

CI Report

⚠️ Action Required:

Please check the failed tests and fix your PR
If you cannot view the failures, ask the CI triggerer to share details
Once fixed, request an NVIDIA team member to trigger CI again

Link to invocation

zhaoyangwang-nvidia · 2026-09-04T06:16:52Z

/bot run --disable-fail-fast

tensorrt-cicd · 2026-09-04T06:22:35Z

PR_Github #71457 [ run ] triggered by Bot. Commit: 04c65de Link to invocation

tensorrt-cicd · 2026-09-04T07:06:55Z

PR_Github #71457 [ run ] completed with state SUCCESS. Commit: 04c65de
/LLM/main/L0_MergeRequest_PR pipeline #58562 completed with status: 'SUCCESS'

CI Report

Link to invocation

zhaoyangwang-nvidia added the api-breaking Accepted LLM API contract change that is backwards-incompatible label Sep 1, 2026

github-actions Bot assigned zhaoyangwang-nvidia Sep 1, 2026

Funatiq self-requested a review September 1, 2026 14:35

Funatiq reviewed Sep 1, 2026

View reviewed changes

zhaoyangwang-nvidia marked this pull request as ready for review September 2, 2026 03:02

zhaoyangwang-nvidia requested review from a team as code owners September 2, 2026 03:02

zhaoyangwang-nvidia requested review from BowenFu, LarryXFly, QiJune, Shixiaowei02, Wanli-Jiang, athena-nv, dc3671, dhansen-nvidia, eopXD, liji-nv and pcastonguay September 2, 2026 03:02

zhaoyangwang-nvidia added 7 commits September 2, 2026 18:49

zhaoyangwang-nvidia force-pushed the trtllm-15947-followup-cleanup branch from aa18fe8 to 757d943 Compare September 3, 2026 02:16

dc3671 approved these changes Sep 3, 2026

View reviewed changes

QiJune approved these changes Sep 3, 2026

View reviewed changes

QiJune added the ci: full pre-merge approved label Sep 3, 2026

2ez4bz approved these changes Sep 3, 2026

View reviewed changes

zhaoyangwang-nvidia enabled auto-merge (squash) September 3, 2026 03:18

BowenFu approved these changes Sep 3, 2026

View reviewed changes

zhaoyangwang-nvidia merged commit 23e5ff1 into NVIDIA:main Sep 4, 2026
7 checks passed

zhaoyangwang-nvidia mentioned this pull request Sep 4, 2026

[None][refactor] BREAKING: Remove the two-model speculative decoding path and dead C++ spec-dec code #18721

Open

1 task

Uh oh!

Conversation

zhaoyangwang-nvidia commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dev Engineer Review

QA Engineer Review

Description

API-breaking changes

Notes for reviewers

Test Coverage

PR Checklist

GitHub Bot Help

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Funatiq left a comment

Choose a reason for hiding this comment

Uh oh!

zhaoyangwang-nvidia commented Sep 3, 2026

Uh oh!

tensorrt-cicd commented Sep 3, 2026

Uh oh!

QiJune left a comment

Choose a reason for hiding this comment

Uh oh!

zhaoyangwang-nvidia commented Sep 3, 2026

Uh oh!

tensorrt-cicd commented Sep 3, 2026

Uh oh!

tensorrt-cicd commented Sep 3, 2026

Uh oh!

tensorrt-cicd commented Sep 3, 2026

Uh oh!

zhaoyangwang-nvidia commented Sep 4, 2026

Uh oh!

tensorrt-cicd commented Sep 4, 2026

Uh oh!

tensorrt-cicd commented Sep 4, 2026

Uh oh!

zhaoyangwang-nvidia commented Sep 4, 2026

Uh oh!

tensorrt-cicd commented Sep 4, 2026

Uh oh!

tensorrt-cicd commented Sep 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

zhaoyangwang-nvidia commented Sep 1, 2026 •

edited

Loading