Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
7f8b837 to
1549941
Compare
stmatengss
left a comment
There was a problem hiding this comment.
Review: 0 critical, 4 informational. Correct approach for skipping physical buffer registration on logical KV anchors.
| @@ -597,6 +598,17 @@ def warmup(self): | |||
|
|
|||
| def register_mem_pool_host(self, mem_pool_host: HostKVCache): | |||
| super().register_mem_pool_host(mem_pool_host) | |||
| self._logical_kv_anchor = ( | |||
| getattr(self.mem_pool_host, "kv_buffer", None) is None | |||
| and getattr(self.mem_pool_host, "entries", None) is not None | |||
There was a problem hiding this comment.
[INFO] (8/10) Duck-typing heuristic: kv_buffer is None and entries is not None. Works for the current DeepSeek-V4 anchor shape, but if a future pool has kv_buffer=None for a different reason (e.g. lazy allocation), this would misclassify it. Consider checking an explicit attribute like is_logical_anchor on the pool if one exists, or adding one.
There was a problem hiding this comment.
Good point. I replaced the kv_buffer/entries heuristic with an explicit is_logical_anchor marker on LogicalHostPool, propagated through HostPoolGroup.
| marker = b"1" | ||
| for i, key in enumerate(key_strs): | ||
| if results[i]: | ||
| continue |
There was a problem hiding this comment.
[INFO] (7/10) Sequential self.store.put() in a loop for each non-existing key. If many logical pages are set simultaneously, this is N individual RPCs to Mooncake instead of a batch write. Fine for small page counts; could become a bottleneck if the anchor pool is large. Consider batching if a batch-put API exists.
There was a problem hiding this comment.
Keeping this as-is for now. These are 1-byte marker values, while the available batch API here is the zero-copy batch_put_from(ptr, size) path for registered buffers. We can optimize if Mooncake exposes a batch bytes put API.
| @@ -652,6 +683,10 @@ def _get_hybrid_page_component_keys( | |||
| suffixes = [f"{base_suffix}_temporal"] + [ | |||
| f"{base_suffix}_conv_{i}" for i in range(conv_num) | |||
| ] | |||
| elif name in getattr(self, "registered_pools", {}): | |||
| # DeepSeek-V4 and other hybrid side pools store one object per | |||
| # logical page. Use the pool name as the storage-key suffix. | |||
There was a problem hiding this comment.
[INFO] (7/10) getattr(self, "registered_pools", {}) silently returns {} if the attribute doesn't exist, making this branch dead code in that case. If registered_pools is guaranteed to be set by register_mem_host_pool_v2 before this method runs, consider using self.registered_pools directly so a missing attribute fails loudly rather than silently skipping the DeepSeek-V4 path.
There was a problem hiding this comment.
Agreed. Changed this to use self.registered_pools directly so a missing initialization fails loudly.
| @@ -843,6 +878,9 @@ def batch_get_v1( | |||
| # Apply extra_backend_tag prefix if available | |||
| keys = self._tag_keys(keys) | |||
|
|
|||
| if getattr(self, "_logical_kv_anchor", False): | |||
There was a problem hiding this comment.
[INFO] (6/10) getattr(self, "_logical_kv_anchor", False) is used here, in batch_set_v1, and in batch_exists, but _logical_kv_anchor is always initialized in __init__. The defensive getattr is harmless but inconsistent with the direct self._logical_kv_anchor access in register_mem_pool_host. Pick one pattern.
There was a problem hiding this comment.
Agreed. _logical_kv_anchor is initialized in init, so I switched the call sites to direct self._logical_kv_anchor access.
|
Update after rebasing on latest main: The latest main has already fixed the startup-side part of this issue by allowing Mooncake registration to skip physical KV buffer registration for logical/hybrid anchor pools, and it also includes more complete v2 handling for hybrid side pools. This PR is still useful as a semantic follow-up: it makes logical KV anchors explicit via So the PR is no longer the sole startup fix, but it tightens Mooncake’s logical-anchor registration and existence-tracking semantics for DeepSeek-V4-style hybrid HiCache. |
|
/tag-and-rerun-ci |
|
/rerun-failed-ci |
| assert self.mem_pool_host.layout in [ | ||
| "page_first", | ||
| "page_first_direct", | ||
| "page_head", | ||
| "page_first_kv_split", | ||
| ], "mooncake store storage backend only support page first, page first direct, page head and page_first_kv_split layout" |
There was a problem hiding this comment.
Should remove to parameter checking phases?
There was a problem hiding this comment.
Removed the extra layout assert from register_mem_pool_host. Backend/layout compatibility should stay in the argument/config normalization path, while this PR only needs to handle the logical-anchor
registration path.
| if self.mem_pool_host.kv_buffer is None: | ||
| # Non-KV logical anchors carry data through v2 side pools only. | ||
| return [True] * len(keys) |
There was a problem hiding this comment.
Don't need to change the position.
There was a problem hiding this comment.
Adjusted the branch order so the non-logical kv_buffer=None fast path stays before key tagging. Logical KV anchors still fall through to the tagged marker-key path.
22d2b89 to
abbeb27
Compare
|
/rerun-failed-ci |
|
/rerun-failed-ci |
Motivation
DeepSeek-V4-Flash uses a hybrid HiCache layout with a logical KV anchor pool. The anchor pool owns page indices but does not own a real KV buffer; the actual data is stored in hybrid side pools such as SWA, compressed KV, indexer, and state pools.
When enabling Mooncake as the HiCache storage backend, Mooncake previously assumed the primary host pool always had a physical
kv_bufferand a Mooncake-supported memory layout. For DeepSeek-V4, this caused startup to fail during host pool registration because the logical anchor haskv_buffer=Noneandlayout=layer_first.Verify fix works. Launch server as follows:
Example Commands
This PR fixes Mooncake storage registration for DeepSeek-V4 hybrid HiCache. #26647
Modifications
Accuracy Tests
This change does not modify model forward logic, kernels, sampling, or numerical computation. No accuracy impact is expected.
Speed Tests and Profiling
This change only affects Mooncake HiCache storage registration and logical KV page existence tracking for DeepSeek-V4 hybrid HiCache.
Checklist
Review and Merge Process
/tag-and-rerun-ci,/tag-run-ci-label,/rerun-failed-ciCI States
Latest PR Test (Base): ✅ Run #27398986088
Latest PR Test (Extra): ❌ Run #27398985992