Repository navigation
[Bugfix] Qwen2.5-Omni: assemble talker spans from runner phase metadata - #5019
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Overall AssessmentThis is a sophisticated and critical bugfix for Qwen2.5-Omni talker phase routing. The fix addresses a fundamental issue where multi-token decode spans were misrouted to prefill, causing text/codec alignment drift during speculative decoding and request replay. Technical AnalysisProblem SeverityThe bug was causing alignment drift in multi-token scenarios:
This affected both accuracy and performance, with the PR noting ASR WER degradation. Solution Quality✅ Root cause correctly identified: The span-length heuristic ( ✅ Clean layered approach:
✅ Stateless decode path: ✅ Backward compatibility: Legacy callers without metadata use the original single-step behavior unchanged. Implementation Excellence
Test CoverageThe comprehensive test suite covers:
Validation
Code Quality
RecommendationAPPROVE - This is a high-quality, critical bugfix that addresses a fundamental issue in Qwen2.5-Omni's talker routing. The implementation is technically sound, well-tested, and maintains backward compatibility while fixing the core alignment issues in multi-token decode scenarios. |
|
Sorry, I am still not sure what problem you are trying to solve? You can clarity your problem statement again? |
Sure, let me restate it with full context. Background. Qwen2.5-Omni generates speech in two stages. A Thinker model writes the text answer; a Talker model then speaks it, by generating a sequence of discrete audio-codec tokens (one 50 Hz audio frame per token) autoregressively. At every decode step, the Talker's input embedding is the sum of two parts: the embedding of its own previous audio token, and one row of Problem. Fix. Route prefill vs decode by the phase metadata the runner already attaches to every request ( Evidence. The ~30-line CPU-only script in #5018 reproduces the misroute directly: its traceback walks into |
| # Ensure we have base embeddings when only ids are provided | ||
| if input_embeds is None and input_ids is not None: | ||
| input_embeds = self.talker.embed_input_ids(input_ids) | ||
| return self.thinker_to_talker_process(input_ids, input_embeds, payload) |
There was a problem hiding this comment.
[P1] Slice the reconstructed prompt at the current prefill offset
Routing a one-token chunked-prefill tail into the prefill branch is necessary, but this still returns the full reconstructed prompt starting at row 0. The runner writes req_embeds[:span_len] into the request’s current scheduled span, so when _omni_num_computed_tokens > 0, a later prefill chunk receives the beginning of the prompt instead of the rows corresponding to its actual positions.
For example, with _omni_num_computed_tokens == 7 and span_len == 1, this path returns prompt embedding row 0, while the runner needs row 7. This silently builds incorrect KV state, so the one-token prefill-tail regression claimed by this PR is not fully fixed.
Please slice both returned IDs and embeddings using the runner offset, e.g. [num_computed_tokens : num_computed_tokens + span_len], before returning them. The regression test should also use deterministic full-prompt embeddings and assert that a non-zero-offset tail returns the correct row; the current mocked prefill test only verifies branch selection and cannot detect this issue.
There was a problem hiding this comment.
Fixed in 196b3f4; details in the comment below.
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
Omni ReviewBot: no human activity for 20 days@LJX-xixi this pull request has had no human commit, comment or review since 2026-09-02. 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. |
Omni ReviewBot routing recordAssigned Strict on zcode (GLM-5.3-Flash) under experiment |
Omni ReviewBot attempt recordReview attempt ended as timeout (Strict attempt outlived its budget; falling back to direct/cursor/auto). |
1 similar comment
Omni ReviewBot attempt recordReview attempt ended as timeout (Strict attempt outlived its budget; 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); 0 finding(s) below.
CI at
f478ecc5dacf(2026-10-09T16:03:13.424523+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
Qwen2.5-Omni talker preprocess stops treating span length as the prefill-versus-decode signal. It uses the runner's per-request phase flag, then computed-token versus prompt length, and only then the old span-length check. Decode spans are built statelessly: position i is thinker_reply[num_computed - prompt_len + i] plus that position's codec embedding, and the reply buffer is left intact. Speculative verify steps and preempt/resume replays therefore keep one text row per codec position, while callers without the metadata still take the existing single-step consuming path.
Change flow
flowchart TD
Meta["[EXISTING] Runner phase metadata and token span"]:::existing
Route["[CHANGED] talker_preprocess phase routing"]:::changed
Prefill["[EXISTING] thinker_to_talker_process"]:::existing
Decode["[NEW] thinker_to_talker_decode"]:::new
Legacy["[EXISTING] thinker_to_talker_decode_one_step"]:::existing
Out["[CHANGED] Span embeddings and reply update"]:::changed
Meta --> Route
Route --> Prefill
Route --> Decode
Route --> Legacy
Prefill --> Out
Decode --> Out
Legacy --> Out
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!
Omni ReviewBot: finding feedback[p1] Chunked prefill tail still returns prompt row 0 — See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement. |
amy-why-3459
left a comment
There was a problem hiding this comment.
Reviewed commit f478ecc5dacf2fb2923b08302a8424e3c83dcd8c.
No actionable bugs introduced by this PR were identified in the static review. This does not establish runtime correctness.
No actionable issues found in this static review.
Validation: Verified HEAD f478ecc and inspected its full diff against 9a44e7e. Reviewed changed code and tests, runner metadata and embedding assembly, intermediate-buffer updates, thinker-to-talker payload construction, talker embedding consumption, and relevant existing tests and deployment configuration. Runtime/GPU tests were not run; no repository code was executed.
linyueqian
left a comment
There was a problem hiding this comment.
Thanks for working on this! The new stateless helper cleanly handles pure decode spans without consuming thinker_reply. Looking at the remaining branches, the earlier prefill-offset finding remains open, and scheduler traces indicate that replays crossing the prompt boundary also miss decode conditioning. These points represent remaining gaps in the span-assembly fix rather than new regressions introduced by your changes.
This feedback is based on a static review of the changed files alongside the runner and scheduler call paths; I did not run unit tests or local GPU/e2e validation. I verified the scheduler behavior against vLLM 0.24 and 0.31. The GPU results mentioned in the PR description reflect an out-of-tree equivalent patch, so we cannot treat them as independent verification of this exact commit head.
| # Ensure we have base embeddings when only ids are provided | ||
| if input_embeds is None and input_ids is not None: | ||
| input_embeds = self.talker.embed_input_ids(input_ids) | ||
| return self.thinker_to_talker_process(input_ids, input_embeds, payload) |
There was a problem hiding this comment.
[important] The earlier prefill-offset concern is still open here: with prompt_len=8, num_computed_tokens=7, and span_len=1, the model returns the full prompt starting at row 0, which leads the runner to copy row 0 into position 7. In addition, there is a replay scenario that offset slicing alone does not fully resolve. When preemption occurs, the scheduler resets num_computed_tokens to 0 while retaining generated tokens, and it can subsequently schedule the prompt plus those generated tokens together in a single span. Because that span is treated as a prefill, this branch returns only the prompt rows, leaving the generated-token suffix without thinker-conditioned decode embeddings. To complete the fix, please slice the prompt portion by the computed-token offset and assemble any decode suffix using the stateless helper. It would also help to verify both cases with the real prefill helper, since the current sentinel mock validates branch selection but cannot catch row-alignment mismatches.
There was a problem hiding this comment.
Fixed in 196b3f4; details in the comment below.
|
@LJX-xixi Are you planning to continue work on this PR? Please reply by 2026-10-19 10:35:50 UTC. A yes or a brief plan is enough; code changes do not need to be finished within these three days. If you are waiting on a review or other maintainer action, please let us know. If there is no reply, new commit, or maintainer takeover by then, we will recheck and close this PR as inactive. It can be reopened when work resumes. |
|
@hsliuustc0106 Yes, I plan to continue with this PR. Thanks for checking in, and thanks @linyueqian for identifying the remaining gaps. I'll keep the follow-up focused on slicing the prompt portion at the computed-token offset and using the stateless decode helper for any generated-token suffix in a replay span. I'll add regression tests using the real prefill helper, then run targeted GPU/E2E validation on the updated PR head. The fixes and that validation are still pending; the earlier out-of-tree results do not validate this PR head. I don't have a firm completion date yet, but I intend to follow through on this scoped fix. |
…dle decode spans statelessly talker_preprocess guessed prefill vs decode from span length, predating the per-request phase metadata from vllm-project#3662. Multi-position decode spans (speculative-decoding verify, preempt/resume replay) were misrouted to the prefill branch; the decode branch filled only one position (later rows stayed on uninitialized torch.empty scratch) and consumed the thinker_reply queue statefully, drifting after any multi-token accept or replay. Route by _omni_is_prefill (with num_computed/prompt_len and span-length fallbacks, same layered pattern as qwen3_tts_talker), and assemble decode spans statelessly: thinker_reply[num_computed - prompt_len + i] + codec_embedding per position, span embeddings built once from embed_input_ids, no queue consumption. Callers without the metadata keep the unchanged one-step path. FIX vllm-project#5018 Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: LJX-xixi <lijinxi413@icloud.com>
…ondition replayed decode suffixes The prefill branch returned the complete reconstructed Talker prompt, and the runner copies the returned rows into the positions it scheduled. A prefill chunk at a non-zero offset (e.g. prompt_len=8, num_computed_tokens=7, span_len=1) therefore received prompt row 0 instead of row 7. After preemption the scheduler resets num_computed_tokens to 0 while keeping the generated tokens, and may schedule the prompt and those tokens in one span. The prefill branch returned only prompt rows, leaving the generated suffix without thinker-conditioned decode embeddings. Slice the prompt rows (ids and embeddings) at _omni_num_computed_tokens, and assemble any generated-token suffix with the stateless decode helper, using the thinker_reply reconstructed in the same call. Without runner metadata the complete prompt is still returned. The tests now run the real prefill helper with distinguishable rows: offset chunks, the 1-token tail, replays crossing the prompt boundary from offsets 0 and 5 (compared against the original step-by-step decode embeddings), a repeated replay, and the legacy path. Signed-off-by: LJX-xixi <lijinxi413@icloud.com>
f478ecc to
fd95fed
Compare
docs/design/feature/preprocess_phase_contract.md makes _omni_prompt_len, _omni_num_computed_tokens and _omni_is_prefill a stable runner-owned interface, and asks models not to fall back silently to the span length when they need chunking correctness. Every in-tree runner (V1, MRv2, NPU) provides these fields, so the span-length heuristic, the full-prompt return and the consuming single-step decode path kept for callers without them were unreachable, and kept a second, stateful way to condition decode positions, including a model-level thinker_reply_part pool that is filled during profiling and consumed across requests. Talker preprocessing now derives the phase and every row from the span's absolute positions: prompt positions take rows of the reconstructed prompt, generated positions take thinker_reply[offset + i], and a replay crossing the prompt boundary uses both. Missing metadata, or a reconstructed prompt whose length differs from the scheduled prompt, raises instead of silently misaligning rows. The stateful decode path and the shared pool are removed. Also use the vLLM-Omni SPDX copyright line in the test file, as required by the check-spdx-header pre-commit hook. Signed-off-by: LJX-xixi <lijinxi413@icloud.com>
|
@linyueqian Thanks for the detailed review. Both gaps are fixed, and I rebased onto current main ( Changes
Design While fixing these, I removed the fallbacks for callers without runner metadata (the span-length heuristic, returning the whole prompt, and the consuming single-step decode with the model-level
The model file is now a near one-for-one replacement of the previous logic (+63/−61 against main). If you would prefer the fallback removal as a separate PR, I can split it. Validation (details in the description): under a stress deploy that forces chunked prefill and preemption replays, AI assistance: Claude (Anthropic) was used for the analysis, the fix and tests, and the GPU validation. I reviewed the changes and the results. |
Signed-off-by: LJX-xixi <lijinxi413@icloud.com>
Signed-off-by: LJX-xixi <lijinxi413@icloud.com>
Head branch was pushed to by a user without write access
|
Thanks for the review. Updated to Current-head validation: 131 focused CPU tests passed; applicable changed-file pre-commit checks passed; 20 CUDA tensor-only cases passed in FP32/BF16. Real-weight audio validation has not been rerun after #9371; the earlier GPU results are now explicitly labeled historical in the description. CUDA #18170 and AMD #14719 tested the previous head AI assistance: Codex assisted with this follow-up fix, validation, and summary. |
…ta (vllm-project#5019) Signed-off-by: LJX-xixi <lijinxi413@icloud.com> Co-authored-by: Claude <noreply@anthropic.com> (cherry picked from commit 6bcfc4d)
Contributor intent (required human answers)
1. What outcome do you want to achieve?
Qwen2.5-Omni deployments in which the Talker serves multiple requests at once (
max_num_seqs > 1) should produce correct speech, including when the Talker's prefill is chunked or a request is preempted and replayed.2. What is the highest-leverage variable for that outcome?
Whether each Talker position receives the conditioning row for the absolute position that the runner actually scheduled.
3. How will this work move that variable?
The Talker now selects each row directly from the absolute positions reported by the runner, instead of inferring the phase from the span length or taking rows from a queue that is consumed once per call.
4. Whose customer or user needs does this address, and what are those needs?
Operators who want the Qwen2.5-Omni Talker to serve several requests concurrently. When they raise Talker concurrency, or serve long audio or video inputs, the generated speech must stay correct. Today the workaround is to keep the Talker at a single sequence or to disable chunked prefill, both of which limit throughput.
Purpose
FIX #5018
Before.
Qwen2_5OmniForConditionalGeneration.talker_preprocesschose prefill or decode from the span length. A chunked-prefill chunk at a non-zero offset received the beginning of the reconstructed prompt instead of its own rows; a one-token prefill tail was treated as decode; a multi-position decode span (speculative-decoding verification, or the generated part of a preemption replay) went to the prefill branch; and a replay containing the prompt plus generated tokens returned only prompt rows. Decode also consumed onethinker_replyrow per call, so replaying the same positions shifted the conditioning.After. The talker assembles each scheduled span from the runner's positions (
_omni_num_computed_tokens,_omni_prompt_len), as defined in the runner-to-model phase contract: prompt positions take rows of the reconstructed Talker prompt at their offset, generated positions takethinker_reply[k]for their generation offsetkand continue using the final text-PAD row after the reply ends, and a replay that crosses the prompt boundary uses both.thinker_replyis no longer consumed, so a decode step, a verify span and a replay of the same positions receive the same rows. Missing metadata, or a reconstructed prompt whose length differs from the scheduled prompt, raises instead of silently misaligning rows.The contract page is unchanged: this PR makes Qwen2.5-Omni a conforming consumer of the existing fields and does not change the runner. The page already lists this PR under References.
Test Plan
vLLM Version: 0.31.0
vLLM-Omni Commit:
de22e99417e0ba028c4a50c569a0745a0bd7e48c, including mainbf98347c9via merge69c2bdfa.The current-head local checks below use Python 3.12.13, vLLM 0.31.0, PyTorch 2.13.0+cu130 and Transformers 5.14.1. The real-weight GPU results are historical, from the earlier
5c79c4f4d-based implementation, before #9371.CUDA_VISIBLE_DEVICES='' python -m pytest tests/model_executor/models/qwen2_5_omni/ tests/model_executor/stage_input_processors/test_qwen2_5_omni_thinker2talker.py -m 'core_model and cpu' --run-level=core_model -qasync_chunk: false, Thinker and Code2Wavmax_num_seqs=1, Talkermax_num_seqs=4. A reference deploy keeps the default Talker settings; a stress deploy sets Talkermax_num_batched_tokens=32with chunked prefill and limits the Talker KV cache to 128 blocks (max_model_len=1984), forcing non-zero-offset prefill chunks and preemption replays. 16 fixed prompts, Thinker greedy (max 80 tokens), Talker deployment sampling (temperature 0.9, top-p 0.8, top-k 40, seed 42, max 1900 tokens). Whisper-base WER against each request's Thinker text.Test Result
On
mainunder the stress deploy, every prefill call returned a row count different from the scheduled span (68/68, 71/71). With this PR, neither new error check triggered in any run.AI assistance: Claude (Anthropic) assisted with the earlier span-assembly changes and real-weight GPU experiments. Codex (OpenAI) assisted with the CI failure analysis, fixture update, persistent-PAD fix, CPU tests and CUDA tensor smoke.