Repository navigation
[perf] Minicpm o4.5 optimization - #5228
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. |
hsliuustc0106
added
the
enhancement
ChatGPT Codex Connector
ecd86ea to
0881b20
Compare
R2-Y
changed the title
hsliuustc0106
left a comment
hsliuustc0106
left a comment
There was a problem hiding this comment.
why this model optmization brings so many changes to core modules
| @@ -0,0 +1,75 @@ | |||
| # MiniCPM-o 4.5 opt-in batching deploy: 3-GPU split layout. | |||
There was a problem hiding this comment.
why we need add another yaml?
R2-Y
changed the title
|
This optimization will affect minicpm-o-4.5 full-duplex work, because talker is split into 2 stages. I will refactor this pr after full-duplex merged, some core modules change will be deleted. |
7cd36aa to
baf7bca
Compare
amy-why-3459
changed the title
|
Thanks for the three-stage split and the solid Code2Wav batching work — the audio Request changes before merge:
|
- split Talker AR codec generation from batched Code2Wav - stream request-owned codec chunks through shared memory - add exact-shape Token2Wav batching without serial fallback - preserve Thinker latent payloads across async stage routing - isolate per-request AR, flow, HiFT, and connector state - keep Token2Wav assets in native fp32 under bf16 runners - update deploy configs, recipe, validation, and benchmarks Signed-off-by: R2-Y <ruiruyang2@gmail.com>
Signed-off-by: R2-Y <ruiruyang2@gmail.com>
Use the total decode step for minimum audio length instead of the 16-token repetition window, preventing EOS from being permanently masked and eliminating repeated max-length audio and noisy truncation. Signed-off-by: R2-Y <ruiruyang2@gmail.com>
- disable reasoning for single-letter Daily-Omni evaluation - scope image-edits bot_task defaults to the image backend - add streaming text/audio client with valid WAV reconstruction - fix Talker sampling regression test signature - update three-stage deployment and CUDA Graph documentation Signed-off-by: R2-Y <ruiruyang2@gmail.com>
- enable the TTS template before rendering audio requests - bound Talker generation by conditioning length and context limits - rebalance single-GPU memory budgets across all three stages - reduce per-stage concurrency to limit GPU contention - remove redundant Talker and Code2Wav batch statistics logging - update MiniCPM-o deployment docs and regression tests Signed-off-by: R2-Y <ruiruyang2@gmail.com>
Align Talker conditions with async-chunk prewarm placeholders, restore sessionless streaming and native duplex stability, and update three-stage deployment tests after rebasing onto main. Signed-off-by: R2-Y <ruiruyang2@gmail.com>
linyueqian
left a comment
linyueqian
left a comment
There was a problem hiding this comment.
The engineering here is substantial and much of it is genuinely good — the strict three-stage continuous batching with runner-owned per-request state, the exact-shape batched vocoder that never falls back to Token2wav.stream, cache_epoch staleness guards with paired lifecycle cleanup, and the EOS min-length fix are all real improvements over the fused stage 1. But before a same-day merge there are two project-level decisions hiding in this PR that I think need to be made explicitly, plus a feature regression and a perf question.
1. This PR and #5291 are two independent implementations of the same stage split. Both rewrite minicpmo_4_5_omni_tts.py wholesale, both rewrite pipeline.py to a 3-stage topology, and they touch the same deploy yamls (#5291 modifies the 2gpu/3gpu profiles this PR deletes) — whichever lands second is a rewrite, not a rebase. The project should pick one implementation and close the other before either merges; reviewing both to completion is wasted effort. Mechanically this PR is the superset (third stage + batching + lifecycle tests) — but that makes the choice more urgent, not less.
2. Native full-duplex — merged three days ago in #3907 — does not survive this PR, and the head commit's title says the opposite. "preserve duplex handoff alignment" fixed the half-duplex 3-stage handoff plumbing; the duplex substrate itself is deleted: the per-turn streaming generators and ~1 s unit waveforms are gone, stage-1 outputs no longer carry any of the metadata the duplex data plane requires (tts_is_last_chunk, turn/epoch ids, segment text), stop_token_ids becomes the binary stop vocab [1] so the data plane's 151645 segment-end gate can never fire, the PIECEWISE cudagraph constraint and its rationale were removed from minicpmo_4_5_duplex.yaml, and 11 duplex regression tests are skip-marked as "not applicable" rather than passing (inline comment). The author said this honestly in the pre-rebase comment ("will affect minicpm-o-4.5 full-duplex work… I will refactor this pr after full-duplex merged") — full-duplex has now merged, so that refactor is no longer deferrable bookkeeping; it's the sequencing question for this merge. Options: (a) restore duplex behavior behind the new topology before landing, (b) land with the duplex yamls and skip-marks reverted/held so session_mode: duplex keeps the fused path, or (c) land as-is with the PR description stating plainly that native duplex is broken pending the follow-up — but that should be a deliberate maintainer decision, not a side effect.
3. Per-request voice cloning is silently dropped. llm2tts still writes ref_audio into the transport buffer, but nothing in the new talker or code2wav consumes it — nothing anywhere produces prompt_cache_id/prompt_wav, so the vocoder always uses the bundled HT_ref_audio.wav (inline comment). That's a user-visible regression absent from the PR description, and it also orphans the ref-wav LRU ↔ vocoder-cache eviction pairing that fixed a worker memory leak during the #3907 cycle.
4. The perf story needs one clarification before the numbers are actionable. The table honestly reports the wins (AUDIO_TTFP −75 %, RTF −61 %, throughput +83 %) and the costs (TPOT +300 %, ITL +302 %, TTFT +15 %) — the text-stream degradation is consistent with three co-located stages contending on one GPU plus the thinker's max_num_seqs 16→4 / memory 0.8→0.65 cuts, and deserves the explanation already requested. Separately, this same PR changes the benchmark harness default (enable_thinking=False for daily-omni) — if the #5233 baseline ran with thinking enabled, the headline throughput/E2EL comparison isn't apples-to-apples. Seconding the request to split the harness change out and state which setting both columns used.
Smaller: the failing pre-commit is just ruff-format on stage_input_processors/minicpmo_4_5_omni.py (one missing blank line, pre-commit run --all-files fixes it); the generic-path edits (scheduler session REPLACE semantics in omni_ar_scheduler, the re-touch of the async-chunk partitioning condition that previously caused the #4527/#4792 starvation cycle, synthesized sender specs in stage_init_utils) would benefit from a sentence each in the description since they alter shared behavior; and get_tts_handoff imported from experimental.fullduplex.engine into a production model file inverts the experimental/production layering.
Live verification note: the shared H20/H200 machines are mid-migration, so I could not independently reproduce the benchmark table or run a duplex session against this branch; happy to do both once hardware is back.
| ) | ||
|
|
||
|
|
||
| @pytest.mark.skip(reason=_FUSED_TALKER_REMOVED_REASON) |
There was a problem hiding this comment.
🔴 [blocking] These 11 skip-marks are the crux of the duplex question: the assertions being skipped (unit-waveform padding, turn-metadata export, ref-LRU↔vocoder-cache eviction pairing) are exactly the behaviors the merged #3907 runtime depends on, and "not applicable" here means the behaviors are gone, not relocated — no equivalent assertions exist against the new talker/code2wav stages. A test suite that skips its duplex regression tests can't tell us whether duplex works, and in this case it doesn't: stage-1 outputs carry none of the metadata the duplex data plane gates on, and the duplex yaml's stop tokens no longer match its segment-end detection. Whatever the merge decision is (restore, hold the duplex yaml, or land with an explicit known-breakage note), these skips should either become real tests against the new topology or be accompanied by a tracking issue that says native duplex is down until the follow-up.
There was a problem hiding this comment.
removed all 11 fused-Talker skips and the stale fused-vocoder path test.
| """ | ||
| ids = payload_data.get("ids", {}) | ||
| hidden_states = payload_data.get("hidden_states", {}) | ||
| tts_token_ids = ids.get("tts") if isinstance(ids, dict) else None |
There was a problem hiding this comment.
🟡 [important] Model-specific payload sniffing (ids["tts"] / hidden_states["tts"]) in the generic connector adapter — and the matched branch has aggressive semantics for shared code: it clears the request's accumulated output tokens, resets num_computed_tokens, and swaps in a zero-filled prompt. Today no other model writes a "tts" key so it's de-facto scoped, but this is precisely the model-logic-in-shared-path pattern already flagged in review — one more model adopting the key silently inherits request-wiping behavior. Suggest routing this through the pipeline's custom_process_* hooks or a capability flag on the stage config rather than key-sniffing in the adapter.
There was a problem hiding this comment.
Changed to an explicit generic payload contract. The connector and AR scheduler only act when meta.replace_streaming_prompt=true and use meta.next_stage_prompt_len; they do not inspect TTS keys. MiniCPM-specific handoff construction/consumption remains in the stage processor and model. The blanket downstream-stage replacement was also narrowed to this explicit metadata flag. Connector direction is resolved by shared stage_receives_chunks / stage_sends_async_output helpers consumed by GPU and NPU runners, rather than runner-local stage or model checks.
| embeds = self.emb_code[0](code) | ||
| return input_ids, embeds, {} | ||
|
|
||
| def _request_generator(self, request_id: str, device: torch.device) -> torch.Generator: |
There was a problem hiding this comment.
🟡 [important] Codec sampling is fully hardcoded here (T=0.8, top_k 25, top_p 0.85, rep 1.05, EOS masked 50 steps) with a per-request generator seeded at a fixed 42 — which makes the stage-1 yaml's temperature: 0.0 / seed: 42 dead configuration and silently removes user seed control over voice sampling. The fixed seed is arguably an improvement in reproducibility over main (which never seeded this path), but given this repo's history with silent seed-propagation changes (#4869 → the TTS perf-regression hunt), this deserves (a) a comment in the yaml saying stage-1 sampling params are model-owned and ignored, and (b) a note in the PR description that voice sampling is now deterministic-fixed regardless of request seed.
| self._min_batch_size = int(extra.get("code2wav_min_batch_size", 1)) | ||
| if self._min_batch_size < 1: | ||
| raise ValueError("MiniCPM-o Code2Wav code2wav_min_batch_size must be >= 1") | ||
| self._default_prompt_id = str(extra.get("prompt_cache_id", "HT_ref_audio")) |
There was a problem hiding this comment.
🟡 [important] This default is currently the only path: nothing produces prompt_cache_id/prompt_wav anywhere in the PR, and while llm2tts still writes the request's ref_audio into the transport buffer, no consumer remains — so every request vocodes with the bundled HT_ref_audio.wav voice. On main, per-request ref voices work (_write_ref_audio_prompt_wav + LRU with paired vocoder-cache eviction). If dropping voice cloning is intentional for this PR, it should be stated in the description and tracked; if not, the ref-audio identity needs to travel with the codes into this stage, and _prompt_features (currently unbounded) will then need the LRU + eviction pairing the fused implementation had — its regression test is among the skip-marked ones.
Signed-off-by: R2-Y <ruiruyang2@gmail.com>
baf7bca to
f2a2169
Compare
Keep prompt replacement payload-driven, preserve duplex and reference-voice metadata through Code2Wav, and document the split-stage deployment and performance contract. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: R2-Y <ruiruyang2@gmail.com>
|
About the part of the adaptation for Full Duplex, I suggest it as a follow-up bugfix PR. I will proposal it asap. |
…zation Signed-off-by: R2-Y <ruiruyang2@gmail.com>
|
Performance update:
Code2Wav streaming improves throughput 3.19x and reduces audio TTFP by 80%.
|
…r payloads Signed-off-by: R2-Y <ruiruyang2@gmail.com>
Sy0307
added
ready
Signed-off-by: R2-Y <ruiruyang2@gmail.com> Co-authored-by: Sy03 <1370724210@qq.com>
The offline example was written against the fused two-stage topology and was never updated when the Talker/Code2Wav split landed in vllm-project#5228, so `run_single_prompt.sh` cannot produce correct audio on current main. Two problems: 1. `sampling_params_list` was built from two entries and then sliced with `[: omni.num_stages]`. With `num_stages == 3` the slice is a no-op and `generate()` raises `ValueError: Expected 3 sampling params, got 2`. Add the missing Code2Wav entry. 2. The Talker used `max_tokens=1`. That was correct while Token2Wav ran in-process and a single decode step produced the whole waveform. After the split, each step advances the codec stream, so `max_tokens=1` stops it after one chunk and yields ~0.16 s of audio. This fails silently: the run still exits 0. Use `min_tokens=50` / `max_tokens=2048` / `stop_token_ids=[1]`, matching stage 1 of the deploy configs. Test Plan: python examples/offline_inference/minicpmo/end2end.py \ --model <local MiniCPM-o-4_5> \ --deploy-config vllm_omni/deploy/minicpmo_4_5_3gpu.yaml \ --query-type text --output-wav ./smoke_out Test Result: Before: `ValueError: Expected 3 sampling params, got 2`. With only fix 1, the run exits 0 but emits a 0.16 s WAV. After: exits 0 and emits 24 kHz mono audio of the expected length (1 ch / 24000 Hz / 10.24 s for the default text prompt); text output unchanged. Verified on 3x H800, vLLM 0.26.0, vLLM-Omni a951dd0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: eval-dev <0xe5bca0@gmail.com>
The offline example was written against the fused two-stage topology and was never updated when the Talker/Code2Wav split landed in vllm-project#5228, so `run_single_prompt.sh` cannot produce correct audio on current main. Two problems: 1. `sampling_params_list` was built from two entries and then sliced with `[: omni.num_stages]`. With `num_stages == 3` the slice is a no-op and `generate()` raises `ValueError: Expected 3 sampling params, got 2`. Add the missing Code2Wav entry. 2. The Talker used `max_tokens=1`. That was correct while Token2Wav ran in-process and a single decode step produced the whole waveform. After the split, each step advances the codec stream, so `max_tokens=1` stops it after one chunk and yields ~0.16 s of audio. This fails silently: the run still exits 0. Use `min_tokens=50` / `max_tokens=2048` / `stop_token_ids=[1]`, matching stage 1 of the deploy configs. Test Plan: python examples/offline_inference/minicpmo/end2end.py \ --model <local MiniCPM-o-4_5> \ --deploy-config vllm_omni/deploy/minicpmo_4_5_3gpu.yaml \ --query-type text --output-wav ./smoke_out Test Result: Before: `ValueError: Expected 3 sampling params, got 2`. With only fix 1, the run exits 0 but emits a 0.16 s WAV. After: exits 0 and emits 24 kHz mono audio of the expected length (1 ch / 24000 Hz / 10.24 s for the default text prompt); text output unchanged. Verified on 3x H800, vLLM 0.26.0, vLLM-Omni a951dd0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: eval-dev <0xe5bca0@gmail.com>
Signed-off-by: R2-Y <ruiruyang2@gmail.com> Co-authored-by: Sy03 <1370724210@qq.com>
5 participants
PLEASE FILL IN THE PR DESCRIPTION HERE.
Purpose
1.Split MiniCPM-o 4.5 into Thinker, Talker AR, and batched Code2Wav stages.
2. Stream request-isolated codec chunks through async connectors.
3. Add exact-shape Code2Wav batching and lifecycle cleanup.
4. Generalize connector routing and runner metadata capabilities.
Test Plan
daily-omni benchmark test
Test Result
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)