Repository navigation
Conversation
Self-reviewWhat I checked
Open question for reviewers: if you would rather keep |
|
This PR appears to belong to: docs/design/module/model_integration.md. Module owners: @tzhouam @gcanlin @rk9595, 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. |
|
@tzhouam @gcanlin — gentle ping on this one when you have a moment. It's the remaining half of the sweep started in #6803 (merged): same Happy to rebase or split it differently if you'd prefer a different shape. |
| for target in child.targets: | ||
| if isinstance(target, ast.Attribute) and target.attr == ATTR: | ||
| return True | ||
| if isinstance(child, ast.AnnAssign): |
There was a problem hiding this comment.
Reject bare annotations; they still leave the SupportsPP attribute missing at runtime.
There was a problem hiding this comment.
Good catch — fixed in f1142e33.
You're right that the AnnAssign branch was wrong in exactly the way this PR exists to guard against: a bare annotation is precisely how vLLM 0.28 lost make_empty_intermediate_tensors, so accepting one would have let the checker green-light the original bug. AnnAssign now only counts when child.value is not None.
While there I also made the Assign branch match a Name target (make_empty_intermediate_tensors = staticmethod(...) at class level), which it previously missed — the two branches now agree on what counts as a binding. Happy to drop that half if you'd rather keep the diff to the one thing you flagged.
Verified in both directions. Added parametrized tests over _provides_attr itself: 5 shapes that bind at runtime (method, self.x = ..., self.x: T = ..., class-level assign, class-level annotated assign) and 3 that don't (bare class annotation, bare self.x: T, unrelated method). Plus an end-to-end check — rewriting qwen2_5_omni_token2wav.py:1447 from self.make_empty_intermediate_tensors = _empty_intermediate_tensors to self.make_empty_intermediate_tensors: object now fails the scan naming that class; before this commit it passed. Reverted, of course.
No class in vllm_omni/model_executor/models/ uses a bare annotation today, so the tightened check is green on main. 9 passed, ~1s, CPU only; ruff check / ruff format --check clean on 0.14.10 (the pinned pre-commit rev).
Also rebased onto current main (ab561485) — the branch was still sitting on fe4a2a74. mimo_audio_code2wav.py still needs its SPDX header; covo_audio_code2wav.py picked one up upstream, so that hunk shrank.
@lishunyang12 could you add the ready label? No Buildkite lane has run on this PR yet, and I'd like CI on it before it merges.
… attribute MiMoAudioToken2WavForConditionalGenerationVLLM and CovoAudioCode2WavForConditionalGeneration declare SupportsPP but never provide make_empty_intermediate_tensors. Since vLLM 0.28 turned that member from a method on the Protocol class into a bare annotation, nothing supplies it, so vllm.model_executor.models.interfaces.supports_pp() reports True for both while any read of the attribute raises AttributeError -- the failure fixed for the talker in vllm-project#6803. Neither stage implements pipeline parallelism: both are vocoders with no inner LM to delegate to, and their forward methods ignore intermediate_tensors (mypy already flagged both signatures as incompatible with the supertype). Drop the declaration rather than inventing a PP implementation they do not have. Add a static contract test over every vllm_omni class declaring SupportsPP, asserting it defines the attribute or assigns it in __init__. The check is AST based because models are too heavy to construct in a unit test and the repo convention is to assign on the instance, which no class-level hasattr sees. Reverting the vllm-project#6803 fix makes this test fail on that class. Closes vllm-project#6859 Signed-off-by: rk9595 <rakesh.kariya@somaiya.edu>
A bare annotation binds nothing at runtime, which is exactly how make_empty_intermediate_tensors went missing in vLLM 0.28, so only count an AnnAssign that carries a value. Also count class-level assignment, which the Assign branch previously missed. Adds bidirectional tests over the checker itself. Signed-off-by: Rakesh Kariya <rakesh.kariya@somaiya.edu>
1e56ffe to
f1142e3
Compare
Omni ReviewBot: no human activity for 7 days@rk9595 this pull request has had no human commit, comment or review since 2026-09-16. 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. |
Bring the branch up to date with main (vLLM 0.30.0 rebase, CoVo-Audio prompt fix). No conflicts; the contract test still passes and finds no new SupportsPP violators. Signed-off-by: Rakesh Kariya <rakesh.kariya@somaiya.edu>
|
Current plan and status. The open review thread from @lishunyang12 is resolved — bare Since the branch had fallen 172 commits behind (including the vLLM 0.30.0 rebase in #7820 and the CoVo-Audio fix in #7909), I have just merged current The change is unchanged in scope: 3 files, +140/-4. Two vocoder stages stop declaring Two things are needed to move this forward, neither of which I can do myself:
Happy to rebase again if it drifts; I would just rather not keep re-merging while it waits, since each update restarts the clock. |
Omni ReviewBot: no human activity for 7 days@rk9595 this pull request has had no human commit, comment or review since 2026-09-23. 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. |
Bring the branch up to date with main. Conflict-free; the contract test still passes 9/9 and finds no new SupportsPP violators among models added upstream. Signed-off-by: Rakesh Kariya <rakesh.kariya@somaiya.edu>
|
Plan unchanged, and the PR is current again. The branch had fallen 139 commits behind, so I have merged Review state: the one blocker @lishunyang12 raised was resolved on 2026-09-16 ( This is now waiting on two maintainer actions, neither of which I can perform:
@Gaohan123 @hsliuustc0106 could one of you add If the direction here is no longer wanted, I would rather hear that and close it than keep it on the stale-bot rotation — this is the second 7-day nudge. Either outcome is fine; I would just like a decision. |
Omni ReviewBot routing recordAssigned Strict on zcode (GLM-5.3-Flash) under experiment |
Omni ReviewBot attempt recordReview attempt ended as failed (step 'review' (agent.review_diff): unhandled error: RuntimeError: zcode exited 1 without a result event: statusCode: undefined } Error: Turn execution failed (traceId: 1f8a1753-d749-4bdd-8d33-65e8ea0fdc54) — check |
1 similar comment
Omni ReviewBot attempt recordReview attempt ended as failed (step 'review' (agent.review_diff): unhandled error: RuntimeError: zcode exited 1 without a result event: statusCode: undefined } Error: Turn execution failed (traceId: 1f8a1753-d749-4bdd-8d33-65e8ea0fdc54) — check |
Omni ReviewBot attempt recordReview attempt ended as failed (step 'review' (agent.review_diff): unhandled error: RuntimeError: zcode exited 1 without a result event: statusCode: undefined } Error: Turn execution failed (traceId: 03c8bcb8-0e7d-46db-bb32-2e38d1d7d000) — check |
Omni ReviewBot attempt recordReview attempt ended as failed (step 'review' (agent.review_diff): unhandled error: RuntimeError: zcode exited 1 without a result event: statusCode: undefined } Error: Turn execution failed (traceId: da4199db-f6f9-45e8-b936-98a8c0ef249f) — check |
vllm-omni-review-bot
left a comment
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); 0 finding(s) below.
CI at
353c43deba5d(2026-10-10T03:28:05.600746+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
Note: The assigned review arm
strict/zcode/GLM-5.3-Flashcould 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
The MiMo-Audio token-to-wav stage and the CoVo-Audio code2wav stage no longer subclass vLLM's SupportsPP. Their forward methods do not read pipeline intermediate tensors, and neither class defines make_empty_intermediate_tensors, so the base class was advertising pipeline-parallel support they do not implement. A new CPU test parses model classes that still list SupportsPP and fails unless that attribute is actually bound, including rejecting a bare annotation with no value.
Change flow
flowchart LR
vocoders["[CHANGED] MiMo and CoVo code2wav drop SupportsPP"]:::changed
flag["[EXISTING] supports_pp to is_pp_supported"]:::existing
profile["[CHANGED] Non-first PP rank no longer expects the missing method"]:::changed
scan["[NEW] AST SupportsPP contract test"]:::new
vocoders --> flag
flag --> profile
scan --> vocoders
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
🤖 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!
4 participants
Closes #6859
Follow-up to #6803, which fixed the MiMo-Audio talker. This is the remaining half of the same sweep.
What broke
MiMoAudioToken2WavForConditionalGenerationVLLM(mimo_audio_code2wav.py) andCovoAudioCode2WavForConditionalGeneration(covo_audio_code2wav.py) declareSupportsPPbut never providemake_empty_intermediate_tensors.vLLM 0.28 turned that member from a method on the
SupportsPPProtocol class into a bare annotation, so declaring the interface no longer supplies one. Verified onmain@fe4a2a74with vLLM 0.28.0:supports_pp()is_supports_pp_attributes(model) and _supports_pp_inspect(model). The first is True purely becauseSupportsPPsits in the MRO. The second is True only incidentally — bothforwardmethods take**kwargs, andsupports_kwcounts aVAR_KEYWORDparameter as acceptingintermediate_tensors, which neitherforwardactually uses.The flag is recorded as
_ModelInfo.supports_pp(registry.py:858) and surfaced asModelConfig.is_pp_supported, so both stages are advertised as pipeline-parallel capable. Under PP>1 a non-first rank callsself.model.make_empty_intermediate_tensors(...)in the runner's profiling path and raisesAttributeError— the same failure as #6790.Fix
Drop
SupportsPPfrom both, rather than inventing a PP implementation they do not have:Qwen2ForCausalLM.forwardaccepts or usesintermediate_tensors.Signature of "forward" incompatible with supertype "SupportsPP"on both classes; those two errors disappear with the declaration removed (25 → 23 errors on these two files, every remaining one pre-existing and unrelated).vllm_omnireadssupports_pporSupportsPPoutside the model classes themselves, so no in-repo behaviour depends on the claim.Regression test
tests/model_executor/models/test_supports_pp_contract.pywalks everyvllm_omniclass declaringSupportsPPand asserts it either definesmake_empty_intermediate_tensorsor assigns it in__init__.The check is static (AST) for two reasons: these models pull in torch and vLLM layers and build tensor-parallel linears, so constructing one in a unit test is not viable; and the repo-wide convention —
glm_tts.py:828,fish_speech_slow_ar.py:225,voxcpm2_talker.py:852, andmimo_audio_llm.pyafter #6803 — is to assign on the instance inside__init__, mirroring upstreamQwen2ForCausalLM, which no class-levelhasattrwould see.It guards the whole class of breakage, not just these two classes:
mainit fails, naming both offenders.MiMoAudioLLMForConditionalGeneration— i.e. it would have caught the original bug.CPU-only, no model construction, runs in ~1s.
Verification
mainlisting both classes, passes here (checked both directions).tests/model_executor/models/mimo_audio+ the new test: 14 passed.ruff checkandruff format --checkclean on all three files.The SPDX headers in the diff were added by the repo's own
check_spdx_headerpre-commit hook when the files were touched; they are not manual edits.