[Green-Vial]

Fixes #7077.

Purpose

This PR adds Multi-Step Merged Decode to vLLM-Omni: the scheduler merges K consecutive decode steps into a single scheduling round, and the runner replays all K steps inside one execute_model call. Each engine-loop iteration now produces K tokens per request instead of 1, amortizing the fixed per-step host cost of the engine loop (schedule, IPC, output processing, block-table maintenance) by K.

The mechanism is opt-in (VLLM_OMNI_MULTI_STEP_STEPS, default 0 = disabled, behavior identical to baseline), works under both async and sync scheduling, and is fully implemented with 41 unit tests.

Reading the numbers below. MiniCPM-o 4.5 is served as a three-stage pipeline: an 8B thinker (understanding) → the talker (20-layer AR speech generation, the stage this PR optimizes) → Code2Wav (vocoder). E2EL and RTF are end-to-end metrics diluted by the two untouched stages: the talker's own per-token time drops 20-26%, while the end-to-end RTF improves ~8% at c1. The Stage1 time/token column is the direct measure of this optimization; E2EL/RTF are its diluted end-to-end projection.

Measured results (MiniCPM-o 4.5 talker, Atlas 910B3, stage-1 FULL cudagraph baselines, 2 scheduling modes x 3 concurrency levels x {K=0, 6, 8, 12, 14} = 30 groups): every configuration improves, and the best window size per leg gives:

Leg (FULL graph)	Best K	E2EL (K=0 baseline -> best)	RTF (K=0 baseline -> best)	Stage1 time/token (baseline -> best)
c1 x32, sync	12	3230.6 -> 2990.9 ms (-7.4%)	0.7921 -> 0.7284 (-8.0%)	9.44 -> 7.50 ms (-20.6%)
c1 x32, async	6	3280.5 -> 3175.8 ms (-3.2%)	0.8034 -> 0.7758 (-3.4%)	9.71 -> 7.49 ms (-22.9%)
c4 x64, sync	14	12178.5 -> 10719.0 ms (-12.0%)	2.9679 -> 2.5273 (-14.8%)	10.39 -> 8.14 ms (-21.7%)
c4 x64, async	14	13178.5 -> 10455.2 ms (-20.7%)	3.2198 -> 2.4723 (-23.2%)	10.28 -> 7.62 ms (-25.9%)
c8 x128, sync	12	21365.6 -> 19419.0 ms (-9.1%)	5.3879 -> 4.7595 (-11.7%)	10.71 -> 8.53 ms (-20.4%)
c8 x128, async	8	21285.4 -> 19530.2 ms (-8.2%)	5.2250 -> 4.7967 (-8.2%)	10.75 -> 8.33 ms (-22.5%)

All percentages are relative to the same leg's K=0 (window disabled) baseline under stage-1 FULL cudagraph. Audio output is identical to baseline: the c1 codec streams are bit-identical (per-token dump compare), and total audio duration is unchanged within <1% at c4/c8 (batching-order effects). Recommended window size: K=12 for sync legs, K=14 for c4 async, K=8 for c8 async; K=12 is the safest single choice across all legs.

Background

vLLM-Omni splits a multimodal request into stages executed by independent engine processes. AR stages (e.g. the MiniCPM-o 4.5 TTS talker, 20 layers, hidden 768) are small models doing pure decode. Every generated token pays one full engine round trip:

schedule() -> [IPC] -> execute_model() -> sample_tokens() -> [IPC] -> update_from_output()

The per-step wall time has two components: a fixed host share (scheduling logic, message serialization, shared-memory transfer, output processing — roughly 2 ms per step under stage-1 FULL cudagraph) and the device forward (roughly 7-8 ms). Because the host share is paid once per token and the batch composition of a decode batch is highly stable (running batches do not take new requests mid-step), the host share can be amortized by replaying K consecutive steps inside one engine iteration and feeding each step's sampled token directly into the next.

Multi-step windows are real sampling (lossless — no draft/reject, no distribution change), mutually exclusive with spec decode, and gated on the talker stage's capability contract.

Platform applicability: the mechanism (planning / accounting / gating / capability contract) is platform-agnostic framework code; the K-step executor currently ships with the NPU runner only. Platforms without the executor are refused by a fail-closed static gate — enabling the flag elsewhere is a no-op. See Porting in the appendix.

Test Plan

Unit tests (41, pure CPU, no NPU required):

python3 -m pytest tests/core/sched/test_multi_step_decode.py -q    # 30
python3 -m pytest tests/worker/test_multi_step_decode_runner.py -q # 11

Coverage: window-size resolution (config/env/clamping/invalid values), static gates (parallelism / spec decode / LoRA / mamba / platform refusal paths), capability contract (stage-qualified tts=True / llm=False, stage flag taking precedence over a blanket flag), plan rewriting (new-request / non-uniform batch / prefill-phase / penalty refusals, budget clamping, KV allocation failure with block-row sync, confirmed-length invariant), reconcile (full report no-op / shortfall rollback / clamping / empty report), runner validation (refusal paths + fail-closed exception fallback), and both scheduling modes (async placeholder accounting and sync direct accounting are both accepted end to end).

Gate-open verification (INFO level, no DEBUG needed):

grep "Multi-step counters" serve.log
# Multi-step counters: executed_windows=101 executed_steps=808
#                      refusals={'no_plan': 11, 'prefill_phase': 6}

executed_windows > 0 with only expected fail-closed refusals (no_plan) means windows really execute; per-group windows/steps are also recorded in the appendix.

Benchmark methodology (full records in Appendix F): Atlas 910B3 single NPU; original OpenBMB/MiniCPM-o-4_5 (bf16, not quantized); three stages co-located on one card; Seed-TTS EN branch, first 32/64/128 sessions for c1/c4/c8, request-rate inf, 2 warmups, no oversample; stage-1 cudagraph_mode: FULL for every group (including the K=0 baseline); serve restarted per group; Mean E2EL / TTFT / AUDIO_RTF + stage1 per-token time recorded. The same matrix is run under async scheduling and under sync scheduling (async_scheduling: false on stage 1), giving 2 x 3 x 5 = 30 groups.

Quick A/B: run the same serve config twice with vllm bench serve --omni --save-result (first without the env var, then with VLLM_OMNI_MULTI_STEP_STEPS=<K>) and compare the saved JSON.

Test Result

