Repository navigation
[BugFix][Duplex][Benchmark] Add soundtrack mix for Omni-DuplexEval sessions - #9175
amy-why-3459 merged 11 commits into
Conversation
The Omni-DuplexEval harness streamed only the padded question audio, so the clip's own audio track never reached the model. Add an opt-in 'soundtrack' mix (--duplex-eval-mix for vllm bench serve, --mix for the standalone generate command) that overlays the question on the clip's audio. The default 'question' mode is unchanged. Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com> Co-authored-by: Cursor <cursoragent@cursor.com>
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. |
amy-why-3459
changed the title
|
This PR appears to belong to: docs/design/module/benchmarking.md. Module owners: @alex-jw-brooks @Bounty-hunter @hsliuustc0106 Routing: @alex-jw-brooks via module of the changed files, CODEOWNERS; @Bounty-hunter via module of the changed files; @hsliuustc0106 via CODEOWNERS @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. |
hsliuustc0106
added
high priority
natureofnature
left a comment
natureofnature
left a comment
There was a problem hiding this comment.
Reviewed the soundtrack decoder/mixer, both CLI paths, prepared-input validation, streaming handoff, and the new regression tests. The default remains question; soundtrack reaches both preparation and generation, and int32 accumulation avoids int16 wraparound before clipping. The current-commit CPU results and MiniCPM-o A/B reported in the PR support this opt-in change; I did not run tests locally. Judged results are still needed before changing nightly baselines or claiming a benchmark-score improvement, which this PR does not do.
【Conclusion: Found no blocking issues in commit a88fe80.】
【Action recommended: None.】
amy-why-3459
left a comment
amy-why-3459
left a comment
There was a problem hiding this comment.
Checked the mixer, both CLI entry points, the prepared-input mix check, and the serving handoff. Default stays question, the int32 accumulate-then-clip is right, and mix is recorded in the meta. Two things worth tightening before soundtrack is used for A/B runs, inline.
| if mix != "question": | ||
| raise NotImplementedError("v1 supports mix=question; soundtrack mixing is reserved for P1") | ||
| if mix not in MIX_MODES: | ||
| raise ValueError(f"mix must be one of {MIX_MODES}") |
There was a problem hiding this comment.
With soundtrack now a real option, this early return can silently reuse a question artifact: re-running generate --mix soundtrack into the same --response-root without --overwrite returns the old <id>.json, so an A/B ends up comparing question against question. It also returns before the mix validation below. Could we either check the existing *.meta.json mix (and fail or regenerate on mismatch), or put the mix into the output path?
There was a problem hiding this comment.
Fixed in 643798c. generate_sample now reads *.meta.json before reusing <id>.json. Metadata with no mix still counts as question, so older artifacts resume under the default. A different mix raises and asks for --overwrite or another --response-root, instead of putting mix in the output path: evaluate still looks up {split}/{id}.json. The mix check also happens before that reuse, so an unknown mix is rejected even when an artifact is already there.
| raise | ||
| return read_audio_pcm16(path) | ||
|
|
||
|
|
There was a problem hiding this comment.
read_audio_pcm16 concatenates decoded frames from the first sample and ignores the audio stream's start_time/pts. For clips whose audio track starts after the video (non-zero start_time or an edit list), the soundtrack gets shifted earlier relative to the padded question audio and the frame timestamps, which is exactly what the reminder windows depend on. Left-padding by stream.start_time * stream.time_base (relative to the container/video start) would keep the overlay aligned; a test with an offset audio stream would pin it.
There was a problem hiding this comment.
Fixed in 643798c. read_soundtrack_pcm16 places each decoded frame by its PTS relative to the video stream start (the container start when the clip has no video). A track that starts later is left-padded, and one that leads the video is trimmed. A shared non-zero start is not padded, so it stays on the same timeline frame extraction uses. The new tests cover a 0.5s audio delay, a shared 0.5s start, and audio that starts 0.5s after the video.
… audio A cached generate result is reused only when its recorded mix matches, so a soundtrack run cannot silently return an earlier question artifact. Clip audio is placed at its presentation time relative to the video start, which keeps a delayed track lined up with the question and the frames. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com>
amy-why-3459
left a comment
amy-why-3459
left a comment
There was a problem hiding this comment.
Looked at 643798c: the mix check before cache reuse and the PTS-based soundtrack placement both address the earlier points, and the new tests cover them well. One remaining question about which timeline the audio is anchored to, inline.
| if stream.start_time is not None and stream.time_base is not None: | ||
| return Fraction(int(stream.start_time)) * Fraction(stream.time_base) | ||
| start = getattr(container, "start_time", None) | ||
| if start: |
There was a problem hiding this comment.
This anchors the soundtrack to the video stream's start_time, but extract_jpeg doesn't use that origin: it seeks to int(timestamp / time_base), i.e. absolute PTS. Only the first request (t=0) lands on the first frame; for a video starting at 0.5s, t=1.0 returns the frame at PTS 1.0s, which is 0.5s after the video start, while the audio for that moment is placed at 0.5s on the session clock. So for clips with a shared non-zero start, the overlay would lead the frames by start_time (and iter_jpegs stops at container.duration, which also isn't offset). Could we either keep origin 0 here, or shift extract_jpeg/iter_jpegs by the same origin so both use one timeline? test_shared_start_offset_stays_on_the_video_timeline currently pins the audio side only; a check that the frame at t=0.5 and the audio at 0.5s refer to the same media time would cover both.
There was a problem hiding this comment.
Agree with @amy-why-3459. I rechecked b2a9926: audio subtracts the video start, but extract_jpeg still compares decoded PTS against timestamp / time_base. With a shared 0.5 s start, session t=1 selects video PTS 1.0 s while audio t=1 represents PTS 1.5 s. This can misalign the evidence used for reminder scoring. I suggest using one origin for both paths and testing distinguishable video frames alongside an audio marker before merging; the shared-start regression should verify that both represent the same media instant, not only that the audio has no padding.
There was a problem hiding this comment.
Frame extraction now adds the video stream's start before converting the session timestamp to PTS, which is the origin read_soundtrack_pcm16 already used. On a clip whose audio and video both start at 0.5s, session t=0 is the first frame and session t=0.5s is the frame half a second later; the audio marker stays at session t=0. test_shared_start_offset_stays_on_the_video_timeline checks both. Fixed in 279796b.
Upstream selects reminder-window text by each chunk's current_time. Scoring whole sentences by their start time dropped in-window tails and recorded 0 without calling the judge. Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com> Co-authored-by: Cursor <cursoragent@cursor.com>
amy-why-3459
left a comment
amy-why-3459
left a comment
There was a problem hiding this comment.
Looked at b2a9926 (chunk-time reminder scoring): _text_deltas refactor, extract_timed_chunks, normalize_response_chunks, the PR branch of evaluate_sample, and the new {sentences, chunks} publish format. The fix itself looks right; two things on the scoring input.
| response = " ".join(item["sentence"] for item in items if lo <= item["start"] <= hi) | ||
| response = " ".join(chunk["text"] for chunk in chunks if lo <= chunk["current_time"] <= hi).strip() | ||
| if not response: | ||
| event_rows.append({"start_time": start, "response_segment": "", "success_score": 0}) |
There was a problem hiding this comment.
Chunks are cut whenever audio_end_ms advances, which happens mid-response as input frames keep streaming, so a word can be split across chunks ("dump" @13.0 / "lings" @13.08). Since extract_timed_chunks strips each chunk and this joins with " ", the judge sees "dump lings", and CJK output gets spaces inserted between every chunk. Keeping the raw delta text in chunks (strip only the final joined string) and joining with "" would reproduce the emitted text exactly; same for the correction branch at L134.
There was a problem hiding this comment.
Agree with @amy-why-3459. On b2a9926, both chunk extraction and normalization strip text, then both PR scoring branches insert a space between chunks. Deltas "dump" and "lings" at different media times therefore reach the judge as "dump lings" even when both are inside the window. I suggest preserving raw whitespace through extraction and normalization, then concatenating selected chunks without inserting characters and stripping only the final result. Before merging, a judge-input regression should cover a split word, CJK text, and a genuine inter-word space across chunk boundaries in both correction and reminder scoring.
There was a problem hiding this comment.
Chunks now keep their raw deltas, including whitespace at the boundary. Both the correction path and the reminder window concatenate with an empty separator and strip only the joined string, so dump + lings stays dumplings, CJK is not split, and a real inter-word space is preserved. The regression covers those three cases on both scoring paths. Fixed in 279796b.
| items = raw["sentences"] | ||
| elif isinstance(raw, list): | ||
| items = raw | ||
| else: |
There was a problem hiding this comment.
This bare-list fallback also covers responses cached by earlier runs: generate_sample reuses an existing <id>.json when only mix matches, so a rerun into an existing response dir silently scores old files by sentence start (the bug this commit fixes) and new ones by chunk time. Worth either treating a bare-list cached response as stale in the reuse check (require --overwrite) or at least logging when PR samples fall back to sentence timing.
There was a problem hiding this comment.
generate_sample now refuses to reuse a cached response that has no chunks list, and asks for --overwrite or another response directory, same as a mix mismatch. Evaluating a bare sentence list directly still scores it by sentence start, since that file has no chunk times. Covered by test_generate_refuses_cached_response_without_chunks. Fixed in 279796b.
natureofnature
left a comment
natureofnature
left a comment
There was a problem hiding this comment.
My earlier review of a88fe80 was too broad: I should have treated silently reusing another mix's artifact as a correctness issue. The metadata check now fixes that, so I am not carrying it forward as an open finding.
I reviewed the subsequent cache, audio-placement, chunk-extraction, publication and scoring changes at b2a9926. Two current correctness issues remain in the existing discussions: audio/video alignment for a nonzero video start and altering text at chunk boundaries before judging. I independently checked both and included the required regression scenarios there.
The PR reports 339 CPU tests passing on this head; its live A/B belongs to a88fe80, before chunk-window scoring. This review was static, with no local test execution. The new scoring path also affects question runs, so keeping that audio default does not isolate the text issue.
【Conclusion: Found blocking code issues in commit b2a9926.】
【Action recommended: Align audio and video when the clip starts at a nonzero timestamp, preserve emitted text across chunks, and verify synchronized frame/audio markers plus unchanged split-word, CJK and whitespace text before merging.】
…tact Frame requests were absolute PTS while soundtrack audio was offset by the video start, so a shared nonzero start played audio ahead of the picture. Reminder scoring also stripped each chunk and joined with a space, which split words and inserted spaces into CJK. Cached sentence lists are no longer reused, because they would still be scored by sentence start. Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com> Co-authored-by: Cursor <cursoragent@cursor.com>
amy-why-3459
left a comment
amy-why-3459
left a comment
There was a problem hiding this comment.
Looked at the delta b2a9926..279796b (frame/audio origin alignment, unstripped chunk text, refusing cached responses without chunks). The extract_jpeg origin change lines up with read_soundtrack_pcm16 and the new brightness assertions cover it; extract_jpeg has no callers outside the duplex-eval code, so the semantic change is contained. Two things on the no-separator join, plus a nit inline.
Also a minor one: _cached_response_has_chunks now refuses every cached bare-list response, including RTD splits, which only read sentences and would score the same as before. If that's intended (one artifact format per directory), fine; otherwise the check could be limited to family == "pr".
| event_rows = [] | ||
| if task == "correction" or not times: | ||
| response = _text(items) | ||
| response = "".join(chunk["text"] for chunk in chunks).strip() |
There was a problem hiding this comment.
normalize_response_chunks still falls back to sentences / a bare sentence list for older artifacts, and those items are stripped sentences (TimedSentence stores piece / pending.strip()). With "".join they now reach the judge as "Got it.Let me check.", so the PR description's "older artifacts keep their previous result" no longer holds for the correction path and for multi-sentence windows (same at L140). Maybe keep " " as the separator when the items came from the sentence fallback, and "" only for real chunks.
There was a problem hiding this comment.
Right, the sentence fallback items are stripped, so "" glued them into "Got it.Let me check.". PR scoring now picks the separator from the response format: "" when the file has chunks, " " for a sentences-only or bare-list response, on both the correction path and the reminder window. Older artifacts score as they did before this PR. has_response_chunks in the clock module makes that call, and the runner cache check uses it too. test_legacy_sentence_responses_keep_space_between_sentences covers both paths. Fixed in 8e17b2c.
| text += delta | ||
| continue | ||
| if current is not None and now != current: | ||
| if text.strip(): |
There was a problem hiding this comment.
Now that chunks are concatenated without a separator, dropping a whitespace-only group here also drops the only space between two words: deltas "Look,"@13, " "@14, "the"@15 become "Look,the". Previously the " ".join hid this. Carrying a whitespace-only group's text into the next chunk (instead of discarding it) would keep the join lossless; normalize_response_chunks has the same not text.strip() skip.
There was a problem hiding this comment.
I don't think this case can happen. _text_deltas gives a whitespace-only delta no timestamp (now is None), and extract_timed_chunks appends it to the open chunk rather than starting a new group. So "Look,"@13, " ", "the"@15 produces "Look, "@13 and "the"@15, and the join gives "Look, the". test_timed_chunks_group_deltas_by_media_time checks that exact sequence. A chunk only ever starts on a delta with non-whitespace text, so the text.strip() check here never drops a chunk mid-stream. It only matters when there are no chunks yet, where it skips leading whitespace. For the same reason, normalize_response_chunks never sees a whitespace-only chunk in files this code writes. Leaving it as is.
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| @pytest.mark.asyncio |
There was a problem hiding this comment.
nit: @pytest.mark.asyncio is applied twice.
Legacy sentence responses are stripped, so PR scoring joins them with a space again; real chunks still concatenate as emitted. The chunk-time requirement for cached responses now applies only to PR samples, since RTD scores sentences. Drop a duplicated asyncio marker. Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
On the Local run on 8e17b2c: |
amy-why-3459
left a comment
amy-why-3459
left a comment
There was a problem hiding this comment.
Looked at the delta 279796b..8e17b2c. The three points from the previous pass are resolved: PR scoring picks "" only when the file actually has chunks and " " for the stripped sentence fallback (on both the correction path and the reminder window), the cached-response chunk requirement is now limited to family == "pr" so RTD sentence caches are reused again, and the duplicated asyncio marker is gone. has_response_chunks keeps the evaluator and the runner cache check on the same definition, and the new tests cover the legacy-spacing and RTD-reuse cases. The explanation on the whitespace-only chunk point also checks out (_text_deltas leaves untimed whitespace attached to the open chunk). No further issues on this delta.
…n soundtrack A clip the model never answers waited forever for response.done, so those sessions never reached the judge. A post-commit listen is now a finished silent sample, and the MiniCPM-o nightly runs that input as soundtrack. Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com> Co-authored-by: Cursor <cursoragent@cursor.com>
| return False | ||
| return client.events.count("response.done") > 0 or _listened_after_commit( | ||
| client.events.events, commit_from | ||
| ) |
There was a problem hiding this comment.
The post-commit response.listen isn't tied to the residual unit the commit flushes. Unit decisions are asynchronous (that's the stated reason for _ensure_final_commit_tail in omniinteract.py), so the silent decision for the last full unit can still land after input_audio_buffer.committed, especially at C8 or with as-fast-as-possible. With no responses created yet, this returns True on that earlier listen and wait_for exits right away. If the residual unit's decision is then to speak, its response.created arrives after we've stopped waiting: the session is closed under it and the sample is published as an empty answer (or flagged as unfinished), not a real silent one.
wait_for_session_completion in omniinteract.py guards against this by requiring a quiet settle_s window after the post-commit decision (and by handling overlap_deferred commits). Could this path reuse that, or at least require no new events for a short settle period before treating the listen as final? A test where an earlier-unit listen arrives after committed and is followed by response.created would cover it.
There was a problem hiding this comment.
Thanks. I added a settle window in 632005d.
The specific ordering in the comment can't happen. A listen from an earlier full unit can't arrive after committed. _on_commit awaits _wait_for_append_tail(), and each append task awaits append_runtime_input, which emits that unit's response.listen through _send_model_output_events before it returns. A live sample with a 26.9 s clip produced 27 listens, and only the residual unit's listen came after committed.
A related race does exist. When an earlier unit decides to speak, model_channel returns early on requires_stage_handoff/requires_tts_stage and reserves the response only when Stage1 emits text or audio. That unit's response.created can therefore trail the residual unit's post-commit listen, and the old predicate would close the session first.
The fix:
responses_finishedaccepts the post-commit listen only after the stream has been quiet for_COMPLETION_SETTLE_S(2.0 s, the same as OmniInteract's_COMPLETION_SETTLE_S). Any new event restarts the window, and an open response still blocks completion.test_generate_waits_for_a_response_created_after_the_post_commit_listen: the fake server sendscommittedand a listen, and 0.2 s later sendsresponse.created, a text delta, andresponse.done. The server keeps servingsession.closein the meantime. The test fails on the previous commit (empty answer) and passes now.
I didn't change the overlap_deferred path. There, a response that is still open at commit already blocks completion, and the response.done > 0 condition behaves as it did before this PR.
A speak decision reserves its response only once Stage1 emits output, so its response.created can trail the residual unit's post-commit listen. Require the event stream to stay quiet for 2 s before treating that listen as the session's final decision. Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com> Co-authored-by: Cursor <cursoragent@cursor.com>
| return False | ||
| if client.events.count("response.done") > 0: | ||
| return True | ||
| return ( |
There was a problem hiding this comment.
The race described in the commit message (a speak decision's response.created trailing the residual unit's post-commit listen) isn't specific to silent sessions, but this shortcut still bypasses the settle window. count("response.done") is session-wide, so in any PR sample where the model already finished a response before the commit, the first poll after commit() returns True as soon as the existing responses are done, before committed, the post-commit listen, or a trailing response.created can arrive. The final reply then gets cut off the same way the silent case did.
Could the shortcut count only response.done events after commit_from (or just apply the same stable_since check to both branches)? A variant of the new test with one complete response sent before the commit would cover it.
There was a problem hiding this comment.
Correct. Fixed in 7ffdbe4.
count("response.done") was session-wide, so a response that finished before the commit made the first poll return True, before committed, the post-commit listen, or a trailing response.created.
The predicate now does both of the things you suggested:
- Only a
response.doneaftercommit_fromcounts. A response that finished earlier does not end the session. - Both endings wait for
_COMPLETION_SETTLE_S(2.0 s) with no further commit, listen, or response event. Aresponse.createdduring that window keeps the session open until that response is done and the window elapses again. An open response still blocks completion.
The quiet window does not count playback.acknowledged. The runner acks every 250 ms for any response that produced audio, and the server echoes each ack, so counting those events would keep the window from ever elapsing.
test_generate_keeps_a_response_opened_after_an_earlier_response_done sends one finished response before the commit, then committed and a listen, then 0.2 s later response.created, a text delta, and response.done. On 632005d the published text is Early.; with this commit it is Early.Late..
docs/cli/bench/serve.md now describes the same rule. Speaking sessions wait the same 2 s after the last response event that silent sessions already waited.
A response.done from before the commit is not the residual unit's decision, and Stage1 can still open a response after it. Count only a post-commit response.done, and wait out the same quiet window used for a post-commit listen. Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com> Co-authored-by: Cursor <cursoragent@cursor.com>
amy-why-3459
left a comment
amy-why-3459
left a comment
There was a problem hiding this comment.
Looked at the delta 632005d..7ffdbe4 (post-commit response.done only, shared quiet window over _COMPLETION_ACTIVITY, and the early/late response test). Excluding the playback-ack echoes from the activity count makes sense, and the new test does fail on the old count("response.done") > 0 shortcut. One remaining case inline.
| return False | ||
| return _count_since(events, "response.done", commit_from) > 0 or _listened_after_commit( | ||
| events, commit_from | ||
| ) |
There was a problem hiding this comment.
This still accepts a response.done for a response that was created before the commit and only finished after it, e.g. the model is mid-reply when the clip ends (likely in soundtrack / reminder sessions). commit_from is taken before client.commit(), so that done counts even if it lands before input_audio_buffer.committed, and the session ends after 2 s of quiet without any decision for the residual unit. The listen branch is stricter: _listened_after_commit needs committed first.
If the residual unit's decision can come after the in-flight response finishes, this drops the same kind of late reply the commit fixes. Requiring the done to belong to a response whose response.created came after committed (or a post-commit listen) would close it, and a variant of the new test with r1 still open at commit (done sent after committed) would cover it.
There was a problem hiding this comment.
Correct. Fixed in 072e613.
commit_from is taken before client.commit(), so a response.done in that gap, or one for a response created before input_audio_buffer.committed, was enough to finish the session. That reply is still in flight when the clip ends. On a normal completion the server promotes the deferred commit and only then runs the residual unit (emitter._apply_outbound_session_event), so the decision arrives after that response.done.
_residual_unit_decided now requires input_audio_buffer.committed and then one of:
- a
response.donewhoseresponse.createdcame after that commit (a cancelled one does not count) - a
response.listenwithmodel_listen, and only after every response that was still open at the commit has finished. A listen that arrives while that reply is open is not the residual unit; the residual is started by the completion - every response that was open at the commit ended
cancelledorfailed. Those endings do not promote a residual unit (can_promote_overlapis false), so waiting for one would turn a barge-in into a drain timeout. The runner already keeps that text as judge input
The quiet window is unchanged: both endings still wait 2 s, and an open response still blocks completion.
test_generate_waits_for_a_response_opened_after_an_in_flight_reply leaves r1 open across the commit, sends committed and r1's response.done, then 0.5 s later opens r2. The settle is 0.2 s. On 7ffdbe4 the published text is Early.; with this commit it is Early.Late.. test_residual_unit_decision_follows_the_committed_event covers the other orderings: a listen before the in-flight done, a response created after the commit, no committed event, and a cancelled or failed ending.
hsliuustc0106
left a comment
hsliuustc0106
left a comment
There was a problem hiding this comment.
@amy-why-3459, one validation TODO for 7ffdbe4a9351aae6b58fd137e675c6db3c6a1d41, separate from the existing completion-predicate review:
- Reconcile the judge configuration used to justify enabling
soundtrackin the nightly. The PR's 66-ID/C8 L20X reference usesQwen/Qwen2.5-VL-7B-Instructwithmax-model-len 128000atb0fdc9aba37c51355313958d9394508d7f95a29e, while the checked-in nightly configuration still sets65536. Please provide the matching 66-ID/C8 result or existing artifact for the intended checked-in judge configuration, identifying the tested runner SHA, model, context limit, hardware, exact command and completion/quality result. Alternatively, explain and deliberately reconcile which configuration the nightly is intended to use. If that validation cannot be run, retain the explicit gap for maintainer decision rather than treating the different-config reference as acceptance.
Acceptance is evidence tied to the intended nightly configuration and relevant runner changes, or an explicit reviewed validation exception. The disclosed older 660-sample failures at 65536 do not prove these 66 IDs fail, so this is an evidence mismatch, not a new demonstrated code defect. I have not executed the workload; the existing completion-predicate concern remains separate and is not repeated here.
A response still open at input_audio_buffer.committed is the in-flight reply. Completing it normally only starts the residual unit, so that response.done no longer ends the session. The session waits for a response created after the commit, or for a listen once that reply has finished. A cancelled or failed reply does not start a residual unit. Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com> Co-authored-by: Cursor <cursoragent@cursor.com>
amy-why-3459
left a comment
amy-why-3459
left a comment
There was a problem hiding this comment.
Looked at the delta 7ffdbe4..072e613 (_residual_unit_decided replacing the post-commit response.done count, the doc update, and the new in-flight-reply tests). This resolves the earlier point: a response.done for a reply that was already open at input_audio_buffer.committed no longer ends the session, and test_generate_waits_for_a_response_opened_after_an_in_flight_reply covers the Early. + Late. case end to end. The test fixtures that now emit input_audio_buffer.committed explicitly match what the new predicate requires.
One remaining question on how cancelled is treated, inline.
| # Nothing will follow a commit whose open replies all ended cancelled or failed. | ||
| return bool(open_at_commit) and finished_open >= open_at_commit and not promotes_residual and not created_after | ||
|
|
||
|
|
There was a problem hiding this comment.
Two things about cancelled here:
- Treating a cancelled open reply as terminal ("nothing will follow") seems at odds with the barge-in note further down (
Barge-in cancels an individual turn, not the input clip). If the residual unit is what barges in on the open reply, thecancelleddone is followed by the residual's ownresponse.created. Since 632005d noted thatresponse.createdcan trail by more than the settle window, this fallback can end the session after_COMPLETION_SETTLE_Sof quiet, before that response opens. - For a response created after the commit, line 312 excludes
cancelledbut acceptsfailed, and this fallback is off whenevercreated_afteris non-empty. So[committed, created r2, done r2 cancelled]never decides and only ends on the drain timeout, while the same sequence withfailedends immediately.
If cancelled replies really never get a follow-up at the end of a clip, a short comment saying so (and treating post-commit cancelled like failed) would make the two cases consistent. Otherwise, waiting for an explicit response.listen or a new response.created after a cancel seems safer. Either way, a unit case for [created, committed, cancelled, late_created, late_done] and [committed, late_created, late_cancelled] would lock it down.
There was a problem hiding this comment.
The safer of the two readings is the right one. Fixed in 377289d.
MiniCPM-o sets supports_barge_in. A barge-in cancels the open reply and the same append continues, so the residual response.created can still arrive after _COMPLETION_SETTLE_S. Treating that cancelled done as "nothing will follow" would close the session first. The runner note that barge-in cancels one turn, not the clip, is the same fact: the text stays judge input, and the session keeps waiting.
cancelled is therefore not a decision, whether the response was open at input_audio_buffer.committed or created after it. The session still ends on a later response.listen, or on a response.done for a response created after the commit.
failed stays a decision, which is why the two statuses differ. can_promote_overlap is false for a failure, and a failed response created after the commit is itself the residual. The sample is already unsuccessful (Response did not complete). A cancellation is the opposite: the turn was interrupted and another response may open.
test_residual_unit_decision_follows_the_committed_event now locks both sequences you named:
[created r1, committed, cancelled r1]does not decide.[created r1, committed, cancelled r1, created r2, done r2]does.[committed, created r2, cancelled r2]does not decide. The same sequence withfaileddoes.
Barge-in cancels the open turn and can still open the residual response after the quiet window, so a cancelled response.done is not a decision. A failed reply still ends the wait: nothing is promoted after it. The nightly description records that the 128000 soundtrack numbers are not acceptance of the checked-in 65536 judge. Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
@hsliuustc0106 The 66-ID soundtrack table is not acceptance of the checked-in judge. That run used 377289d records that distinction in the nightly test description. The PR test result now says the same thing: the 128000 numbers are not acceptance of the 65536 configuration, and a 66-ID run on the checked-in judge is still open. The thresholds stay the 2026-10-20 question reference (1.8 / 1.4 / 0.28). I am not changing |
amy-why-3459
left a comment
amy-why-3459
left a comment
There was a problem hiding this comment.
Looked at the delta 072e613..377289d. Both earlier points are addressed: a cancelled ending (in-flight or residual) no longer decides the session, and failed is now terminal in both positions, so the two paths are consistent. The new _residual_unit_decided cases cover in-flight cancelled, cancelled followed by a residual, residual cancelled, and residual failed, and the nightly JSON description now states that the 128000-context numbers are not acceptance of the 65536 judge.
One thing to keep in mind rather than a blocker: with cancelled no longer terminal, a session where barge-in cancels the open reply and the server then sends neither a residual response nor a model_listen falls back to the drain timeout again. The fake server in finish_tail always sends the listen after a cancel, so it encodes that assumption; it would be good to confirm the MiniCPM-o realtime server actually emits it after a barge-in cancel near the end of the clip. No further concerns on this delta.
Contributor intent (required human answers)
1. What outcome do you want to achieve?
Make Omni-DuplexEval session input match the benchmark by sending the clip's own audio, so Proactive Reminder scores reflect the model rather than a harness that drops that audio.
2. What is the highest-leverage variable for that outcome?
The audio stream sent during an eval session: the instruction alone, or the instruction mixed with the clip soundtrack.
3. How will this work move that variable?
Adds a
soundtrackmix (--duplex-eval-mixforvllm bench serve,--mixfor the standalonegeneratecommand) that overlays the question audio on the clip's audio. The CLI default staysquestion. The MiniCPM-o nightly (tests/dfx/perf/tests/test_minicpmo_4_5_omni_duplex_eval.json) now selectssoundtrack. The Qwen3-Omniserver_vadnightly stays onquestion.PR scoring follows upstream
proactive_reminder.py: a published response is{"sentences", "chunks"}, RTD readssentences, and a reminder window keeps chunks whosecurrent_timefalls in[reminder, reminder + 10s]. A bare sentence list is still scored by sentence start, so older artifacts keep their previous result.4. Whose customer or user needs does this address, and what are those needs?
Developers and CI maintainers evaluating duplex models. They need accuracy numbers that can be compared with the paper, and failure analysis that is not distorted by missing input.
Purpose
generate_samplepreviously rejected every mix exceptquestion(NotImplementedError, reserved for a later phase). The session therefore streamed only the dataset's question audio. That audio is padded with silence to the clip length, so the model never heard the clip's own soundtrack. Post-event reminder prompts that ask the model to repeat a word from the clip, or to react when something returns on screen, were scored against a session that contained neither the spoken word nor the clip audio.After this change,
soundtrackdecodes the clip audio to 16 kHz mono PCM16, sums it sample by sample with the padded question audio, and clips to int16. A clip with no audio track keeps the question audio alone.questionremains the CLI default forvllm bench serve --omni --dataset-name omni-duplex-evaland for the standalone runner.A native-duplex session that never speaks used to time out waiting for
response.doneand never reached the judge. The runner now finishes that session when aresponse.listenwithmodel_listenarrives after the commit, publishes empty judge input, and scores 0. When the clip ends on a model-unit boundary, the last PCM sample is withheld so the commit flushes a residual unit and that decision covers the end of the clip.Behavior is documented in
docs/cli/bench/serve.md(--duplex-eval-mix). The same choice is--mixon the standalonegeneratecommand.The same document describes the judge input. Scoring a whole sentence by its start time dropped any tail that landed inside the reminder window and recorded 0 without calling the judge. Chunks group consecutive text deltas that share a media time. An empty window still scores 0 without a judge call.
The existing thresholds stay 1.8 RTD content, 1.4 RTD temporal, and 0.28 PR all-success. The soundtrack reference below meets them. The accuracy config records
"mix": "soundtrack", and the guard rejects a benchmark or a score file recorded with another mix.Test Plan
vLLM Version: 0.32.0
vLLM-Omni Commit: b0fdc9a
pytest tests/benchmarks/duplex tests/e2e/accuracy/omni_duplex_eval tests/e2e/features/fullduplex/test_omni_duplex_eval_cli.py tests/dfx/perf/tests/test_runner_metadata.pywith this worktree onPYTHONPATH.current_timeis inside the window.Qwen/Qwen2.5-VL-7B-Instruct(max-model-len 128000), on the runner inb0fdc9aba3. Same 66 IDs as the nightly. Run once withsoundtrackand once withquestion.Test Result
CPU on
b0fdc9aba3: 430 passed. Pre-commit passed.Judged 66-ID C8 on one L20X,
b0fdc9aba3runner, withQwen/Qwen2.5-VL-7B-Instructatmax-model-len 128000. Both mixes completed 66/66 and judged 66/66. The checked-in nightly judge is still--max-model-len 65536. These 128000 numbers show that soundtrack can meet the existing thresholds under that longer context. They are not acceptance of the 65536 nightly configuration. A 66-ID run on that checked-in judge is still open for a maintainer decision.soundtrackmeets the checked-in thresholds with the same one-sample PR margin as the earlierquestionreference (11/36). Ten of the 36 PR samples produced no judge text undersoundtrack, including correction ids 2 and 11, which previously failed the drain timeout instead of scoring 0. Two of 66 soundtrack sessions had no measured client audio; the nightly only requires one session with audio.Earlier, on
a88fe80ad8and before chunk-window scoring, an unjudged A/B ofPR_post_event_reminderids 261–270 found in-window speech on 5 of 10 soundtrack samples and on 1 of 10 question samples. That table is not a judged score.Not run on
b0fdc9aba3: the 66-ID judged pass at the checked-in judge limit of 65536, the full 660-sample judged pass, an H100/B200 or NPU nightly, and a repeated-run stability check. A 660-sample soundtrack pass onb2a9926896, before the silent-session fix, published 634/660; its judge also rejected long RTD videos at a 65536-token context. Those numbers are not the nightly baseline, and they do not show whether these 66 IDs pass or fail at 65536.