Repository navigation
Conversation
…every chunk MiniCPM-o 4.5's Code2Wav stage builds its HiFT vocoder with weight norm live on all 82 convolution weights and leaves it that way for the life of the server. An inference weight never changes, so every streamed audio chunk recomputes `g * v / ||v||` for each of them -- a norm reduction and a broadcast multiply per convolution -- and gets the same tensor back every time. `parametrize.remove_parametrizations(..., leave_parametrized=True)` stores the weight the module is already using and drops the parametrization, so the arithmetic is unchanged; it simply happens once, at load, rather than on the streaming path. Measured on an Atlas A3 inside one process on one loaded HiFT, arms alternating: 18.16 ms per vocoder call with the parametrization live against 13.12 ms folded -- -5.03 ms, -27.7% -- and the five samples per arm do not overlap. Output is unchanged to 7e-4 absolute against a 0.23 peak, which is the run-to-run noise of the vocoder itself: the same arm re-run differs by 7.3e-4, the two arms by 6.3e-4. In situ, timed around the call itself during two paired rounds with the dies swapped, -1.05 and -9.59 ms per call; the spread is contention, since an in-situ call is mostly spent waiting at HiFT's own host-to-device copies, and the mean of -5.3 ms is where the isolated figure says it should be. Five vocoder calls per request, counted in the same runs. End-to-end latency does not resolve it, and the honest reason is structural rather than a measurement failure: on an unmodified tree stage 2 runs behind the autoregressive stages, so those ~25 ms per request sit off the critical path and four paired die-swapped rounds read +0.38%, +1.14%, -1.18%, -2.30%. The saving becomes visible exactly when the AR stages get faster, which is the direction every open performance change on this model is pushing. So the claim here is the per-call figure, and the end-to-end rounds are reported only as a no-regression check. HiFT does ship a `remove_weight_norm()` of its own, but it calls the legacy `torch.nn.utils.remove_weight_norm`, which only knows the forward-pre-hook form. `flashcosyvoice` selects `torch.nn.utils.parametrizations.weight_norm` whenever torch provides it, so on any current torch that method raises `ValueError: weight_norm of 'weight' not found` rather than folding anything -- and nothing in the serving path calls it. `fold_weight_norm` handles both spellings and returns the names it folded, so a no-op shows up in the log line instead of passing silently. The fold is on by default; `token2wav_hift_fold_weight_norm: false` in the stage's `extra` config turns it off. It is not accelerator-specific -- the work it removes is the same everywhere -- and that is why the unit tests are plain CPU tests over both spellings. Signed-off-by: junbo <88826518+Junbo2002@users.noreply.github.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. |
|
Self-review, as Blast radius. One new module-level function, four lines calling it in The four things I would attack first if this landed in my queue:
Why it has not been done already. HiFT ships its own |
|
This PR appears to be related to model: minicpm. Model owners: @y-null @Junbo2002, 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. |
hsliuustc0106
added
omni
Omni ReviewBot: no human activity for 21 days@Junbo2002 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: no human activity for 30 days@Junbo2002 this pull request has had no human commit, comment or review since 2026-09-02. Please consider closing this PR if the work is no longer planned. 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 cursor (cursor-grok-4.6-high) under experiment |
vllm-omni-review-bot
left a comment
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
1 actionable finding(s).
CI at
0b56999c4bf5(2026-10-10T07:56:11.319404+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
Full review analysis
Scan:
| Category | Result |
|---|---|
| Tests / verification | 1 finding(s) below |
| Security | no finding reported |
| Docs / comments | no finding reported |
| Behavior / compatibility | no finding reported |
| Correctness | no finding reported |
Validated:
- [resolved] 'changing loaded weights' / state_dict key rewrite: fold is after hift.pt load; load_weights returns named_parameters() post-fold. Residual: no consumer of HiFT state_dict after construction found in serving, but tests never load a real hift.pt.
- [resolved] HiFTGenerator.remove_weight_norm (hifigan.py:478) uses legacy remove_weight_norm and calls m_source.remove_weight_norm which SourceModuleHnNSF does not define; PR does not call it. Residual: name-gate skip still no-ops.
- [resolved] leave_parametrized=True bit-identity: tests assert_close rtol=0 atol=0 at test_code2wav_batching.py:1362; residual: fixture has no ConvTranspose1d/f0_predictor (round 1)
- [resolved] production CUDA/NPU graphs capture after fold (batched_token2wav.py:234; npu minicpmo_4_5_code2wav.py:230); residual: test_cuda_graph_wrapper.py:59 still unfolds (round 1)
- [claim-verified] 82 HiFT convs off-fixture: _build_hift upsample_rates=[8,5,3] → conv_pre+3 ups+conv_post+96 res+36 source_res+5 f0 = 82; source_downs are plain Conv1d and correctly skipped.
- [claim-verified] only product caller of fold_weight_norm is
_build_backendon token2wav.hift (code2wav.py:906); StepAudio2/GLM-TTS/CosyVoice3 build separate HiFTGenerator instances
1 actionable finding(s).
Verdict: COMMENT
See inline comments below.
🤖 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!
| else: | ||
| from torch.nn.utils.parametrizations import weight_norm | ||
|
|
||
| self.conv_pre = weight_norm(nn.Conv1d(4, 4, 3, padding=1)) |
There was a problem hiding this comment.
[P2] The new fold tests only build _WeightNormedStack with weight-normed Conv1ds…
Evidence and suggested fix
The new fold tests only build _WeightNormedStack with weight-normed Conv1ds and call fold_weight_norm directly (test_code2wav_batching.py:1337-1357). They never enter MiniCPMO45Code2Wav._build_backend (the only backend test monkeypatches it away at test_code2wav_batching.py:1140) and never set token2wav_hift_fold_weight_norm to false — that extra exists only at minicpmo_4_5_code2wav.py:905-907. Production HiFT ups are weight_norm(ConvTranspose1d(...)) (hifigan.py:432-441, via upsample_rates=[8,5,3] in step_audio2_token2wav.py:99-106). Deleting those four dispatcher lines, or a fold that skipped ups, still leaves pytest -sv tests/model_executor -m 'core_model and cpu' green. Drive a backend-level case that the extra flag actually gates the call, and put a weight-normed ConvTranspose1d on the fixture (assert its .weight is in folded).
Evidence: Trigger: the PR’s new CPU tests construct _WeightNormedStack and call fold_weight_norm directly; they never run MiniCPMO45Code2Wav._build_backend or pass token2wav_hift_fold_weight_norm. Unmet requirement: those tests stay green if the production dispatcher is deleted, and they never assert a folded ConvTranspose1d even though production HiFT ups are that type. tests/model_executor/models/minicpmo_4_5/test_code2wav_batching.py:1337 self.conv_pre = weight_norm(nn.Conv1d(4, 4, 3, padding=1)) ; test_code2wav_batching.py:1338 self.body = nn.Sequential(weight_norm(nn.Conv1d(4, 4, 3, padding=1))) ; test_code2wav_batching.py:1357 folded = fold_weight_norm(model) ; vllm_omni/model_executor/models/minicpmo_4_5/minicpmo_4_5_code2wav.py:905 if bool(extra.get("token2wav_hift_fold_weight_norm", True)): ; minicpmo_4_5_code2wav.py:906 folded = fold_weight_norm(token2wav.hift) . Unchanged by this diff, present in the PR-time tree: test_code2wav_batching.py:1140 monkeypatch.setattr(model, "_build_backend", build_backend) ; vllm_omni/model_executor/models/step_audio2/step_audio2_token2wav.py:101 upsample_rates=[8, 5, 3], ; vllm_omni/model_executor/models/cosyvoice3/code2wav_core/hifigan.py:433-435 weight_norm( / ConvTranspose1d( . Grep: token2wav_hift_fold_weight_norm has no test-file hits; only code2wav.py:905.
Suggestion: self.conv_pre = weight_norm(nn.Conv1d(4, 4, 3, padding=1))
self.up = weight_norm(nn.ConvTranspose1d(4, 4, 4, stride=2, padding=1))
self.body = nn.Sequential(weight_norm(nn.Conv1d(4, 4, 3, padding=1)))
amy-why-3459
left a comment
amy-why-3459
left a comment
There was a problem hiding this comment.
Reviewed commit 0b56999c4bf50a6b2f9605ff84954f7a2632e0ac.
No actionable bugs were found in this static review. This does not prove the code is correct.
No actionable issues found in this static review.
Validation: Verified the exact head and inspected the full diff against the specified merge base, both changed files, backend construction and weight loading, the vendored HiFT implementation, batching and graph initialization, NPU integration, and relevant tests. Runtime/GPU tests were not run.
4 participants
Purpose
MiniCPM-o 4.5's Code2Wav stage builds its HiFT vocoder with weight norm live on
all 82 convolution weights, and leaves it that way for the life of the
server. An inference weight never changes, so every streamed audio chunk
recomputes
g * v / ||v||for each of them — a norm reduction plus a broadcastmultiply, per convolution, per call — and gets the same tensor back every time.
parametrize.remove_parametrizations(..., leave_parametrized=True)stores theweight the module is already using and drops the parametrization. The
arithmetic that produces the weight is unchanged; it simply happens once, at
load, instead of on the streaming path.
Why this has been sitting there
HiFT does ship a
remove_weight_norm()of its own — but it calls the legacytorch.nn.utils.remove_weight_norm, which only knows the forward-pre-hookspelling, while
flashcosyvoiceselectstorch.nn.utils.parametrizations.weight_normwhenever torch provides it. On anycurrent torch that method raises
rather than folding anything, and nothing in the serving path calls it anyway.
fold_weight_normtherefore handles both spellings and returns the names itfolded, so a no-op shows up in the log line instead of passing silently.
Scope and blast radius
which is why the tests are plain CPU tests over both spellings. The numbers
below are from an Atlas A3; no claim is made about the size of the win
elsewhere.
_build_backend, afterToken2wav(...)has loadedflow.ptandhift.ptinside its own constructor. The stage'sload_weightsputs nothingfurther into HiFT, so no later weight load can be surprised by it.
state_dictkeys —parametrizations.weight.original0/1become
weight. Nothing in the serving path reads that state dict afterconstruction.
token2wav_hift_fold_weight_norm: falsein the stage'sextraconfig turns itoff.
Test Plan
vLLM Version:
0.1.dev1+ge5588e49b(built fromvllm-project/vllm@e5588e49b)vLLM-Omni Commit: this branch is against
main. The served numbers belowwere taken on
ecd9d99dainstead, and here is why: the vLLM in the Ascend imagethese machines run (
0.1.dev1+ge5588e49b) has novllm.v1.engine.utils.CoreEngineLaunch,which
vllm_omni/engine/stage_engine_startup.pyimports onmain, somaincannot start a server there at all. The changed code is identical on both bases
—
_build_backendinminicpmo_4_5_code2wav.py— and the unit tests below runon
mainitself.1. Unit tests
tests/model_executor/models/minicpmo_4_5/test_code2wav_batching.py, 4 newcases, all pure CPU: the folded weight is bit-identical to the effective weight
the module was already using and the forward output is unchanged — parametrized
and legacy-hook spellings; a plain module folds nothing; the fold is
idempotent.
2. What the fold removes, on the real vocoder
Both arms in one process, on one loaded HiFT, alternating. Folding is
reversible for measurement purposes: re-applying
weight_normto a foldedconvolution recovers exactly the same effective weight (
g = ||v||,v = w), sothe two arms can be swapped as often as we like. That takes host load, die
identity and weight layout out of the comparison, which matters on a shared box.
3. In situ, during a ranked run
The same paired round, but with a
time.perf_counter_ns()wrapper around theHiFT call itself in both arms, so the change is attributed where it happens
rather than inferred. (A host-side timer is fair here because HiFT's own
_istftcopies its window up from host memory, which drains the device queue —so the call is effectively synchronous on both arms.)
4. End-to-end, paired and die-swapped
Two arms differing only by this diff, both on
ecd9d99da, the baseline deployconfig, no environment variables. Both arms run at the same time, one per die,
and the round is repeated with the dies swapped — this box is shared and its two
dies are not equally fast, so only a same-round paired delta means anything.
Test Result
1. Unit tests
tests/model_executor/models/minicpmo_4_5/, same machine, same interpreter:main(e51fe6ec1)ecd9d99daecd9d99daThe four new cases are the whole difference. The three failures are on
mainitself, identical in both arms and unrelated to this change:
test_code2wav_batching.py::test_npu_patch_preserves_ragged_estimator_contractand two
test_cuda_graph_wrapper.pycases, which want a newer vLLM than theAscend image on this machine ships. The 13 errors in the
ecd9d99darows were amissing
pytest-mock, installed since.2. Per vocoder call — Atlas A3 / 910C, one process, arms alternating
82 weights folded. The ranked configuration makes 5 vocoder calls per
request, counted in the run in §4.
This is on an otherwise idle device. Two things say the
livearm here is afaithful stand-in for the as-loaded module: re-applying
weight_normto a foldedconvolution recovers the same effective weight by construction, and its 18.16 ms
lands within 2% of the 17.80 ms that the untouched base spends per vocoder call
outside its own host-to-device copies (§4).
3. Numerically unchanged
The difference between the arms is smaller than the difference between two
runs of the same arm, which is what "bit-exact arithmetic on a
non-deterministic device" looks like. The CPU unit tests assert exactness
directly.
4. In situ, ranked configuration
A
time.perf_counter_ns()wrapper around the HiFT call itself, in both arms,during a paired ranked round:
160 calls in every arm — five per request, exactly. The per-round spread is
large because an in-situ HiFT call is mostly waiting, and how long it waits
depends on what the AR stages are doing on the same die; that is why the round is
run under both die assignments. Averaged over the two, −5.3 ms per call,
which is where the isolated −5.03 ms of §2 says it should be.
For context on what that 66 ms is: a separate probe on the untouched base times
HiFT's own three unpinned host-to-device copies (the harmonic multiplier, and the
STFT window twice) at 4.42 + 2 x 22.11 = 48.6 ms per call of blocking. So of
a 66.4 ms vocoder call, 48.6 ms is the thread waiting at those copies and
17.8 ms is everything else HiFT does — which is the 18.16 ms of §2, and the
5.03 ms this PR removes comes out of it. (Those copies are a separate problem and
not this PR's; removing them does not need this change and this change does not
need them removed.)
5. End-to-end
The two §4 rounds — same protocol, with the timer in both arms — read −1.18%
and −2.30% on the same column. So across four paired die-swapped rounds the
deltas are +0.38%, +1.14%, −1.18%, −2.30%: the sign does not settle, and this
change is not resolvable end-to-end on this base. I would rather say that than
pick the two rounds I like.
That is expected rather than disappointing. On this base stage 2 runs behind
the Talker: a saving inside the vocoder is hidden by the AR stage in front of it,
and only the final chunk is on the critical path, so ~25 ms of vocoder time per
request turns into a few ms of e2el — well under the 1.35% two identical arms
have measured apart on these dies (
ctlitself drifted 2.1% between rounds 1and 2). It stops being hidden as soon as the AR stages get faster, which is what
most of the open performance PRs on this branch do, and on any deployment where
stage 2 is not sharing a die with them.
The arms are otherwise identical: 32/32 completed, 435 output tokens and 135.64 s
of audio in all four arms. The measurements that should carry this PR are §2
and §4.