Repository navigation
[TRTLLM-17273][chore] Remove dead code in trtllm-serve ahead of the module moves - #21176
JunyiXu-nv wants to merge 1 commit into
Conversation
…odule moves
Delete code no production path uses, so the upcoming serve file moves do
not carry it along:
- PrefixBlockSet in serve/router_utils.py (never instantiated anywhere;
the KV-cache-aware router keeps its own per-server set state) and the
then-unused List import.
- ServerState.is_healthy in serve/router.py (health polling goes through
Router._check_server_health; nothing calls the method).
- The is_delegating_client parameter of build_disagg_routers (the function
deleted the value before any use; both callers passed it by keyword, so
no positional call shifts).
- The commented-out block in
HarmonyAdapter._parse_tool_call_from_harmony_message, deleted rather
than restored: uncommented it would raise UnboundLocalError (it reads
function_name before its first assignment), and the recipient cleanup it
wanted is already done where function_name is assigned. Also two
commented-out debug-log lines in
stateful_stream_harmony_tokens_to_openai_deltas.
- The "build_config" entry of cli_derived_fields in commands/serve.py:
is_non_default_or_required is never called with that name, and
build_config is not a TorchLlmArgs field. The llm_args.pop("build_config")
calls stay: removing them would change which stale YAML keys are
swallowed.
- OpenAIServer._vocab_size, read by a single test and no production code;
the test keeps its logit-bias assertion and drops the property read.
Keep a single definition of SERVER_TIMING_HEADER in the dependency-free
serve/_perf_metrics_schema.py, re-exported by serve/perf_metrics.py and
serve/visual_gen_metrics.py so both existing import paths keep working
and neither serving path has to import the other's metrics module.
No behavior change.
Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
/bot run |
|
PR_Github #81129 [ run ] triggered by Bot. Commit: |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Build-config inclusion tensorrt_llm/commands/serve.py |
Explicit build-config scalar flags no longer automatically cause build_config to be included in llm_args. Other inclusion checks remain. |
Shared timing header
| Layer / File(s) | Summary |
|---|---|
Shared response-header constant tensorrt_llm/serve/_perf_metrics_schema.py, tensorrt_llm/serve/perf_metrics.py, tensorrt_llm/serve/visual_gen_metrics.py |
The shared schema defines SERVER_TIMING_HEADER. Both metrics modules import it instead of defining the constant locally. |
Router API and helper removal
| Layer / File(s) | Summary |
|---|---|
Disaggregated router construction tensorrt_llm/serve/router.py, tensorrt_llm/serve/disagg_coordinator.py |
build_disagg_routers no longer accepts is_delegating_client. Its coordinator call sites no longer pass that argument. |
Router helper removal tensorrt_llm/serve/router.py, tensorrt_llm/serve/router_utils.py |
ServerState.is_healthy and PrefixBlockSet are removed. |
Server and Harmony cleanup
| Layer / File(s) | Summary |
|---|---|
Harmony adapter comment cleanup tensorrt_llm/serve/harmony_adapter.py |
Commented-out parsing and streaming code is removed. Executable logic remains unchanged. |
Vocabulary-size property removal tensorrt_llm/serve/openai_server.py, tests/unittest/llmapi/apps/test_harmony_parsing.py |
OpenAIServer._vocab_size and its corresponding test assertion are removed. The test for logit-bias conversion without a tokenizer remains. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Other
Possibly related PRs
- NVIDIA/TensorRT-LLM#20877: Adds
Server-Timingresponse data in serving and VisualGen paths, which shares the header contract centralized here.
Suggested reviewers: schetlur-nv
Merge Risk: 
f2a29
-
The cleanup is mergeable with no identified integration or runtime risk requiring resolution.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% 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. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly identifies the ticket, change type, and primary change: removing dead code from trtllm-serve before module moves. Description check The description includes the required Description, Test Coverage, and PR Checklist sections. It explains the purpose, scope, behavior impact, removed items, compatibility considerations, and validatio… 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: Description check
-
Explanation
The description includes the required Description, Test Coverage, and PR Checklist sections. It explains the purpose, scope, behavior impact, removed items, compatibility considerations, and validation results. It also reports the known test-race failure and its follow-up validation.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% 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 2 functions across 5 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
PR_Github #81129 [ run ] completed with state Resource maintenance in LLM/main/L0_Test-x86_64-Single-GPU #9581: L40S-PyTorch-2 is temporarily paused due to Kubernetes agent provisioning and node issues.
|
|
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. |
|
/bot run |
|
PR_Github #81559 [ run ] triggered by Bot. Commit: |
|
PR_Github #81559 [ run ] completed with state
|
|
/bot run |
|
PR_Github #81625 [ run ] triggered by Bot. Commit: |
|
PR_Github #81625 [ run ] completed with state |
Dev Engineer Review
PrefixBlockSet,ServerState.is_healthy,build_disagg_routers’sis_delegating_clientparameter,OpenAIServer._vocab_size, and commented-out Harmony code. Callers must no longer rely on the removed API members.SERVER_TIMING_HEADERnow has one definition in_perf_metrics_schema.py.perf_metrics.pyandvisual_gen_metrics.pyre-export it, preserving the reported import paths.build_configchange removes explicit CLI scalar checks that previously triggered inclusion inllm_args. The objectives report thatllm_args.pop("build_config", None)remains to preserve handling of stale YAML keys.QA Engineer Review
The Harmony parsing test was modified. It no longer checks
OpenAIServer._vocab_sizewhen no tokenizer is configured. It retains the assertion that convertinglogit_biaswithvocab_size=NoneraisesValueError. The test is listed intests/integration/test_lists/test-db/l0_cpu.ymlunderTestStripIncompleteMessagesReporting. Coverage verdict: needs follow-up. The supplied validation reports one teardown-timing-race failure in the disaggregated run, with 1263 passed, 1 skipped, and 2 xfailed; it also reports that the failure passed in another full-suite run and twice in isolation.Per-File QA Perspective
tensorrt_llm/commands/serve.py: Check that removing explicit build-config scalar checks does not change intendedllm_argsconstruction. Stale YAML handling reportedly remains.tensorrt_llm/serve/_perf_metrics_schema.py: Check that the sharedSERVER_TIMING_HEADERremains"Server-Timing"and is available through both server metrics modules.tensorrt_llm/serve/disagg_coordinator.py: Check coordinator and client router construction after removing theis_delegating_clientarguments.tensorrt_llm/serve/harmony_adapter.py: Only commented-out code was removed; no executable behavior change is reported.tensorrt_llm/serve/openai_server.py: Verify callers do not depend on the removed_vocab_sizeproperty.tensorrt_llm/serve/perf_metrics.py: Check that the existing import path still providesSERVER_TIMING_HEADER.tensorrt_llm/serve/router.py: Check callers against the updatedbuild_disagg_routerssignature. TheServerState.is_healthymethod was also removed.tensorrt_llm/serve/router_utils.py: Verify no callers depend on the removedPrefixBlockSetclass or its exports.tensorrt_llm/serve/visual_gen_metrics.py: Check that the existing import path still providesSERVER_TIMING_HEADER.tests/unittest/llmapi/apps/test_harmony_parsing.py: The test removes the_vocab_sizecheck and retains thelogit_biaserror assertion. The test is listed intests/integration/test_lists/test-db/l0_cpu.yml.Description
Dead code deleted ahead of the trtllm-serve module moves (epic TRTLLM-17056, task TRTLLM-17273): a move PR copies code verbatim, so anything dead would be carried into the new files and cleaned up there with worse history. This PR deletes six dead items, each proven below on the base commit, keeps a single definition of
SERVER_TIMING_HEADER, and (optional seventh item, flagged for reviewers) deletes theOpenAIServer._vocab_sizeproperty that only one test read.Part of the trtllm-serve refactor: cleanup before the moves.
PrefixBlockSet(serve/router_utils.py), its__all__entry, and the then-unusedListimportgit grep -n "PrefixBlockSet"-> exactly the definition (router_utils.py:90) and the__all__entry (:86); no caller, test, or docis_delegating_clientparameter ofbuild_disagg_routers(serve/router.py), its docstring sentence, thedel, and both keyword call sites inserve/disagg_coordinator.pygit grep -n "is_delegating_client"-> only those six lines; the function randel is_delegating_clientbefore any use, so the callers' opposite values changed nothing. Both callers passed it by keyword: no positional call shiftsServerState.is_healthy(serve/router.py)git grep -nE "\.is_healthy\b"andgit grep -nE "['\"]is_healthy['\"]"-> both no hit (two pipe-free commands so the raw markdown stays copy-paste-correct; their union is the alternation\.is_healthy\bor quotedis_healthy); health polling goes throughRouter._check_server_health.ServerState._sessionand_base_urlstay:KvCacheAwareServerStateuses themSERVER_TIMING_HEADERdefinitions (serve/perf_metrics.py:197,serve/visual_gen_metrics.py:12)git grep -n "SERVER_TIMING_HEADER = "-> exactly those two; the modules do not import each otherserve/harmony_adapter.py(:959-962,:1437-1438,:1442):959-962is deleted, not restored: uncommented it raisesUnboundLocalError(it readsfunction_namebefore its first assignment), and the recipient cleanup it wanted is done wherefunction_nameis assigned (str(msg_recipient).split("functions.")[-1]). The to-do comment at:650stayscli_derived_fields["build_config"](commands/serve.py)cli_maybe_overrides, which never containsbuild_config;build_configappears zero times inllmapi/llm_args.py, so it is not aTorchLlmArgsfield;tests/unittest/llmapi/test_llm_args.pyasserts"build_config" not in llm_args(:3619,:3644,:3704)OpenAIServer._vocab_size(serve/openai_server.py)git grep -nw "_vocab_size"-> the definition and one test read (tests/unittest/llmapi/apps/test_harmony_parsing.py:1564); nogetattrstring form anywhere. The test keeps itspytest.raiseslogit-bias assertion and drops the property read with itsobject.__new__(OpenAIServer)server; the file'sOpenAIServertry-import goes with it (ten try-imported names remain)The unused local
vocab_sizeinopenai_chatstays: evaluating it is the firstself.tokenizer.tokenizerdereference, so deleting it changes the answer of a server without a tokenizer.SERVER_TIMING_HEADERnow lives inserve/_perf_metrics_schema.py, a leaf module importing onlytypingandtyping_extensions, and is re-exported by both old modules, sofrom tensorrt_llm.serve.perf_metrics import SERVER_TIMING_HEADERandfrom tensorrt_llm.serve.visual_gen_metrics import SERVER_TIMING_HEADERkeep working. Keeping either existing definition instead would couple the two serving paths: the LLM server would import the VisualGen metrics module or VisualGen serving (and the published clientserve/scripts/benchmark_visual_gen.py) would loadperf_metrics.pyand, through it,tensorrt_llm.llmapi.disagg_utils. Note for a later move ofvisual_gen_metrics.py: both modules use the imported name themselves today, which keeps the formatters from stripping the import; a moved module should addSERVER_TIMING_HEADERto its__all__.Coordination with in-flight PRs: #21062 and #13400 edit
openai_server.pyin the region of the deleted_vocab_sizeproperty (one inserts methods below it, one refactors it into a delegate that still has no production reader); #17439 appends tests directly after the edited test intest_harmony_parsing.py; #20843 has adisagg_coordinator.pyhunk whose context includes the rewritten comment; #20739, #19197 and #20280 touch the two metrics files on other lines. Whichever side lands second rebases; none of these changes the deadness of any item. For #17056 and #16909 the whole-PR diff is not fetchable throughgh pr diff; their per-fileopenai_server.pypatches (files API) contain no_vocab_sizereference, so the worst case is a textual rebase conflict near the deleted helper — deadness unaffected.Out of scope, on purpose:
DisaggPerfMetricsCollector(max_requests)looks dead (del max_requests) but is not — deleting the parameter would make the disagg config's top-levelperf_metrics_max_requestsfall throughextract_disagg_cfg's**kwargsto the context and generation servers, where it is a realBaseLlmArgsfield, silently configuring the workers; it needs its own PR together with the five YAML files intests/integration/defs/disaggregated/test_configs/that set the key. The fivellm_args.pop("build_config", None)calls also stay: they still swallow stale YAML keys, and removing them is a behavior change.Behavior
No behavior change.
Moves
No file moves; one constant moved to
serve/_perf_metrics_schema.pywith both old modules re-exporting it (the re-exports are plain imports that the modules themselves also use).CI bookkeeping
serve/openai_server.py,serve/router.py,serve/router_utils.pyentries still match existing files)Test Coverage
No new tests: each deleted item has no caller, and the existing tests cover every changed file (ran on this branch with the editable build at the base commit):
git grep -nE "PrefixBlockSet|is_delegating_client|async def is_healthy" -- tensorrt_llm tests-> empty;git grep -nw "_vocab_size" -- tensorrt_llm/serve tests/unittest/llmapi-> empty;git grep -n "SERVER_TIMING_HEADER = " -- tensorrt_llm-> one line (serve/_perf_metrics_schema.py);git grep -n '"build_config":' -- tensorrt_llm/commands-> emptypython -c "from tensorrt_llm.serve import perf_metrics, visual_gen_metrics, _perf_metrics_schema as s; assert perf_metrics.SERVER_TIMING_HEADER is visual_gen_metrics.SERVER_TIMING_HEADER is s.SERVER_TIMING_HEADER == 'Server-Timing'"-> passespython -c "import tensorrt_llm.serve.scripts.benchmark_visual_gen"-> passespytest -m cpu_only tests/unittest/disaggregated -q-> 1263 passed, 1 skipped, 2 xfailed; the one failure,test_kv_transfer_fail_stop.py::test_openmpi_subcommunicator_abort_also_terminates_other_executor_in_job, is a teardown-timing race on a loaded box (asserts an mpirun worker is already reaped right after launcher exit): it passed in another full-suite run of the same commit and passes twice in isolationpytest -m cpu_only tests/unittest/_torch/visual_gen/test_trtllm_serve_endpoints.py tests/unittest/llmapi/test_llm_args.py -q -k "serve_endpoints or non_default_or_required or build_config or Server"-> 160 passed, 451 deselectedLLM_MODELS_ROOT=... pytest -rs tests/unittest/llmapi/apps/test_harmony_parsing.py tests/unittest/llmapi/apps/test_harmony_channel_validation.py-> 79 passed, no "not importable" skippre-commit run --files <all ten files>twice -> clean both timesis_non_default_or_required(old vs new extracted sources over an exhaustive bounded input grid plus Hypothesis examples): identical everywhere exceptparam_name == "build_config", where the only change is the deleted entry's branch; four planted bugs in the dict handling were each detected by the propertyPR Checklist
Please review the following before submitting your PR:
🤖 Generated with Claude Code