Repository navigation
[TRTLLM-17273][chore] Choose the trtllm-serve post-processing path once per server - #21392
JunyiXu-nv wants to merge 8 commits into
Conversation
…ce per server OpenAIServer decided nine times, in four handlers, whether a response's post-processed value comes from a postproc worker (read the value the worker stored on outputs[0]) or must be computed in the server process (call the post-processor inline). The flag never changes after construction, so the server now chooses a strategy object once: select_postproc_strategy picks WorkerPostprocStrategy or InlinePostprocStrategy from num_postprocess_workers; __init__ pre-caches it and a cached property keeps server instances that unit tests build without __init__ working. The nine decision points become one strategy call each, and the conditional post-processor unpacks they fed are gone. CompletionOutput gains the read-only public accessor postprocess_result so the server no longer reads the executor's private field; the api-stability reference records the new property. The submission-side uses of postproc_worker_enabled (what generate_async receives, filling postproc_args for in-server post-processing, the Responses store decision) are deliberately unchanged. No behavior change. With more than one candidate the two modes still differ in what the frontend records (worker mode: only outputs[0] carries the result and metrics arrive merged; inline mode records metrics per candidate); that known difference is documented on the strategy class and left for a separate change. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
/bot run |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. WalkthroughCompletion outputs now expose the stored postprocessed value. OpenAI server handlers select and use worker or inline strategies for chat, completion, Harmony, and Responses API processing. ChangesPostprocessing strategy
Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Refactor Sequence Diagram(s)sequenceDiagram
participant OpenAIServer
participant select_postproc_strategy
participant PostprocStrategy
participant CompletionOutput
OpenAIServer->>select_postproc_strategy: select strategy from worker setting
select_postproc_strategy-->>OpenAIServer: return strategy
OpenAIServer->>PostprocStrategy: apply generated result
PostprocStrategy->>CompletionOutput: read postprocess_result when using worker strategy
CompletionOutput-->>PostprocStrategy: return stored value
PostprocStrategy-->>OpenAIServer: return processed result
Possibly related issues
Suggested reviewers: Merge Risk:
|
|
PR_Github #81846 [ run ] triggered by Bot. Commit: |
|
PR_Github #81846 [ run ] completed with state
|
Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
/bot run --disable-multi-gpu-test |
|
PR_Github #81865 [ run ] triggered by Bot. Commit: |
|
PR_Github #81865 [ run ] completed with state |
…tproc-strategy Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #82500 [ run ] triggered by Bot. Commit: |
|
PR_Github #82500 [ run ] completed with state
|
…tproc-strategy Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #82556 [ run ] triggered by Bot. Commit: |
…tproc-strategy Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #82577 [ run ] triggered by Bot. Commit: |
|
PR_Github #82556 [ run ] completed with state |
|
PR_Github #82577 [ run ] completed with state
|
…tproc-strategy Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #82709 [ run ] triggered by Bot. Commit: |
|
PR_Github #82709 [ run ] completed with state
|
…tproc-strategy Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #82808 [ run ] triggered by Bot. Commit: |
|
PR_Github #82808 [ run ] completed with state
|
Pin main at 1ceb4e6. Keep both source changes and use the existing unittest/serve directory registration instead of a redundant per-file entry. Validation: 51 focused postprocessing and disaggregated tests passed; focused pre-commit passed. Protected API collection is blocked by the missing preallocate_nccl_window_buffer.bytes operator, identically on pinned main. No private-access baseline files are introduced because NVIDIA#21393 has not landed. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
Pin main at 1ceb4e6. Keep both source changes and the main-side package license headers. Drop redundant unittest/serve per-file registrations and the duplicate stand-in health tests, already covered by the real-constructor system endpoint tests. Validation: 122 focused tests passed, including 22 public runtime health and 14 private-access lint tests. The 3 VisualGen setup errors match pinned main exactly (12 shared health tests passed on both). Protected API collection also hits the same missing preallocate_nccl_window_buffer.bytes operator on main. Focused pre-commit passed. Keep the current 9 postprocess accesses and _save_bytes baseline location because NVIDIA#21392 and NVIDIA#21552 have not landed. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #84413 [ run ] triggered by Bot. Commit: |
Dev Engineer Review
OpenAIServerselects and caches a post-processing strategy fromnum_postprocess_workers. Response handlers use it for worker-result retrieval or inline processing, including an async callback for inline Responses API handling. Submission-side checks still usepostproc_worker_enabled.CompletionOutput.postprocess_resultexposes the stored worker result and returnsNonewhen no worker stored a result. The implementation documents that worker mode stores a result only on the first output when there are multiple choices, unlike inline mode’s per-candidate metrics behavior. No current review findings were supplied, so severity counts are unavailable.QA Engineer Review
tests/unittest/serve/test_postproc_strategy.pyadds eight tests for strategy selection, sync and async behavior, server-level caching, and the accessor. The file is listed intests/integration/test_lists/test-db/l0_cpu.yml. Modified tests update worker-result access in disaggregated Responses API and Anthropic streaming coverage. The executor test adds an accessor assertion, and the gRPC metrics test setsnum_postprocess_workersto zero. The Anthropic route and executor tests are listed inl0_cpu.yml; the disaggregated test,run_llm_with_postproc.py, and gRPC metrics test were not found in the searched CI or manual-QA lists.The author reports passing CPU test runs, an LLM API apps failure that also occurred on the base commit, and mixed CI results. The latest referenced run has no reported outcome. Coverage verdict: needs follow-up for the outstanding CI result and reported base-commit failure.
Per-File QA Perspective
tensorrt_llm/executor/result.py— Adds thepostprocess_resultaccessor. Verify access to a stored worker result and theNonecase.tensorrt_llm/serve/openai_server.py— Routes response post-processing through a cached strategy. Verify worker and inline paths across streaming, non-streaming, and async Responses API requests.tensorrt_llm/serve/postprocess_handlers.py— Adds worker and inline strategies with sync and async methods. Verify that worker mode does not invoke inline processing and inline mode invokes the correct processor.tests/integration/test_lists/test-db/l0_cpu.yml— Addsunittest/serve/test_postproc_strategy.py. It also lists the changed Anthropic route and executor tests.tests/unittest/api_stability/references/completion_output.yaml— Addspostprocess_resultto the API-stability reference.tests/unittest/disaggregated/test_disagg_responses_api.py— Updates access to the worker result in disaggregated response tests. The file was not found in the searched CI or manual-QA lists.tests/unittest/llmapi/apps/test_anthropic_routes.py— Updates access to streamed frames. The file is listed inl0_cpu.yml.tests/unittest/llmapi/run_llm_with_postproc.py— Updates access to generated output results. The file was not found in the searched CI or manual-QA lists.tests/unittest/llmapi/test_executor.py— Adds an accessor assertion for the stored result. The file is listed inl0_cpu.yml.tests/unittest/serve/test_postproc_strategy.py— Covers strategy selection, sync and async behavior, server-level caching, and the accessor. The file is listed inl0_cpu.yml.tests/unittest/grpc/openengine/test_metrics.py— Setsnum_postprocess_workersto zero in test arguments. The file was not found in the searched CI or manual-QA lists.Description
OpenAIServerdecided nine times, in four handlers, whether a response's post-processed value comes from a post-processing worker (read the value the worker stored onoutputs[0]) or must be computed in the server process (call the post-processor inline). The flag that drives the decision,postproc_worker_enabled, never changes after construction. This PR chooses the path once per server:select_postproc_strategy(new, intensorrt_llm/serve/postprocess_handlers.py) picksWorkerPostprocStrategyorInlinePostprocStrategyfromnum_postprocess_workers;__init__pre-caches the choice and afunctools.cached_property(the same pattern as_is_visual_gen) keeps the many unit-test servers built withobject.__new__working — a plain attribute set only in__init__would break every one of them.CompletionOutputgains the read-only public accessorpostprocess_result, so the serving code stops reading the executor's private_postprocess_resultfield; the field and its writer (the frontend's handling of a postproc worker's output) are unchanged.Part of the trtllm-serve refactor (epic TRTLLM-17056, task TRTLLM-17273): cleanups that land before the handler moves, so that moved handlers carry one strategy call per site instead of nine copies of a private-field read.
The nine decision points
All in
tensorrt_llm/serve/openai_server.py; eachif/ternary onself.postproc_worker_enabledbecomes one call._create_chat_responsepromise.outputs[0]._postprocess_resultvspost_processor(promise, args)self._postproc.apply(promise, postproc_params)openai_chat→chat_stream_generatorself._postproc.apply(first_response, postproc_params)self._postproc.apply(res, postproc_params)openai_completion→completion_responseself._postproc.apply(response, postproc_params)openai_completion→completion_generatorself._postproc.apply(output, params)chat_harmony→create_streaming_generatorself._postproc.apply(first_response, postproc_params)self._postproc.apply(res, postproc_params)openai_responses→create_responseresponses_api_create_response(...)await self._postproc.apply_async(promise, lambda: responses_api_create_response(...))— the lambda is evaluated only in inline mode, so no coroutine is created andself.enable_store/self.tokenizerare read exactly as beforeopenai_responses→create_streaming_generatorself._postproc.apply(res, postproc_params)The two conditional unpacks that only fed the ternaries (
if not self.postproc_worker_enabled: post_processor, args = ...in the chat and harmony stream generators) are deleted; in the Responses streaming generator the unpack keeps onlyargs(post_processorbecame unused,args.streaming_processoris still read).As a concrete example, the chat stream generator's first-response site (second row of the table) was:
and is now:
The strategies themselves (in
tensorrt_llm/serve/postprocess_handlers.py):PostprocStrategyis the abstract base of the two; its docstring records how a server chooses (once, fromnum_postprocess_workers) and the known n > 1 recording difference below.Deliberately unchanged — the eleven submission-side uses of
postproc_worker_enabled: whatgenerate_asyncreceives (_postproc_params=... if ... else None, and the completion path's unconditional pass), fillingpostproc_args.tokenizer/num_prompt_tokensfor in-server post-processing, the Responses store decision and its warning, plus the property itself. They decide what a request carries, not how a result is read; unifying them would change behavior.Behavior
No behavior change. With more than one candidate (
n > 1) the two modes already differ in what the frontend records (worker mode: onlyoutputs[0]carries the result and metrics arrive merged; inline mode records metrics per candidate). That difference is documented in thePostprocStrategydocstring and deliberately not fixed here; it is a separate behavior-change PR.The accessor (API review)
CompletionOutput.postprocess_result(read-only property,Any): what the post-processor returned for this output when post-processing ran in a postproc worker;Noneotherwise.tests/unittest/api_stability/references/completion_output.yamlrecords the new property.references_committed/is unchanged. Requesting theapi-compatiblelabel.Moves
None.
CI bookkeeping
openai_server.pyandexecutor/are already listed, so the multi-GPU stages run on this PR)unittest/serve/test_postproc_strategy.pyregistered intests/integration/test_lists/test-db/l0_cpu.yml(after theunittest/scriptsentry)In-flight work on the same code
Checked open PRs touching
serve/openai_server.py,serve/postprocess_handlers.pyandexecutor/result.pyat branch time. "Typed chat completion core behind a thin HTTP wrapper" has not landed, and the chat SSE generator's error-handling change (#20867, merged) already moved the two chat stream decision points to their current shape, which this PR edits in place. The Responses streaming-generator rewrite (keepalive, sequence renumbering) has not landed; the Responses decision points are edited where they are today. Open PRs that touch nearby code but not the nine decision points: #21072, #21060, #14092 (Responses handlers), #21111 (chat path), #19461 (completion merge helper), #20946 (# type: ignorecomments). Whichever lands second rebases.Test Coverage
New (CPU, registered in
l0_cpu.yml;scripts/test_to_stage_mapping.pynames its four stages: CPU-Generic-arm-1, CPU-Generic-arm-2, CPU-Generic-x86-1, CPU-Generic-x86-2):tests/unittest/serve/test_postproc_strategy.py: the flag→strategy mapping; the worker strategy readsoutputs[0].postprocess_resultof a realGenerationResultBaseand acceptspostproc_params=None; the inline strategy calls the post-processor exactly once and returns its value; workerapply_asyncreturns the stored value without callinginline; inlineapply_asyncawaits it exactly once; a server chooses fromnum_postprocess_workers(0 → inline, 2 → worker) through the realpostproc_worker_enabledproperty and caches the choice;CompletionOutput(0).postprocess_result is Noneand returns the stored value.Changed tests:
tests/unittest/grpc/openengine/test_metrics.py: its minimal generator args now explicitly setnum_postprocess_workers=0, so the real server constructor can select the inline strategy during the HTTP metrics contract test.tests/unittest/llmapi/test_executor.py: also asserts the public accessor returns the worker's value.tests/unittest/llmapi/run_llm_with_postproc.py(GPU, postproc-worker e2e): reads the public accessor, exercising it end to end.tests/unittest/llmapi/apps/test_anthropic_routes.py,tests/unittest/disaggregated/test_disagg_responses_api.py: the fake engine outputs now carry the public attribute name the worker strategy reads (one-word rename each; assertions unchanged).Commands run locally (CPU-only dev box, source-built artifacts matching the base commit; each file in its own pytest process, as the CPU stage runs them):
pytest -m cpu_only tests/unittest/serve/test_postproc_strategy.py -q— 8 passedpytest -m cpu_only tests/unittest/llmapi/test_executor.py -q— 44 passed (needsLLM_MODELS_ROOT: 3 of its 44 tests load tokenizers)pytest tests/unittest/api_stability -q— 64 passed (run without-m, so the serve API and CLI references are checked too)pytest -m cpu_only tests/unittest/disaggregated/test_disagg_responses_api.py tests/unittest/disaggregated/test_openai_disagg_service.py tests/unittest/disaggregated/test_openai_server_stream_errors.py tests/unittest/disaggregated/test_router.py -q— 322 passedpytest -m cpu_only tests/unittest/disaggregated -q— 1411 passed, 1 skipped, 2 xfailedpytest -m cpu_only tests/unittest/llmapi/apps -q— 2260 passed, 1 failed:test_render_standalone.py::TestCpuOnlyStartup::test_the_launcher_loads_a_cuda_stub_only_when_the_driver_is_missingfails identically on the unmodified base commit in this environment (driver-detection test; unrelated to this change)git grep -n "_postprocess_result" -- tensorrt_llm/serve— no output;grep -c "postproc_worker_enabled" tensorrt_llm/serve/openai_server.py— 13python scripts/test_to_stage_mapping.py --tests "unittest/serve/test_postproc_strategy.py"— names the four CPU stages listed above;grep -c "pytest.mark.cpu_only"— 1pre-commit run --files <all changed>— twice, all hooks pass, no file re-modifiedFollow-up validation on
5806927953: reproduced the CPU CIhttp_enablementfailure before the fixture correction; 73 CPU tests passed across OpenEngine metrics, post-processing strategy, runtime control, RL control authentication, and sleep-mode construction after the correction. Focused lint and commit hooks passed. The all-files hook run passed its formatting/lint/consistency checks, but type checking could not complete becausemypyis unavailable and that hook interpreter has a native-library ABI mismatch.GPU coverage is left to CI: the e2e wrappers that run every handler with
num_postprocess_workers0 and 2 (chat and completions on the single-GPU stage; harmony and Responses on the H100 stage), and the postproc-worker example test that now reads the public accessor.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.🤖 Generated with Claude Code