Skip to content

test(vllm): fix omni 0.23 contract guards (regressed by #11374) - #11516

Merged
dagil-nvidia merged 1 commit into
release/1.3.0from
tanmayv/fix-omni-023-contract-tests
Jul 10, 2026
Merged

test(vllm): fix omni 0.23 contract guards (regressed by #11374)#11516
dagil-nvidia merged 1 commit into
release/1.3.0from
tanmayv/fix-omni-023-contract-tests

Conversation

@tanmayv25

@tanmayv25 tanmayv25 commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Overview

PR #11374 (vllm-omni v0.21.0rc1v0.23.0rc1) turned the vllm-runtime / Test CPU jobs red on release/1.3.0 — the parent commit was green, the merge commit failed all four jobs, and every subsequent cherry-pick (e.g. #11455) inherits it.

Three pre_merge contract-guard tests reacted to omni 0.23 internals. In all three the guarded runtime path still works — only the guards are over-strict — so this PR updates the guards, with no product-code change.

Root cause per test

  • test_engine_core_struct_contract — omni 0.23's bundled vLLM adds a trailing multimodal_output field to EngineCoreOutput. The CI assertion printed:

    Actual: (... 'num_nans_in_logits', 'multimodal_output', 'is_segment_finished', 'new_prompt_len_snapshot')
    

    vllm_processor.py constructs EngineCoreOutput by keyword and only sets fields it knows (output_kwargs + if "…" in __struct_fields__), so the new optional field just defaults — construction is unaffected. Fix: add multimodal_output to the omni field variants.

  • test_request_exposes_all_token_ids — omni monkeypatches vllm.v1.request.RequestOmniRequest, whose body doesn't redeclare _all_token_ids (it inherits it), so the inspect.getsource(Request) text check fails. The attribute still exists at runtime; InstrumentedScheduler is fine. Fix: walk the MRO so the inherited attribute is detected, while a genuine rename in vLLM still fails.

  • test_all_diffusion_parallel_config_fields_covered — omni 0.23 adds DiffusionParallelConfig.mask_sp_padding (internal sequence-parallel detail, defaults False, not a benchmark-facing knob). Fix: add to _SKIP_FIELDS, consistent with the existing sequence_parallel_size / enable_expert_parallel skips.

Verification (on the real vllm-omni 0.23.0rc1 / vllm 0.23.0 release/1.3.0 image)

  • Contract guards: 3 failed with the baked test files (reproduces CI) → 3 passed with this PR's files.
  • Full test_omni_base_handler.py: 3 passed.
  • Runtime introspection confirmed the why: EngineCoreOutput really has multimodal_output and keyword-constructs with it defaulting to None; Request is monkeypatched to OmniRequest with _all_token_ids resolvable via the MRO; DiffusionParallelConfig.mask_sp_padding present.
  • End-to-end sanity on the omni image: text serving (Qwen3-0.6B, valid /v1/chat/completions) and diffusion generation (Z-Image-Turbo, valid 512×512 PNG via /v1/images/generations) both succeeded with no errors in the guarded paths.

Not addressed

The GPU test_omni_serve_deployment[omni_i2v] failure on the same run is a separate ffmpeg / "No valid H.264 encoder" environment issue, unrelated to the pin.

🤖 Generated with Claude Code


Open in Devin Review

PR #11374 bumped vllm-omni v0.21.0rc1 -> v0.23.0rc1, turning the
vllm-runtime CPU test jobs red on release/1.3.0. Three contract-guard
tests reacted to omni 0.23 internals; the guarded runtime paths still
work, so only the guards need updating:

- test_engine_core_struct_contract: omni 0.23's bundled vLLM adds a
  trailing `multimodal_output` field to EngineCoreOutput. Construction
  in vllm_processor.py is keyword-based with defaults, so it is
  unaffected; add `multimodal_output` to the omni field variants.
- test_request_exposes_all_token_ids: omni monkeypatches Request with
  OmniRequest, whose body does not redeclare `_all_token_ids` (it
  inherits it). Walk the MRO so the inherited attribute is detected
  while a real rename in vLLM still fails.
- test_all_diffusion_parallel_config_fields_covered: omni 0.23 adds
  DiffusionParallelConfig.mask_sp_padding, an internal SP detail that
  is not a benchmark-facing knob; add it to _SKIP_FIELDS.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: tanmayv25 <tanmay2592@gmail.com>
@tanmayv25
tanmayv25 requested review from a team as code owners July 10, 2026 01:39
@github-actions github-actions Bot added test backend::vllm Relates to the vllm backend labels Jul 10, 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 1 potential issue.

Open in Devin Review

Comment on lines 437 to 441
omni_output_extra_fields = (
"multimodal_output",
"is_segment_finished",
"new_prompt_len_snapshot",
)

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.

🔍 vllm_processor.py does not forward multimodal_output to EngineCoreOutput

The new multimodal_output field is added to the valid omni output field variants in this test, but components/src/dynamo/frontend/vllm_processor.py:666-674 only dynamically forwards is_segment_finished and new_prompt_len_snapshot when constructing EngineCoreOutput. It does not check for or forward multimodal_output. If the downstream OutputProcessor or omni formatters (like AudioFormatter at components/src/dynamo/vllm/omni/output_formatter.py:280) expect multimodal_output to be populated on EngineCoreOutput, it would be None/default when constructed by vllm_processor.py. This may be intentional if multimodal_output is only populated by the omni engine path (not the disaggregated router path through vllm_processor.py), but worth confirming.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@dagil-nvidia
dagil-nvidia merged commit c59188c into release/1.3.0 Jul 10, 2026
93 of 94 checks passed
@dagil-nvidia
dagil-nvidia deleted the tanmayv/fix-omni-023-contract-tests branch July 10, 2026 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend::vllm Relates to the vllm backend size/S test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants