Repository navigation
Conversation
…8235) Under a scheduled batch of N requests where only M (M < N) carry mm conditioning in a step, _split_prompt_conditioning emits per-request lists of length M while to_payload_element routes payloads by the request's index in the full batch; idx >= M falls back to element[0] and hands one request's reference voice to the others. Align the conditioning lists to the scheduled batch size, padding with None so conditioning-less slots take the code2wav skip path instead of aliasing request 0. Mirrors the vllm-project#4370/vllm-project#4373/vllm-project#4870/vllm-project#4415 cross-request corruption fix pattern. Co-Authored-By: Claude Code <noreply@anthropic.com> Signed-off-by: Sworol <171231900+Sworol@users.noreply.github.com>
|
Self-review note Verified on CPU (no GPU available):
Not requesting the |
|
This PR appears to belong to: docs/design/module/model_integration.md, docs/design/module/ar_runtime.md. Module owners: @tzhouam @gcanlin @fake0fan Routing: @tzhouam via module of the changed files, semantic router, CODEOWNERS; @gcanlin via module of the changed files, semantic router; @fake0fan via module of the changed files @Sworol, 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. |
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
CosyVoice3 talker now pads per-request prompt-conditioning lists to the full scheduled batch size with None after _split_prompt_conditioning, so to_payload_element no longer falls back to request 0's speaker embedding when only a subset of the batch carried mm kwargs this step. That stops concurrent requests from synthesizing in another in-flight reference voice for the common trailing-hole case. A new CPU core_model regression suite pins the old aliasing behavior and the post-align routing.
Change flow
flowchart TD
A["[EXISTING] Scheduled batch N; mm kwargs have M rows"]:::existing
B["[EXISTING] _split_prompt_conditioning → lists len M"]:::existing
C["[CHANGED] talker forward derives batch_size"]:::changed
D["[NEW] _align_conditioning_to_batch pads with None"]:::new
E["[EXISTING] to_payload_element routes by batch idx"]:::existing
F["[NEW] CPU regression tests pin alias vs None"]:::new
A --> B --> C --> D --> E
D --> F
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
09db1c38025e(2026-09-28T20:31:23.028153+00:00): required check(s) blocking:buildkite/vllm-omni(missing),docs/readthedocs.org:vllm-omni(pending), and -4 more.
See inline comments below.
| """ | ||
| if len(speech_token_list) >= batch_size: | ||
| return speech_token_list, speech_feat_list, embedding_list, speech_token_len_list | ||
| pad = batch_size - len(speech_token_list) |
There was a problem hiding this comment.
[P1] Trailing None pad assumes mm rows are a dense batch prefix
_align_conditioning_to_batch only appends None to the end of the M-length lists. That fixes to_payload_element aliasing for holes at indices [M, N), which the new tests cover, but the PR itself describes mixed batches where decode-only requests lack mm this step while others carry conditioning. Under continuous batching those holes need not be a suffix: if mm-carrying requests are not exactly batch slots 0..M-1 in the same order, trailing pad still maps the wrong conditioning (or None) to the wrong idx, so cross-request voice mixing remains. Fix by expanding to full N with None at the actual batch indices that lacked mm this step (presence mask / request-id map), and add a regression where a non-prefix hole must not receive another request's embedding.
There was a problem hiding this comment.
Both findings confirmed — thanks for the review.
P1: _align_conditioning_to_batch only holds when the mm rows are a dense batch prefix. They are not in general: _extract_mm_kwargs collates rows in scheduled_new_reqs (scheduler) order, while payload routing (idx, request_token_spans) runs over input_batch.req_ids, which reorder_batch_to_split_decodes_and_prefills stably reorders to [decode, short_extend, long_extend, prefill] — so in mixed decode+prefill steps the mm-carrying prefills sit at the batch tail, and interleaved text-only requests break the prefix assumption even in pure prefill steps. Trailing-pad then hands decode slots a real row and shifts the prefill slots.
Fix: the runner will inject mm_row_map alongside request_token_spans — a per-batch-position list mapping each request to its row in the collated mm kwargs (or None when it carried no mm this step). The talker forward realigns the per-request lists to batch positions, so to_payload_element(element, idx) routes each request to its own conditioning. Regressions added for a non-prefix hole (must not receive another request's embedding) and for the missing-request_token_spans case.
P2: correct — batch_size = len(speech_token_list) as a fallback silently disables alignment. The fallback will fail closed (a loud warning instead of the element[0] alias), and a forward-level test will exercise short conditioning lists without request_token_spans.
| # request reuses the conditioning emitted in its prefill step), | ||
| # align them to the full batch so no request aliases element 0. | ||
| batch_spans = kwargs.get("request_token_spans") | ||
| batch_size = len(batch_spans) if batch_spans is not None else len(speech_token_list) |
There was a problem hiding this comment.
[P2] Forward batch_size fallback silently disables the fix
batch_size is len(request_token_spans) when present, else len(speech_token_list), which makes _align_conditioning_to_batch a no-op and leaves the element[0] alias intact. The new tests call the helper with an explicit batch_size and never exercise this forward derivation, and request_token_spans is not materialized in this sparse checkout to confirm the runner always injects it for CosyVoice3. Either fail closed / log when spans are missing while lists are short, or add a forward-level test that passes short conditioning lists plus request_token_spans of length N and asserts per-idx routing.
Omni ReviewBot: finding feedback[p1] Trailing None pad assumes mm rows are a dense batch prefix —
This finding appears in the bot's COMMENT review, but GitHub could not place it as an inline diff comment. If you are the PR author and disagree, react 👎 to this comment. The disagreement will be shown to the maintainer; it does not approve or merge the PR. |
Omni ReviewBot: no human activity for 7 days@Sworol this pull request has had no human commit, comment or review since 2026-09-29. 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. |
|
Re-evaluating #8240 against post-#8224 main per your triage note (@hsliuustc0106): Why #8224 already covers the hardening cases:
So we don't have a concrete failing case to hand over for a focused GPU run, and Thanks to @Arifuzzamanjoy for the careful bisect with two independent speaker |
What broke
Fixes #8235. Under concurrent serving, CosyVoice3 can return another in-flight request's reference voice: when a step's scheduled batch has N requests but only M of them carry mm conditioning kwargs, the requests without conditioning in that step silently inherit request 0's speaker embedding / prompt feature and synthesize in that voice instead of their own.
How to reproduce
CPU-only; no GPU or model weights needed. This builds a scheduled batch of N=3 where only M=2 rows carry conditioning and asserts the routing bug on
main:On
main(without the fix)test_un_aligned_short_list_aliases_request_zeroshowsto_payload_element(element=emb_list, idx=2, ...)returns request 0's embedding (all-1s) for a batch index that has no conditioning — the aliasing; the aligned tests error since_align_conditioning_to_batchdoes not exist. On this branch all 6 tests pass.Root cause
_split_prompt_conditioningsplits the collated mm kwargs into per-request lists whose length equals the number of requests that carried conditioning this step (M).to_payload_elementroutes each request's payload by its index in the full scheduled batch (element[idx], idx in[0, N)). When M < N,element[idx] if idx < len(element) else element[0]silently falls back to element 0 foridx >= M, handing request 0's reference voice to every conditioning-less request in the batch. Same cross-request corruption class previously fixed for Qwen3-TTS (#4370/#4373/#4870) and MOSS-TTS (#4415).Fix
Add
CosyVoice3Model._align_conditioning_to_batch, which pads the per-request conditioning lists to the full scheduled batch size withNone, then call it in the talkerforwardright after_split_prompt_conditioning. The batch size is derived fromrequest_token_spans(the runner already injects it for every scheduled request; the same key is used by nemotron_voicechat / moss_tts / minicpmo-4.5 / breeze-tts-2). Padding withNoneis safe:EmbeddingsStructfields aretorch.Tensor | None, and the code2wav consumer skips slots wherespeech_token/speech_feat/embeddingisNone. No-op when the lists are already batch-aligned. Diff is 2 files, 194 insertions — no unrelated refactor.Validation
tests/model_executor/models/cosyvoice3/test_cosyvoice3_conditioning_alignment.py(CPU,core_model/cpumarks), 6/6 pass:pytest tests/model_executor/models/cosyvoice3/test_cosyvoice3_conditioning_alignment.py -q→6 passed._align_conditioning_to_batchas a no-op makes the aligned assertions fail (idx=2returns element[0]), confirming the regression test pins the bug.test_cosyvoice3_model_helpers.py(the cosyvoice3 CPU test importing the touched module) passes;test_cosyvoice3_components.pyhas 3 failed / 7 errors, reproduced on cleanmain(CPU platform cannot select a diffusion attention backend) — pre-existing, unrelated.AI assistance
Used Claude Code (Claude Code CLI) to investigate the root cause, implement the fix, and author the regression test, under my direction. I reviewed every generated change, verified the test reproduces the reported failure on
mainand passes on this branch, and ran the reported validation commands myself.🤖 Generated with Claude Code