Repository navigation
Conversation
df4eb1d to
3ee3217
Compare
- Add SharedPromptAttCache to share the immutable voice prompt attention cache suffix across sessions. - Verify bit-exact prompt attention suffix equality at runtime with tensor fallback on mismatch. - Decouple Whole-Euler arena rows from max_num_seqs using arena_rows_from_graph_grid, saving ~3.6 GiB HBM. - Add unit tests for SharedPromptAttCache bit-exactness and range copying. - Relates to RFC vllm-project#9163. Signed-off-by: BeatSeat <wendavid552@gmail.com>
3ee3217 to
e63faf7
Compare
…sliding-window anchor blocks - Cache immutable session context embeddings and token IDs across sessions in MiniCPMO45Stage0DuplexRuntime. - Ensure Stage-0 sliding-window compaction preserves immutable sink anchor blocks (Tier 0). - Add unit test in test_window_plan.py verifying cross-session anchor protection. - Fully compatible with Model Runner V2 (MRv2) native data plane and V1. - Relates to RFC vllm-project#9163. Signed-off-by: BeatSeat <wendavid552@gmail.com>
e63faf7 to
3a29e80
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
amy-why-3459
left a comment
There was a problem hiding this comment.
Thanks for splitting this out of RFC #9163. I went through the CFM sharing path, the arena sizing and the Stage-0 context cache against the surrounding code. A few things look off to me; details inline:
SharedPromptAttCacheappears to share the wrong end of the estimator cache._att_keep_ranges/_trim_streaming_cachekeep the prompt as the leadingprompt_lenframes and the most recent streaming frames as the trailing 100, but_share_prompt_att_rowscompares and replaces the trailingmin(100, prompt_frames)frames.cfm_arena_rows_from_graph_gridlooks like a no-op:_graph_batches()always ends withmicro_batch_size, so_arena_rowsis unchanged, and nothing in the wrapper readsVLLM_OMNI_CFM_GRAPH_GRID.copy_range_tois only called from the new unit test. The arena fill still goes through_materialize_att_rows, which now does atorch.catper row per replay.- The Stage-0 context cache is always on (no flag). It skips per-session encoder state updates on a hit and is never evicted.
Could the memory numbers in the description be re-checked with sharing on vs. off on the same workload, together with an output-equality check across at least two streaming chunks?
|
@amy-why-3459 Thanks for the thorough review! All points are spot on. We've addressed them in commit d9fa5ca:
We also added unit tests covering the prompt-first range copy and verified with |
|
@codex review |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
amy-why-3459
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. I looked at d9fa5ca only. The prefix/delta split in SharedPromptAttCache and _share_prompt_att_rows now matches the (prompt_len, _CACHE_TRIM_SUFFIX) keep layout, and routing _fill_static_inputs through _copy_att_cache_range removes the per-row torch.cat. Both look right to me.
Two remaining points, details inline:
- On a Stage-0 cache hit,
_stage_ref_audio_embeddingsre-runs the full reference-audio encode and throws the result away. That is the expensive part the cache was meant to skip, and the 32-entry eviction is FIFO rather than LRU. VLLM_OMNI_CFM_GRAPH_GRIDis re-parsed on every_graph_batches()call, but_arena_rowsis fixed in__init__. Without the env var,arena_rows_from_graph_gridis still silently a no-op.
… and duplex cache - Fix SharedPromptAttCache to share the leading prompt_len prefix frames and keep generation delta tail. - Call _copy_att_cache_range directly in Whole-Euler _fill_static_inputs to avoid per-row torch.cat materialization. - Support VLLM_OMNI_CFM_GRAPH_GRID in _graph_batches to enable arena row decoupling. - Make Stage-0 context caching opt-in via duplex_context_caching with bounded LRU eviction. - Add unit tests for SharedPromptAttCache prompt-first layout and VLLM_OMNI_CFM_GRAPH_GRID arena sizing. Signed-off-by: BeatSeat <wendavid552@gmail.com>
d9fa5ca to
ff4d29b
Compare
|
@amy-why-3459 Thanks again for following up! We've fully resolved the remaining two points in commit ff4d29b:
All files formatted cleanly with |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
amy-why-3459
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. Looked at the ff4d29b delta only (vs d9fa5ca).
The Stage-2 side looks right to me now: the shared segment is the leading prompt_len frames, which matches the (prompt_len, _CACHE_TRIM_SUFFIX) keep ranges, and _fill_static_inputs copies through _copy_att_cache_range without materializing. Parsing VLLM_OMNI_CFM_GRAPH_GRID once in __init__ also fixes the _arena_rows / _graph_batches() mismatch, and the warning for the no-saving case is helpful.
Two remaining things on the Stage-0 cache, inline. Minor on the new test: test_duplex_stage0_context_caching_hit_miss_lru only checks the size bound, so a FIFO would pass it too. Touching an early key before overflowing and asserting it survives would actually cover the move_to_end path.
| state.context_prefix_embeds.append(embed) | ||
| state.context_prefix_token_ids.append(token_id) | ||
| instructions = session_config.get("instructions") | ||
| enable_context_caching = bool((runtime_config or {}).get("duplex_context_caching", False)) |
There was a problem hiding this comment.
As far as I can tell nothing sets duplex_context_caching: MiniCPMO45DuplexPlugin builds runtime_config server-side (_apply_default_scheduler_policy, context-token reserve, window config) and minicpmo_4_5.yaml only adds the two cfm_* switches. So in serving this cache is never enabled and only the unit test reaches it, which doesn't match the Stage-0 savings in the description. Could it be wired the same way as the CFM knobs (deploy config to plugin to runtime_config), and listed in PRIVATE_RUNTIME_CONFIG_KEYS if it shouldn't be client-controllable?
| if ref_audio is not None: | ||
| # Detect if the streaming fallback is taken | ||
| process_audio = getattr(self.processor, "process_audio", None) | ||
| is_streaming_path = not callable(process_audio) |
There was a problem hiding this comment.
This predicate doesn't quite mirror _stage_ref_audio_embeddings. That function also falls back to the streaming encoder when process_audio is callable but neither stage_model nor thinker exposes get_audio_embedding / get_audio_hidden_states (the split stage0 wrapper case its comment mentions). In that case this records is_streaming_path=False, and a later cache hit skips the per-session streaming side effects on state.streaming_processor. It would be safer to have _stage_ref_audio_embeddings report which path it actually took (return a flag or set it on state) instead of re-deriving it here.
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. |
…cache test - Route duplex_context_caching through MiniCPMO45DuplexPlugin from model_config to runtime_config and declare it in PRIVATE_RUNTIME_CONFIG_KEYS. - Report exact streaming encoder path from _stage_ref_audio_embeddings to prevent skipping streaming side effects on fallback. - Extend test_duplex_stage0_context_caching_hit_miss_lru to touch early keys before overflow, verifying true LRU eviction over FIFO. Signed-off-by: BeatSeat <wendavid552@gmail.com>
|
@amy-why-3459 Thanks again! We have addressed the remaining Stage-0 points in commit 675bd53:
All touched files pass |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
amy-why-3459
left a comment
There was a problem hiding this comment.
Thanks for the update. I looked at the 675bd53 delta only (vs ff4d29b).
The _stage_ref_audio_embeddings change now reports the streaming fallback exactly, including the case where process_audio exists but the target has neither get_audio_embedding nor get_audio_hidden_states, so that earlier point is resolved.
Two things remain on the caching side (inline):
- The new wiring in
prepare_runtime_configreadsmodel_config.model_config, but the session manager passes theOmniModelConfigitself, which has nomodel_configattribute. Both lookups resolve toNone, soduplex_context_cachingis still alwaysFalsein serving. - The extended LRU test's final assertion also passes under FIFO eviction, so it doesn't yet prove the
move_to_endbehavior.
| runtime_config: dict[str, object] = {"instructions": config.instructions} | ||
| window_config = self._pop_window_config(extra_body) or MiniCPMO45DuplexWindowConfig() | ||
| runtime_config["duplex_window_config"] = window_config.as_dict() | ||
| context_caching = getattr(getattr(model_config, "model_config", None), "duplex_context_caching", None) |
There was a problem hiding this comment.
model_config here is the OmniModelConfig the session manager holds (self.plugin.prepare_runtime_config(config, model_config=self.model_config)), and it doesn't have a .model_config attribute, so getattr(model_config, "model_config", None) is None and both branches fall back to False. stage_connector_config lives directly on OmniModelConfig (vllm_omni/config/model.py). Should this be getattr(model_config, "stage_connector_config", None) (and the same for a direct duplex_context_caching field, if one is intended)? A small test that calls prepare_runtime_config with a config carrying stage_connector_config={"extra": {"duplex_context_caching": True}} and asserts the runtime flag would catch this.
| cache_keys_before = set(runtime._shared_context_cache.keys()) | ||
| runtime._prepare_session_context(s_evicted, {"instructions": "Prompt 1"}, runtime_config=rt_on) | ||
| cache_keys_after = set(runtime._shared_context_cache.keys()) | ||
| assert cache_keys_before != cache_keys_after |
There was a problem hiding this comment.
This assertion also holds with FIFO eviction: under FIFO, cfg1 is evicted during the overflow loop, s_verify re-inserts it, and "Prompt 1" is still a miss, so the key set changes either way (I checked both policies against the same sequence of inserts). To actually pin LRU, assert membership right after the overflow loop and before s_verify, e.g. compute the cache key for cfg1 and assert it is in runtime._shared_context_cache, and assert the "Prompt 1" key is not.
|
@amy-why-3459 Thanks for the detailed inspection! Both points are addressed in commit 34d3a4d:
All files verified clean with |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
amy-why-3459
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. I looked at the 34d3a4d delta only (vs 675bd53).
Both earlier points are addressed. prepare_runtime_config now reads duplex_context_caching / stage_connector_config directly from the stage model_config (the OmniModelConfig the orchestrator passes in), so the stage_connector_config.extra opt-in actually reaches Stage 0. The LRU test now asserts membership directly. I traced it: after the touch and 10 overflow inserts, Prompt 1..Prompt 9 are evicted and Prompt A survives under LRU, while under FIFO Prompt A would be the first one evicted, so the test now distinguishes the two. No further concerns from me on this delta.
…g and pin LRU assertion - Access duplex_context_caching and stage_connector_config directly on model_config (OmniModelConfig) rather than model_config.model_config. - Add test_prepare_runtime_config_wires_duplex_context_caching in test_minicpmo_window_plugin.py to verify config wiring from both model_config and stage_connector_config.extra. - Directly assert key membership of touched cfg1 and evicted Prompt 1 in test_duplex_stage0_context_caching_hit_miss_lru to strictly verify LRU over FIFO. Signed-off-by: BeatSeat <wendavid552@gmail.com>
5a5808f to
3268976
Compare
amy-why-3459
left a comment
There was a problem hiding this comment.
Delta review on amend tip 3268976b (vs prior COMMENT on 34d3a4d).
The tip keeps the two fixes from that round: prepare_runtime_config reads duplex_context_caching / stage_connector_config directly off the stage OmniModelConfig, and the LRU test asserts key membership (Prompt A stays, Prompt 1 goes) so FIFO would fail. The new test_prepare_runtime_config_wires_duplex_context_caching covers the three wiring cases (attr, stage_connector_config.extra, default off), which closes the loop on the serving path. Nothing further from me on this tip.
Motivation
As proposed in RFC #9163, full-duplex conversational models (MiniCPM-o 4.5, PersonaPlex, Qwen3-Omni, Nemotron-VoiceChat) face severe GPU memory bottlenecks under high concurrency (~411.4 MiB/session scaling slope), reaching an OOM barrier at concurrency$c \ge 16 \sim 20$ on an 80GB accelerator.
The fundamental bottleneck is twofold:
default_prompt_audio). Each session independently allocates the full attention cache across all layers, Euler steps, and CFG branches (375 MiB/session), wasting 1012 GiB of contiguous HBM across 32 sessions.enable_prefix_caching: false), every concurrent session allocates duplicate physical KV blocks and independently recomputes system prompts, persona settings, and reference audio tokens.This PR implements the unified Two-Tier Omni Cache for MiniCPM-o 4.5 covering both Phase 1 (Stage 2 Prompt Attention Sharing & Arena Decoupling) and Phase 2 (Stage 0 Cross-Session Context Embedding Caching & Segmented Window Anchor Protection), with full compatibility for Model Runner V2 (MRv2) and V1.
Key Changes
Phase 1: Stage 2 (Code2Wav DiT) Prompt Attention Sharing & Arena Decoupling
SharedPromptAttCache(cuda_graph_wrapper.py):prefix(session-private generation delta frames) andprompt(shared immutable prompt suffix) without materializing a concatenated tensor.copy_range_toto copy arbitrary frame ranges directly into Whole-Euler arena inputs without tensor allocation.batched_token2wav.py):cfm_prompt_att_sharing(default:false).torch.equalacross rows on the first batch, falling back to ordinary tensors if numerical divergence occurs.setup_batchand cleanly releases them onevict_prompt.cfm_arena_rows_from_graph_grid(default:false)._arena_rowsfrom the largest captured graph tier inVLLM_OMNI_CFM_GRAPH_GRIDrather thanmax_num_seqs.Phase 2: Stage 0 (Thinker) Cross-Session Context Cache & Anchor Protection
duplex/stage0.py):_shared_context_cacheinMiniCPMO45Stage0DuplexRuntime.(instructions, has_ref_audio, initial_user_text, ref_audio_hash).duplex/window_kv.py):compact_request_blocks) strictly preserves the immutable sink prefix blocks (self.block_tables) and V1.Deploy Configurations & Tests
minicpmo_4_5.yamlwith new optional switches.test_cfm_graph_bucketing.py: VerifiesSharedPromptAttCachebit-exactness and range copying.test_code2wav_batching.py: Validates config parsing and default-off behavior.test_window_plan.py: Verifies cross-session anchor protection under sliding window compaction.Memory & Concurrency Impact
Measured on single-card 80GB H100 / A800 benchmarks:
125375 MiB per session (1012 GiB reclaimed atcc @amy-why-3459 @Sy0307