Repository navigation
[NPU][CosyVoice3] Add CPU STFT fallback - #7520
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4b82a05f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| target_dtype = x.dtype | ||
| spec = torch.stft( | ||
| x, | ||
| x.float().cpu(), |
There was a problem hiding this comment.
Keep host STFT transfers out of CUDA graph capture
When MiniCPM-o runs on CUDA with enable_hift_graph, HiFTGraphWrapper._capture calls this shared HiFT _inference_pre_istft implementation inside torch.cuda.graph; the new .cpu() device-to-host transfer is not capturable, so graph construction raises during BatchedToken2Wav initialization instead of enabling the existing feature. Preserve the device-local STFT path on CUDA, or move the CPU fallback outside the captured region.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 827cd8d8. The CPU fallback is now gated to device.type == "npu"; CUDA and CPU retain the native device-local STFT/ISTFT path, so MiniCPM-o HiFT CUDA graph capture no longer performs a device-to-host transfer. I also added regression coverage for non-NPU native execution, NPU routing, and fallback precision/device restoration.
|
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 @peterDengcx, 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. |
Signed-off-by: dengchangxiong <dengchangxiong1@huawei.com>
f4b82a0 to
827cd8d
Compare
|
Self-review: I verified that the fallback is scoped to NPU, while non-NPU STFT/ISTFT continues to use the native device-local path. I reviewed the shared MiniCPM-o CUDA graph call path and added regression coverage for NPU routing, CPU float32/complex64 execution, output device/dtype restoration, and the unchanged native path. Focused Ruff checks pass; pytest was not run locally, per request. |
linyueqian
left a comment
There was a problem hiding this comment.
Approving at 827cd8d8. Ascend has no torch.stft / torch.istft, so CosyVoice3's HiFT vocoder fails before it produces a waveform; the change routes only those two transforms through a CPU float32 round trip and only when the input lives on an NPU, then moves the result back to the caller's device and dtype. The CUDA and XPU paths are byte-for-byte what they were, including _get_stft_window and the CUDA-graph-captured path, and the CPU contract tests pin the routing, the precision of the fallback and the device and dtype restoration. The cost is the stated one: on NPU every chunk pays two host transfers plus a CPU FFT, which is the right trade until Ascend grows the operator. One consistency point about how the platform is detected is inline; it does not change the verdict.
Two coordination notes rather than review findings: #7521 rewrites other parts of this same file for the bounded-window streaming path and is about to merge, so whichever of the two lands second needs a small rebase here (the hunks do not overlap); and the hardware evidence is the NPU lane, which I have made run by adding ready. Reviewed statically; fork head, no PR code executed. Merge on a green general lane.
| block.remove_weight_norm() | ||
|
|
||
| def _stft(self, x): | ||
| if x.device.type == "npu": |
There was a problem hiding this comment.
[suggestion] The rest of the tree gates Ascend-specific paths on current_omni_platform.is_npu() (step_audio2_token2wav.py:148, common/alias_free_activation.py, common/qwen3_code_predictor.py) or carries them as patches under vllm_omni/platforms/npu/models/, where the HiFT sine-source fix for Step-Audio2 already lives. A per-call x.device.type == "npu" check works, but it is decided on every chunk and it is the only place in the vocoder that keys on the tensor's device string rather than the platform. Either switch these two branches to current_omni_platform.is_npu() or move _stft_on_cpu / _istft_on_cpu into an NPU patch module so the shared HiFT stays platform-neutral. Not holding the approve on it.
Signed-off-by: dengchangxiong <dengchangxiong1@huawei.com> Co-authored-by: dengchangxiong <dengchangxiong1@huawei.com> Signed-off-by: Matthieu Laneuville <matthieu.laneuville@surf.nl>
Signed-off-by: dengchangxiong <dengchangxiong1@huawei.com> Co-authored-by: dengchangxiong <dengchangxiong1@huawei.com>
Motivation
CosyVoice3 code2wav invokes
torch.stftandtorch.istftin the HiFiGAN vocoder. The NPU backend does not support these operators, so synthesis fails before a waveform is produced.Modification
Self-test
BC-breaking
No public API or configuration change. On NPU only, the CPU fallback may increase waveform synthesis latency.
Checklist
AI assistance
This PR was prepared with Codex assistance.