Repository navigation
[Model] Accept MiniCPM-o duplex video-only units - #8971
Conversation
e973ca4 to
f732305
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
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/ar_runtime.md, docs/design/module/model_integration.md. Module owners: @tzhouam @fake0fan @Gaohan123 Routing: @tzhouam via module of the changed files, CODEOWNERS; @fake0fan via module of the changed files; @Gaohan123 via module of the changed files @amy-why-3459, 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. |
|
@natureofnature PTAL |
|
The commit path finalizes the turn before draining all queued VISION units. With Please submit the queued units before sealing the turn, and add a regression test covering two frame-only appends followed by one commit/response.create, including a case with residual speech. |
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
8053409 to
e836a25
Compare
NumberWan
left a comment
There was a problem hiding this comment.
LGTM
The commit path on e836a25 drains queued VISION units before the terminal reservation. Two frame-only appends, with or without a speech tail, stay in order, and a commit during an active response keeps those frames ahead of the speech tail. The shared prefix hook is a no-op for other models. A VISION unit no longer takes the default audio token budget, and the audio-encoder index stays on the first window until real audio is encoded.
NumberWan
left a comment
There was a problem hiding this comment.
LGTM
The commit path on e836a25 drains queued VISION units before the terminal reservation. Two frame-only appends, with or without a speech tail, stay in order, and a commit during an active response keeps those frames ahead of the speech tail. The shared prefix hook is a no-op for other models. A VISION unit no longer takes the default audio token budget, and the audio-encoder index stays on the first window until real audio is encoded.
3706293 to
2e839d4
Compare
|
@natureofnature PTAL |
natureofnature
left a comment
There was a problem hiding this comment.
The earlier queued-frame commit issue is addressed: prefixes now drain in order before the terminal frame/speech payload. I reviewed the buffer/runner, scheduler/Stage0, client/demo/browser paths and regression tests at 2e839d4.
I suggest running the current-head MiniCPM-o cases on the supported H100/A3 target with the original checkpoint before merging, since these changes alter encoder state, prompt slots and native response lifecycle: single-session speech, video input, and video-only units followed by speech should complete with expected audio/lifecycle events and no stale encoder state or padding mismatch. Also cover two queued frames with and without speech, response creation enabled/disabled, active-response deferral and rollback, confirming FIFO delivery without loss or duplication. The reported 889 CPU passes are for a6007c4; current-head backend Buildkite statuses are failing, so their failure causes and relevant test results still need to be established. Current-head pre-commit, wheel and CodeQL checks passed.
【Conclusion: Found blocking code issues and validation gaps in commit 2e839d4.】
【Action recommended: Fix audio budgeting, tail-frame alignment and queued-frame limits; verify mixed inputs and manual commits preserve frames, token counts and response completion on the supported target before merging.】
| # previous one with its sampled terminator + </unit>. | ||
| lead_tokens = duplex_first_append_context_reserve(runtime_config) if seq <= 1 else 2 | ||
| token_budget = lead_tokens + 1 + vision_tokens | ||
| elif seq <= 1: |
There was a problem hiding this comment.
[P2] The first audio append can now have seq > 1 after a VISION unit. In a manual-response session with one frame-only append followed by 2 s of speech and a commit, this branch reserves 25 slots for the audio, while Stage0's first-audio path consumes the longer first window and builds one unit (13 slots). The remaining 12 slots become padding in the model context. Could the budget track the first audio window independently of append sequence, and cover VISION → multi-second audio by checking reserved slots against produced embeddings?
There was a problem hiding this comment.
Fixed. The first PCM payload is marked independently of the append sequence, because a VISION unit advances seq without opening the mel window.
After one frame and a 2 s speech commit, the audio reservation is 13 slots: the previous terminator, </unit>, <unit>, and the ten audio tokens of the single first-window unit. It is no longer 25. Three buffered seconds reserve 25, which is the two units that window actually builds. A later exact chunk still uses the per-second budget. The marker is removed before the payload is handed to Stage0.
test_the_first_audio_window_after_a_vision_unit_is_not_the_append_sequence checks those slot counts. test_first_audio_after_vision_keeps_the_first_mel_window_across_rollback checks that rolling the first audio reservation back still treats the retry as the first window.
There was a problem hiding this comment.
The direct VISION → speech case is fixed, but the first-window marker can still disagree with Stage0 after automatic silence. If VISION starts a response and a silence continuation runs, that continuation advances the audio encoder without going through this buffer. A subsequent 2 s speech payload buffered with overlap_action='listen' is still marked first_audio_window on commit: the scheduler reserves 13 slots, while Stage0 now builds 25, and the embedding slice truncates the tail. Could the budget state also track accepted silence continuations, with a VISION → generated silence → buffered 2 s speech regression asserting reserved and produced token counts match? Marker, continuation bypass.
There was a problem hiding this comment.
Fixed in bf64486. An accepted silence continuation now marks the PCM buffer's audio window as started, because that unit is encoded outside the buffer and consumes the 1035 ms first window. A later 2 s speech payload buffered with overlap_action=listen is therefore budgeted as two normal chunks (25 slots), matching what Stage0 builds, instead of the single first-window unit (13).
test_speech_after_silence_continuation_is_not_the_first_audio_window covers VISION → generated silence → buffered 2 s speech. test_accepted_silence_consumes_the_first_audio_window covers the buffer marker.
| tail_frames_sent = await client.stream_pcm( | ||
| tail_pcm, | ||
| chunk_ms=args.chunk_ms, | ||
| realtime=not args.no_realtime_pacing, | ||
| video_frames=video_frames[-1:], | ||
| stacked_video_frames=stacked_video_frames[-1:], |
There was a problem hiding this comment.
[P2] With --input-video, an external 1.4 s WAV, and --tail-silence-s 1, this second stream_pcm starts its frame clock at zero while the server still has 0.4 s of PCM buffered. The append closing the cumulative 2.0 s unit therefore has no held frame; the frame arrives at 2.4 s, so the unit containing the end of the utterance loses its visual input. Keeping the tail on the same media timeline, or streaming it together with the input, would preserve alignment. A 1.4 s input + 1 s tail regression should check that the append closing 2.0 s carries the last frame. Source.
There was a problem hiding this comment.
Fixed. stream_pcm now accepts media_origin_ms and unit_index, and the tail after an external WAV continues that clock instead of starting again at zero.
For 1.4 s of audio plus 1 s of tail silence, the append whose cumulative audio_end_ms is 2000 carries the last frame. The frame no longer waits until 2400. A clip with no audio track still holds its last frame on the tail's own clock, since the server has no buffered PCM to continue.
test_stream_pcm_tail_stays_on_the_clip_media_timeline covers the 1.4 s + 1 s case and expects frames at 1000 ms and 2000 ms.
| if raw: | ||
| self._frame_queue.append(frame_group) | ||
| else: | ||
| self._vision_queue.append(dict(payload, audio="", video_frames=frame_group)) |
There was a problem hiding this comment.
[P2] Resource-limit case: with auto_response=False, each frame-only append is retained here, but pending_byte_count still returns only len(_buffer) and these reservations have zero byte_count. The runner consequently charges zero pending bytes, so a long camera session can keep accumulating base64 images past max_pending_input_bytes_per_session. Could queued and reserved image bytes participate in the limit, with accounting restored/released on rollback/commit? A small configured limit should reject further frames without retaining the rejected payload, and clear/commit should release the accounted bytes. Source.
There was a problem hiding this comment.
Fixed. pending_byte_count now includes the base64 retained in _vision_queue, so a manual frame-only session is charged against max_pending_input_bytes_per_session.
A frame that does not fit is rolled back and is not left in the queue. A queued VISION unit carries those bytes on its reservation, and clear/commit releases them. Prefix units release their charge when they leave the pending buffer, because the prefix append itself does not hold the reservation.
test_queued_vision_units_count_toward_pending_bytes_until_rollback_or_commit covers the buffer accounting. test_manual_vision_frames_honor_the_pending_byte_limit uses a 100-byte limit: the second frame is rejected, the first frame stays, and clear then commit both return the session to zero pending bytes.
There was a problem hiding this comment.
The queued-frame fix covers the immediate path, but deferred commits still drop the prefix charge at this head. With an active response and a 100-byte limit, committing two 40-byte VISION frames retains both while _hold_commit_prefixes releases the first 40 bytes; a third 40-byte frame is then accepted although 120 bytes remain retained. Keeping each held prefix charged until successful submission or discard, with a deferred-commit limit/clear/cancel/promotion regression, would close this resource-limit path. Source.
There was a problem hiding this comment.
Fixed in bf64486. A held VISION prefix keeps its pending-byte charge until that prefix is submitted or the retained turn is discarded. clear, response.cancel, and clear_committed_audio release whatever is still held; a successful prefix submit releases only that prefix.
With a 100-byte limit, committing two 40-byte frames during an active response still rejects a third 40-byte frame. Promotion releases the charge after the prefixes are submitted. Covered by test_deferred_vision_prefixes_stay_charged_until_discard.
|
The three P2 comments are fixed locally and will be on the updated branch:
The Simple · Model Executor failure on build 17604 was The H100/A3 checkpoint runs called out in the review (single-session speech, video input, and video-only units followed by speech) are not claimed here. |
2e839d4 to
d5abb37
Compare
|
@natureofnature PTAL |
natureofnature
left a comment
There was a problem hiding this comment.
Rechecked the current 21-file change, including the fixes and shared runner paths. The tail-frame alignment fix looks correct. I added the remaining continuation and deferred-prefix cases to the original discussions.
I suggest completing the current-head MiniCPM-o speech, video, and video-then-speech cases on the supported H100/A3 target with the original checkpoint before merging, since these paths depend on encoder state and native response lifecycle. Expected results are matching token/embedding counts, complete audio and response/session events, and FIFO manual/deferred commits without loss or duplication. The two new regression scenarios in the threads also need passing results. Current pre-commit, wheel, CodeQL and docs checks pass; the accessible records do not provide current-head backend/model results.
【Conclusion: Found blocking code issues and validation gaps in commit b268b9f.】
【Action recommended: Keep audio budgets correct after generated silence and retain deferred-frame byte charges; verify mixed-input response completion and deferred commits on the supported target before merging.】
|
Both blocking points on b268b9f are addressed in bf64486.
The H100/A3 MiniCPM-o speech, video, and video-then-speech runs are still to be collected on this head. |
|
@natureofnature PTAL |
natureofnature
left a comment
There was a problem hiding this comment.
Rechecked the changes since b268b9f and the affected submission, cancellation, and cleanup paths. Both previously reported code issues are fixed: accepted silence continuations update the audio-window state, and deferred VISION prefixes remain charged until submission or discard.
I suggest running the new continuation-budget and deferred-prefix regressions on this head, then completing the MiniCPM-o speech, video, and video-then-speech cases on the supported H100/A3 target with the original checkpoint before merging. These paths depend on real encoder state and response lifecycle: token and embedding counts should match, audio and response/session events should complete, and manual/deferred commits should preserve FIFO input without loss or duplication. The new runner tests use a recording stage port, and the earlier 889-pass result belongs to a6007c4.
【Conclusion: Static review found no blocking code issues in commit bf64486.】
【Action recommended: Run the new budget and byte-limit regressions; verify matching embeddings, complete responses, and ordered mixed-input commits on H100/A3 before merging.】
|
Thanks for the recheck. Ran the new regressions on bf64486: This includes These are CPU tests with the recording stage port, as you noted. The MiniCPM-o speech, video, and video-then-speech runs on H100/A3 with the original checkpoint are still pending on this head; I will attach token/embedding counts and the response/session event results when they are done. |
bf64486 to
f6b0131
Compare
Official duplex sessions need AUDIO, VISION, and OMNI units, including frame-only input without synthetic silence, and realtime pacing should follow the media clock instead of accumulating send overhead. A VISION unit reserves only the tokens it actually feeds, so surplus </unit> pad embeddings do not stay in the KV. The streaming audio encoder keeps its own chunk index, so the first audio after a VISION unit still uses the first-chunk window. Vision prefixes are drained before a turn is committed, including prefixes retained while a response is still running. Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com>
f6b0131 to
f293a1c
Compare
|
The shared duplex append is still a voice/PCM entry, and this PR is the point where that stops fitting. There is no That matches the old shape, where a frame rode a voice append. It does not match a unit that can open a turn by itself. MiniCPM after this PR accepts AUDIO, VISION, and OMNI, and a frame with empty PCM can open a response. AURA is already the other way around: video is required and audio is optional ( Please make the shared append an input unit, not a voice append that vision hangs off. One entry point, modalities chosen by the payload, and each model plugin keeps declaring which of audio/video are required or optional. A frame-only unit has to be able to open a turn without being represented as an empty audio append. Renaming every existing |
A silence continuation is encoded outside the PCM buffer, so later speech was still reserved as the first mel window and its tail was truncated. Held VISION prefixes were dropped from the pending-byte limit while they were still retained. Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com>
f293a1c to
c32b787
Compare
vllm-project#8971 calls _reserve_commit_prefixes from auto-response commit; the SimpleNamespace fixture must provide those helpers (and reservation rollback) so retained-PCM consume coverage still runs. Signed-off-by: sheng <sheng.gao.dev@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Correction on the name. The entry to rename is
A frame-only turn is therefore still submitted through a method and command named audio. This PR makes that legal for MiniCPM (VISION unit, no PCM) and Please rename this Python entry to |
Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com>
…project#9151) Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor intent (required human answers)
1. What outcome do you want to achieve?
MiniCPM-o 4.5 duplex accepts a video frame as its own VISION unit when the clip or microphone has no audio, and realtime input stays on the media clock. A later audio unit in that same session is still the model's first audio chunk.
2. What is the highest-leverage variable for that outcome?
Whether a frame with no PCM is submitted as a complete VISION unit, and whether that unit is the same shape the model was trained on. Frames that ride real or silent audio must still use the OMNI speech gate, so a camera frame cannot reopen a finished turn.
3. How will this work move that variable?
MiniCPM duplex no longer requires audio. A frame group with empty PCM is reserved immediately and does not wait for a partial microphone chunk. Stage 0 builds that unit without padding the audio buffer, and vision-only input can open a response. The scheduler reserves exactly the tokens that unit feeds: session context on the first append, or the previous terminator plus
</unit>after that, then<unit>and the frame blocks. Surplus slots were</unit>pad embeddings left in the KV. Audio plus frames keeps the existing speech gate. The audio encoder has its own chunk index, so VISION units do not consume the first-chunk window.DuplexClientBasepaces realtime PCM from the stream start clock, so send overhead does not slow the input clock.4. Whose customer or user needs does this address, and what are those needs?
People using the MiniCPM realtime duplex demo and browser client with camera or video. Mute and clips with no audio track need to keep sending frames, and the model decides listen or speak. The next spoken turn still has to be encoded as the first audio chunk. Evaluators also need the recorded window settings and input timeline; transport success is not answer correctness.
Purpose
Before this change, MiniCPM-o duplex treated audio as required. A frame had to ride an audio unit, so video-only input was rejected and a muted camera could not send frames by themselves. Relative sleeps in the duplex client also accumulated send overhead and slowed the input media clock.
After this change:
audiofield used to fall through to the default 64-token audio budget, which left about 61</unit>pad embeddings in the KV on every frame.audio_encoder_chunk_idxadvances only when audio is encoded. A VISION unit still incrementsaudio_chunk_idx, but the first real audio keeps the 1035 ms first window,prefix_extra_frames=0, and the first-chunk sample count.DuplexClientBasepaces realtime PCM from the stream start clock.input_timeline.json.test_duplex_video_only_units_then_audio_inputsends two camera frames with no PCM, waits for each listen-or-speak decision, then sends the same 1400 ms speech window as the audio duplex test.Affected behavior is covered by the MiniCPM duplex input, scheduler budget, and Stage 0 paths,
examples/online_serving/minicpmo/README.md, andexamples/online_serving/realtime_web/README.md. Sliding-window mode bounds retained history; it does not guarantee answer accuracy.okchecks transport and lifecycle only.If an append is deferred and commit runs while a VISION unit is still queued behind a partial microphone chunk, that commit emits the VISION unit and can leave the partial audio buffered. That path is overlap deferral, not the muted-camera or video-only demo path.
Test Plan
vLLM Version: 0.31.0
Validated follow-up commit:
a6007c4022CPU validation (installed serving/test environment, no model weights needed):
source /data/why/scripts/env.sh python -m pytest -q tests/engine/duplex \ tests/model_executor/models/minicpmo_4_5/duplex \ tests/worker/test_native_duplex_hooks.py \ tests/examples/test_minicpmo_realtime_duplex_simple_demo.py \ tests/clients/test_duplex_client.pyTest Result
f73230520f: both two-frame manual-commit cases (with/without residual speech) failed because buffered input remained after the commit acknowledgement.test_duplex_single_session_response_required,test_duplex_single_session_video_input, andtest_duplex_video_only_units_then_audio_input) still require H100/A3 and the checkpoint. Full unit-test/backend CI evidence on the updated head remains required before merge.The shared PCM-buffer interface provides a no-op commit-prefix hook by default; MiniCPM owns the multi-unit framing policy. The runner owns ordered prefix submission and acknowledges the commit only after prefix appends are accepted. The existing reservation rollback path is retained. The lifecycle contract is documented in
docs/design/fullduplex.md.