Repository navigation
[WIP][Bugfix][MiniCPM-o] Stop pre-warm state from dropping the admitting codec chunk - #7995
chickeyton wants to merge 1 commit into
Conversation
|
This PR appears to belong to: docs/design/module/ar_runtime.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 @chickeyton, 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. |
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
4f07c7b to
e716086
Compare
Pre-check reportSelf-review of
Verdict: 0 blocking, 3 warnings. Blocking issues found and fixedThree ✗ were found on the first pass and are fixed in the current head:
Remaining warnings, with rationaleCode quality — one new Code quality — five new Simplification — the two Code2Wav changes are defence in depth. Once the model-runner change lands, neither is needed to make the reported failures go away: the final end-to-end run logs no stream-position recovery warning at all, because the desync no longer happens. They are here because both issues ask for the stage to survive a bad chunk rather than take every session on the replica down with it, and because the transport has documented silent-drop paths that can produce the same gap from a different direction. Both paths are reachable and covered by tests. A reviewer who would rather land the one-line ownership fix alone can say so and I will split them out. What was verifiedEvery new test fails without the fix. Reverting Pre-commit passes end to end, not skipped: The end-to-end evidence is in the PR description: 13 failed / 7 passed on |
|
#7992 just merged and closed #7979 along with #7962, so here's how I think the two fit. The setup buffer in your NEWREQDIAG ( The runner check and the Code2Wav containment here still look worth having, so a stray setup buffer or a dropped chunk can't take the stage down again. I haven't run this on top of #7992. Your trace also fills in the part I could only guess at on #7978, thanks. |
…odec chunk `OmniGPUModelRunner._update_additional_information` skipped a newly scheduled request's `additional_information` whenever the request also carried a non-empty `model_intermediate_buffer` and the model opted into replace semantics. That guard exists so a fresh boundary marker is not clobbered by the stale chunk it supersedes (vllm-project#6406), but it also fires when the buffer is only the request's own setup state. MiniCPM-o 4.5 Code2Wav is the one model with `replace_runtime_additional_information = True`. Async-chunk pre-warm gives its Stage-2 requests a setup buffer holding `duplex` and `global_request_id`, and every codec chunk that admits such a request travels in `additional_information`. So the first chunk of every Talker generation reached the vocoder with all producer metadata stripped and `_parse_item` substituted defaults, which killed the Stage-2 engine two different ways: * A real codec window lost `cache_epoch` and `chunk_seq`, so the vocoder recorded epoch 0 / chunk 0 for what was really epoch 1 / chunk 0. The next chunk arrived correctly labelled and raised `new_epoch_requires_first_chunk` (vllm-project#7979). * The control-only segment boundary is one placeholder token plus `code_flat_numel == 0`. Losing that zero made the placeholder look like codec data, so a one-frame non-final window raised `chunk_below_lookahead_window` (vllm-project#7978). Decide by what the buffer contains: a buffer carrying an `OmniPayload` section is a producer snapshot and still wins, while a buffer holding only setup state no longer shadows the chunk. Two Code2Wav changes keep a stage-wide failure off the table when a chunk does go missing anyway. A stream-position gap now resets or skips the affected request instead of raising, since the transport can legitimately lose a payload and killing the engine drops every session on the replica; a backward position is still refused, because replaying it would advance the codec cache twice. A non-final window narrower than the encoder's pre-lookahead kernel is withheld and rides on the next window rather than being vocoded alone. Signed-off-by: chickeyton <ngton2014@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| prompt_wav=item.prompt_wav, | ||
| token2wav=item.previous.token2wav, | ||
| ) | ||
| for item in held_items |
There was a problem hiding this comment.
[P2] Track the stream position even when the first window is withheld
When the first window of a request or a new cache epoch is shorter than the lookahead minimum, item.previous is None, so this condition skips recording its epoch and chunk sequence even though _held_tokens retains its frames. This breaks the recovery logic: replaying the initial chunk appends the same frames again; switching epochs before any decode mixes the old held frames into the new epoch; and, when an older epoch already has cached state, the next chunk of the new epoch triggers the reset again and discards that new epoch's held frames.
Please retain the epoch and sequence for withheld windows independently of whether a decoder cache has been initialized, and use that position for replay rejection and epoch transitions. Add regression coverage where the first window of a request or epoch is short; the existing withholding tests establish a decoder cache before withholding.
| _OMNI_PAYLOAD_SECTIONS = frozenset({"meta", "codes", "ids", "embed", "hidden_states", "latent"}) | ||
|
|
||
|
|
||
| def _is_replacement_snapshot(buffer: dict) -> bool: |
There was a problem hiding this comment.
Please move it to utils
I am testing it again, maybe some of the changes is not needed anymore |
Update after rebasing onto
|
| Tree | Result | Code2Wav fatals | stage 2 has no live replica |
|---|---|---|---|
main @ c7cd37b6 (before #7992) |
13 failed, 7 passed, 25:33 | new_epoch_requires_first_chunk kills Stage 2 |
33 |
main @ f7e28348, without this PR |
20 passed, 0 failed, 35:28 | none | none |
main @ f7e28348, with this PR |
20 passed, 0 failed, 34:11 | none | none |
test_window_rebuild_and_next_session, the case that produced the #7979 crash, passes on plain f7e28348. This PR changes nothing on the current main.
Why #7992 fixes it
It is the only production change among the four commits added since c7cd37b6 that touches this path. It stops Stage-0's prompt being stored on the request state for resumable submissions, and that prompt is exactly what _prewarm_async_chunk_stages copied into the Stage-2 request via copy_request_snapshot(original_prompt). So the pre-warm buffer holding duplex and global_request_id no longer exists.
That is the same defect this PR describes, removed from the producer side instead of the consumer side. With an empty buffer, _update_additional_information falls through to the chunk's own additional_information and no producer metadata is stripped, so neither new_epoch_requires_first_chunk nor chunk_below_lookahead_window can be reached that way.
One thing that did not change
The ownership rule in OmniGPUModelRunner._update_additional_information is untouched by #7992. Running this PR's two runner regression tests against f7e28348's production code still gives:
FAILED tests/worker/test_omni_gpu_model_runner.py::test_new_request_setup_buffer_does_not_shadow_its_admitting_chunk
FAILED tests/worker/test_omni_gpu_model_runner.py::test_new_request_setup_buffer_does_not_shadow_a_boundary_placeholder
2 failed
A newly scheduled request's payload is still discarded whenever the request also carries a setup-only model_intermediate_buffer and the model uses replace semantics. #7992 removes the one input shape that reached it in this pipeline; it does not change the rule. The bug is latent rather than fixed, and any future path that puts a non-payload buffer on a new async-chunk request reintroduces the same silent metadata loss.
e716086 to
6755cd5
Compare
|
@Gaohan123 after testing it again, I found that issue #7978 and #7979 are fixed by PR #7992 already, you may consider closing issue #7978 and this PR |
|
Thanks for your contribution |
Purpose
Fixes #7979 and #7978.
#7978 and #7979 share the same root cause. They were filed separately because they raise different
MiniCPMO45Code2WavBatchErrorreasons, but both are the same lost payload: the codec chunk that admits a Stage-2 request is dropped before the vocoder ever sees its producer metadata.new_epoch_requires_first_chunkis what that loss looks like when the dropped chunk is a real codec window;chunk_below_lookahead_windowis what it looks like when the dropped chunk is the control-only segment boundary. One change to the model runner removes both. That is why they are fixed together here rather than in two PRs.Both are merge-CI failures of
Omni · MiniCPM-o 4.5 Duplex Testthat kill the Stage-2 engine core and take every duplex session on that replica with it (stage 2 has no live replica).Root cause (one bug, two signatures)
OmniGPUModelRunner._update_additional_informationskips a newly scheduled request'sadditional_informationwhenever that request also carries a non-emptymodel_intermediate_bufferand the model opts into replace semantics:The guard was added in #6406 so a fresh boundary marker is not clobbered by the stale chunk it supersedes. It also fires, however, when the buffer holds nothing but the request's own setup state.
MiniCPM-o 4.5 Code2Wav is the only model with
replace_runtime_additional_information = True. Async-chunk pre-warm gives its Stage-2 requests a setup buffer holdingduplexandglobal_request_id, and the codec chunk that admits such a request arrives inadditional_information. So the first chunk of every Talker generation reaches the vocoder with all producer metadata stripped, and_parse_itemsubstitutes defaults.Instrumented run of the failing CI test at
c7cd37b6, probing both sources on every new request:The chunk is present and is thrown away. That single loss produces both reported failures, depending only on which chunk happens to admit the request:
cache_epochandchunk_seq, so Code2Wav records epoch 0 / chunk 0 for what is really epoch 1 / chunk 0. The following chunk arrives correctly labelled at epoch 1 / chunk 1, and_parse_itemraisesnew_epoch_requires_first_chunk.code_flat_numel == 0. Losing that zero makes the placeholder look like codec data, so a one-frame non-final window reachesdecode_batchand raiseschunk_below_lookahead_window {"frames":1,"minimum":4}.The second signature reproduces directly against unpatched
main: feed Code2Wav one boundary placeholder whose producer meta was stripped and it raises that exact JSON.Changes
vllm_omni/worker/gpu_model_runner.py— decide by what the buffer contains, not by which source it is. A buffer carrying anOmniPayloadsection (meta,codes,ids,embed,hidden_states,latent) is a producer snapshot and still wins, which keeps #6406's boundary-marker behaviour. A buffer holding only setup state no longer shadows the chunk that admitted the request.OmniNPUModelRunnerinherits this.vllm_omni/model_executor/models/minicpmo_4_5/minicpmo_4_5_code2wav.py— two containment changes, so a missing chunk can never again take the stage down:batched_token2wav.pyonly renames_pre_lookahead_lentopre_lookahead_lenso the model layer can read it.Test Plan
Hardware: L20X, vLLM 0.29.0,
openbmb/MiniCPM-o-4_5,vllm_omni/deploy/minicpmo_4_5.yaml. Baseline and patched runs use the merge-CI command:Unit:
tests/worker/,tests/model_executor/models/minicpmo_4_5/,tests/model_executor/stage_input_processors/,tests/engine/duplex/. Lint:ruff checkandruff format --check.Test Result
End-to-end, same command, same machine, same deploy config:
stage 2 has no live replicamain(c7cd37b6)new_epoch_requires_first_chunkkills Stage 2The baseline reproduces merge build #15824 exactly: same error JSON, same 13/7 split, same cascade into
DuplexSessionClosedError: expired: request_cleanupandstage 2 has no live replicafor the following cases.With the patch, Code2Wav logs no stream-position recovery warning at all: the runner change removes the desync rather than papering over it, and the two Code2Wav changes stay unexercised as the safety net they are.
The four
test_minicpmo_4_5_duplex.pycases tracked separately in #7962 (TimeoutError: Timed out waiting for duplex CI speech response.created, plus the resume assertion) also pass here. That follows from the same cause: the stripped chunk also losesduplex_turn_id,turn_endandtts_is_last_chunk, so the first chunk of each turn could not be attributed to its response.Unit tests,
CUDA_VISIBLE_DEVICES="":The same 6 fail identically on
mainatc7cd37b6(4 intest_batched_omni_output.py, 2 intest_cuda_graph_wrapper.py); they need a CUDA device and this run had none. Nothing else regressed.Every new test fails on unpatched production code and passes with the fix. Running the 7 new cases with
vllm_omni/reverted toc7cd37b6and onlytests/from this branch gives7 failed; with the fix applied,7 passed.pre-commit run --from-ref c7cd37b6 --to-ref HEADpasses end to end, includingruff check,ruff format,mypy-3.10, SPDX, forbidden imports, thetorch.cudaratchet, and the test-marks hook.New regression coverage:
test_new_request_setup_buffer_does_not_shadow_its_admitting_chunkand..._a_boundary_placeholderpin the runner contract for both signatures.test_streaming_new_request_marker_replaces_terminal_chunk_snapshotis unchanged and still passes, so [Bugfix][MiniCPM-o] Fix async-chunk snapshot replacement and prompt cleanup #6406's behaviour is preserved.chunk_seq, a replayed chunk, a retired epoch, short-window withholding and its flush, and a boundary placeholder whose metadata was stripped.🤖 Generated with Claude Code