Repository navigation
fix(minicpmo45): deadline-align native duplex silence continuation - #7059
linyueqian merged 4 commits into
Conversation
|
RFC #6494 tracks this as a P2 cadence follow-up for #7063. The reported fixed silence continuation adds roughly 400 ms per output interval, but the current PR is still draft, DCO-blocked, and its evidence is not yet current-main GPU/NPU evidence. Please preserve the distinction between cadence improvement, audio fidelity, and steady-state RTF. |
54aca8b to
bed23d1
Compare
|
Current-main CUDA data point: RTX 6000 Ada 48 GB, vLLM 0.28.0, vLLM-Omni
So on CUDA the ~1.35 s cadence shows up only in the silence-continuation regime (consistent with ≈ 0.35 s of pipeline work plus the full 1 s sleep), and the PR brings it back to the chunk period. Output is unchanged: same 20 chunks and transcript in both runs, |
|
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/entrypoints.md. Module owners: @alex-jw-brooks @linyueqian @NickCao @Tiagosf00, 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. |
Self-ReviewThe implementation is very simple, we have two time variables:
And two functions:
I also added compatibility tests for PersonaPlex since this pipeline is shared by different models. The last commit I made was just cleaning up some confusing variable names and unnecessary conditions made by AI. I also left some AI-generated comments in the code; let me know if you think they’re unnecessary. I've been manually testing this model in the full-duplex mode using the UI, but the results are still not good. I've already tested this model in other frameworks and the quality and responsiveness of the output were much better, so there are still bugs to find out in this implementation. This sleep issue was just a simple one I found. Also, in an earlier version of the main branch, when vllm 0.25 was still used, it used to work better, so I suspect there was some regression. This is a good open-source model and I hope to see it running smoothly on vllm-omni soon. @Sy0307 Ready for review. |
|
#7413 merged as |
…d framework Re-port of the deadline-aligned silence continuation onto the unified full-duplex framework (vllm-project#7413). The rewritten engine-resident runner still slept a fixed chunk_period after every audio delta, keeping the ~1.8x unpaced regime (stream_rtf 1.66 on v0.29 vs 0.985 with this fix). - compute_silence_continuation_deadline() now drives the scheduler: unit N+1 is due at submission_time_N + chunk_period, only the remaining budget is slept, and deadlines advance from the prior deadline so pipeline time does not accumulate as drift. A stall longer than one period submits immediately and restarts from now. - The deadline chain lives on DuplexModelSessionState (last_native_submit_monotonic / silence_deadline_monotonic) and is committed at the runtime-acceptance boundary (ModelChannel on_append_accepted) before any returned output event can clear it; a real (non-silence) input re-anchors the chain, and clear_continuation() drops it at turn boundaries. - Adds deadline-arithmetic + session-state reset tests. Measured: new-main baseline stream_rtf 1.66 (2 audio turns) vs 0.985 (4 turns) with the deadline pacing on the old framework. Signed-off-by: Tiagosf00 <tiagotsf2000@gmail.com>
122861e to
c5a97a5
Compare
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
linyueqian
left a comment
There was a problem hiding this comment.
Reviewed at 7d2d2f5f (the re-target onto the engine-owned scheduler, plus a merge of current main that leaves the PR's own hunks unchanged). Thank you for carrying this across #7413; the design survived intact and it lands in the right place. compute_silence_continuation_deadline is a clean pure function with the four cases the description promises: first continuation anchored to the last accepted submission, later ones advancing from the stored deadline so processing time does not accumulate, a schedule more than one period overdue collapsing to one immediate submission plus a restart from now, and a non-negative sleep in every branch. The timing state lives on the model session state, the acceptance callback commits it only when the append actually submitted, and a real (non-silence) append re-anchors the chain through _reanchor_chain. Reset points check out: barge-in and cancel are caught by the epoch check after the sleep, session close cancels the sleeping task and run_in_wire_order refuses non-open sessions, and MiniCPM-o clears both timestamps in clear_continuation. PersonaPlex has no plugin on this runner and AURA is not on it, so only MiniCPM-o changes behaviour.
Two timing interleavings survive, inline. The [important] one is the case this PR exists to get right: a continuation whose delay was computed while a real append was already in flight is queued behind that append, the real append re-anchors the chain when it is accepted, and then the queued continuation submits immediately anyway and overwrites the anchor with its pre-computed deadline, so one silence unit goes out up to a period early right after user speech. The [suggestion] is a smaller one: a stall during the sleep stores the pre-stall deadline, so the catch-up is followed by a second immediate continuation instead of one. A second [suggestion] covers the test gap both reviewers noted: the new tests exercise the arithmetic and clear_continuation() but never run _schedule_silence_continuation under a fake clock, so a revert of the sleep to a flat period would still pass.
Validation: static read of the seven-file diff against origin/main and of the scheduler path around the sleep, plus a two-model panel (grok found no production defect and the test gap; codex found the two interleavings, which I traced in the code); no PR code executed. I added ready and re-fired the general lane at this head after the merge push.
| # sleep only the remaining budget. The deadline is stored by the | ||
| # acceptance callback when the append actually submits, so skipped or | ||
| # stale continuations never advance the clock. | ||
| delay_s, next_silence_deadline = compute_silence_continuation_deadline( |
There was a problem hiding this comment.
[important] The delay here is computed from the chain as it stands before the sleep, and the only guards after the sleep are append_tail is not append_tail, _real_input_waiting(), and the stale check. If a real (non-silence) append is already in flight when this runs (so append_tail is that task and _real_input_waiting() is false because the input is submitting, not waiting), the continuation is queued behind it in wire order; the real append is accepted first and _reanchor_chain sets last_native_submit_monotonic to its submit time and clears the deadline, and then the queued silence submits at once (its delay was relative to the old anchor) and _on_append_accepted overwrites the anchor with its pre-computed next_silence_deadline. Concretely with a 1 s period: last silence at 0, user audio starts submitting at 0.8, model output arrives at 1.0, the scheduler computes zero delay and queues silence behind the speech; speech is accepted at 1.1 and re-anchors, silence submits at 1.1 and stores deadline 2.0, so the next unit is due at 2.0 instead of 2.1 and the one just sent was 0.7 s early relative to the speech. Neither before_append nor the epoch check sees a timing change. Simplest fix: have _still_valid (or the acceptance callback) compare model_state.last_native_submit_monotonic against the value captured when the delay was computed and drop the continuation if it changed, letting the next output reschedule from the fresh anchor.
| # Commit timing state once the runtime accepts the append; a real | ||
| # (non-silence) input re-anchors the chain. | ||
| model_state.last_native_submit_monotonic = submit_time | ||
| model_state.silence_deadline_monotonic = next_silence_deadline |
There was a problem hiding this comment.
[suggestion] This stores the deadline that was computed before the sleep. If the event loop stalls during the sleep so the append submits well after that deadline (say the plan was deadline 2.0 and next 3.0 but submit_time is 5.0), the chain now says the next unit is due at 3.0; the following scheduling pass sees now - deadline > period, collapses to deadline = now, and submits again immediately, so a stall yields two back-to-back continuations rather than the single catch-up the docstring promises. Deriving the stored value from the actual submission when it landed late, for example next = next_silence_deadline if submit_time <= next_silence_deadline - chunk_period_s else submit_time + chunk_period_s, keeps the one-catch-up contract.
| (0.0, 10.0, None, None, 0.0, 10.0), | ||
| ], | ||
| ) | ||
| def test_compute_silence_continuation_deadline( |
There was a problem hiding this comment.
[suggestion] These cases pin the arithmetic well, but nothing here runs _schedule_silence_continuation itself: with a fake monotonic clock and a patched asyncio.sleep, a test that drives two continuations then a real append and asserts the recorded sleeps and the re-anchored deadline would catch both interleavings above and would fail if the scheduler ever went back to a flat sleep(chunk_period). Right now that revert passes this file, and removing _reanchor_chain fails nothing.
7d2d2f5 to
6b933f8
Compare
Signed-off-by: Tiagosf00 <tiagotsf2000@gmail.com>
…lines Addresses review of the unified-framework deadline pacing: - Snapshot the submission anchor when a silence continuation is planned (after any wait for pending silence) and use it both for the deadline arithmetic and in before_append: if a real append re-anchored the chain while this continuation was queued, the outdated unit is skipped before submission, so its acceptance callback cannot overwrite the new timing. - When the actual submission is more than one chunk period past the planned deadline, save submit_time + chunk_period as the following deadline instead of the stale planned one, preventing an immediate catch-up unit. Small delays keep the planned cadence so ordinary jitter does not accumulate as drift. - Drop the unused silence_continuation / next_silence_deadline plumbing on AppendAttempt and the _start_append deadline parameter; the acceptance callback captures the planned deadline directly. - Add focused scheduler tests (fake monotonic clock, controlled append completion, simulated sleep) covering normal cadence, in-flight real append skip, long-stall deadline clamp, small wakeup jitter, the exact one-period boundary, and failed-append timing preservation. Signed-off-by: Tiagosf00 <tiagotsf2000@gmail.com>
…guard Follow-up to the scheduler review round: - Compare the submission-anchor snapshot with numeric equality (==) instead of object identity in _still_valid(); acceptance assigns fresh monotonic floats, but the guard should reason about the timestamp value, and tests now use explicitly different submission times. - Gate the stage port's submit() on the existing port object (the runner's context captured it, so swapping h.port never took effect), capture the actual append tasks from _start_append(), and drive the in-flight scenario on an explicit fake-clock timeline (anchor t, real append t+0.8 blocking, silence planned t+1.0, release t+1.1, fresh continuation at t+1.8 with following deadline t+2.8). Release the gate in cleanup; drop GatedPort. - Inject wakeup jitter and the exact one-period boundary through the stall mechanism before submission (stall applies to the next positive sleep; zero-duration scheduling yields do not consume it), so assertions verify the scheduler's sleep, not a clock changed after it returned. - Replace arbitrary asyncio.sleep(0.05) synchronization with awaiting the real append tasks; the failure test asserts the append task returns False before checking timing is unchanged. - _install_fake_clock() returns list[float] to match its stall list; type gated_submit's parameter as DuplexStageSubmission. Signed-off-by: Tiagosf00 <tiagotsf2000@gmail.com>
6b933f8 to
67101c9
Compare
linyueqian
left a comment
There was a problem hiding this comment.
Re-reviewed at 67101c9c. All three items from the first pass are addressed. The interleaving gap is closed the right way: _schedule_silence_continuation snapshots last_native_submit_monotonic after the wait for pending silence and _still_valid now refuses the unit when that anchor has moved, so a real append accepted between planning and submission re-anchors the chain instead of letting an early silence unit land on top of it. The acceptance callback clamps a late submission (submit_time > next_silence_deadline stores submit_time + chunk_period), so a stalled event loop no longer leaves the chain pointing at a deadline already in the past. The now-unused silence_continuation and next_silence_deadline fields on AppendAttempt are gone. And test_deadline_pacing_scheduler.py drives the scheduler itself with a fake monotonic clock and a simulated asyncio.sleep: normal cadence, an in-flight real append skipping the outdated unit and re-anchoring, a long stall, and a small wake-up delay preserving the planned deadline, which is exactly the coverage I asked for. Approving once the general lane is green on this head.
|
@linyueqian Thanks for the attention and review on this PR. |
…llm-project#7059) Signed-off-by: Tiagosf00 <tiagotsf2000@gmail.com> Signed-off-by: Matthieu Laneuville <matthieu.laneuville@surf.nl>
…llm-project#7059) Signed-off-by: Tiagosf00 <tiagotsf2000@gmail.com>
Summary
Fixes #7063.
Native duplex currently waits for a full
chunk_periodafter each output chunk has already been processed. With a 1-second period and around 400 ms of pipeline work, audio chunks arrive roughly every 1.4 seconds.This PR counts processing time as part of the period:
Solution
The scheduler now keeps a monotonic deadline for each session:
The submission time is captured before the runtime call, but the timing state is updated only after the append succeeds and the session epoch is still valid. This update happens before returned output events are processed, allowing a terminal event to clear the state without it being restored afterward.
The input chunk duration and silence payload are unchanged.
Validation
Current-main NPU comparison
Tested with vLLM 0.28.0 using the same 3.73-second input, reference audio and client settings.
b78c31ed)122861e)Only gaps after
input_audio_buffer.committedwere included. The number of measured gaps differs because some output arrived before the commit marker.The PR reduced the observed post-commit mean from 1137 ms to 639 ms without changing the number of chunks or the transcript.
CUDA comparison
An independent test on an RTX 6000 Ada found the same direction:
That test used a different input sample and response length, so its absolute numbers should not be directly compared with the NPU results. It produced the same 20 chunks and transcript in both CUDA runs, with byte-identical output WAV files.
Thanks to @twu3202.
Tests
Added CPU coverage for:
Experiments after #7413
All tests were done using NPUs.
Comparison before and after PR/#7413.
Before rebase (
c5a97a51):Current PR (
67101c9c):Tiago Fernandes - Huawei - AI Innovation Lab