Repository navigation
[tts][bugfix] Fix CosyVoice3 sampling and stage handoff - #6424
linyueqian merged 7 commits into
Conversation
Signed-off-by: akshatvishu <akshatnayak197@gmail.com>
Signed-off-by: akshatvishu <akshatnayak197@gmail.com>
Signed-off-by: akshatvishu <akshatnayak197@gmail.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
This PR appears to belong to: docs/design/module/model_integration.md. Module owners: @tzhouam @gcanlin @akshatvishu, 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. |
|
Reviewed at I ran the e2e on one H20 (vLLM 0.27.0, torch 2.13.0+cu132, bf16, CUDA graphs on stage 0), comparing base with only the three
The PR does stop the crash. It does not fix what produces it. [blocking] The vocoder device fix does not apply under
|
Signed-off-by: akshatvishu <akshatnayak197@gmail.com>
Signed-off-by: akshatvishu <akshatnayak197@gmail.com>
|
Thanks for the detailed review @linyueqian; I have updated the branch with the following changes:
Measurement caveats
Testing and validation
The branch still conflicts with current |
Signed-off-by: akshatvishu <akshatnayak197@gmail.com>
linyueqian
left a comment
There was a problem hiding this comment.
Both blocking findings are resolved. Re-reviewed at ad499969a308679676a0ad933d0d2168234950c2 against merge base dc72559d5.
CUDA evidence you asked for
You noted that an H20 or H100 run on the latest commit was required before the PR could claim the original CUDA failure is fixed. I ran it on one H20 (vLLM 0.27.0, torch 2.13.0+cu132, bf16, CUDA graphs on stage 0), with _SKIP_CUDA_ISSUE_6416 patched out locally so the CUDA path actually executes, and with the talker sampler instrumented.
| Run level | Weights | Result |
|---|---|---|
advanced_model |
real, Whisper check on | 3 passed |
core_model |
dummy, matches the AMD ready job | 3 passed |
Sampler counters across all six server runs: 0 rows with NaN out of 201 / 201 / 108, 0 rows with +inf, 0 rows with no finite logit, and the new ValueError guard never fired.
Transcripts at advanced_model:
zh_001 收到好友从远方寄来的生日礼物那份意外的惊喜与深深的感动让我热泪盈眶 exact
zh_002 收到好友从远方进来的生日礼物那份意外的惊喜与深深的感动让我热泪盈眶 one homophone, 寄来 -> 进来
en_001 Hello, this is a voice cloning test with English text. exact
For comparison, the same GPU on 38eb12bf gave 46 all-NaN rows out of 81 and transcribed さぁさっ, さぁさっ, and empty. #4644 recorded 0.81, 0.62 and 0.03 on H100. The streaming case is the one #4644 flagged for leaking reference text, and there is no leak now, just a Whisper homophone.
On this evidence the CUDA skipif can come off. I would keep it in this PR if you would rather land the fixes first and enable CUDA in a follow-up, but the data supports enabling it now.
On the root cause
num_key_value_heads = 2 is the fix that matters. I confirmed the checkpoint's nested CosyVoice-BlankEN/config.json declares num_attention_heads: 14 and num_key_value_heads: 2, while the outer CosyVoice3Config declared only the 14, so vLLM's engine metadata sized the KV cache for 14 KV heads against layers built with 2. That explains the NaN logits, and it explains why #6416 showed up with enforce_eager: false. The fact that the finite check never fires in any of the six runs is the confirmation that it is a safety net rather than the fix.
_get_stft_window on the base HiFTGenerator is the right place for the device fix, and core_model passing is the direct proof: that path was dead before, since hift.to(device) only runs inside load_weights, which the dummy loader never calls.
Non-blocking
-
cosyvoice3.py:687,logits.masked_fill_(~finite_logits, float("-inf"))also maps+infto-inf, which turns a maximally likely token into a forbidden one. I measured zero+infrows so this is theoretical, buttorch.isposinfdeserves separate handling from NaN if it is ever reachable. -
cosyvoice3.py:686, raisingValueErrorinsidesample()kills the EngineCore for every in-flight request, not just the offending one. Failing loudly beats returning random speech, so this is an improvement over the previous behaviour, but a per-request abort would be better if the runner supports it. Worth a follow-up rather than a change here. -
num_key_value_headsis hardcoded next to the other Qwen2-0.5B values. That matches the file's existing style, but since the failure mode is silent NaN rather than a load error, reading it from the nested config (AutoConfigis already imported) or asserting the two agree at construction would stop this recurring on a future checkpoint.
Note for maintainers, not this PR
#6454 landed in main as dc72559d5 and is byte-identical to this PR's original register_buffer(..., persistent=False) hunk, without the _get_stft_window half. So main currently carries the incomplete fix and the stft input and window must be on the same device failure under load_format: dummy is live there, masked only because these tests are still skipped. This PR is what closes it. Worth landing reasonably promptly for that reason.
Caveats
H20 is not H100 or MI325, and vllm_omni/deploy/cosyvoice3.yaml states its defaults are H100 tuned. TensorRT was unavailable, so campplus used ONNX Runtime, which is the same path your COSYVOICE3_TRT=0 runs take. CUDA graphs and the CUDA attention backend were exercised, since stage 0 runs with enforce_eager: false and this is where the original assert fired.
LGTM.
Signed-off-by: akshatvishu <akshatnayak197@gmail.com>
|
Thanks for the detailed re-review and H20 validation @linyueqian ! In the current PR, I've only removed Also, could you re-run CI? (in-case my last push undid the first ci check) |
…#6424) Signed-off-by: akshatvishu <akshatnayak197@gmail.com> Signed-off-by: AndyZhou952 <jzhoubc@connect.ust.hk>
…#6424) Signed-off-by: akshatvishu <akshatnayak197@gmail.com>
…#6424) Signed-off-by: akshatvishu <akshatnayak197@gmail.com>
Purpose
Fixes #6416.
CosyVoice3 voice cloning could pass invalid probabilities to
torch.multinomial, which killed stage 0 during decoding. Fixing that crash allowed the same supported request path to reach three failures that had been hidden by the sampler crash and test skips. BF16 tensors could not cross the shared stage serializer, serialized prompt conditioning was decoded as flat keys and dropped, and the causal HiFT STFT window could stay on CPU while its input was on the GPU.Fix
Sampling and attention geometry. The CosyVoice sampler now uses vLLM's
random_sampleinstead oftorch.multinomial. It excludes NaN and both infinities after request masks and logit processors run, and it raises a clear error if a row has no finite token. The RAS fallback clones the selected score before masking it, so the only legal token can be restored without adding another Python device check or changing random number generator advancement. The outer CosyVoice config also exposesnum_key_value_heads = 2, which matches the released nested Qwen config. Before this change, the Qwen layers and KV cache used 2 KV heads while vLLM attention metadata fell back to 14. The field is fixed because this integration supports one released CosyVoice3 checkpoint whose other backbone dimensions are already fixed in the same config. Valid scores keep the same categorical distribution, but a fixed seed can produce a different token sequence because vLLM andtorch.multinomialconsume random numbers differently. The finite row check also adds one device synchronization per sampler batch.Stage handoff. The shared serializer now sends tensor storage through a
uint8view and restores the tensor with its recorded PyTorch dtype and shape. The change matches vLLM's raw tensor byte pattern, preserves BF16 exactly, and keeps the existing wire bytes for previously supported dtypes. CosyVoice now uses the shared decoder, which rebuilds nested keys such asembed.speech_token. The removed local decoder returned dotted keys, so serialized prompt conditioning was silently dropped. Decoding keeps one CPU byte copy, as the previous NumPy path did.Vocoder device state. The causal HiFT STFT window is a nonpersistent buffer, so normal model moves include it without adding a checkpoint key.
_stftand_istftalso move and retain the window when its device differs from the input. The use site check is needed because vLLM's dummy loader does not call the model weight loading hook. The first mismatched call performs one device copy, and later calls reuse the moved buffer.Online coverage. The three CosyVoice tests are marked
core_modelbecause blocking AMD CI runs them with dummy weights. The same tests load real weights and use Whisper quality checks when they run with--run-level advanced_modelorfull_model. ROCm requests use a 600 second client timeout because a cold AITER MHA kernel build took 301 seconds on one MI300X. CUDA keeps the existing 120 second default.Test Plan
HIP_VISIBLE_DEVICES=0 COSYVOICE3_TRT=0 \ .venv/bin/python -m pytest -o addopts='' -s -v \ tests/e2e/online_serving/test_cosyvoice3_tts_expansion.py \ -m slow --run-level advanced_modelHIP_VISIBLE_DEVICES=0 COSYVOICE3_TRT=0 \ .venv/bin/python -m pytest -o addopts='' -s -v \ tests/e2e/online_serving/test_cosyvoice3_tts_expansion.py \ -m slow --run-level core_model.venv/bin/python -m pytest -o addopts='' -q \ tests/engine/test_data_entry_keys.py \ tests/model_executor/stage_input_processors/test_cosyvoice3_stage_input_processors.py \ tests/model_executor/models/cosyvoice3/test_cosyvoice3_model_helpers.py \ tests/model_executor/models/cosyvoice3/test_cosyvoice3_components.py \ tests/model_executor/models/cosyvoice3/test_cosyvoice3_config.py \ --run-level core_modelvLLM Version: 0.27.0
vLLM-Omni Commit: ee889a0
Test Result
The test environment used one MI300X with 192 GB VRAM, ROCm 7.2.3, Python 3.12.13, BF16, and model snapshot
29e01c4e8d000f4bcd70751be16fa94bf3d85a18.The real weight test passed all three Chinese sync, Chinese streaming, and English sync cases with the default stage 0
TRITON_ATTNbackend:The targeted real weight test also passed with stage 0 forced to
ROCM_AITER_FAthrough--stage-overrides '{"0":{"attention_backend":"ROCM_AITER_FA"}}':The AITER specialization confirms that the loaded attention implementation used 14 query heads and 2 KV heads. The CPU config regression separately checks that the outer vLLM metadata contract resolves the same 2 KV heads. The dummy weight test passed all three cases after the STFT use site fix:
The focused CPU suite passed on the latest head:
Pre-commit passed for all 11 changed files. The earlier H20 review was performed at
38eb12bf, before the RAS, finite row, STFT use site, and attention geometry fixes. The latest head still needs an H20 or H100 rerun to verify the CUDA Flash Attention path before the CUDA test coverage is considered resolved. An MI300X cannot provide that proof because ROCm uses different attention backends, kernels, and graph implementations. The MI300X results therefore validate only the ROCm Triton and ROCm AITER paths.BEFORE SUBMITTING: read CONTRIBUTING.md and run the precheck-pr skill with the code agent for a self-check against project conventions.
(anything written below this line will be removed by GitHub Actions)