Per-leg best window sizes (vs each leg's K=0 baseline, stage-1 FULL graph) are shown in Purpose. Summary of the full 30-group sweep (Appendix F):

• Every windowed configuration improves both E2EL and RTF over its own K=0 baseline. No configuration regressed.
• Sync scheduling is the stronger showcase: at c1 (the official single-concurrency metric) K=12 cuts E2EL by 7.4% and RTF by 8.0%; the same K under async cuts E2EL by 2.9%. Sync legs also keep K=6..14 all positive.
• Stage1 per-token time drops 20-26% at the best K on every leg — the engine-loop host share is amortized as designed.
• K=14 degrades the c1 legs (both modes) from burstier output pacing; K=8 under-amortizes at c1/c4. K=12 is the robust default; c4 async prefers K=14.

Interpretation

RTF and E2EL move together here (unlike host/device-mixed stages): every leg's best K improves both, because the talker is the pipeline's step-rate bottleneck and the window reduces its step time directly.

Sync vs async: the sync scheduler keeps at most one window in flight (schedule -> execute -> update strictly serial), so window accounting is simpler (no placeholder fence, direct optimistic num_computed_tokens rollback) and windows stay open longer under load — hence larger, more consistent gains. Async scheduling overlaps planner and executor and closes windows more readily; it still gains, but less.

E2EL vs stage1 per-token: stage1 is one of three co-located stages, so the end-to-end gain (~7-20%) is smaller than the stage1 gain (~20-26%); the remainder of the pipeline (thinker, codec decoder) is untouched.

Where the gain comes from, and its limits

Of the ~9.4 ms single-step time under stage-1 FULL cudagraph, the engine-loop host share (schedule, IPC round trips, update_from_output, block-table maintenance — roughly 2 ms) is amortized K-fold — this is the direct source of the gain. The device forward plus runner-internal host work (input prep, attention metadata, kernel launch, sampling — roughly 7.4 ms) executes every in-window step as usual and is not amortized, which caps the gain far below K-fold (measured ~20-26% at the stage level, consistent with a ~21% host share). Four boundaries:

1. Per-step device syncs remain: early-exit checks (flag.item()) and synchronous sampler readback interrupt the device pipeline each step, compressing the amortization gain.
2. Multi-concurrency closes windows under bursty arrivals: windows require an empty waiting queue and an all-decode batch; bursty arrivals are refused and each refused step pays the plan->reserve->reconcile churn. Under a steady stream the windows stay open (see appendix window counts).
3. K-token burst emission perturbs the downstream pipeline: stage2 consumes stage1 chunks with a tempo coupling; larger K means burstier chunks — the main cause of K=14's c1 regression and of the K=8/14 E2EL degradation at high concurrency.
4. The host share is bounded: amortizing it cannot remove the device forward; the ceiling is the host fraction of the step (~21% here), which is why the stage-level gain saturates around 20-26% rather than approaching K-fold.

Engineering conclusion: recommend K=12 (opt-in), default 0 — with no configuration the behavior is identical to baseline. For c4-concurrency async deployments K=14 measured best; K=12 remains within noise of it.

Appendix

A. Design

A.1 Window planning and accounting

Async scheduling tracks in-flight tokens with num_output_placeholders. The window makes one engine step look like "one scheduling round, K sampled tokens per request":

• After the base schedule, the planner allocates K-1 extra KV slots (allocate_slots), appends the new blocks to new_block_ids, and inflates placeholders += K-1, computed += K-1, num_in_flight_tokens += K-1.
• Invariant: computed - placeholders (confirmed KV length) is unchanged by the patch — no vLLM core accounting is touched.
• When the runner reports K tokens, the async scheduler consumes the placeholders back to the pre-patch level.
• If fewer than K tokens were produced (early exit or single-step fallback), reconcile_window_shortfall rolls the extra reservation back so computed matches the actually-written KV slots.

Under sync scheduling no placeholder fence exists (one window in flight max, the engine loop is strictly serial), so the same patch only inflates computed and num_in_flight_tokens; the shortfall is rolled back directly.

A.2 Multi-concurrency safety

A frozen batch delays admission of new requests for K steps. The planner therefore refuses to open windows whenever the waiting queue is non-empty (new work is admitted first) or when the batch contains newly scheduled requests. Under bursty arrivals windows simply close and reopen; TTFT is not blocked by windowed steps.

A.3 Fail-closed three-layer gating

Any layer can refuse; refusal is always a lossless fallback to the normal single-step path, with the scheduler-side reconcile absorbing the reservation:

1. Static gate (scheduler): single rank, no spec decode, no LoRA, not encoder-decoder, not mamba-align, and platform has a window executor (currently NPU only).
2. Plan gate: per-request admission (decode phase, no logprobs/penalties/bad words/structured output), per-request budget clamp (remaining max_tokens / context), whole-window fallback on KV allocation failure.
3. Runner validation: re-checks everything against runner-local state (batch consistency, model contract, sampler constraints); any exception falls back to single step.

A.4 Model capability contract

A stage only opens windows if its model class declares the contract:

• Direct: supports_multi_step_decode = True on the AR model class; or
• Stage-qualified (wrapper architectures serving several stages): supports_multi_step_stages = ("tts",) — gating resolves per model_config.model_stage, so the thinker stage of the same wrapper never opens windows.

The contract requires:

• preprocess(input_ids, input_embeds, **info): builds the step's decode embedding purely from the request-local state chain (previous sampled codec token + per-request cache);
• make_omni_output: advances that state chain in place;
• intermediate steps need no wire-format packaging — hidden states and codec deltas are accumulated and shipped once at window end.

Models that don't satisfy the contract simply don't declare it; the gates refuse and behavior is identical to baseline.

A.5 Runner K-step loop

One uniform decode layout is rebuilt up front and reused for all K steps (batch composition is frozen): query_start_loc / query_pos / num_scheduled_tokens, _build_attn_state, batch descriptor and cudagraph mode. Then per step:

for step in range(window_k):
    # 1. advance computed/positions/seq_lens (host-side copies, non-blocking H2D)
    # 2. compute_slot_mapping (KV slot mapping)
    # 3. model.preprocess(): per-request embedding from request-local state
    # 4. set_ascend_forward_context + _model_forward
    # 5. extract_multimodal_outputs -> accumulate hidden / codec delta / finished
    # 6. binary STOP/CONTINUE token: derived from request-local state under
    #    greedy sampling (skips the device sampler; any uncertainty falls back
    #    to the exact single-step sampler path)
    # 7. bookkeeping: -1 placeholders in token_ids_cpu/output_token_ids
    #    (backfilled at window end)
    # 8. all requests finished -> early exit (skip dead rows' device time)
# window end: stack K sampled steps -> backfill -> async feedback path
#             (set_async_sampled_token_ids) -> _build_window_output
#             -> stashed as _multi_step_pending_output for sample_tokens()

Amortized per window: schedule(), IPC round trips, _update_states, block table commit, cudagraph decisions, output build/serialization. Two further in-window amortizations (WIA) trim the loop itself:

• Rotary prebuild: the window's K * num_reqs cos/sin rows are filled with one update_cos_sin; each step copies the prebuilt block into the rows the captured graph reads (a plain device copy) instead of per-step index-select/repeat/chunk.
• Engine-token derivation: the talker's binary STOP/CONTINUE token is int(state.finished) once the min_tokens mask is released, so the device logits-prep + sampler + token-gather path is skipped. Safety gates refuse on non-greedy / logit-bias / native-duplex sampling, and the derived tokens are emitted in the engine's token dtype (int32) so the async feedback scatter into the int32 input_ids buffer stays type-correct.

FULL-graph padding (NPU-specific): under FULL cudagraph replay, FIA attention's TND layout requires sum(query_start_loc) == hidden_states rows. When cudagraph padding makes num_tokens_padded > num_reqs, the window loop mirrors the single-step _pad_query_start_loc_for_fia behavior by inserting a dummy request row (its KV rows point at block 0; reshape_and_cache slices by unpadded token count so nothing is written) and passes the padded num_reqs_padded to _build_attention_metadata.

B. Changes

Framework-generic (scheduler side, platform-agnostic):

File	Change
vllm_omni/core/sched/multi_step_decode.py	New: window planner (plan_multi_step_window), reconcile (reconcile_window_shortfall), static gate, capability resolution, budget clamp, resolve_multi_step_steps; async and sync accounting paths
vllm_omni/core/sched/output.py	Scheduler output gains multi_step_plan: dict[str, int] (req_id -> K)
vllm_omni/core/sched/omni_ar_scheduler.py	schedule() tail tries window planning (try/except, falls back); update_from_output() reconciles per plan
vllm_omni/engine/arg_utils.py	multi_step_decode_steps: int = 0, forwarded to model config
vllm_omni/config/model.py	multi_step_decode_steps: int = 0

Platform rollout (NPU runner; other platforms pending port):

File	Change
vllm_omni/platforms/npu/worker/multi_step_decode.py	New: fail-closed validation, K-step replay with WIA (rotary prebuild, engine-token derivation, lightweight bookkeeping), diagnostic counters
vllm_omni/platforms/npu/worker/npu_ar_model_runner.py	execute_model() head hook (validate -> execute -> return); sample_tokens() hands over the stashed output; env-gated per-step timing

Model instance (MiniCPM-o 4.5; other models declare per the contract):

File	Change
vllm_omni/model_executor/models/minicpmo_4_5/minicpmo_4_5_omni.py	Wrapper declares stage-qualified supports_multi_step_stages = ("tts",)
vllm_omni/model_executor/models/minicpmo_4_5/minicpmo_4_5_omni_tts.py	Talker declares supports_multi_step_decode = True

Tests: tests/core/sched/test_multi_step_decode.py (30), tests/worker/test_multi_step_decode_runner.py (11).

B.1 Commit breakdown

The PR is three commits, each self-contained and independently correct (reviewable in order):

1. Multi-step merged decode: K decode steps per engine step on async scheduling — the core mechanism: scheduler-side planning/accounting/reconcile, fail-closed three-layer gating, capability contract, the NPU runner's K-step replay loop, and the 41 unit tests. Everything works end to end on async scheduling.
2. Prebuild attention metadata for multi-step decode windows — inside the window loop, steps 1..K-1 reuse step-0's attention metadata (pure decode windows only vary in per-step sequence lengths); the per-step builder remains as fallback. First WIA item, still async-only.
3. Multi-step windows on sync scheduling + window-internal amortization — extends windows to the sync scheduler (direct accounting, step-0 input from the request-local output chain) and adds the remaining WIA items (rotary prebuild, engine-token derivation) plus the fixes that make both paths exact: the min_tokens mask is counted from the runner's request-local output length (the sampler metadata view is None on the window path, which stuck the mask and re-scheduled finished requests for ~1900 dead windows under sync), and derived tokens are emitted int32 to match the engine token layout (an int64 src crashes the input scatter on the batch-reorder path). Includes the env-gated stage1 timing used to produce the per-token numbers below.

C. Usage

# 1) Environment variable (preferred for A/B)
export VLLM_OMNI_MULTI_STEP_STEPS=12   # 0=off (default); 2~16 = window size

# 2) deploy yaml, per-stage engine_extras (recommended for production)
#   vllm_omni/deploy/xxx.yaml -> stages[1]:
#     async_scheduling: false        # sync scheduling (optional; both modes supported)
#     engine_extras:
#       multi_step_decode_steps: 12
#   and platforms.npu.stages[1].compilation_config.cudagraph_mode: FULL

# 3) CLI
vllm serve ... --multi-step-decode-steps 12

D. Tuning and porting

D.1 Window size K (swept: K=6/8/12/14)

• Recommended K=12: positive on every leg; best for sync c1/c8 and second-best (within noise) elsewhere. K=14 beats it only on the c4 legs; K=8 under-amortizes at c1/c4; K=14 degrades the c1 legs.
• RTF-first: K=12 (or K=14 on c4-async deployments); E2EL/P99-first: K=12 and watch the high-concurrency groups.
• Theory: with p = the amortizable engine-loop host share (~21% of the single-step time under stage-1 FULL graph), the single-stream stage gain approaches 1/(p/K + (1-p)) only if in-window per-step host cost stays below a full engine step and there are no intermediate syncs — in practice ~20-26%.

D.2 Deciding whether this class of optimization fits (methodology)

1. Look at the windowed stage's own metric first (stage1 wall time per token), to avoid multi-stage dilution; when E2E and stage metrics disagree, locate the mechanism with the stage metric, then inspect pipeline interaction.
2. Always verify the gates actually opened: grep "Multi-step counters" serve.log for executed_windows > 0; the planned-vs-executed gap is the refused-churn cost.
3. Repeat runs to calibrate noise; conclusions inside the noise band don't count.
4. E2EL distribution: larger K raises the share of tail requests with remainder steps; watch P99.

D.3 Porting to other models

Satisfy the capability contract, two steps:

1. Declare supports_multi_step_decode = True (wrapper architectures declare supports_multi_step_stages on the outer class).
2. Confirm preprocess() depends only on request-local state (per-step info comes from the runner's model-intermediate buffer, including duplex_token_offset / computed-token counts).

Models that don't satisfy the contract simply don't declare; gates refuse and behavior matches baseline.

D.4 Porting to other platforms (GPU/...)

The K-step executor currently ships with the NPU runner only. Three steps:

1. Add the execute_model() / sample_tokens() hooks on the target platform's AR runner (reuse the validation and K-step loop from the NPU worker module, replacing platform-specific forward-context / rotary calls).
2. Handle that platform's graph-replay padding constraint (on NPU: the FIA TND dummy request).
3. Relax the platform check in the static gate.

D.5 Known boundaries (gated automatically, no manual action needed)

• Structured output, logprobs, frequency/presence/repetition penalties, bad_words, allowed_token_ids
• Spec decode, LoRA, encoder-decoder, mamba align, PCP/DCP/CP, PP/TP/DP>1
• Routed experts, KV transfer criteria, prefix-cache-hit steps
• Non-empty waiting queue / steps scheduling new requests (multi-concurrency admission protection)

E. Rollback

• Multi-step windows: remove engine_extras.multi_step_decode_steps, set it to 0, or unset the environment variable — behavior identical to baseline.
• Sync scheduling / FULL graph: restore the baseline deploy yaml (async_scheduling default, cudagraph_mode: PIECEWISE).
• Code level: revert the individual commits (no invasive changes beyond the config fields).

F. Test records

F.1 Protocol — Atlas 910B3 single NPU; original OpenBMB/MiniCPM-o-4_5 (bf16); three stages co-located; Seed-TTS EN, first 32/64/128 sessions (c1/c4/c8), request-rate inf, 2 warmups, no oversample; stage-1 cudagraph_mode: FULL for all groups; serve restarted per group; async and sync scheduling each measured over the full K sweep. Window execution confirmed per group via runner counters (executed_windows/executed_steps, only no_plan refusals). All groups completed with zero failed requests.

F.2 Equivalence verification — codec streams bit-identical between windowed and baseline runs (per-token dump compare, c1); total audio duration identical at c1 (134.92 s across every configuration); c4/c8 total durations within <1% across K (batching-order effects). The min_tokens and token-dtype fixes in commit 3 are what make the sync and async window paths exact.

F.3 Full sweep — mode | conc | K | E2EL(ms) | TTFT(ms) | RTF | audio(s) | stage1 ms/token. Stage1 ms/token: single-step path at K=0, window path at K>0.

Sync scheduling:

conc	K=0	K=6	K=8	K=12	K=14
c1	E2EL 3230.6 / TTFT 425.4 / RTF 0.7921 / st1 9.44	3141.0 / 428.0 / 0.7658 / 7.91	3069.5 / 423.1 / 0.7460 / 7.65	2990.9 / 429.4 / 0.7284 / 7.50	3072.8 / 435.9 / 0.7475 / 7.44
c4	12178.5 / 486.9 / 2.9679 / 10.39	11040.2 / 507.2 / 2.5938 / 8.66	10945.1 / 499.8 / 2.5813 / 8.52	11650.6 / 504.6 / 2.8189 / 8.48	10719.0 / 502.3 / 2.5273 / 8.14
c8	21365.6 / 500.1 / 5.3879 / 10.71	19722.1 / 501.9 / 4.8991 / 8.79	20541.3 / 510.4 / 5.0195 / 8.64	19419.0 / 517.1 / 4.7595 / 8.53	20385.8 / 510.1 / 5.0072 / 8.47

Async scheduling:

conc	K=0	K=6	K=8	K=12	K=14
c1	3280.5 / 427.8 / 0.8034 / 9.71	3175.8 / 427.9 / 0.7758 / 7.49	3217.1 / 441.2 / 0.7832 / 7.20	3186.8 / 429.1 / 0.7776 / 6.71	3220.1 / 428.3 / 0.7848 / 6.66
c4	13178.5 / 485.4 / 3.2198 / 10.28	11134.0 / 501.6 / 2.5895 / 8.45	12823.8 / 504.3 / 3.1465 / 8.37	11495.0 / 505.9 / 2.7416 / 8.19	10455.2 / 502.3 / 2.4723 / 7.62
c8	21285.4 / 508.6 / 5.2250 / 10.75	20086.8 / 504.2 / 4.9678 / 8.34	19530.2 / 513.4 / 4.7967 / 8.33	20153.6 / 513.1 / 4.9753 / 7.81	20703.3 / 515.2 / 5.0991 / 7.68

Cell format: E2EL(ms) / TTFT(ms) / RTF / stage1 ms-per-token. Bold = best E2EL per leg; audio (s): c1 = 134.92 for every cell; c4 = 279.0-281.4; c8 = 531.6-538.8.

F.4 Window counters (representative; windows/steps at the best K per leg): sync c1 K12 251/3012, async c1 K6 601/3606, sync c4 K14 401/5614, async c4 K14 401/5614, sync c8 K12 851/10212, async c8 K8 1301/10408. Executed steps / K = executed windows (full windows; early exits reconciled by the scheduler shortfall path).

F.5 Reproducibility notes — run-to-run E2EL variance on this host is roughly +/-3% (c1). All groups above are single runs per cell; the best-K conclusions were double-checked with a repeated c1 sync K=12 A/B (-6.2% to -7.7% E2EL across repetitions, same direction).

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[vllm-omni-review-bot]

This PR appears to be related to model: minicpm.

Model owners: @y-null

@Green-Vial, 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.

[vllm-omni-review-bot]

Omni ReviewBot triage note

Automated triage of commit 9a5b83e1ac78 produced:

• Priority: high. Prompt maintainer attention is suggested.

These are automated triage suggestions only — the final decision belongs to the maintainers.

[Green-Vial]

Self-review notes:

• Reviewed the full diff: scheduler-side window planning/accounting is fail-closed — every refusal path falls back to the normal single-step schedule, and reconcile_window_shortfall keeps the placeholder/KV accounting consistent.
• Runner-side re-validation re-checks all invariants against runner-local state; the K-step loop mirrors the single-step _sample path (logit bias, duplex hook, model sampler with prepared metadata), so sampling semantics are unchanged.
• Verified windows really execute on the MiniCPM-o 4.5 talker via the INFO-level counters (executed_windows>0) and a 30-group benchmark matrix (2 graph modes x {off, K=8/10/12/14} x 3 concurrency), zero failures.
• 41 unit tests pass (30 scheduler + 11 runner); platforms without the window executor are refused by the static gate.
• No core vLLM accounting logic is modified — a window is expressed as one normal multi-token scheduling step.

[Green-Vial]

After submitting the PR, I reviewed it again and noticed an issue. I’m working on a fix and will push an update once it’s ready.

[Green-Vial]

After submitting the PR, I reviewed it again and noticed an issue. I’m working on a fix and will push an update once it’s ready.

Have fixed！

[lishunyang12]

Check the pre-schedule token count so the final prefill token cannot open a decode window.

[Green-Vial]

Fixed in d03aed3: the gate now compares the pre-schedule count (num_computed_tokens - scheduled_tokens ) againstnum_prompt_tokens , so the final prefill token — which leavesnum_computed_tokens == num_prompt_tokens after the base scheduler advances it — is refused instead of opening a decode window, with a unit test covering that boundary.

[lishunyang12]

Preserve duplex metadata in window outputs so Code2Wav does not prematurely close the stream.

[Green-Vial]

Fixed in d03aed3: the window loop now accumulatesnative_duplex /duplex_epoch /duplex_turn_id /llm_output_text_utf8 /turn_end (turn_end OR-accumulated so a turn boundary inside the window is never lost) and ships them under the same flatmeta.* keys as the single-step path, leaving non-duplex payloads byte-identical.

[vllm-omni-review-bot]

Omni ReviewBot: no human activity for 12 days

@Green-Vial this pull request has had no human commit, comment or review since 2026-09-16. Please confirm the current plan and next step. The author or a maintainer decides whether to change the PR state.

To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline.

[vllm-omni-review-bot]

Omni ReviewBot routing record

Assigned Strict on cursor (cursor-grok-4.6-high) under experiment fleet-strict-cursor-grok46-zcode-glm53flash-5050-c5-z10-20261002.

[vllm-omni-review-bot]

Omni ReviewBot review

7 actionable finding(s).

CI at d03aed315bdb (2026-10-09T23:05:01.293174+00:00): no required or operator-watched checks were configured.

Full review analysis

Scan:

Category	Result
Tests / verification	5 finding(s) below
Security	no finding reported
Docs / comments	1 finding(s) below
Behavior / compatibility	no finding reported
Correctness	1 finding(s) below

Validated:

• [resolved] prefill gate (d03aed3 / lishunyang12): _request_admits_multi_step uses num_computed_tokens - scheduled_tokens (multi_step_decode.py:220); test_plan_window_refused_for_final_prefill_token covers computed==prompt. Residual: mixed-batch still refuse-all, not per-request
• [resolved] duplex meta shipped under meta.*; turn_end OR-accumulated; residual: non-turn_end keys last-step-win.
• [resolved] pipeline.py:57 model_stage="tts" matches supports_multi_step_stages = ("tts",) — off-fixture vs the PR test's WRAPPER="tts".
• [claim-verified] 42 test_* functions in the two new files (PR said 41).
• [claim-verified] table arithmetic: 3230.6->2990.9 = -7.42%; 9.44->7.50 = -20.55%; 13178.5->10455.2 = -20.66%. Measurement JSON not in-repo.
• [claim-verified] wrap-then-plan order: _wrap_omni_scheduler_output (omni_scheduler_mixin.py:178) does not copy multi_step_plan; plan mutates the wrapped output afterward (omni_ar_scheduler.py:285-297)

7 actionable finding(s).

Verdict: REQUEST CHANGES

Findings

• [P1] Accepted windows return from execute_model at npu_ar_model_runner.py:506 before… — vllm_omni/platforms/npu/worker/npu_ar_model_runner.py

Evidence for Accepted windows return from execute_model at npu_ar_model_runner.py:506 before…

Accepted windows return from execute_model at npu_ar_model_runner.py:506 before the existing on_requests_finished call at :522-523. execute_multi_step_window only runs _update_states (platforms/npu/worker/multi_step_decode.py:426) and never the model hook; plan_multi_step_window also does not inspect finished_req_ids, so waiting/new-req gates still allow a uniform decode of remaining siblings. MiniCPMO45OmniTTSForConditionalGeneration.on_requests_finished queues ids that _flush_deferred_cleanup pops from _request_audio_states/_request_generators at the next forward(); the omni wrapper also drops duplex sessions. Those finished_req_ids are one-shot on that SchedulerOutput, so a talker that finishes while siblings keep windowing never has its per-request state queued. Call the same hook immediately before execute_multi_step_window (IDs only; flush still happens at forward start).

Evidence: Trigger: concurrent MiniCPM-o talker decode (c>1) where request A finishes and the next schedule still opens a window for remaining siblings — plan_multi_step_window never inspects finished_req_ids (if scheduler_output.scheduled_new_reqs: return False only). Adverse effect: A's TTS _request_audio_states/_request_generators (and wrapper duplex sessions) are never queued for cleanup because those IDs are one-shot on that SchedulerOutput. npu_ar_model_runner.py:498 execute_multi_step_window(self, scheduler_output, multi_step_plan) then :506 return None — skips the unchanged hook at :522-523 if scheduler_output.finished_req_ids and hasattr(self.model, "on_requests_finished"): / self.model.on_requests_finished(scheduler_output.finished_req_ids). platforms/npu/worker/multi_step_decode.py:426 deferred = runner._update_states(scheduler_output) is the window's only state update (no model hook). Unchanged by this diff, present in the PR-time tree: minicpmo_4_5_omni_tts.py:586-594 self._deferred_cleanup_ids.update(...) / _request_generators.pop / request_audio_states.pop; :628 self._flush_deferred_cleanup(); minicpmo_4_5_omni.py:648-663 wrapper on_requests_finished pops _minicpmo45_duplex_request_sessions then forwards to the inner model.

Suggestion: if scheduler_output.finished_req_ids and hasattr(self.model, "on_requests_finished"):
self.model.on_requests_finished(scheduler_output.finished_req_ids)
execute_multi_step_window(self, scheduler_output, multi_step_plan)

---

_{🤖 This review was generated by InferMatrix Copilot, an open-source repo-maintenance agent for PR review, CI debugging and issue triage. Try it on your own repo, and ⭐ star it if it helped!}

[vllm-omni-review-bot]

[P0] New tests/worker/test_multi_step_decode_runner.py:17 collection-imports vllm_om…

Evidence and suggested fix

New tests/worker/test_multi_step_decode_runner.py:17 collection-imports vllm_omni.platforms.npu.worker.multi_step_decode, and that module top-level-imports vllm_ascend (multi_step_decode.py:60-62). Unchanged CUDA merge-gate .buildkite/cuda/test-ready.yml:18 (pytest -m 'core_model and cpu' --ignore=tests/diffusion --ignore=tests/model_executor) discovers tests/worker via pyproject testpaths=["tests"]; markers apply only after collection, so ImportError fails the whole Simple · Other Test job. Sibling tests/worker/test_native_duplex_hooks.py:2480-2481 already avoids importing the NPU runner because vllm_ascend is absent on those CPU images. Stub vllm_ascend in sys.modules before the import (same idea as tests/platforms/npu/test_310p_patches.py fakes) and set pytestmark = [pytest.mark.core_model, pytest.mark.cpu]. Do not importorskip('vllm_ascend') — that would skip these CPU-fake tests on the same lane.

Evidence: Trigger: CUDA merge-gate Simple · Other Test collects tests/worker. Unchanged by this diff, present in the PR-time tree: .buildkite/cuda/test-ready.yml:18 timeout 20m pytest -sv -m 'core_model and cpu' --ignore=tests/diffusion --ignore=tests/model_executor --cov=vllm_omni --cov-report=term-missing:skip-covered --cov-report=xml; unchanged pyproject.toml:215 testpaths = ["tests"]. Collection imports tests/worker/test_multi_step_decode_runner.py:17 import vllm_omni.platforms.npu.worker.multi_step_decode as msd_module (no pytestmark; every other tests/worker file has pytestmark = [pytest.mark.core_model, pytest.mark.cpu]) before -m can deselect. That module unconditionally does vllm_omni/platforms/npu/worker/multi_step_decode.py:60 from vllm_ascend.ascend_forward_context import set_ascend_forward_context and :61-62 import vllm_ascend.ops.rotary_embedding as vllm_ascend_rotary / from vllm_ascend.ops.rotary_embedding import update_cos_sin. Unchanged sibling tests/worker/test_native_duplex_hooks.py:2480-2481 These checks parse the source instead of importing it: the NPU runner needs / vllm_ascend, which is absent on the CPU test images that run this file.` Adverse effect: collection raises ImportError and fails the entire job; importorskip('vllm_ascend') would skip the CPU-fake validator on that same lane.

Suggestion: import sys
import types

pytestmark = [pytest.mark.core_model, pytest.mark.cpu]

_ascend = types.ModuleType("vllm_ascend")
_fwd = types.ModuleType("vllm_ascend.ascend_forward_context")
_fwd.set_ascend_forward_context = lambda *a, **k: None
_ops = types.ModuleType("vllm_ascend.ops")
_rot = types.ModuleType("vllm_ascend.ops.rotary_embedding")
_rot.update_cos_sin = lambda *a, **k: None
_rot._cos = None
_rot._sin = None
_rot._cos_sin_cache = None
sys.modules.setdefault("vllm_ascend", _ascend)
sys.modules.setdefault("vllm_ascend.ascend_forward_context", _fwd)
sys.modules.setdefault("vllm_ascend.ops", _ops)
sys.modules.setdefault("vllm_ascend.ops.rotary_embedding", _rot)

import vllm_omni.platforms.npu.worker.multi_step_decode as msd_module
from vllm_omni.platforms.npu.worker.multi_step_decode import validate_multi_step_plan

[Green-Vial]

Fixed in bb7589b. The module under test is now loaded from source with stubbed vllm_ascend modules (sys.modules.setdefault, so a real install wins on NPU hosts) — the pattern tests/platforms/npu/test_310p_patches.py already uses — rather than importorskip, which would skip these pure-CPU validator tests on the same lane. Verified: pytest -m 'core_model and cpu' now selects all 53 tests in the two multi-step files; it selected none of them before.

[vllm-omni-review-bot]

[P1] tests/core/sched/test_multi_step_decode.py adds CPU planner/reconcile coverage…

Evidence and suggested fix

tests/core/sched/test_multi_step_decode.py adds CPU planner/reconcile coverage (no vllm_ascend import) with no pytestmark. Unchanged CUDA PR/merge Simple Other Test (.buildkite/cuda/test-ready.yml:18, same command in test-merge.yml) runs pytest -sv -m 'core_model and cpu', and pyproject.toml documents core_model as L1&L2 tests run in each PR. Unmarked tests are collected then deselected, so the prefill-gate/KV-accounting cases never run on that lane. Sibling tests/core/sched/test_omni_ar_scheduler_streaming.py:25 sets pytestmark = [pytest.mark.core_model, pytest.mark.cpu]. Add the same pytestmark after the imports.

Evidence: Trigger: this PR adds tests/core/sched/test_multi_step_decode.py with pytest imported but no pytestmark (after the import at line 21 the next code is WINDOWED_ARCH / a fixture). Adverse effect / unmet requirement: CUDA PR CI Simple Other Test only executes tests matching core_model and cpu, so these planner/reconcile cases are collected then deselected and never run. tests/core/sched/test_multi_step_decode.py:15 import pytest; tests/core/sched/test_multi_step_decode.py:21 from vllm_omni.core.sched.multi_step_decode import (; tests/core/sched/test_multi_step_decode.py:31 # The one architecture that declares ``supports_multi_step_decode`` today. — no pytestmark between import and tests. Unchanged by this diff, present in the PR-time tree: tests/core/sched/test_omni_ar_scheduler_streaming.py:25 pytestmark = [pytest.mark.core_model, pytest.mark.cpu]; unchanged by this diff, present in the PR-time tree: .buildkite/cuda/test-ready.yml:18 timeout 20m pytest -sv -m 'core_model and cpu' --ignore=tests/diffusion --ignore=tests/model_executor --cov=vllm_omni --cov-report=term-missing:skip-covered --cov-report=xml; unchanged by this diff, present in the PR-time tree: pyproject.toml:225 "core_model: L1&L2 tests (run in each PR)",

Suggestion: from vllm_omni.core.sched.multi_step_decode import (
ENV_MULTI_STEP_STEPS,
MAX_MULTI_STEP_STEPS,
model_supports_multi_step,
plan_multi_step_window,
reconcile_window_shortfall,
resolve_multi_step_steps,
scheduler_allows_multi_step,
)

pytestmark = [pytest.mark.core_model, pytest.mark.cpu]

[Green-Vial]

Fixed in bb7589b. Both modules now carry pytestmark = [pytest.mark.core_model, pytest.mark.cpu], so the CPU lanes select them instead of collecting and deselecting.

[vllm-omni-review-bot]

[P1] validate_multi_step_plan is invoked unconditionally from NPUARModelRunner.execu…

Evidence and suggested fix

validate_multi_step_plan is invoked unconditionally from NPUARModelRunner.execute_model (npu_ar_model_runner.py:494). An empty/missing multi_step_plan is the default (OmniSchedulerOutput.multi_step_plan defaults to {}; multi_step_decode_steps defaults to 0), but validate treats it as return _refuse("no_plan") (multi_step_decode.py:106). _refuse increments _refusal_counts and logs Multi-step plan refused at runner: %s (first occurrence) on first hit (multi_step_decode.py:88-91). NPUARModelRunner is the shared NPU AR runner (npu_ar_worker.py:19, docstring: thinker/talker), so every default-disabled NPU AR stage emits a misleading refusal warning and poisons the planned-vs-executed diagnostic on the baseline path. Return None without calling _refuse when there is no plan.

Evidence: Trigger: every NPUARModelRunner.execute_model when the stage never planned a window (default K=0 or ineligible arch/stage). npu_ar_model_runner.py:494 multi_step_plan = validate_multi_step_plan(self, scheduler_output) is unconditional. multi_step_decode.py:104-106 plan = getattr(scheduler_output, "multi_step_plan", None) / if not plan: / return _refuse("no_plan"). _refuse at multi_step_decode.py:88-91 _refusal_counts[reason] = _refusal_counts.get(reason, 0) + 1 then logger.warning("Multi-step plan refused at runner: %s (first occurrence)", reason). Default empty plan in this diff: vllm_omni/core/sched/output.py:104 multi_step_plan: dict[str, int] = field(default_factory=dict); vllm_omni/config/model.py:126 multi_step_decode_steps: int = 0. Unchanged by this diff, present in the PR-time tree: npu_ar_worker.py:12 """NPU AR worker for thinker/talker stages in Omni model.""" and npu_ar_worker.py:19 self.model_runner = NPUARModelRunner(self.vllm_config, self.device). Adverse effect: the first execute_model on any default-disabled NPU AR server logs a false "plan refused" warning; every later step increments _refusal_counts["no_plan"], so the planned>>executed diagnostic cannot distinguish a real systematic refusal from the baseline path.

Suggestion: if not plan:
return None

[Green-Vial]

Fixed in 981388f. An empty plan returns None without calling _refuse, so a default-configured NPU AR stage no longer logs "plan refused" on its first step and no longer accumulates no_plan per step. A fresh windowed run now reports refusals={} in the counters line.

[vllm-omni-review-bot]

[P1] This PR adds multi_step_decode_steps: int = 0 on OmniModelConfig (model.py:126)…

Evidence and suggested fix

This PR adds multi_step_decode_steps: int = 0 on OmniModelConfig (model.py:126) and OmniEngineArgs (arg_utils.py:172), and the planner tells operators to leave that key at 0 in the deploy config (multi_step_decode.py:50). load_deploy_config only copies active_stream_window (stage_config.py:663); OmniStageModelConfig / _ModelEngineOverrides still only declare active_stream_window (omni_config.py:259/57). A top-level YAML multi_step_decode_steps: 12 is dropped before merge, so resolve_multi_step_steps keeps K=0. Wire it as a per-stage engine field next to active_stream_window on OmniStageModelConfig and _ModelEngineOverrides (not _PIPELINE_WIDE_ENGINE_FIELDS — the thinker must not inherit a talker K), document it on the MiniCPM-o talker stage, and add a factory test that stage-1 multi_step_decode_steps: 12 survives merge_pipeline_deploy → to_omegaconf → OmniEngineArgs. Env VLLM_OMNI_MULTI_STEP_STEPS is currently the only documented opt-in.

Evidence: Trigger: operator follows the planner's deploy-config contract and sets top-level YAML multi_step_decode_steps: 12 (same pattern as active_stream_window). Adverse effect: the key never reaches OmniEngineArgs/OmniModelConfig, so resolve_multi_step_steps stays at default 0 and windows never open unless VLLM_OMNI_MULTI_STEP_STEPS is set. vllm_omni/core/sched/multi_step_decode.py:50 Rollback: leave ``multi_step_decode_steps`` at 0 (the default) in the deploy config.; vllm_omni/engine/arg_utils.py:172 multi_step_decode_steps: int = 0; vllm_omni/config/model.py:126 multi_step_decode_steps: int = 0. Unchanged by this diff, present in the PR-time tree: vllm_omni/config/stage_config.py:663 "active_stream_window": int(raw_dict.get("active_stream_window", 0) or 0), — load_deploy_config kwargs have no multi_step_decode_steps; vllm_omni/config/omni_config.py:259 active_stream_window: int = Field(default=0, ge=0) — OmniStageModelConfig still omits the new field; vllm_omni/config/omni_config.py:57-58 _ModelEngineOverrides only lists active_stream_window: int; vllm_omni/config/omni_config.py:729 _MODEL_ENGINE_FIELDS = frozenset(_ModelEngineOverrides.__annotations__) — structured factory copies only that whitelist.

[Green-Vial]

Fixed in 645a70c. Declared the field on both _ModelEngineOverrides (the typed engine projection) and OmniStageModelConfig (built from it), so a per-stage engine_extras.multi_step_decode_steps: 12 survives load_deploy_config -> merge_pipeline_deploy -> _stage_engine_values -> _build_model_config. It is intentionally not a pipeline-wide field: the thinker must not inherit the talker's K. Added a factory test pinning stage-1 only, and verified the test fails with KeyError when the projection declaration is removed.

[vllm-omni-review-bot]

[P2] shrink_refused_multi_step_window is a new public fail-closed helper (multi_step…

Evidence and suggested fix

shrink_refused_multi_step_window is a new public fail-closed helper (multi_step_decode.py:223) and NPUARModelRunner.execute_model always calls it after validate_multi_step_plan returns None (npu_ar_model_runner.py:512). tests/worker/test_multi_step_decode_runner.py only imports validate_multi_step_plan; repo-wide there are no tests/ hits. Pin a CPU unit test: plan {r0:8, r1:8} with num_scheduled_tokens 8 / total 16 becomes scheduled 1 and total_num_scheduled_tokens drops by the reclaimed count; a missing/empty plan is a no-op.

Evidence: Trigger: validate_multi_step_plan returns None while scheduler_output still carries a K-token plan; execute_model then always shrinks before the single-step fallback. Unmet requirement: this new public helper has no test. vllm_omni/platforms/npu/worker/multi_step_decode.py:223 def shrink_refused_multi_step_window(scheduler_output: SchedulerOutput) -> None: — public shrink. multi_step_decode.py:240-250 plan = getattr(scheduler_output, "multi_step_plan", None) / if not plan: return / scheduler_output.num_scheduled_tokens[req_id] = 1 / scheduler_output.total_num_scheduled_tokens -= reclaimed — empty plan is a no-op; otherwise K→1 and total is reduced by the reclaimed count. vllm_omni/platforms/npu/worker/npu_ar_model_runner.py:494 multi_step_plan = validate_multi_step_plan(self, scheduler_output) then :512 shrink_refused_multi_step_window(scheduler_output) — called on every None return so a refused K-token schedule cannot run as a plain multi-token decode. tests/worker/test_multi_step_decode_runner.py:18 from vllm_omni.platforms.npu.worker.multi_step_decode import validate_multi_step_plan — runner CPU tests import only the validator; repo-wide grep of shrink_refused_multi_step_window has no tests/ hits. Adverse effect if shrink is wrong/untested: fallback execute_model would still see num_scheduled_tokens=K and write KV on the unproduced K-1 slots (stale embeddings + cache holes).

[Green-Vial]

Fixed in 981388f. Covered shrink_refused_multi_step_window: full shrink ({r0:8, r1:8} -> 1/1, total 16 -> 2), unplanned requests left untouched, and the empty/missing-plan no-op.

[vllm-omni-review-bot]

[P2] plan_multi_step_window:363-367 skips num_output_placeholders inflation when asy…

Evidence and suggested fix

plan_multi_step_window:363-367 skips num_output_placeholders inflation when async_scheduling is false; reconcile_window_shortfall then rolls back only num_computed_tokens. That is the live MiniCPM-o path (deploy YAMLs set async_scheduling: false; OmniARScheduler inherits the plan hook). tests/core/sched/test_multi_step_decode.py pins async_scheduling=True in both fixtures and every plan/reconcile assertion is the async placeholder-fence path; the only sync coverage is runner-side test_validate_accepts_sync_scheduling. Module docstring L2 and scheduler_allows_multi_step docstring L151 still say async-only even though the gate does not check async_scheduling. Add a CPU planner test with async_scheduling=False that asserts placeholders stay uninflated and shortfall reconcile rolls back computed tokens only, and update those two docstrings.

Evidence: Trigger: enabling multi_step_decode_steps on MiniCPM-o with existing sync deploys (unchanged by this diff: vllm_omni/deploy/minicpmo_4_5_duplex.yaml async_scheduling: false) selects OmniARScheduler, which calls plan_multi_step_window. Adverse/unmet: that commit skips the placeholder fence and has no planner unit test, while public docs still claim async-only. vllm_omni/core/sched/multi_step_decode.py:2 """Multi-step decode window planning for AR stages on async scheduling.; L151 AR stages on async scheduling are eligible.; L363-367 async_mode = bool(getattr(scheduler.scheduler_config, "async_scheduling", True)) then if async_mode: request.num_output_placeholders += extra (sync leaves placeholders untouched, then always request.num_computed_tokens += extra). tests/core/sched/test_multi_step_decode.py:126 and :164 both scheduler_config=SimpleNamespace(async_scheduling=True), — no async_scheduling=False planner case. Runner-only sync check is tests/worker/test_multi_step_decode_runner.py:138 def test_validate_accepts_sync_scheduling():.

[Green-Vial]

Fixed in 981388f. Added a planner test with async_scheduling=False asserting placeholders stay uninflated and that the shortfall reconcile rolls num_computed_tokens back directly. The "async only" wording is gone from scheduler_allows_multi_step, the module header, and the runner's fail-closed notes; the header's fence description now matches the implementation (explicit per-request hold-back, since the placeholder counters alone do not fence).

[vllm-omni-review-bot]

Omni ReviewBot: finding feedback

[p1] tests/core/sched/test_multi_step_decode.py adds CPU planner/reconcile coverage… — tests/core/sched/test_multi_step_decode.py:21

See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement.

[vllm-omni-review-bot]

Omni ReviewBot: finding feedback

[p1] Accepted windows return from execute_model at npu_ar_model_runner.py:506 before… — vllm_omni/platforms/npu/worker/npu_ar_model_runner.py

See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement.

[vllm-omni-review-bot]

Omni ReviewBot: finding feedback

[p0] New tests/worker/test_multi_step_decode_runner.py:17 collection-imports vllm_om… — tests/worker/test_multi_step_decode_runner.py:17

See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement.

[vllm-omni-review-bot]

Omni ReviewBot: finding feedback

[p1] This PR adds multi_step_decode_steps: int = 0 on OmniModelConfig (model.py:126)… — vllm_omni/engine/arg_utils.py:172

See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement.

[vllm-omni-review-bot]

Omni ReviewBot: finding feedback

[p1] validate_multi_step_plan is invoked unconditionally from NPUARModelRunner.execu… — vllm_omni/platforms/npu/worker/multi_step_decode.py:106

See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement.

[amy-why-3459]

Reviewed commit d03aed315bdb3491aca88b1e2289bcd2f6ed934f.

Found two actionable correctness bugs in multi-step decode: incorrect rotary positions for concurrent requests and unsafe async fallback accounting.

[P1] Build rotary positions in the order consumed by the window

vllm_omni/platforms/npu/worker/multi_step_decode.py:333

The prebuild flattens positions request-first, but the execution loop consumes contiguous blocks of num_reqs rows as step-first data. With two requests at positions 100 and 200 and K=2, this builds [100, 101, 200, 201], while the loop requires [100, 200, 101, 201]. Consequently, whenever rotary prebuild succeeds with multiple requests, even step 0 applies the wrong rotation to the second request, corrupting attention and generated audio. Reverse the comprehension nesting to match the per-step slices.

[P1] Fence async windows before allowing shortfall rollback

vllm_omni/core/sched/multi_step_decode.py:366

Increasing placeholders and computed tokens equally does not fence subsequent scheduling: num_tokens_with_spec + placeholders - computed remains 1, as the added test_plan_window_preserves_confirmed_count_invariant explicitly asserts. Async scheduling can therefore queue another window using the first window's full reserved length. If the runner refuses the first window—for example because debugger is enabled, which the scheduler gate does not reject—shrink_refused_multi_step_window executes only one token, but the queued schedule still starts K positions later. Reconciliation changes the scheduler request counters without repairing that queued schedule, leaving unwritten KV positions in the attention context and corrupting subsequent output. Prevent scheduling on outstanding windows or repair/invalidate dependent queued schedules before permitting fallback or early-exit rollback.

Validation: Statically inspected the exact base-to-head diff, all changed files, scheduler and runner callers, model sampling/output handling, and both added test files. Cross-checked relevant locally available dependency source. The added runner tests exercise validation, not window execution. No repository code, runtime tests, or GPU/NPU tests were run.

[linyueqian]

The async window accounting and finished-request cleanup need attention before this can land. The inline comments describe the affected paths; I also left a suggestion regarding the rotary prebuild layout.

Static review at d03aed3 against minicpm-challenge, including the runner, scheduler, model lifecycle, and added tests. I did not run tests, NPU benchmarks, or e2e, and have not independently verified the reported performance or audio equivalence.

[linyueqian]

[important] We should fence dependent async schedules until the window is reconciled, or ensure already-issued schedules are correctable. Adding K-1 to both placeholders and computed tokens leaves num_tokens + placeholders - computed == 1, allowing another decode step to be queued before this window returns (which the new invariant test explicitly expects). If the runner refuses the window, such as when prefix caching is enabled, it writes only one token while the queued schedule still starts K positions ahead. reconcile_window_shortfall repairs the live counters, not the already-queued positions. Even without a refusal, _request_window_budget does not account for in-flight windows: with 116 confirmed tokens, 100 output tokens, max_tokens=112, max_model_len=128, and K=8, two queued windows cover positions 115..122 and 123..130. The latter exceeds the context limit despite the base scheduler clamp. Please add test coverage for both a refused window followed by a queued step and stacked windows near the context limit.

[Green-Vial]

Fixed in 221cd42. Planned-but-unreconciled windows are now tracked per request (req_id -> K on the scheduler), a second window cannot stack on top of one, and OmniARScheduler.schedule() holds such requests out of the pass until update_from_output reconciles them. That closes the scenario: no follow-up schedule is ever emitted against a window's optimistic positions, so a runner refusal (one token written) is fully recoverable by reconcile_window_shortfall + shrink_refused_multi_step_window.

Your reading of the accounting is exactly right, and the test you cite did encode that premise: adding K-1 to both num_output_placeholders and num_computed_tokens leaves num_tokens + placeholders - computed == 1, so the counters alone never fenced — the fence is now explicit rather than implied.

The two concrete triggers you named also cannot fire any more: enable_prefix_caching is now part of the scheduler-side static gate (9a5b83e), so the planner no longer emits a window the runner refuses with a shrink; and the stacked-window case you described is unreachable with at most one window in flight, which also makes _request_window_budget (max_model_len - num_tokens) exact instead of optimistic.

Tests added: the planner refuses a window while one is outstanding and accepts it again after reconcile; the deferral filters exactly the fenced requests out of the running queue while plain decodes still run; the sync accounting path inflates only num_computed_tokens and rolls it back on shortfall; and a fully deferred pass needs no new guard because the runner's existing num_scheduled_tokens <= 0 path already returns an empty output before _prepare_inputs (#5196) while still running _update_states and on_requests_finished.

Scope note: a genuine exception thrown while the window executes still propagates rather than being swallowed, so the refusal set the fence must absorb is the static-validation one — which the scheduler now mirrors.

[linyueqian]

[important] Please preserve model.on_requests_finished on the window path. A schedule can retire finished request A while opening a window for still-running request B. Because this branch returns early before lines 522-523, and execute_multi_step_window only calls _update_states (which cleans up runner state without notifying the model), the MiniCPM talker never queues request A for _flush_deferred_cleanup. Its per-request generators and audio states therefore remain retained after completion. Moving the callback into shared handling or invoking it right after the window state update would address this. Please also cover a finished request retiring alongside an active window.

[Green-Vial]

Fixed in 88f223c. Extracted notify_finished_requests() and call it right after _update_states inside the window executor — the same point the single-step path uses — so a request that retires while its siblings keep windowing is queued for _flush_deferred_cleanup instead of retaining its _request_audio_states / generator (and the omni wrapper drops its duplex session). Covered by tests for the forwarding case plus the empty-ids and missing-hook no-ops.

[linyueqian]

[suggestion] Consider aligning this layout with the replay loop and adding a two-request test with unequal positions. The list comprehension is request-major, but lines 587-594 consume step-major slices: positions 100 and 200 with K=2 produce [100, 101, 200, 201], so the first slice is [100, 101] rather than [100, 200]. Reversing the loop nesting would match the consumer order. I have not established that the active MiniCPM path consumes this cache, so I am not treating this as evidence of audio corruption.

[Green-Vial]

Fixed in 2a61fef. You were both right, and thank you for describing it as an ordering question rather than a claim — I could not leave it as-is.

The prebuild flattened positions request-major while the replay loop consumes them step-major (each step copies j*num_reqs..(j+1)*num_reqs into the rows the captured graph reads), so two requests at 100/200 with K=2 built [100,101,200,201] where the loop needs [100,200,101,201] — request 1 would get request 0's rotation from step 0 on. Reversed the nesting and extracted the flatten into window_cos_sin_positions(); the added tests pin the batched order, the per-step slices, and the pre-fix order as a regression guard.

On whether the cache is actually consumed on this model: it is not, today. I added a diagnostic counter (cos_prebuilt_windows in the existing "Multi-step counters" line) and a windowed c4 run reports 0 — _prebuild_window_cos_sin early-returns because the Ascend cos/sin cache is absent, which is also why update_cos_sin is effectively inert here. So this was a latent ordering bug rather than a source of corrupted audio, and no multi-concurrency re-measurement is needed on its account.