Repository navigation
[Bugfix][Frontend] Forward the serve options omni's handler wiring drops - #8209
apex-mochen wants to merge 7 commits into
Conversation
|
This PR appears to belong to: docs/design/module/error_contracts.md, docs/design/module/profiling.md, docs/design/module/entrypoints.md. Module owners: @alex-jw-brooks @linyueqian @NickCao Routing: @alex-jw-brooks via module of the changed files, module named in the PR description, CODEOWNERS; @linyueqian via module of the changed files, module named in the PR description, CODEOWNERS; @NickCao via module of the changed files, module named in the PR description, CODEOWNERS @apex-mochen, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer. Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment. |
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
Omni ReviewBot routing recordAssigned Strict under experiment |
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
PR description
Omni already accepts upstream serve flags through make_arg_parser, but its own handler construction in omni_init_app_state and _init_duplex_chat forwarded only a subset of those kwargs. This change passes default_chat_template_kwargs into OpenAIServingResponses, enable_per_request_metrics into chat/completion (including duplex chat), enable_flash_late_interaction into ServingScores, and log_error_stack into the duplex OnlineRenderer. Missing attributes keep the upstream defaults via getattr, so partial test namespaces do not raise. The user-visible effect is that those CLI flags now reach the same constructors upstream uses, instead of being parsed and then ignored.
Change flow
flowchart TD
A["[EXISTING] Parsed vllm serve --omni args"]:::existing
B["[CHANGED] omni_init_app_state and _init_duplex_chat kwargs"]:::changed
C["[EXISTING] Responses/chat/completion/scores/renderer ctors"]:::existing
D["[NEW] Constructor-capture wiring regression tests"]:::new
E["[EXISTING] Template kwargs, per-request metrics, flash-LI, error stacks"]:::existing
A --> B
B --> C
C --> E
D --> B
classDef existing fill:#e5e7eb,stroke:#6b7280,color:#111827
classDef changed fill:#fef3c7,stroke:#d97706,color:#451a03,stroke-width:2px
classDef new fill:#dcfce7,stroke:#16a34a,color:#052e16,stroke-width:2px
classDef removed fill:#fee2e2,stroke:#dc2626,color:#450a0a,stroke-width:2px
CI at
e00bc2999893(2026-09-27T20:13:21.936724+00:00): required check(s) blocking:buildkite/vllm-omni(missing), and -5 more.
No actionable findings.
|
Author self-review (@apex-mochen), as the review bot asks. What I checked
No GPU was used or needed. |
Omni ReviewBot: no human activity for 7 days@apex-mochen this pull request has had no human commit, comment or review since 2026-09-28. Please confirm the current plan and next step. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
|
Current plan and next step, as the bot asks. State: the PR is complete and unchanged since 09-27 ( Blocker: none on my side — this needs a maintainer review/decision. There is no code work pending, and the branch has no conflicts with Plan: I will keep the branch current and rebase if it goes stale. If there is no review by 10-09 I will ask the maintainer who has been handling the frontend/init area for this repo. This PR is one of a family of five wiring fixes from the same audit (#8169, #8209, #8212, #8242, #8244) — if you would prefer them combined, split differently, or reviewed in a particular order, say so and I will restructure. Next step from me: none until there is reviewer feedback; happy to answer anything specific in the meantime. |
|
Hi @alex-jw-brooks, could you take a look at this one when you have a moment? The branch is a single commit on top of main (0 behind), all checks are green (pre-commit, build 3.11/3.12, DCO), and the change is six one-line kwargs at six construction sites (+6/-0 production, +141 tests) — the handler wiring now forwards the same serve options the CLI parser already accepts. Happy to rebase or adjust if needed. |
|
Still active. The current head is mergeable and I’m keeping the scope limited to forwarding the serve options that omni’s handler wiring currently drops. My next step is human review; I’ll rebase and rerun the focused frontend tests if main moves in a way that affects this path, and I can address review feedback promptly. |
| enable_force_include_usage=args.enable_force_include_usage, | ||
| enable_log_outputs=args.enable_log_outputs, | ||
| enable_log_deltas=args.enable_log_deltas, | ||
| enable_per_request_metrics=getattr(args, "enable_per_request_metrics", False), |
There was a problem hiding this comment.
[P2] Consume the per-request metrics flag in Omni's response generators
Passing the flag reaches the inherited constructor, but OmniOpenAIServingChat overrides both chat_completion_full_generator() and chat_completion_stream_generator() and neither reads self.enable_per_request_metrics or builds the upstream timing metrics. Instead, response metrics are still filtered by _filter_stage_metrics_detail() solely according to the request's return_stage_metrics extra-body flag. An ordinary chat request with --enable-per-request-metrics enabled therefore still receives no timing metrics, including on the duplex chat path. Please honor this flag when constructing the Omni responses and add a response-level regression test; recording constructor kwargs does not verify the advertised behavior.
e00bc29 to
2e0e9b4
Compare
Re-applies the vllm-project#8209 serve-option forwarding fixes on top of f9b97d2: forward enable_per_request_metrics in the shared chat kwargs, forward default_chat_template_kwargs to OpenAIServingResponses, and forward enable_flash_late_interaction to ServingScores. Signed-off-by: apex-mochen <2756823972@qq.com>
|
Hi @hsliuustc0106, thanks for the review! Re: P2 per-request metrics flag in response generators — noted. This PR forwards the serve options that the handler wiring drops; consuming the flag inside the overridden generators is a separate extension. Happy to open a follow-up PR for that, or expand scope here if you prefer — just let me know. |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 120s (try 2 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 600s (try 3 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 120s (try 2 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 600s (try 3 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; falling back to direct/cursor/auto). |
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
Changes since the previous review
- 0 new inline finding(s); 1 finding(s) below.
CI at
2e0e9b437d61(2026-10-10T15:52:44.210186+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
Note: The assigned review arm
strict/cursor/cursor-grok-4.6-highcould not complete this review, so it was produced by the fallback armdirect/cursor/auto. It is excluded from the routing experiment.
Full review analysis
PR description
vllm serve --omni already parses upstream frontend flags, and this patch forwards three of them at Omni's own handler setup. Shared chat and batch construction now passes enable_per_request_metrics, OpenAIServingResponses receives default_chat_template_kwargs, and ServingScores receives enable_flash_late_interaction. Duplex chat goes through the same chat helper, so it gets the metrics flag; the renderer already received log_error_stack. The PR text lists six new call sites, but the frozen diff adds only those three lines, and OpenAIServingCompletion is left unchanged.
Change flow
flowchart LR
Args["[EXISTING] Parsed serve args"]:::existing
ChatInit["[CHANGED] _init_chat_services chat kwargs"]:::changed
Responses["[CHANGED] OpenAIServingResponses template kwargs"]:::changed
Scores["[CHANGED] ServingScores flash-late-interaction flag"]:::changed
Completion["[EXISTING] OpenAIServingCompletion"]:::existing
Tests["[NEW] Constructor-capture regression tests"]:::new
Args --> ChatInit
Args --> Responses
Args --> Scores
Args --> Completion
ChatInit --> Tests
Responses --> Tests
Scores --> Tests
Completion --> Tests
classDef existing fill:#e5e7eb,stroke:#6b7280,color:#111827
classDef changed fill:#fef3c7,stroke:#d97706,color:#451a03,stroke-width:2px
classDef new fill:#dcfce7,stroke:#16a34a,color:#052e16,stroke-width:2px
classDef removed fill:#fee2e2,stroke:#dc2626,color:#450a0a,stroke-width:2px
Findings
- [P1] Completion wiring still drops per-request metrics —
vllm_omni/entrypoints/openai/api_server.py:1060
Evidence for Completion wiring still drops per-request metrics
omni_init_app_state builds OpenAIServingCompletion without enable_per_request_metrics, so --enable-per-request-metrics still never reaches /v1/completions. Chat and batch get the flag from _init_chat_services (getattr(args, "enable_per_request_metrics", False) at the shared kwargs), but the completion call only passes return_tokens_as_token_ids, enable_prompt_tokens_details, and enable_force_include_usage. The new test_multistage_wiring_forwards_options_upstream_passes records that constructor and asserts the value is True when the flag is set and False from _minimal_args(). A missing keyword is None, so both assertions fail and the reported 66 passed result does not match this head. Pass the same attribute into OpenAIServingCompletion, using the same default as the chat kwargs.
🤖 This review was generated by InferMatrix Copilot, an open-source repo-maintenance agent for PR review, CI debugging and issue triage. Try it on your own repo, and ⭐ star it if it helped!
Omni ReviewBot: finding feedback[p1] Completion wiring still drops per-request metrics — See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement. |
linyueqian
left a comment
There was a problem hiding this comment.
Option forwarding is incomplete: the new constructor-capture test expects per-request metrics on completions, but the completion constructor still omits the option. Please complete this wiring before merge.
Reviewed 2e0e9b4 statically. The finding below follows from the recorded constructor kwargs; no PR code or tests were executed. Main CUDA has not reported on this head.
| for name in ("renderer", "responses", "chat", "chat_batch", "tokenization", "anthropic"): | ||
| assert captured[name].get("default_chat_template_kwargs") == template_kwargs, name | ||
| for name in ("chat", "chat_batch", "completion"): | ||
| assert captured[name].get("enable_per_request_metrics") is True, name |
There was a problem hiding this comment.
[important] Forward enable_per_request_metrics to OpenAIServingCompletion as well. The new _recording_ctor stores keyword arguments passed by omni_init_app_state, but the completion constructor still omits this flag. For completion, the assertion at line 1308 sees None instead of True, and the disabled assertion at line 1315 similarly sees None instead of False. The newly added regression test cannot pass with the current wiring, and completions continue to ignore the requested setting. Please add the forwarding argument at the completion construction site and verify both flag values.
Re-applies the vllm-project#8209 serve-option forwarding fixes on top of f9b97d2: forward enable_per_request_metrics in the shared chat kwargs, forward default_chat_template_kwargs to OpenAIServingResponses, and forward enable_flash_late_interaction to ServingScores. Signed-off-by: apex-mochen <2756823972@qq.com>
…ponses Upstream init_generate_state (vLLM v0.31.0) passes enable_per_request_metrics to chat, completion and responses. Omni's copy only forwarded it to chat, so the completion constructor never saw the option and the regression test added by this PR could not pass. OpenAIServingResponses had the same gap. Cover responses in the wiring test for both values, add the off value to the duplex test, and drop the OpenAIServingRealtime stub: main no longer constructs it in omni_init_app_state. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: apex-mochen <265981728+apex-mochen@users.noreply.github.com>
2e0e9b4 to
4e27e52
Compare
|
Thanks @linyueqian, you're right: the completion wiring was missing on Root cause. The original revision ( Fixed in
Verification (local, CPU only):
On this head, Two related items are out of scope for this PR, and I'm happy to follow up on them separately:
Could you take another look when you have a chance? |
Omni ReviewBot: no human activity for 7 days@apex-mochen this pull request has had no human commit, comment or review since 2026-10-14. Please confirm the current plan and next step. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
|
@linyueqian — following up on your review: the completion wiring you flagged is fixed in Plan, since the bot asked: no further changes on this PR unless you'd like some. The two follow-ups I listed above (consuming the flag in Omni's chat generators, per @hsliuustc0106, and the |
Omni ReviewBot: supersededThe CI failure noted on |
|
@apex-mochen the change looks good to me, and Buildkite ran on 4e27e52 after the |
Omni ReviewBot: no human activity for 7 days@apex-mochen this pull request has had no human commit, comment or review since 2026-10-26. Please confirm the current plan and next step. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
…-forwarded # Conflicts: # vllm_omni/entrypoints/openai/api_server.py
|
Hi @linyueqian — merged One conflict, in Worth flagging:
|
|
@linyueqian — all GitHub checks are green on
Nothing in the diff changed beyond the merge resolution described above — one line of forwarded option plus the constructor-capture guard test. |
|
Thanks for the review @linyueqian! Pushed |
|
@linyueqian — done, merged main ($(.commit.sha.Substring(0,10))) into ix/serve-options-not-forwarded. New head is |
|
@apex-mochen, one current-head test issue on 748a082:
This finding follows from source inspection; I did not execute the test or observe it fail in CI. |
|
@hsliuustc0106 — good catch, pushed 761ab28 to fix the assertion. You're right that |
|
@apex-mochen, the
Source-inspected only; I did not execute these tests or observe this failure in CI. |
vLLM's resolve_default_chat_template_kwargs does
dict(args.default_chat_template_kwargs or {}), so an unset value
normalizes to {} rather than None. Fix the assertion to match.
Signed-off-by: apex-mochen <2756823972@qq.com>
_TimestampMiddleware dispatches through the callable it captured in __init__ as `_asgi` (a TraceContextMiddleware when a tracing provider is configured, otherwise the app itself). Replacing `_inner` after construction only swaps the attribute-delegation target, so the stub is never invoked and the `called` assertions cannot pass. Replace `_asgi` instead, which is what the wrapper actually awaits. Signed-off-by: apex-mochen <2756823972@qq.com>
761ab28 to
95f38f0
Compare
|
@linyueqian — just a heads-up that all CI checks are green on the new head 95f38f0 (build 3.11/3.12, pre-commit, DCO, CodeQL, Analyze). Also fixed the P2 assertion issue @hsliuustc0106 flagged (is None → == {} to match the resolver normalization). Ready for your review whenever you have a moment. |
|
@hsliuustc0106 — both items are addressed at head P2 (default-template-kwargs assertion). Already in at P2 (timestamp middleware dispatch stub). Fixed in What I could execute here:
What I could not execute: the guards module through pytest on this machine. It is CPU-only (python 3.12, torch 2.13.0+cpu, vLLM On CI: pushing to the branch does not create a new Buildkite build here — the Note on the SHA: the diff first landed as |
4c79594 to
d156e97
Compare
vLLM Version: v0.31.0 · vLLM-Omni commit:
2d9909525a8a805ed9c3a56a5287a48687654270(this branch is two commits on top of it, head4e27e522)Test Plan / Result: the exact commands and their results are under "Tests" below; the base-vs-head negative control is under "Negative control".
Problem
vllm serve --omnibuilds its serve parser with vLLM's own (vllm_omni/entrypoints/cli/serve.pyimports
make_arg_parserfromvllm.entrypoints.launchers.cli_args), so frontend options areaccepted. Omni then constructs the handlers in its own copy of upstream's
init_generate_state, andthat copy passes only a subset of the kwargs upstream passes to the same constructors, so some
options are silently ignored.
On
main(2d990952),enable_per_request_metricsandenable_flash_late_interactiondo notappear in
vllm_omni/entrypoints/openai/api_server.pyat all, andOpenAIServingResponsesis builtwithout
default_chat_template_kwargs.Reproduction
The flag reaches the parsed args and then stops:
The handlers then never see
enable_per_request_metrics, so the per-request timing metrics thatupstream gates on this flag are not enabled, and
OpenAIServingResponsesparses responses with itsown template kwargs instead of the server-wide default that the renderer applies.
Fix
Forward the options at the construction sites, matching what upstream
init_generate_state(vLLM v0.31.0) passes:
_init_chat_services->OmniOpenAIServingChat/OmniOpenAIServingChatBatch(shared by the multi-stage and duplex paths)enable_per_request_metrics--enable-per-request-metrics: per-request timing metricsOpenAIServingCompletionenable_per_request_metricsOpenAIServingResponsesenable_per_request_metricsOpenAIServingResponsesdefault_chat_template_kwargsinit_generate_stateServingScoresenable_flash_late_interaction--enable-flash-late-interaction(True by default upstream, so it matters when disabled)Upstream passes
enable_per_request_metricsto chat, completion and responses and to no otherconstructor; all three now receive it.
enable_per_request_metricsandenable_flash_late_interactionare read asgetattr(args, ..., <upstream default>), matching upstream's ownenable_flash_late_interaction=getattr(args, ..., True)invllm/entrypoints/pooling/factories.py:callers in this repository build partial argument namespaces
(
tests/entrypoints/openai_api/test_duplex_api_server.pydoes exactly that), so for them an absentoption has to keep meaning its upstream default instead of raising
AttributeError.Tests
tests/entrypoints/openai_api/test_api_server_guards.pygains two regression tests. They replacethe handler classes with recorders and assert which options reach each constructor:
omni_init_app_statewith the options on, then again with them off, andchecks chat, chat batch, completion and responses for both
TrueandFalse(so a hardcoded valuecannot pass), plus
default_chat_template_kwargsandenable_flash_late_interaction;_init_duplex_chatand checksTrueandFalsethe same way.Local, CPU only:
tests/entrypoints/openai_api/as a whole: 1674 passed on this branch and 1672 passed on a cleanmaincheckout, with the same 21 failures and 75 errors on both. Those come from missing CUDA,cache_ditand model servers in my environment; none are new.Negative control
main'sapi_server.py(2d990952, byte-identical) with this PR's tests: exactly the two newtests fail.
With the previous head's
api_server.py(no completion or responses forwarding), the multi-stagetest fails at
completion; with only the responsesenable_per_request_metricsline removed, it failsat
responses.ruff checkandruff format --check(0.14.10),check_test_marks.pyand mypy are clean.GPU: the main CUDA CI has not run on this head yet, so there are no GPU results.
Relation to #7646
#7646 (merged) moved chat, chat-batch and duplex construction into
_init_chat_servicesand passeslog_error_stackon the duplex path, so this PR no longer touches either. Site 1 is now a singleline in that shared helper.
Out of scope
tool_strict_levelto chat, responses and anthropic, anddisabled_thinking_effortto anthropic; Omni's wiring passes neither.enable_per_request_metricsinside Omni's own chat generators (raised in review) needs its own change.AI assistance
This patch was produced with AI assistance (Claude Code). The audit that located the missing
forwarding, the patch and the regression tests were AI-generated; every claim above was verified by
running the tests locally against this PR's base, including the negative control.
Alternative approaches
argsnamespace into the handlers instead of thenamed options: those constructors take explicit keyword arguments, so an
opaque namespace would weaken their contract and hide typos at the call site.
init_generate_stateand inherit its constructionwholesale: it also builds the entire generate-side surface, which
omni_init_app_statedeliberately builds itself. Forwarding the missingoptions keeps that structure untouched.
Compatibility
No flag is added, removed, or renamed. Each option is passed to the constructor
upstream already passes it to. With the options unset, every constructor receives
exactly what it received before (the upstream defaults), so existing deployments
are unaffected; with an option set, the handler finally sees it.