Repository navigation
[Bugfix] Guard MTP input collection in batched MRV2 preprocess - #9540
sphinxkkkbc wants to merge 2 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
amy-why-3459
left a comment
amy-why-3459
left a comment
There was a problem hiding this comment.
Thanks for extracting the #9346 regression into a focused fix with coverage.
The batched-path _has_mtp guard matches the contract enforced later (decode_mtp_rows and not _has_mtp → Deferred MTP is not supported), and leaving the per-request mtp_inputs path ungated is the right asymmetry so an explicit retired-contract return still fails loudly.
The has_mtp parametrization on test_decode_cohort_uses_batch_hook is useful: it keeps asserting ordinary writeback/state updates for non-MTP models while checking that apply_frame_embeddings is not invoked.
One coordination note: the omni_model_state.py hunk is the same blob as in #9507. If that PR lands first, this one’s remaining value is mainly the unit coverage — worth either landing the test here or folding it into #9507 so the regression doesn’t come back without a guard.
| i, req_idx, start, _, _, _ = entry | ||
| decode_mtp_rows.append((i, start)) | ||
| if self._has_mtp: | ||
| decode_mtp_rows.append((i, start)) |
There was a problem hiding this comment.
Gating both decode_mtp_rows and text_step_batches here is necessary and consistent: appending either alone would still trip the elif decode_mtp_rows RuntimeError (or cat mismatched text steps). Good that ordinary batched_indices / buffer updates stay outside the guard.
| if has_mtp: | ||
| assert torch.equal(seen_text_steps[0], torch.tensor([[51.0, 52.0], [61.0, 62.0]])) | ||
| else: | ||
| assert seen_text_steps == [] |
There was a problem hiding this comment.
Nice pairing with the production change. For has_mtp=False, asserting seen_text_steps == [] is the right signal that apply_frame_embeddings was not called; the earlier writeback/state asserts still cover the non-MTP preprocess path.
|
This PR matches CODEOWNERS paths: /tests/. Code owners: @NickCao @yenuo26 Routing: @NickCao via CODEOWNERS; @yenuo26 via CODEOWNERS @sphinxkkkbc, 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. |
|
I’m happy with whichever approach you prefer. I’d lean toward folding the regression tests into #9507 given the small scope, and I’m happy to close this PR once they’re included. |
|
Thanks @sphinxkkkbc for #9540 — extracting the MTP guard from #9507 and validating it with the parameterized test_decode_cohort_uses_batch_hook made the fix explicit and well-tested. I also really appreciate being credited as a co-author. Now that #9507 is merged, could you rebase #9540 onto main and keep it as the standalone test-coverage PR? That way the regression test stays independently reviewable. |
…t_sample hooks Signed-off-by: boatman <109857087+sphinxkkkbc@users.noreply.github.com> Co-authored-by: DarkMatter9527 <192868502+DarkMatter9527@users.noreply.github.com>
cf1e326 to
cbf7877
Compare
amy-why-3459
left a comment
amy-why-3459
left a comment
There was a problem hiding this comment.
Thanks for rebasing onto main after #9507 landed — tip cbf7877 now correctly keeps only the regression coverage.
The remaining has_mtp parametrization on test_decode_cohort_uses_batch_hook is the right standalone value: for has_mtp=False it still asserts ordinary writeback/input_ids updates across the MRV2 / standard / legacy hooks, and it asserts apply_frame_embeddings is never invoked (seen_text_steps == []). For has_mtp=True the previous text-step expectation is preserved. That locks the #9346 contract without reintroducing the production hunk already on main.
No further correctness concerns from this pass. Fine to keep as the dedicated test PR as DarkMatter9527 suggested.
|
@amy-why-3459 PTAL, this PR now only adds the regression coverage. |
amy-why-3459
added
ready
|
CUDA CI passed on main, I think failed AMD/XPU CI are not related to this PR, PTAL @amy-why-3459 |
yenuo26
left a comment
yenuo26
left a comment
There was a problem hiding this comment.
Reviewed tip 41b97c02 (static; tests/worker_v2/test_omni_model_state.py only).
Looks good / CLEAN for self-approve. After #9507 landed the production _has_mtp gate, this PR’s remaining value is the has_mtp parametrization on test_decode_cohort_uses_batch_hook: for False it keeps ordinary writeback/input_ids updates across MRV2 / standard / legacy hooks and asserts apply_frame_embeddings is never invoked (seen_text_steps == []); for True the prior text-step expectation is preserved. That locks the #9346 contract with a negative control.
No redundant helpers, no Buildkite/YAML churn. Agree with @amy-why-3459 that keeping this as the dedicated regression-coverage PR is fine. I did not re-run the 86-pass suite locally.
PLEASE FILL IN THE PR DESCRIPTION HERE.
Contributor intent (required human answers)
All four questions must be answered by the human contributor before submission.
An agent may reuse explicit human answers with a public-safe source reference;
it must not invent, infer, or prefill missing answers. If an answer is missing
or materially ambiguous, ask the human and wait. Do not publish private
conversation content without authorization.
1. What outcome do you want to achieve?
Fix the MRV2 preprocess regression introduced by #9346 for models with a batch decode preprocess hook but no built-in MTP capability.
2. What is the highest-leverage variable for that outcome?
The incorrect assumption that executing a batch decode preprocess hook implies participation in the generic MTP path.
3. How will this work move that variable?
Gate MTP row and text-step collection on
_has_mtp, while preserving ordinary batched input preparation.4. Whose customer or user needs does this address, and what are those needs?
Users running models such as VoxCPM2 on MRV2 need ordinary decode preprocessing to work without requiring the generic MTP contract.
Purpose
Fixes a regression introduced by #9346 and identified during #9507. The batched decode preprocess branch unconditionally collected
decode_mtp_rowsand text steps. For models withouthas_talker_mtp, those rows subsequently triggeredDeferred MTP is not supported, even though the model had not requested MTP execution.Only collect MTP inputs when
_has_mtpis enabled. Models without this capability still execute their batch preprocess hook and update inputs and request state normally.The per-request
mtp_inputscheck intentionally remains unchanged. Explicitly returningmtp_inputswithout the supported MTP capability still raises an error, so use of the retired generic deferred contract cannot silently lose required computation.This corrects the existing MRV2 model-integration contract; no public configuration or user-guide changes are required.
VoxCPM2-specific unit and E2E tests are left to the follow-up that makes MRV2 its default runner.
Thanks to @DarkMatter9527 for identifying and fixing this during #9507. The author is credited as a co-author.
Test Plan
vLLM Version: 0.32.0
vLLM-Omni Commit: 596e92f
Extend
test_decode_cohort_uses_batch_hookto cover models with and without MTP capability across the MRV2 hook, the standard hook, and its legacy signature.Verify that ordinary input preparation and state updates still occur, while non-MTP models do not invoke MTP embedding composition.
Test Result
Before the fix, all three new non-MTP cases fail with
Deferred MTP is not supported. After the fix, all six parameterized cases pass.After: The full test file passed: 86 passed.
cc @amy-why-3459 If #9507 lands first, I’m happy to close this PR.
BEFORE SUBMITTING: read CONTRIBUTING.md and run the precheck-pr skill with the code agent for a self-check against project conventions.
Note
To avoid agent spam, vLLM-Omni limits contributors without write access to 5 open PRs. If this blocks well-intentioned critical work, please contact a committer to request bypass list access.
(anything written below this line will be removed by GitHub Actions)