Repository navigation
[Bugfix] Fix MiniCPM duplex response-required CI test - #9019
natureofnature merged 5 commits into
Conversation
Use the complete fixture for both response-required turns. The old 1.4 s crop relied on a retained-tail replay bug fixed by vllm-project#8931; without the replay, the native model can keep listening after the second commit. Cover residual-audio release across two turns, report commit-local timeout events, and retain duplex artifacts in merge CI for future failures. Signed-off-by: natureofnature <wzliu@connect.hku.hk>
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. |
Signed-off-by: natureofnature <wzliu@connect.hku.hk>
|
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 PTAL |
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
amy-why-3459
left a comment
amy-why-3459
left a comment
There was a problem hiding this comment.
Thanks for the careful bisection, the parent / merge / copy-order-control table makes the root cause convincing: the old test only passed because the un-released residual tail from turn 1 was being replayed into turn 2, and #8931 correctly stopped that. Sending the complete fixture and pinning the release behavior with a CPU regression is the right split.
Two small points inline: the same cropped-input pattern is still used by the video / still-image tests in this file, and the CPU regression could check the replay symptom a bit more directly. Neither blocks the CI fix.
| # at 1.4 s can legitimately leave the native model listening on a later turn. | ||
| args.turn_duration_ms = [0] * args.turns | ||
| result = asyncio.run(run_demo(args)) | ||
| assert result["ok"] is True |
There was a problem hiding this comment.
The same reasoning seems to apply to test_duplex_single_session_video_input (L351) and test_duplex_single_session_still_image_input (L581): both still send [args.first_turn_ms] * args.turns (1.4 s crop on both turns) and assert done_count == 2. Before #8931 they would also have benefited from the replayed tail on turn 2.
They passed in your 18-pass advanced run, but was that a single run? Since the cropped response-required case went 0/3 deterministically, it'd be good to either confirm those two are stable across a few repetitions (vision input may be what tips the model to speak), or add a short comment there explaining why the crop is still safe. Switching them to [0] isn't free because the video test asserts video_frame_count == 4, so a comment may be the cheaper option.
| assert h.runner.model_state.committed_audio_payload is None | ||
| assert h.session.pending_input_bytes == 0 | ||
| assert len(h.port.submissions) == (turn + 1) * 2 | ||
| final_append = h.port.submissions[-1].prompt["model_intermediate_buffer"]["duplex"] |
There was a problem hiding this comment.
Nice that the two turns use different PCM values. Optional: the final-append assertion catches a replayed tail indirectly (via the padded length). Also asserting the non-final unit of each turn is pure current-turn audio, e.g. base64.b64decode(h.port.submissions[-2].prompt["model_intermediate_buffer"]["duplex"]["payload"]["audio"]) == pcm_f32(16000, value=value), would make it explicit that nothing from turn 0 leaks into turn 1's stream, and keep the test meaningful if the padding/unit size ever changes.
Signed-off-by: natureofnature <wzliu@connect.hku.hk>
amy-why-3459
left a comment
amy-why-3459
left a comment
There was a problem hiding this comment.
Thanks for fd6232a2 — this adds the direct streaming-ownership check I suggested on the CPU residual-audio regression (non-final unit is pure current-turn PCM). Looks good; no new issues from this delta.
The optional note about the video / still-image tests still using the 1.4 s crop is unchanged and still non-blocking.
| tts_output(h.stage0_request_id(), samples=samples, turn_end=True, turn_id=turn) | ||
| ) | ||
| done = find(events, "response.done") | ||
| await h.run(commands.AckPlayback(played_ms=1000, response_id=done.response_id)) |
There was a problem hiding this comment.
The tail-audio release is already guarded by lines 706–716: committed_audio_payload is None, plus the two PCM rounds not being equal. The later TTS and AckPlayback parts are the playback completion mentioned in the PR, but the result of h.run(...) is discarded. When AckPlayback fails it returns an error event rather than raising an exception, so as written, that line passes no matter what it returns.
There was a problem hiding this comment.
@yenuo26 Good catch — the ACK result was being discarded. Fixed in f3c3cc9: each turn now asserts response.done.status == "completed", exactly one playback.acknowledged event, history_committed is True, and a committed playback cursor of 1,000 ms. An ACK error now fails the test.
The full session runner CPU module passes: 55 passed. Changed-file pre-commit checks also pass. GPU E2E tests were not rerun for this assertion-only change.
Signed-off-by: natureofnature <wzliu@connect.hku.hk>
natureofnature
added
ready
natureofnature
added
ready
natureofnature
added
CI/CD
amy-why-3459
left a comment
amy-why-3459
left a comment
There was a problem hiding this comment.
Thanks for f3c3cc91 — asserting response.done completed plus a successful playback.acknowledged (history committed, played_ms) tightens the residual-audio regression without changing the fixture/ownership coverage. Merge-main b70432a4 does not touch the PR's two test files beyond that delta. No new issues from this incremental look.
The earlier optional note about the video / still-image tests still using the 1.4 s crop remains non-blocking.
4 participants
Contributor intent
1. What outcome do you want to achieve?
Fix the MiniCPM duplex two-turn test failure in Buildkite #17383, preserving correct audio ownership and the requirement for two complete spoken responses.
2. What is the highest-leverage variable for that outcome?
The audio supplied to the response-required test: it should contain the complete fixture rather than depend on replaying residual audio from the previous turn.
3. How will this work move that variable?
Send the full fixture on both turns and cover residual-audio cleanup with a regression test.
4. Whose customer or user needs does this address, and what are those needs?
Maintainers and contributors need merge CI to detect broken duplex behavior and distinguish missing responses from the model's native listen decisions.
Purpose
Fixes #9018.
Fix the second-turn
response.createdtimeout exposed after #8931. The test selected the 1.4-second window with the highest mean absolute PCM amplitude from a 5.47-second fixture and required a response on both turns. On the failing revision, the second input was committed successfully, but the model returnedmodel_listen=True, with no error event.The parent revision passed because it retained and replayed the previous turn's padded audio tail.
_commit_and_start_auto_responseretained a payload, then copied it to addrealtime_item_id; append cleanup compared object identity against the copy and could not release the retained original. #8931 correctly moved the copy before retention. Restoring only the old copy order on the merge revision makes the cropped-input test pass again, isolating this dependency.This change:
Serving code, native listen policy and timeout values are unchanged. Existing runtime design contracts and user documentation remain applicable; this PR changes only two test files.
Test Plan
vLLM Version: 0.31.0; torch 2.13.0; transformers 5.14.1.
vLLM-Omni Commit:
653f20bc0163942e6dfe1c113dea111e270c54e3, based on90576488a.Validation used one H800 80 GB, the existing three-stage single-GPU deployment, CUDA graphs enabled, and offline model revision
503e754207c94da6bb26850b4469f367c9ea3582(matching the failing CI log).CPU tests:
pytest -q tests/engine/duplex/test_session_runner.py \ -m 'core_model and cpu' --run-level core_modelFull advanced test combination used by the affected CI step:
CUDA_VISIBLE_DEVICES=0 HF_HUB_OFFLINE=1 TRANSFORMERS_OFFLINE=1 \ pytest -sv tests/e2e/online_serving/test_minicpmo_4_5_duplex.py \ tests/e2e/online_serving/test_minicpmo_4_5_window.py \ -m 'advanced_model and cuda' --run-level advanced_modelThe GPU command requires the model and reference-audio assets to be cached. For a local checkpoint tree, set
MODEL_PREFIXsoopenbmb/MiniCPM-o-4_5resolves beneath it.Test Result
28283fda3, original 1.4-second input5a5083ad3, original 1.4-second inputThe repetitions used a temporary test parametrization with the same scenario and assertions. That parametrization and the diagnostic control are not included in this PR.
2fbb176f8: 18 passed, 4 deselected, 16 warnings in 677.81 seconds. This run did not use the temporary repetition.2fbb176f8: 251 passed across the session runner, scenario driver and Buildkite uploader tests. The residual-audio regression fails on the parent and passes after [Model] Add JoyAI speech input to unified duplex sessions #8931.Local validation used H800; the affected Buildkite job used H100. These local results do not establish an H100 Buildkite pass.
Self-review
c8ccb8aed; its intervening change concerns MOSS models.