Skip to content

perf: remove trtllm postprocessing workers from the args as post processing workers are not effective in dynamo - #12592

Merged
richardhuo-nv merged 5 commits into
mainfrom
rihuo/remove_pp_workers_and_warn
Aug 5, 2026
Merged

perf: remove trtllm postprocessing workers from the args as post processing workers are not effective in dynamo#12592
richardhuo-nv merged 5 commits into
mainfrom
rihuo/remove_pp_workers_and_warn

Conversation

@richardhuo-nv

@richardhuo-nv richardhuo-nv commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add _strip_postprocess_workers() helper in llm_worker.py that removes num_postprocess_workers from TRT-LLM engine args before the engine is initialized, and emits a WARNING-level log if the value was > 0.
  • Call _strip_postprocess_workers() after all engine arg sources (--extra-engine-args YAML and --override-engine-args JSON) have been merged, so it covers every code path.
  • Add 4 unit tests: 3 direct tests of the helper (positive value, zero, absent) and 1 integration test that verifies the setting is absent from engine_args passed to get_llm_engine when supplied via an --extra-engine-args YAML file (the pattern used in the kimi-k2.5, deepseek-v32-fp4, and gpt-oss-120b recipes).

Why: TRT-LLM's num_postprocess_workers spawns workers for its own post-processing pipeline, which conflicts with Dynamo's response-streaming path. When the setting is forwarded to TRT-LLM it has no useful effect and can occupy extra cpus resources causing perf degradation.

Validation

  • pytest -m unit components/src/dynamo/trtllm/tests/test_trtllm_unit.py -k "postprocess" — all 4 new tests pass.
  • Manually verified the warning fires on a config containing num_postprocess_workers: 4 and is absent when the key is not set.

Related Issues

🚫 This PR is NOT linked to an issue:

  • Confirmed — no related issue

Open in Devin Review

Summary by CodeRabbit

  • Bug Fixes
    • Prevented unsupported post-processing worker settings from being passed to TensorRT-LLM engine initialization.
    • Added a warning when a positive post-processing worker count is configured.
    • Preserved valid behavior when the setting is zero or not provided.

@richardhuo-nv
richardhuo-nv requested review from a team as code owners August 3, 2026 21:52
@github-actions github-actions Bot added fix backend::trtllm Relates to the trtllm backend labels Aug 3, 2026

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

Open in Devin Review

Comment thread components/src/dynamo/trtllm/tests/test_trtllm_unit.py Outdated
Comment thread components/src/dynamo/trtllm/workers/llm_worker.py
@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The TensorRT-LLM worker removes num_postprocess_workers before engine configuration. Positive values produce a warning. Tests cover helper behavior and asynchronous initialization.

Changes

TensorRT-LLM postprocess cleanup

Layer / File(s) Summary
Strip postprocess worker arguments
components/src/dynamo/trtllm/workers/llm_worker.py
The worker removes num_postprocess_workers after engine-argument synchronization. It warns when the removed value is positive.
Validate argument stripping
components/src/dynamo/trtllm/tests/test_trtllm_unit.py
Tests cover positive, zero, and absent values. An asynchronous initialization test verifies removal from YAML extra engine arguments before engine creation.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly states that TRT-LLM post-processing workers are removed from engine arguments to improve Dynamo performance.
Description check ✅ Passed The description explains the change, rationale, tests, validation, and required no-issue confirmation, but it omits the reviewer-start section.

Comment @coderabbitai help to get the list of available commands.

Comment thread components/src/dynamo/trtllm/workers/llm_worker.py Outdated
@pull-request-size pull-request-size Bot added size/L and removed size/M labels Aug 3, 2026
@datadog-official

This comment has been minimized.

@richardhuo-nv richardhuo-nv changed the title fix: remove trtllm postprocessing workers from the args as post processing workers are not effective in dynamo perf: remove trtllm postprocessing workers from the args as post processing workers are not effective in dynamo Aug 3, 2026
@github-actions github-actions Bot added perf and removed fix labels Aug 3, 2026

@krishung5 krishung5 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that the default value of num_postprocess_workers is 0, so stripping directly sounds good, thanks!

@richardhuo-nv
richardhuo-nv force-pushed the rihuo/remove_pp_workers_and_warn branch from b6eeb47 to a4ba166 Compare August 3, 2026 23:11
@richardhuo-nv
richardhuo-nv force-pushed the rihuo/remove_pp_workers_and_warn branch from a4ba166 to e69e37d Compare August 3, 2026 23:11
Comment thread components/src/dynamo/trtllm/workers/llm_worker.py
@richardhuo-nv
richardhuo-nv requested a review from a team as a code owner August 4, 2026 22:34
@richardhuo-nv richardhuo-nv added perf and removed perf labels Aug 5, 2026
Comment thread components/src/dynamo/trtllm/tests/test_trtllm_unit.py Outdated
@pull-request-size pull-request-size Bot added size/M and removed size/L labels Aug 5, 2026
@richardhuo-nv
richardhuo-nv enabled auto-merge (squash) August 5, 2026 18:04
@richardhuo-nv
richardhuo-nv merged commit dbeaa5b into main Aug 5, 2026
104 checks passed
@richardhuo-nv
richardhuo-nv deleted the rihuo/remove_pp_workers_and_warn branch August 5, 2026 18:56
hhzhang16 added a commit that referenced this pull request Aug 6, 2026
dyn-3691-extract-shared-target-pid-cuda-customstorage-operation-layer

* 'main' of https://github.com/ai-dynamo/dynamo: (65 commits)
  fix(frontend): emit SGLang stream role once (#12741)
  docs(fern): promote v1.3.1 to current release (#12752)
  fix(docs): remove duplicate unscoped community-rail CSS rules (#12615)
  feat(operator): migrate CRD storage to v1beta1 (#11904)
  fix: synchronize self-benchmark capacity across DP ranks (#12021)
  chore(deps): bump dynamo-tokenizers to 1.8.0 (#12707)
  fix(frontend): preserve split UTF-8 characters (#12688)
  docs: align Kubernetes build selector with CLI (#12729)
  fix(frontend): preserve completion backend error status (#12706)
  fix(operator): replace snapshot pods after GMS restart (#11286)
  refactor(media): rename installer module, drop --packages per review
  fix(media): harden installer against three pre-redesign review findings
  fix(media): verify installs in a fresh interpreter; teach --pip-args= form
  test(serve): install test-time decoders at the validated bounds
  feat(media): explicit installer for additional media decoders
  docs(spica): correct kv_load_ratio support guidance (#12714)
  feat(operator): add experimental grove.forceScalingGroup for single-node components (#11772)
  fix(vllm): declare entry-stage engine_input_source in GLM-Image NIXL config (#12709)
  chore: bump trtllm to v1.3.0rc23 (#12532)
  perf: remove trtllm postprocessing workers from the args as post processing workers are not effective in dynamo (#12592)
  ...

Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend::trtllm Relates to the trtllm backend perf size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants