Repository navigation
[Bugfix] Apply --fingerprint-mode / --fingerprint-value in omni's init paths - #8244
apex-mochen wants to merge 3 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, semantic router, CODEOWNERS; @linyueqian via module of the changed files, module named in the PR description, semantic router, CODEOWNERS; @NickCao via module of the changed files, module named in the PR description, semantic router, 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. |
|
Author self-review (@apex-mochen), as the review bot asks. What I checked
No GPU was used or needed. |
ace4ea3 to
7b011fc
Compare
…t paths omni inherits vLLM's serve args through make_arg_parser, so --fingerprint-mode and --fingerprint-value are accepted and documented, but omni never applies them: upstream applies the mode at the top of init_generate_state, before any serving class is constructed, because the mode lives in module globals and each BaseServing.__init__ reads it once and caches it on self.system_fingerprint. omni_init_app_state builds those serving classes itself (multi-stage, duplex and diffusion paths) and never calls init_generate_state, so every omni server keeps the default full fingerprint no matter what the CLI says. Apply the same call, with the same getattr defaults, at the one point that dominates all three construction paths. Signed-off-by: apex-mochen <265981728+apex-mochen@users.noreply.github.com>
7b011fc to
ba5b1ab
Compare
Omni ReviewBot routing recordAssigned Direct under experiment |
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
PR description
Omni accepted upstream --fingerprint-mode / --fingerprint-value via the shared serve parser but never applied them, because omni_init_app_state builds serving handlers itself and never calls upstream init_generate_state. This PR copies upstream’s one-shot set_default_fingerprint_mode(...) into the top of omni_init_app_state, above duplex early-return and pure-diffusion construction, so module globals match CLI before any handler caches system_fingerprint. With no flags, behavior stays full; with flags set, responses carry the operator-requested fingerprint. Three CPU regression tests pin multi-stage reachability, getattr defaults restoring a stale mode, and duplex ordering.
Change flow
flowchart TD
A["[EXISTING] CLI --fingerprint-mode / --fingerprint-value"]:::existing --> B["[CHANGED] omni_init_app_state applies set_default_fingerprint_mode"]:::changed
B --> C["[EXISTING] Module fingerprint globals"]:::existing
C --> D["[EXISTING] Multi-stage / duplex / diffusion serving constructors"]:::existing
B --> E["[NEW] fingerprint_* regression tests in test_api_server_guards.py"]:::new
E --> C
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
ba5b1abb2a50(2026-09-28T20:25:20.259023+00:00): required check(s) blocking:buildkite/vllm-omni(missing), and -5 more.
No actionable findings.
|
Hi maintainers, could someone review this PR and trigger the required Buildkite checks? Pre-commit and the wheel build are passing, but I don't see a Buildkite status on the current head. Related PR #8242 is also awaiting review and CI. Happy to make any changes requested. Thanks! |
|
Hi @alex-jw-brooks, software engineering student / open-source contributor here. This fix applies --fingerprint-mode / --fingerprint-value in omni's CLI entrypoints, where they're currently parsed but ignored. Code and tests are ready, checks pass. Could you take a look when you have a moment? |
| # (here, in the duplex path below, and in the diffusion path) and never calls | ||
| # ``init_generate_state``, so without this the ``--fingerprint-mode`` / | ||
| # ``--fingerprint-value`` flags are accepted and silently ignored. | ||
| set_default_fingerprint_mode( |
There was a problem hiding this comment.
[P2] Include the configured fingerprint in Omni chat responses
The mode is now set before handler construction, but OmniOpenAIServingChat.chat_completion_full_generator() overrides upstream's generator and constructs OmniChatCompletionResponse at vllm_omni/entrypoints/openai/serving_chat.py:2530 without system_fingerprint=self.system_fingerprint. Consequently, a valid non-streaming /v1/chat/completions request still returns system_fingerprint: null with --fingerprint-mode custom --fingerprint-value omni-fingerprint. Please propagate the cached fingerprint into the response and add a response-level regression test; the new tests only inspect the module global after initialization.
There was a problem hiding this comment.
Rechecked head ba5b1abb2a50ff08cdf26d39f70888b197934ec1; this finding still applies. Setting the global mode fixes initialization, but the non-streaming Omni chat response at serving_chat.py:2530 still omits system_fingerprint=self.system_fingerprint, so --fingerprint-mode custom --fingerprint-value omni-fingerprint still produces system_fingerprint: null on that path. The new tests inspect the global after initialization and do not assert the response field.
I would include the response propagation and a response-level regression test here so the accepted flags work for Omni chat clients. If this is intentionally limited to initialization wiring, please narrow the PR's response-level claims and explicitly document which routes remain unsupported, with a tracked follow-up.
This was a static re-review; I did not execute the fork's tests locally. The visible checks passed, but no Buildkite result is visible on this head and the merge gate remains BLOCKED.
There was a problem hiding this comment.
Thanks for re-checking — agreed, and done in this PR. b6cca317 passes system_fingerprint=self.system_fingerprint into the OmniChatCompletionResponse built by chat_completion_full_generator (as upstream's generator does), and adds a response-level test: it runs the real multistage init with --fingerprint-mode custom --fingerprint-value omni-fingerprint (and none) and asserts the field on the response returned by chat_completion_full_generator.
Evidence: reverting only serving_chat.py turns the custom case red (assert None == 'omni-fingerprint'); with the fix, -k fingerprint gives 5 passed and the full test_api_server_guards.py 69 passed. ruff 0.14.10 is clean and the branch merges cleanly with current main.
Scope note: streaming chunks (OmniChatCompletionStreamResponse) and the diffusion chat paths (handler built via for_diffusion, which bypasses BaseServing.__init__) still don't carry the fingerprint. I've left them out to keep this PR small and will open a tracked follow-up, unless you'd prefer them here.
There was a problem hiding this comment.
Thanks again for the recheck. Following up on the scope note in my previous reply: 520e5c08 also covers the streaming path, so both non-streaming and streaming chat responses now carry the configured fingerprint in this PR.
- Non-streaming (
b6cca317, unchanged):OmniChatCompletionResponseinchat_completion_full_generatorgetssystem_fingerprint=self.system_fingerprint. - Streaming (
520e5c08): same rule as upstream'sChatCompletionStreamResponse— the fingerprint goes on the terminal chunk (finish_reasonset) wheninclude_usageis off, and on the trailing usage chunk when it is on. Applied to the text, audio, image and fallback-stop chunks inchat_completion_stream_generator.
Tests: test_cli_fingerprint_reaches_the_streaming_chat_chunks runs the real stream generator for custom / none × include_usage on/off × ["text"] / ["text", "audio"] (the latter ends on the fallback stop chunk), and asserts exactly one chunk carries the fingerprint and that it is the final one. Reverting only serving_chat.py to b6cca317 turns the 4 custom streaming cases red; with the fix -k fingerprint gives 13 passed, the full test_api_server_guards.py 77 passed, and test_stream_finish_reason.py + test_serving_chat_metrics.py 19 passed. ruff 0.14.10 is clean.
Not changed: the diffusion chat paths (_create_diffusion_chat_completion). That handler is built by for_diffusion, which skips BaseServing.__init__, so there is no vllm_config / self.system_fingerprint to send, and upstream has no equivalent path. Happy to handle it separately if you want it covered.
Could you take another look when you have a moment?
|
Hi @hsliuustc0106, thanks for the review! Re: P2 fingerprint in Omni chat responses — agreed that the configured fingerprint should surface in responses. That's a broader behavior change than this PR's scope (which only wires the existing flags through the init paths). I'd prefer to keep this PR focused on the wiring fix and open a follow-up for the response-level fingerprint propagation. Let me know if you'd rather I bundle it here. |
…responses OmniOpenAIServingChat.chat_completion_full_generator overrides upstream's generator and builds OmniChatCompletionResponse itself, so it never set system_fingerprint: with --fingerprint-mode custom the non-streaming /v1/chat/completions response still returned null. Pass the cached self.system_fingerprint, as upstream's generator does, and add a response-level regression test (custom value surfaces; none mode stays null). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: apex-mochen <265981728+apex-mochen@users.noreply.github.com>
…nses Mirror upstream's streaming behaviour: stamp system_fingerprint on terminal chunks (finish_reason set) when include_usage is off, and on the trailing usage chunk when it is on. Covers the text, audio, image and fallback-stop chunks built by OmniOpenAIServingChat.chat_completion_stream_generator. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: apex-mochen <265981728+apex-mochen@users.noreply.github.com>
Omni ReviewBot: no human activity for 7 days@apex-mochen this pull request has had no human commit, comment or review since 2026-10-12. 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. |
Omni ReviewBot: no human activity for 14 days@apex-mochen this pull request has had no human commit, comment or review since 2026-10-12. Please consider marking this PR as draft until work can resume. 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. |
|
Closing this stale bugfix PR to keep the queue focused on core feature work. It is not a technical rejection: the branch and review history remain available, and it can be reopened if maintainers want it after the core work lands. |
vLLM Version: 0.30.0 · vLLM-Omni commit:
5ce6f5dc49ead82b24bb5e1c19849cea67c51d3e(this branch is one commit on top of it)Test Plan / Result: the exact commands and their pass counts are tabulated under "Tests" below; the base-vs-head negative control is under "Negative control".
Problem
vllm-omniinherits vLLM's serve args throughmake_arg_parser, so--fingerprint-modeand--fingerprint-valueare accepted (and documented in--help), but omni never applies them:--fingerprint-mode nonestill returnsa full fingerprint, and
--fingerprint-valueis inert.Upstream applies the mode once, before any serving class is constructed, at
the top of
init_generate_state:because the mode lives in module globals and each serving class reads it exactly
once, at construction time:
omni_init_app_statebuilds the whole generate-side state itself(
vllm_omni/entrypoints/openai/api_server.py:749) and never callsinit_generate_state, in any of its three construction paths:OmniOpenAIServingChat/OpenAIServingCompletion/ … builtinside
omni_init_app_state;omni_init_app_statedelegates to_init_duplex_app_stateandreturns before the generate branch (
api_server.py:800-802), which then buildsthe chat handler in
_init_duplex_chat(:727);OmniOpenAIServingChat.for_diffusion(:813), whichbypasses
BaseServing.__init__(cls.__new__(cls)).So every served omni process keeps the process-global default
fullmode nomatter what the CLI says, while the same flag on the same model under
vllm servebehaves as documented.This is the "accepted but silently ignored flag" class that omni's own CLI
already guards against elsewhere:
serve.py:258-282rejects seven parallelismflags at parse time precisely because they "would silently disagree with those
sources of truth". These two flags are not rejected and not wired.
Reproduction
No GPU and no live server needed — the flag reaches (or fails to reach) module
state during init:
vllm_omni/entrypoints/openai/api_server.pyto upstream bytes(md5
7189287bf5789f4fad9a74bb7c492d1f, asserted before the run) and keep thethree tests below.
pytest tests/entrypoints/openai_api/test_api_server_guards.py -k fingerprintBefore the fix the two mode assertions report the default fingerprint
(
vllm-<version>-01234567) whereNone/omni-fingerprintis expected, andthe duplex-ordering test reports the default where
omni-duplexis expected.Fix
Apply the mode at the one point that dominates all three construction paths —
near the top of
omni_init_app_state, above the served-model-name resolution and above both early returns (the duplex delegation and the pure-diffusion path)(
+13lines, one import plus twelve):The call is a byte-for-byte copy of upstream's, including the
getattrdefaults, so an
argsobject without these attributes (older callers, tests)behaves exactly like upstream's default rather than clearing the mode.
Tests
Three regression tests in
tests/entrypoints/openai_api/test_api_server_guards.py,all driving the real
omni_init_app_state, plus one shared fixture thatsnapshots and restores the module globals (
_DEFAULT_MODE/_CUSTOM_VALUE) sothe mode cannot leak into other tests:
test_cli_fingerprint_mode_reaches_the_serving_layer— control asserts thedefault really does stamp a fingerprint, then
fingerprint_mode="none"mustyield
Noneandcustom+fingerprint_value="omni-fingerprint"must yieldthat literal.
test_fingerprint_mode_keeps_upstream_default_when_args_omit_it— with theattributes absent and a stale
none/"stale-value"configuredbeforehand, init must restore upstream's
fulldefault (exercises bothgetattrfallback branches).test_fingerprint_mode_is_set_before_the_duplex_init_path— stubs_init_duplex_app_stateand asserts the mode is already applied when thatpath runs, pinning the ordering the duplex early-return depends on.
Executed locally on this branch (CPU, WSL,
vllm 0.30.0):pytest .../test_api_server_guards.py -k fingerprintpytest .../test_api_server_guards.pypytest .../test_duplex_api_server.py .../test_omni_sleep_wakeup.pypytest .../test_video_server.pycache_dit), unrelated to this changeruff check/ruff format --checkon both touched filesAlternatives considered
serve.py:258-282rejects theseven parallelism flags. That would also remove the silent disagreement, but it
makes an inherited upstream flag unusable rather than correct, and the flags
are honest capability switches (they only shape the
system_fingerprintfield). Wiring them is the smaller change: no CLI surface changes, no new
error message. If you would rather have the rejection, I am happy to redo it
that way.
init_generate_stateinstead of copying this onestatement is not available: it also constructs the whole generate-side
surface (
state.serving_chat, pooling, etc.), whichomni_init_app_statedeliberately builds itself. Applying the one global statement leaves that
structure untouched.
Negative control
api_server.pyis restored to its upstream bytes at file level (the driverasserts the md5
7189287b…and that the added text is gone before running —not
git stash, which silently no-ops here), with the new tests left in place.All three fail, each for the production reason:
Re-applying the patch turns them green again (3 passed), and no other test in the
file changes state.
Edge-case and branch review
customwithout--fingerprint-value→None, exactly as upstream (the modeis passed through; upstream does not validate the pair at this call site).
full/None, tested.early-return; the diffusion handler never reads the mode either way, which is
upstream's behaviour for a handler upstream also does not construct.
set_default_fingerprint_modeis a plain global assignment; thesecond init overwrites the first, no accumulation.
is the point of this change.
Related PRs
Same wiring-drift family (an upstream behaviour that
build_appassumes butomni_init_app_statenever performs). Each is an independent hunk and they canbe reviewed separately:
/tokenize,/detokenizein diffusion/duplex).init_parser_metrics).serving_generative_scoringnever set.set_default_fingerprint_modenever called.If you would rather review these as a single "omni init parity" change set, say
so and I will fold them into one PR with a combined test matrix.
AI assistance
AI assistance was used to prepare this change. Every command and result above was
run locally on this branch's frozen source (md5
c09db4024190d44eb4b412ae463337ba) and is reported verbatim.Compatibility
The flags already exist (they come from upstream's parser) and nothing is added,
removed or renamed. With neither flag passed, the mode stays
fulland everyresponse carries exactly the fingerprint it carries today; with a flag passed,
responses carry what the operator asked for, which is the point of the change.
The process-global scope is upstream's own design: matching it is what makes
vllm serveandvllm serve --omniagree.