Repository navigation
[Performance][MiniCPM-o] Zero-copy block-aligned KV sliding window with in-place Re-RoPE for duplex Stage-0 - #7821
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 @BeatSeat, 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. |
Omni ReviewBot: three questions on the performance claim@BeatSeat this PR reads as a performance or value claim:
Before the full evidence checklist, three short questions:
When you answer, the evidence that settles it is: base and head SHA, hardware, model, workload, warm-up and repeat count, mean or percentiles with their spread, and a correctness/quality-equivalence signal; an end-to-end claim also needs stage attribution. |
|
@0z5a @amy-why-3459 PTAL, I think this implementation is better align with the official implementation and have better performance. |
Yes, I agree that avoiding the full re-prefill and reusing the retained KV with Re-RoPE is a better direction than the rebuild path in #7631, and it also looks closer to the official sliding/reindex behavior. My main concern is correctness. Could we have an A/B correctness regression that compares Re-RoPE against re-prefill after both a single trim and multiple consecutive trims? In particular, I would like to check: retained K after Re-RoPE vs. K from a clean re-prefill; If these stay within a tight numerical tolerance across repeated trims, then I think this is a substantially better execution strategy than the re-prefill approach. ^_^ |
Yes, but current implementation aligns to the official implementation of minicpm-o4.5 (in their official repo). I think re-prefill would work differently, but I think I would do the test. |
Yes, but the current implementation aligns with the official implementation of minicpm-o4.5 (in their official repo). I think re-prefill would work differently, but I think I would do the test. |
amy-why-3459
left a comment
There was a problem hiding this comment.
Requesting changes for the four correctness issues attached inline. Validation included source inspection and minimal reproductions using extracted production methods, including a CUDA slot-mapping check. Full checkpoint-backed end-to-end generation was not run.
Architecture: please move the new model-specific window logic out of the shared scheduler and AR model runner into duplex helpers/controllers, retaining only thin lifecycle hooks in the shared classes.
- Move MiniCPM-specific mode parsing, watermarks, prefix handling, and plan generation into
model_executor/models/minicpmo_4_5/duplex/. - Put scheduler-side window application behind a duplex helper with an explicit interface to scheduler-owned allocation and request state. The shared scheduler should not need to interpret MiniCPM-specific
basic/contextmodes or constants such asmax_units * 12. - Move reanchor instruction handling, position changes, and KV rotation dispatch into a duplex worker helper. Keep model/backend-specific RoPE and cache-layout handling in the model's duplex implementation, with explicit supported-layout checks.
- Keep the necessary session-update and pre-forward hooks in the scheduler/runner. Allocation and GPU mutation must still execute in their owning processes; a serving plugin alone cannot perform them. The existing
DuplexSamplingHelper/mixin is a useful precedent for keeping duplex logic outside the generic runner.
Please also replace copied mock implementations in the wiring tests with calls through the production update path. Cover append -> scheduler compaction -> worker state update -> subsequent scheduling, and assert that both sides agree on computed counts and block IDs. Include a non-duplex regression and a test that verifies a window trim actually occurred.
| # Post-trim table and post-trim position resolve to the pre-trim slot: the | ||
| # deletion shifted the index by exactly as many pages as the position moved, | ||
| # so this is the row the token was written into, not a copy of it. | ||
| slots = compute_slot_mapping(block_ids, positions - plan.delta, block_size) |
There was a problem hiding this comment.
[P1] Build the slot-mapping block table on the positions device
The runner creates positions on self.device, but compute_slot_mapping() constructs table = torch.as_tensor(block_ids, dtype=torch.long) on CPU. Its subsequent table[block_index] therefore indexes a CPU tensor with CUDA indices. A minimal CUDA reproduction using the production helper raises: indices should be either on cpu or on the same device as the indexed tensor (cpu). The first non-empty reanchor on CUDA will fail here.
Please keep the block table, positions, and resulting slots on compatible devices, and add a test that exercises rotate_cached_keys with an actual CUDA cache. The current CPU-only numerical tests do not cover this failure.
| compacted_block_ids = list(bt.block_table.np[req_idx, : bt.num_blocks_per_row[req_idx]]) | ||
|
|
||
| old_computed = int(self.input_batch.num_computed_tokens_cpu[req_idx]) | ||
| self.input_batch.num_computed_tokens_cpu[req_idx] = max(0, old_computed - plan.delta) |
There was a problem hiding this comment.
[P1] Do not compact and decrement state already updated from the scheduler
This hook runs after super()._update_states(scheduler_output). For a streaming update, the parent installs the scheduler-provided block IDs and computed-token count through _update_streaming_request() and re-adds the request to the input batch. Those values already reflect scheduler-side compaction, so deleting the gap again above and subtracting plan.delta here applies the same logical trim twice.
A minimal reproduction with the production hook starts from the post-scheduler table [0, 2, 3] and computed count 48 for a 16-token trim; the hook changes them to [0, 3] and 32. This also makes old_computed the wrong endpoint for the retained-key rotation.
Please make the scheduler authoritative for the final logical state and have the worker apply the corresponding KV transformation once. Carry the pre-trim computed length explicitly in the reanchor instruction. The regression test should exercise the real parent state-update path; the copied _MockRunner implementation currently starts from pre-trim state and misses this issue.
| if duplex_mgr is not None: | ||
| duplex_mgr.reanchor_block_table(session.request_id, plan) | ||
|
|
||
| session.num_computed_tokens -= plan.delta |
There was a problem hiding this comment.
[P1] Compact the session token sequences together with the KV block table
Only num_computed_tokens is reduced here; prompt_token_ids, _all_token_ids, and the prompt count still describe the untrimmed sequence. The subsequent upstream _update_request_as_session() truncates _all_token_ids at the new computed count and appends to the existing prompt. That removes a suffix rather than the intended middle gap and leaves the two token histories inconsistent.
In a minimal reproduction with a 64-token prompt, a 16-token trim, and a one-token append, _all_token_ids ends up with 49 tokens while prompt_token_ids has 65. Subsequent scheduling therefore uses inconsistent token ranges, and the prompt length is not actually bounded by the window.
Please apply the same prefix-plus-retained-tail compaction to the session token sequences, counts, and associated position metadata as one consistent state transition before the normal append. Also verify compaction actually succeeded before changing the count or emitting a rotation instruction.
| ) | ||
|
|
||
| geometry = duplex_window_geometry( | ||
| prefix_tokens=96, |
There was a problem hiding this comment.
[P1] Align the cache spec's sink geometry with the per-session prefix
The installed cache spec fixes prefix_tokens=96, hence sink_chunks=6 for 16-token blocks. However, the plugin computes duplex_window_prefix_tokens from the actual instructions and reference audio, and the scheduler uses that per-session value. MiniCPMO45DuplexWindowManager.reanchor_block_table() requires plan.sink_blocks == spec.sink_chunks.
For example, an actual prefix of 112 tokens produces seven sink blocks and raises at the first trim against the six-block spec. Custom instructions or reference audio can therefore make an accepted session fail only when the window rolls over.
Please use a consistent geometry contract and support a request-local sink boundary, or explicitly validate a fixed-prefix restriction at session creation. Add coverage for differing instruction/reference-audio prefix lengths.
| ) | ||
| ) | ||
|
|
||
| req_state = runner.requests.get(req_id) if hasattr(runner, "requests") else None |
There was a problem hiding this comment.
req_state.block_ids contains one block-ID list per KV cache group. Calling list(req_state.block_ids) preserves that nesting, but rotate_cached_keys() passes it to compute_slot_mapping() as a flat block table. With a single group such as ([0, 2, 3],), indexing logical block 2 indexes a group dimension of size 1, causing the first non-empty reanchor to fail.
Please select the block table belonging to each layer's KV cache group before computing slots. The current wiring test supplies a flat block_ids=[0, 2, 3], so it misses the production structure; add coverage using the actual grouped representation.
| if duplex_mgr is None: | ||
| return None | ||
|
|
||
| freed_tokens = duplex_mgr.reanchor_block_table(session.request_id, plan) |
There was a problem hiding this comment.
The sink mismatch remains at this head, with a different failure mode. The installed spec fixes sink_chunks=6, while a request with a 112-token prefix produces plan.sink_blocks=7. reanchor_block_table() frees the gap starting at block 7, but the inherited compact_block_table() searches for the gap starting at the spec's block 6. Because block 6 is still live, compaction returns 0.
The caller then returns without updating token counts or issuing Re-RoPE, even though blocks have already been freed and replaced with null entries. A minimal reproduction using the production reanchor and compaction methods confirms this state.
Please make both operations use the same request-local sink boundary. The dynamic-prefix test currently replaces compact_block_table() with a lambda returning 16, which masks this failure; exercise the inherited implementation instead.
| high_watermark = int(window.get("basic_window_high_tokens", 8000) or 8000) | ||
| low_watermark = int(window.get("basic_window_low_tokens", 6000) or 6000) | ||
| else: | ||
| max_units = int(window.get("context_max_units", 24) or 24) |
There was a problem hiding this comment.
context_previous_max_tokens is accepted, validated, and included in the runtime configuration, but the context-mode planner never reads it and instead adds a hard-coded 500. Consequently, setting this option to 16, as the new E2E helper does, has no effect on window planning.
Please implement the configured previous-context budget, or reject the option explicitly until supported. Add a regression test showing that changing this value changes the resulting retention or trim decision.
| self._maybe_apply_stage0_reanchor() | ||
| return deferred_state_corrections_fn | ||
|
|
||
| def _maybe_apply_stage0_reanchor(self) -> None: |
There was a problem hiding this comment.
It is not suggested to take a model-specific implementation here
| sink_end = plan.sink_end | ||
| moved_from = plan.moved_from | ||
|
|
||
| # Compact session token sequences and counts consistently |
There was a problem hiding this comment.
Keeping MiniCPM-specific window policy in this directory makes sense: basic/context semantics, audio-unit accounting, prompt-prefix construction, and supported position transformations belong to the model. However, this helper also directly modifies scheduler-owned token histories and counters, while the worker helper interprets framework-owned KV groups and cache layouts.
Please separate the model's retention decision from the framework operation that applies it:
- Keep model-specific thresholds, prefix calculation, and RoPE eligibility/parameters in the MiniCPM implementation.
- Put block freeing and compaction behind a KV-manager operation with an explicit request-local sink boundary.
- Let the scheduler own the corresponding token-history and counter updates, deriving all changes and the worker instruction from the same applied compaction result.
- Let the worker/backend layer resolve the correct KV group and supported Key-cache layout, then invoke the model's position transformation before the next forward.
This boundary matters for correctness: the dynamic-sink mismatch currently allows blocks to be freed without completing compaction, and the worker assumes a flat block table where production state is grouped. Moving these responsibilities behind explicit framework interfaces would allow their invariants to be enforced and tested together.
The existing paging primitives under experimental/ar_diffusion/kv_cache/paged.py are a potential shared foundation. Please avoid simply moving the entire helper into a common directory: its policy constants, RoPE assumptions, and cache-layout inference need to be separated first.
A small, explicit compaction interface is sufficient for this PR; a general window-policy framework can follow when another consumer needs it. Please cover the real scheduler → worker update path, including differing prefix lengths, grouped block tables, and rejection before any state mutation when a plan is unsupported.
3436679 to
ffe4971
Compare
linyueqian
left a comment
There was a problem hiding this comment.
Tested on vLLM 0.30.0 with the real openbmb/MiniCPM-o-4_5 model. The basic window E2E passes on the main control and crashes with this PR at the first nonempty reanchor. Details and proposed changes are inline.
Setup: one H20-3e (143771 MiB), Python 3.12.13, torch 2.13.0+cu132, FlashAttention 3; model snapshot 503e754207c94da6bb26850b4469f367c9ea3582. Both sides ran sequentially on the same physical GPU, in the same fresh isolated environment, using the stock three-stage single-GPU config and cached BF16 weights.
- Control: main
59db6a4285e9086391efc85a6eff6030a6294878(includes the 0.30 upgrade and #7992). - PR side: the same main plus
ffe49710b59beb6d9dfa65e34f994ac650444b8b, applied cleanly withgit cherry-pick --no-commit. No corrective source patch was applied.
| Check | Result |
|---|---|
| Main: complete window E2E suite | 9 passed (868.11 s) |
| PR-on-main: basic session-reopen smoke | 1 failed, 8 deselected (478.80 s) |
| PR-on-main: separate diagnostic rerun | Same failure in the first test; stopped with -x (231.47 s) |
| PR's three new unit-test files | 38 passed (3.79 s) |
| CUDA tensor probe using observed cache strides, FP32/BF16 | Proper transpose+split writes rotated K to backing storage and preserves V/sink in both dtypes |
The control suite covers session reopening, reference audio, more than two minutes of continuous audio/camera input, and buffered flushing. The tensor probe is not a corrected-model E2E pass; other PR E2E cases remain unverified because the engine dies in the first basic case.
Reproducer for the serving failure, from that PR-on-main checkout:
python -m pytest -s -v tests/e2e/online_serving/test_minicpmo_4_5_window.py \
-m 'advanced_model and cuda' --run-level advanced_model \
-k 'test_window_rebuild_and_next_session and basic' -xThe error is ValueError: expected 128 RoPE frequencies for head_dim=256, got 64 from rotate_cached_keys; the client then sees DuplexSessionClosedError: ... expired: request_cleanup. The installed backend packs K and V in the last dimension, which explains the backend-layout incompatibility on 0.30.
Suggested sequence: (1) fix backend cache views and storage-preserving writes, with a real CUDA regression; (2) land basic-mode reuse with exact whole-unit retention, consistent scheduler/worker state and bounded worker history; (3) implement context's previous+suffix partial forward while reusing retained KV, keeping an explicit fallback until then; (4) compare with the official cache algorithm, then publish reproducible latency/quality measurements.
The lifecycle E2Es do not establish official numerical parity or audio quality. Full re-prefill and retained-KV reuse are different multilayer computations; the latter is the direction needed to reproduce the official cache behavior. No speedup claim is validated by this run. The raw PR crashes before sustained post-trim behavior can be assessed. The earlier 0.29 setup was cancelled when switching to 0.30 and is not counted as a completed test.
| for layer_idx, kv_cache in enumerate(runner.kv_caches): | ||
| group_idx = kv_groups[layer_idx] if (kv_groups and layer_idx < len(kv_groups)) else 0 | ||
| layer_block_ids = cls.resolve_group_block_ids(runner, req_id, req_idx, group_idx=group_idx) | ||
| k_pool = kv_cache[0] if getattr(kv_cache, "dim", lambda: 0)() == 5 else kv_cache |
There was a problem hiding this comment.
[P1] Resolve the actual vLLM 0.30 key-cache layout before rotating
I tested main 59db6a4285 with this PR (ffe49710b5) applied, using vLLM 0.30.0 and the real MiniCPM-o-4_5 checkpoint on an H20-3e. test_window_rebuild_and_next_session[basic-False-three-stage-single-gpu] passes on main alone but the PR kills Stage 0 at the first nonempty reanchor:
window_kv.py:744 -> rotate_cached_keys
ValueError: expected 128 RoPE frequencies for head_dim=256, got 64
The vLLM 0.30 FlashAttention implementation reads its packed (B,H,N,2*D) cache using kv_cache.transpose(1, 2).split(self.head_size, dim=-1). The diagnostic rerun recorded raw shape [20778,8,16,256], strides [32768,256,2048,1], and a committed scheduler trim 66 -> 34 before the worker raised. This branch instead sends the entire packed K/V tensor into a helper expecting (B,N,H,D). Please resolve the key view through a backend/layout contract, validate it before committing the trim, and exercise the actual CUDA cache storage in a regression test. The existing 38 new unit tests all pass in this environment despite this serving failure.
A CUDA tensor probe using these observed strides confirms that transpose+split yields a writable key view and preserves V/sink in FP32 and BF16. This supports the adapter direction, but is not a corrected serving E2E pass. Assert storage aliasing or use stride-aware writes: a separate synthetic contiguous-BHNC case makes the current reshape allocate a copy, whereas the observed LBNHC case aliases correctly.
| else: | ||
| max_units = int(window.get("context_max_units", 24) or 24) | ||
| prev_max = int(window.get("context_previous_max_tokens", 500) or 500) | ||
| low_watermark = prefix_tokens + max_units * 12 |
There was a problem hiding this comment.
[P1] Preserve context-mode unit counts and previous-text semantics
max_units * 12 is not a unit-retention policy. Direct calls to this production policy with prefix=96, max_units=24 and previous_max_tokens=500 return no trim for both 25 and 60 completed 12-token units; a single synthetic 1100-token unit is instead cut at position 736, inside that unit. These are deterministic policy probes, separate from the model E2E result.
The new scheduler branch also bypasses _prepare_minicpmo45_stage0_window, so reading context_previous_max_tokens here only changes a token watermark: it no longer accumulates evicted speak text or inserts previous:. The official context implementation retains prefix KV, forwards only the new previous+suffix, then concatenates retained-unit KV after reindexing K.
A practical split would be to land basic-mode KV reuse first, while explicitly routing context through the existing implementation until exact unit accounting and the partial previous+suffix forward are implemented. That fallback is still re-prefill behavior, not official cache-reuse parity. Keep the mode's existing public meaning instead of silently replacing it with a token-budget approximation.
| # context_length_exceeded instead of moving the window start backwards. | ||
| return None | ||
| boundaries = unit_starts(geometry, unit_tokens) if unit_tokens else None | ||
| moved_from = align_up(projected - target, geometry.block_size) |
There was a problem hiding this comment.
[P2] Distinguish the amount to drop from the absolute retained-tail start
projected - target is a drop length, but this line treats it as an absolute position; line 215 then subtracts the sink again. Running the production planner with prefix=96, block_size=16, window=6000, high=8000, computed=8100 and pending=12 gives moved_from=2016 and delta=1920. The resulting length is 6192, exceeding this function's own target of 6096 by 96 tokens.
Please derive the absolute cut from the sink end plus the required drop, then check the resulting retained length. Separately, the production caller never supplies unit_tokens, and align_up(unit_start) below can move a supplied boundary into a unit. Exact whole-unit eviction and arbitrary page alignment need an explicit treatment (for example explicit repacking or a supported fallback); rounding a semantic boundary is not equivalent. Add non-page-aligned unit/prefix cases and assert both the retained-unit list and the post-trim length.
| k_A = torch.cat([k_init[:PREFIX_LEN], k_tail_A], dim=0) | ||
| v_A = torch.cat([v_init[:PREFIX_LEN], v_tail_A], dim=0) | ||
|
|
||
| # Method B: Clean Re-prefill Ground Truth |
There was a problem hiding this comment.
[P2] Use the official cache transformation as the correctness oracle
Here compute_kv projects token embeddings directly, so a retained token's K/V never depends on preceding tokens. This checks the RoPE identity, but cannot establish equivalence to clean re-prefill in a multilayer decoder: retained hidden states already encode the evicted context, and re-rotating K does not remove that information from K/V.
A seeded two-layer causal toy decoder reproduces this distinction: first-layer K agrees to 4.4e-15 and V exactly, while second-layer K/V and next-step logits differ. This is a mathematical counterexample, not a MiniCPM quality measurement.
Keep these algebra tests, but compare production cache surgery against the official slice/reindex algorithm using identical pre-trim KV, exact unit boundaries and previous text. Then test real-model next-token/output behavior across repeated trims, reference audio and camera input. Compare full re-prefill separately as a behavior/performance baseline, without treating it as a generally equal numerical oracle.
| ) | ||
|
|
||
| if MiniCPMO45DuplexSchedulerHelper.find_duplex_window_manager(self) is not None: | ||
| self._maybe_reanchor_streaming_window(session, update) |
There was a problem hiding this comment.
[P2] Carry worker-history eviction across the new reanchor path
This branch bypasses the existing window preparation for every append once the custom manager is installed. Stage 0 still appends each completed unit's embeddings/token IDs to state.window_units in _finalize_window_unit; its only removal of old units is in _window_replacement_parts when stage0_window.replace=True. The reanchor helper only emits stage0_reanchor, so that cleanup is no longer reached.
Please carry the same committed unit-eviction decision to worker history, or stop retaining embeddings that the KV-reuse path no longer needs. Include a long-session assertion that both paged KV and worker-held unit history remain bounded. This is a static lifecycle finding; the current CUDA crash prevents measuring a sustained post-trim memory curve.
aa0decc to
ffd7fef
Compare
|
Reviewed head 1. [P1] Update scheduler window history atomically with KV compaction
On subsequent ordinary appends, the fallback helper runs again using those stale coordinates. A direct-method CPU reproducer gives: The next append computes a negative Please update the completed-unit bookkeeping, prune the scheduler history, and rebase its coordinates as part of the same retention transaction. Add a sequence test covering reanchor → ordinary append → fallback rebuild using the actual scheduler and worker helpers. 2. [P1] Preserve the exact retained token range when pruning worker unit historyRelevant code, and the planner call The production planner call does not provide For example, with unit token ranges A later fallback cannot reconstruct the same retained sequence from this history and can hit the existing rebuild-length mismatch guard. Please either select cuts that are both page-aligned and whole-unit-aligned, falling back safely when no such cut exists, or slice the boundary unit's token IDs and embeddings consistently. The current bounded-history test uses an exact multiple of the unit length and does not cover this case. 3. [P2] Keep watermark semantics consistent with the fallback pathThe existing With Small audio appends can therefore repeatedly trigger the expensive re-prefill before reaching the new zero-copy threshold, undermining the intended turn-boundary improvement. Please normalize both high and low watermarks to the same total-length/content-length convention, and test the actual scheduler dispatch around the Validation and remaining coverageThe three added CPU test files all passed locally: 42 passed. Pre-commit, both wheel builds, DCO, and docs checks were green at review time. The additional reproducer executes the actual PR method bodies extracted via AST with lightweight session/manager objects. The scheduler-state example stubs a successful block-compaction return; it demonstrates the bookkeeping inconsistency, not an end-to-end GPU failure. The partial-unit example executes the actual eviction helper. I did not run real-weight inference or verify the latency claims. One additional coverage concern: |
ffd7fef to
e4cc71c
Compare
…th in-place Re-RoPE for duplex Stage-0 - Attention cache layout: Support vLLM 0.30 FlashAttention packed layout (num_blocks, num_kv_heads, block_size, 2 * head_dim) and ensure in-place tensor storage mutation without copy allocation - Sliding window planning: Fix drop length calculation in plan_position_reanchor to correctly cut from sink_end + drop_aligned, preventing excess retention - Mode routing: Restrict zero-copy re-anchor to basic mode and explicitly route context mode through official fallback (_prepare_minicpmo45_stage0_window) to preserve previous-text semantics - Worker history: Evict completed window units in state.window_units upon stage0_reanchor to keep worker memory strictly bounded across long streaming sessions; preserve exact retained token ranges when delta lands inside a unit - History synchronization: Update scheduler window units and coordinates atomically with KV compaction and propagate completed units to worker - Watermark semantics: Normalize basic_window_high_tokens and low_tokens total sequence lengths to content-only budgets for DuplexWindowGeometry - Tests: Add regressions for vLLM 0.30 packed FlashAttention cache layout, storage aliasing, worker history lifecycle eviction, exact unit slicing, and normalized watermarks Signed-off-by: BeatSeat <wendavid552@gmail.com>
…tall Signed-off-by: BeatSeat <wendavid552@gmail.com>
4ec91aa to
59d246e
Compare
|
I have resolved the issue, please check again @amy-why-3459 @Gaohan123 @linyueqian |
|
Reviewed commit
Validation on one NVIDIA L20X (all three MiniCPM-o-4_5 stages on GPU 5), vLLM 0.30.0 / Torch 2.13.0+cu130: duplex AV streaming at 1 frame/s with realtime pacing, basic window high/low=8000/6000, block_size=16, Stage-0 max_model_len=16384, prefix caching off. The source snapshot only has logging instrumentation; the client explicitly enables the basic window. No behavior fix or compatibility patch was applied.
The related CPU test files passed 46 tests. Both targeted reproducers reproduced the issues above. All three server health checks after the video cases succeeded; no request failure, OOM or CUDA error occurred during these cases. Peak device memory in the longest case was 56,374 MiB. Audio-encoder cache resets and HiFT graph-limit eager fallbacks were observed; these are distinct from Stage-0 window fallback. The successful video runs do not establish cache correctness: each observed reanchor was executed twice. They also did not exercise the Stage-0 fallback rebuild path; the history issue was established with a targeted CPU reproducer. This was a single-session stability test, not a quality evaluation, concurrent-load test, infinite-duration guarantee or A/B performance benchmark. |
|
@BeatSeat Brilliant idea and great performance improvement. Cheers for the final solution of correctness and looking forward to more insightful collaboration. "Happy writing kernels!" |
…nsor slicing - Enforce exactly-once worker reanchor execution across runner metadata refreshes by assigning unique reanchor_id, guarding execution via _applied_stage0_reanchor_ids, and sanitizing scheduled_new_reqs in scheduler_output. - Designate maybe_apply_reanchor as the single owner of worker history eviction, removing duplicate eviction call from _stage_prefill_embeddings_only. - Fix history slicing with non-aligned prefix by properly retaining [0, sink_end - prefix_tokens) tokens in the sink and dropping the exact relative interval [drop_start, drop_end). - Implement multi-row tensor row slicing in _evict_window_units_for_reanchor to preserve exact 1-to-1 correspondence between embedding rows and token IDs for fallback prompt rebuild. - Add unit tests verifying exactly-once reanchor execution, metadata refresh resilience, and multi-row tensor row slicing with non-aligned prefix. Signed-off-by: BeatSeat <wendavid552@gmail.com>
|
Follow-up review: comparison with the OpenBMB reference implementation Revision scope: The analysis and experiments below used PR commit The reference used here is OpenBMB MiniCPM-o-4_5, revision 503e754207c94da6bb26850b4469f367c9ea3582. 1. Page alignment changes retention semantics relative to the referenceThe reference basic window removes whole units immediately after the exact preserved prefix. The PR planner rounds the prefix up to a page boundary. Requiring the tail cut to be both a unit boundary and a page boundary does not prevent retaining a fragment of the oldest unit inside that rounded sink. A small reproduction using the PR's actual
Thus, 12 tokens from the oldest unit remain in the sink. The same retained token originally at position 128 moves to position 100 in the reference, but to 112 in the PR. This is a context/position-policy difference, not floating-point noise. The new commit does not change Please either preserve the reference token set through an appropriate fallback/boundary-handling path, or explicitly document this as a different retention policy and evaluate its quality. Add a test that compares retained token identities and resulting positions, not only total lengths. 2. The window trigger occurs at a different pointThe reference duplex generation path feeds the unit-end token, registers the completed unit, and then enforces the window against the actual cache length. The PR plans before an append using Consequently, the unit crossing a watermark may see different history. Matching high/low watermark numbers alone does not establish behavioral equivalence. Please cover threshold crossings with generated output and align the trigger timing when testing parity. 3. Separate reference parity from the old vLLM-Omni rebuild baselineThe OpenBMB basic implementation already retains existing KV and repositions the surviving keys. It does not fully re-prefill the retained context after every eviction. The PR's paged block-table approach is a useful engineering adaptation, but a speedup over the old vLLM-Omni rebuild path is not a speedup over the official implementation. Our earlier single-run, approximately ten-minute A/B measured a reduction in the median two-step window-operation interval from 200.49 ms to 90.47 ms (54.9%) against that old vLLM-Omni path. This was not a pure rotation-kernel measurement, and the tested head contained the subsequently addressed replay issue. We have not benchmarked the official implementation end to end. Also, the inspected 4. RoPE algebraic equivalence is narrower than model-output equivalenceFor identical retained keys, positions, and frequencies, applying one rotation by Calling the original functions on the same synthetic keys (seed 0, shape
These are synthetic key-tensor comparisons, not next-token logit errors, quality scores, or evidence that either numerical path is more accurate. They do not establish parity with a full re-prefill: cached higher-layer representations were computed with the original history, whereas a re-prefill recomputes those representations after eviction. 5. Context mode remains outside the new fast pathThe reference context mode can forward the updated previous-text/suffix region while retaining and reindexing the tail KV. The PR's new fast path explicitly supports only basic mode and routes context mode through the existing rebuild path. This is a remaining scope/performance difference, rather than evidence of a new context-mode regression. Audio findings and suggested validationIndependent Whisper large-v3 transcription of the earlier A/B outputs, with Whisper small cross-checks of suspicious responses, found more questionable fragments in the tested head output, including repeated/incoherent words and a final Chinese self-introduction. These observations do not establish that this PR caused an audio-quality regression: only one A/B run was available, ASR can misrecognize speech, and the new fix has not been retested. The earlier head run did not exercise the fallback rebuild path, so the history-rebuild bug should not be presented as the demonstrated cause of those audio artifacts. Recommended validation order:
|
…s vs unpaged reference - Document the architectural retention difference between paged zero-copy KV and the unpaged OpenBMB reference implementation in window_plan.py. - Add unit test test_paged_retention_policy_vs_unpaged_reference explicitly validating retained token spans, positions, and boundary mapping under non-page-aligned prefix configurations. Signed-off-by: BeatSeat <wendavid552@gmail.com>
|
Hi @amy-why-3459, thank you so much for the thorough live GPU validation (up to 614s video streaming!), the sharp diagnosis of the reanchor execution lifecycle, and the thoughtful architectural comparison with OpenBMB's unpaged reference implementation! Also special thanks to @0z5a for the kind words and encouragement. We have pushed two commits ( 1. Exactly-Once Worker Reanchor Execution & Single-Owner History Eviction (
|
|
We investigated the CI failure in
All 50 unit tests pass locally, all GitHub Actions workflows ( We have pushed commit |
…ex window setup In unit tests or minimal environments (such as test_stage0_runtime_load), vllm_config is instantiated as a minimal mock (e.g., SimpleNamespace) without cache_config or max_model_len on model_config. Guard attribute access across model initialization and worker reanchor helpers: - Fall back to DUPLEX_WINDOW_BLOCK_SIZE (16) if cache_config is missing or block_size is None. - Fall back to 8192 if model_config or max_model_len is omitted. - Defensively access runner.cache_config in maybe_apply_reanchor when computing sink_tokens. - Add test_duplex_window_install_tolerates_missing_cache_or_model_config in test_window_wiring.py. Signed-off-by: BeatSeat <wendavid552@gmail.com>
902dd48 to
396e569
Compare
Rename tests/model_executor/models/minicpmo_4_5/duplex/test_rerope_ab_correctness.py to test_rerope.py and update module docstring to align with test_window_plan.py and test_window_wiring.py. Signed-off-by: BeatSeat <wendavid552@gmail.com>
Summary of Changes
Background & Prior Work
This PR supersedes the initial design in #7631, which rebuilt prompt embeddings and triggered a full ~6,000-token re-prefill (~384ms stall) upon trimming, causing severe downstream audio underruns in continuous full-duplex conversations.
Deep Dive: Implementation Details
The core challenge of sliding-window attention in continuous multi-modal streaming is twofold:
Our solution splits this cleanly across the Scheduler (logical & physical block management) and the Worker (in-place Key tensor rotation).
1. Zero-Copy Block-Aligned KV Compaction (Scheduler Side)
In PagedAttention, requests map sequence tokens to non-contiguous physical memory blocks via a
block_table: List[int]. We partition the Stage-0 context into three distinct zones:block_size = 16). Must never be evicted.block_size.The physical GPU memory pages allocated to retained blocks (
The evicted block IDs (
block_allocator.free(), instantly freeing VRAM on GPU.session.num_computed_tokensandsession.num_prompt_tokensare decremented by exactly2. In-Place Re-RoPE Transformation (Worker / Kernel Side)
When block table compaction removes$\Delta$ tokens from the middle, the retained tokens at original logical positions $m \in [P + \Delta, L)$ are remapped to new logical positions:
$$m' = m - \Delta$$
Standard rotary position embedding encodes position$m$ into key vector $\mathbf{K}m$ via 2D block-diagonal rotation:$m\theta_d$ :
$$\mathbf{K}m = \mathbf{R}{\Theta, m} \mathbf{K}{\text{raw}}$$
where each coordinate pair rotates by angle
$$\mathbf{R}_{\theta_d, m} = \begin{pmatrix} \cos(m \theta_d) & -\sin(m \theta_d) \ \sin(m \theta_d) & \cos(m \theta_d) \end{pmatrix}$$
Algebraic Derivation:
Because rotation matrices form an abelian group under addition ($\mathbf{R}{\Theta, a} \mathbf{R}{\Theta, b} = \mathbf{R}{\Theta, a + b}$):
$$\mathbf{R}{\Theta, m'} = \mathbf{R}{\Theta, m - \Delta} = \mathbf{R}{\Theta, -\Delta} \mathbf{R}_{\Theta, m}$$
Multiplying both sides by $\mathbf{K}{\text{raw}}$ yields:
$$\mathbf{K}{m'} = \mathbf{R}_{\Theta, -\Delta} \mathbf{K}_m$$
In equivalent complex form ($z_m = k_{\text{even}} + i \cdot k_{\text{odd}}$ ):
$$z_{m'} = z_m \cdot e^{-i \Delta \theta_d} = z_m \cdot \Big(\cos(\Delta \theta_d) - i \sin(\Delta \theta_d)\Big)$$
Critical Properties of the Transformation:
A fused CUDA kernel reads the
block_table[prefix_blocks:], applies the 2D rotation in SRAM registers, and writes back in-place:End-to-End Control Flow
sequenceDiagram autonumber participant Audio as Streaming Input participant Sched as OmniARScheduler participant Alloc as BlockAllocator participant Runner as GPUARModelRunner participant GPU as Physical KV Cache (VRAM) Audio->>Sched: Streaming Chunk (Context exceeds 8000 tokens) Note over Sched: Trigger Block-Aligned Trim Plan (Δ = 2000 tokens) Sched->>Alloc: Free evicted physical block IDs Sched->>Sched: Compact block table: [Prefix] + [Retained] Sched->>Sched: Decrement num_computed_tokens by Δ Sched->>Runner: Forward Model Input + ReanchorSpec(Δ=2000, prefix=96) Runner->>GPU: Launch Fused In-Place Re-RoPE Kernel on Retained K Blocks Note over GPU: K vectors rotated by exp(-i * Δ * θ), V cache untouched Runner->>GPU: Standard PagedAttention Forward Pass Runner-->>Audio: Next Audio Token (Turn-cut latency under 3ms)Performance Benchmarks
Evaluated on NVIDIA A100-SXM4-80GB with
MiniCPM-o-4_5duplex Stage-0:1. Latency & Audio Stability Comparison
2. Mathematical Parity & Output Equivalence
Verification & Testing
Benchmark and evaluation scripts (
benchmark_kv_window_real.py,benchmark_kv_window_ab.py,demo_duplex_interactive.py) are strictly kept out of this PR to maintain a clean production diff.The PR touches the following components:
vllm_omni/model_executor/models/minicpmo_4_5/duplex/window_plan.py: Block-aligned geometry and re-anchor planner.vllm_omni/model_executor/models/minicpmo_4_5/duplex/window_kv.py: Vectorized in-place Re-RoPE phase shift kernel & spec manager.vllm_omni/core/sched/omni_ar_scheduler.py: Block table compaction, memory free, and token accounting.vllm_omni/worker/gpu_ar_model_runner.py: Model runner hook for Stage-0 KV re-anchoring.vllm_omni/model_executor/models/minicpmo_4_5/minicpmo_4_5_omni.py: Stage-0 thinker spec wiring.6.
vllm_omni/model_executor/models/minicpmo_4_5/duplex/policy.py:MiniCPMO45DuplexWindowConfigschema and validation.7.
vllm_omni/model_executor/models/minicpmo_4_5/duplex/plugin.py: Session runtime config parsing and prefix token registration.8.
tests/model_executor/models/minicpmo_4_5/duplex/test_window_plan.py: Window geometry arithmetic and edge-case unit tests.9.
tests/model_executor/models/minicpmo_4_5/duplex/test_window_wiring.py: Scheduler compaction and Re-RoPE numerical invariance tests.10.
tests/engine/duplex/test_minicpmo_window_plugin.py: Plugin configuration parsing, validation, and token reservation tests.11.
tests/e2e/online_serving/test_minicpmo_4_5_window.py: Real-server online duplex serving regressions (continuous speech, video frames, buffered flush).12.
tests/e2e/online_serving/helpers/minicpmo_window_e2e.py: E2E test client driver.13.
.buildkite/common/ci_source_file_dependencies.yml&.buildkite/cuda/test-merge.yml: CI trigger and pipeline configuration.