[Bugfix][KV Connector] MooncakeStore: exclude non-prefix-cacheable (QSA ring) groups; fix align-mode check - #55027
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
fa6456d to
f6db171
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. |
|
This pull request has merge conflicts that must be resolved before it can be |
f6db171 to
928ba63
Compare
ivanium
left a comment
There was a problem hiding this comment.
Nice fix. Only leaving some simplification suggestions. connector.py and scheduler.py can perhaps be simplified a bit too
| store_layer_names = { | ||
| layer_name | ||
| for group in self._kv_cache_groups | ||
| for layer_name in group.layer_names | ||
| } | ||
| seen_storage_ptrs: set[int] = set() | ||
| cache_tensors: list[torch.Tensor] = [] | ||
|
|
||
| for cache in kv_caches.values(): | ||
| for layer_name, cache in kv_caches.items(): | ||
| if layer_name not in store_layer_names: | ||
| continue |
There was a problem hiding this comment.
Not sure if we really need this.
There was a problem hiding this comment.
We still need this, otherwise non-cacheable groups like QSA ring will still be registered
3d290b8 to
ef9c42f
Compare
ef9c42f to
2b123ab
Compare
|
This pull request has merge conflicts that must be resolved before it can be |
2b123ab to
45bac07
Compare
45bac07 to
d962d27
Compare
| local_block_ids = self.kv_cache_config.select_block_ids( | ||
| blocks.get_block_ids(), | ||
| self._store_group_ids, | ||
| ) |
There was a problem hiding this comment.
after seeing many of this pattern, I am thinking if we should do
| local_block_ids = self.kv_cache_config.select_block_ids( | |
| blocks.get_block_ids(), | |
| self._store_group_ids, | |
| ) | |
| local_block_ids = \ | |
| blocks.get_block_ids(group_ids=self._store_group_ids) |
Where we extend KVCacheBlocks.get_block_ids() with group_ids: Optional[list[int]]
There might be some other places scheduler has already passed full block_ids in, in which case we can do a manual for loop. This should help remove kv_cache_config.select_block_ids() interface
| def select_block_ids( | ||
| self, | ||
| block_ids: Sequence[list[int]], |
There was a problem hiding this comment.
see comments in mooncake store scheduler.py. Perhaps we can remove this API
d962d27 to
96f18ff
Compare
|
/ci run |
|
✅ Triggered Buildkite CI #88610 for commit |
Keep transferable KV cache groups distinct from the prefix-cacheable subset used by hash-addressed stores. Preserve the full transfer projection for connectors such as NIXL while making MooncakeStore scheduler, worker, and coordinator paths operate only on storable groups. Validate Mamba align mode from the spec directly and add regression coverage for QSA ring groups, block-id projection, and worker tensor registration. Fixes vllm-project#54726 Co-authored-by: OpenAI Codex <codex@openai.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Zhewen Li <zhewenli@inferact.ai>
Use KVCacheConfig's prefix-cacheable group projection directly in MooncakeStore and remove the connector-local helper. Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: Zhewen Li <zhewenli@inferact.ai>
a6916cc to
59f6056
Compare
|
/ci run |
|
✅ Triggered Buildkite CI #88686 for commit |
…SA ring) groups; fix align-mode check (vllm-project#55027) Signed-off-by: Zhewen Li <zhewenli@inferact.ai> Co-authored-by: Zhewen Li <zhewenli@inferact.ai> Co-authored-by: OpenAI Codex <codex@openai.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…SA ring) groups; fix align-mode check (vllm-project#55027) Signed-off-by: Zhewen Li <zhewenli@inferact.ai> Co-authored-by: Zhewen Li <zhewenli@inferact.ai> Co-authored-by: OpenAI Codex <codex@openai.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Purpose
Fix #54726: Qwen3.8-Flash-Next with prefix caching and
MooncakeStoreConnectorfails because its QSA ring scratch cache is transferable, but is not valid hash-addressed prefix data.The QSA cache returns a
CircularBufferSpec;CircularBufferSpec.prefix_cacheableis explicitlyFalse. Its capacity is 4 without speculative tokens and 8 with MTP=3, while aligned attention/Mamba groups use block size 800. MooncakeStore previously built its hash/store/lookup layout from everyenable_kv_transfer=Truegroup, so the ring reached a divisibility check intended only for storable prefix groups.A second issue was hidden behind that failure: MooncakeStore inferred Mamba align mode by comparing each Mamba block size with the globally rewritten
cache_config.block_size. With heterogeneous groups, that global value can differ from the resolved Mamba group size even whenmamba_cache_modeis correctly"align".Fix
KVCacheConfig.prefix_cacheable_group_idsandprefix_cacheable_groupsbeside the existing transfer projection.KVCacheBlocks.get_block_ids(group_ids=...)so scheduler consumers can request group projections without adding a generic selector toKVCacheConfig.spec.mamba_cache_mode == "align".This deliberately does not change
transfer_group_ids,transfer_groups, orselect_transfer_block_ids. NIXL and other P/D connectors continue to transfer allenable_kv_transfer=Truegroups, including the QSA ring scratch state.Duplicate-work check
Checked issue #54726 and its comments, plus open PR searches for
54726 in:bodyandMooncake QSA prefix cacheable. The only matching open PR is this PR (#55027).Tests
Result on the current head: 412 passed, 14 warnings in 146.17s.
Result: 77 passed. The remaining GPU integration case failed during connector initialization because NIXL is unavailable in the test environment; it did not reach block selection.
Result: all applicable hooks passed, including Ruff and mypy.
Model evaluation
Results:
All 2638 requests returned a non-empty answer, all finished with
stop, and there were no API errors. After the reset, Mooncake performed 1416 Get calls covering 7092 keys / 72.59 GB, with zero failed keys and zero transfer errors. The original block-size assertion did not occur.AI assistance
AI coding agents (OpenAI Codex and Claude) assisted with analysis, implementation, testing, and review. The human submitter reviewed the changes. The commit includes DCO sign-off and AI co-author trailers.