Skip to content

[HiCache] Unified Mooncake Registration for Logical Anchors and Draft Pools - #29859

Open
stmatengss wants to merge 7 commits into
sgl-project:mainfrom
stmatengss:upstream-fix
Open

stmatengss wants to merge 7 commits into
sgl-project:mainfrom
stmatengss:upstream-fix

Conversation

@stmatengss

@stmatengss stmatengss commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

This PR unifies the handling of two Mooncake registration failure modes that share the same root cause: pools that cannot be registered with Mooncake's zero-copy interface.

Motivation

Both currently require separate workarounds or cause crashes.

Modifications

Introduce a unified is_mooncake_registerable property that allows pools to declare whether they support Mooncake registration:

  1. Allocator compatibility check: Added is_mooncake_compatible property to HostTensorAllocator (False for mmap, True for MooncakeHostTensorAllocator)
  2. Property delegation: HostKVCache.is_mooncake_registerable delegates to self.allocator.is_mooncake_compatible
  3. Logical anchor override: LogicalHostPool overrides to False (no buffer exists)
  4. Proactive registration check: _maybe_register_draft_with_storage() checks the property before attempting registration, avoiding exceptions entirely

Accuracy Tests

Speed Tests and Profiling

Checklist

Review and Merge Process

  1. Ping Merge Oncalls to start the process. See the PR Merge Process.
  2. Get approvals from CODEOWNERS and other reviewers.
  3. Trigger CI tests with comments or contact authorized users to do so.
    • Common commands include /tag-and-rerun-ci, /tag-run-ci-label, /rerun-failed-ci
  4. After green CI and required approvals, ask Merge Oncalls or people with Write permission to merge the PR.

CI States

Latest PR Test (Base): ❌ Run #28534083240
Latest PR Test (Extra): ❌ Run #28534083143

stmatengss and others added 7 commits July 1, 2026 15:06
Adds a property to HostKVCache base class (default True), LogicalHostPool
(False, no physical buffer), and HostPoolGroup (delegates to anchor).

This enables proactive detection of non-registerable pools before attempting
Mooncake zero-copy registration.

Part of unified fix for PRs sgl-project#29035 and sgl-project#26649.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds _logical_anchor_key, _batch_set_logical_anchor, and
_batch_get_logical_anchor methods for tracking logical anchor page
existence in Mooncake without physical KV buffers.

Marker keys use _logical_kv suffix to avoid collision with real KV keys.
Uses 1-byte markers since Mooncake put() rejects zero-length payloads.

Part of unified fix for PRs sgl-project#29035 and sgl-project#26649.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- register_mem_pool_host: detect non-registerable pools via
  is_mooncake_registerable property, set _logical_kv_anchor flag
- batch_exists: check marker keys for logical anchors
- batch_get_v1: delegate to marker key check for logical anchors
- batch_set_v1: write marker keys for logical anchors

Logical anchors have no physical buffer, so use marker keys for
existence tracking instead of zero-copy I/O.

Part of unified fix for PRs sgl-project#29035 and sgl-project#26649.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add _maybe_register_draft_with_storage() method to handle draft pool
  registration with storage backends that support v2 registration
- Use try/except to gracefully handle registration failures (e.g., when
  draft host pool uses mmap instead of MooncakeHostTensorAllocator)
- Track registration success via draft_registered_in_storage flag
- Allows L2 (host DRAM) operations to continue even when L3 (storage
  backend) registration fails for draft pools

This fixes the issue where EAGLE with Mooncake L3 in standalone mode
would crash during startup due to draft pool registration failures.

Part of unified fix for PRs sgl-project#29035 and sgl-project#26649.
Add unit tests for _maybe_register_draft_with_storage() method:
- Test skip when storage or draft is disabled
- Test graceful handling when backend lacks v2 support
- Test successful registration flow
- Test graceful handling of registration failures
- Test specific EAGLE + Mooncake standalone case with mmap allocator

These tests verify that draft L2 (host DRAM) operations continue
working even when L3 (storage backend) registration fails.

Part of unified fix for PRs sgl-project#29035 and sgl-project#26649.
Resolve conflicts in three files to integrate upstream changes:

- memory_pool_host.py: Upstream moved HostKVCache to pool_host/base.py.
  Added is_mooncake_registerable property to the new location.
  LogicalHostPool: combined our property with upstream's layout parameter.

- cache_controller.py: Upstream added _maybe_register_draft_with_storage()
  with function pointer approach (draft_page_get_func/set_func). Wrapped
  Mooncake register_mem_host_pool_v2 in try/except for graceful degradation
  when EAGLE draft pools use non-Mooncake allocators.

- mooncake_store.py: Simplified logical anchor detection using upstream's
  kv_buffer is None check while preserving _logical_kv_anchor flag for
  marker key tracking in batch operations.

- Updated test_draft_pool_graceful_degradation.py to match function pointer
  API and test Mooncake-specific failure scenarios.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ompatibility

Replace reactive try/except in draft pool registration with a proactive
property check. The root cause for both EAGLE draft pools (mmap allocator)
and DeepSeek-V4 logical anchors (no buffer) is that they cannot be
registered with Mooncake's zero-copy interface.

- Add `is_mooncake_compatible` property to HostTensorAllocator (False) and
  MooncakeHostTensorAllocator (True).
- `HostKVCache.is_mooncake_registerable` now delegates to allocator.
- `_maybe_register_draft_with_storage` checks the property before calling
  register_mem_host_pool_v2, avoiding the exception entirely.
- LogicalHostPool still overrides to False (no buffer at all).

This provides a single, clean abstraction for all non-registerable pools.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the is_mooncake_registerable property for host pools and is_mooncake_compatible for allocators to determine Mooncake compatibility. It also implements logical anchor handling in MooncakeStore using marker keys for existence tracking and adds corresponding unit tests. The review feedback highlights three key improvements: wrapping self.store.put in a try-except block to prevent unhandled exceptions from crashing the backup thread, fixing an AttributeError in the unit tests caused by attempting to write to the read-only __class__.__name__ attribute, and using getattr to safely access is_mooncake_compatible on the allocator.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +1315 to +1331
def _batch_set_logical_anchor(self, keys: List[str]) -> List[bool]:
"""Set marker keys for logical anchor pages.

Uses 1-byte markers since Mooncake put() rejects zero-length payloads.
Only writes keys that don't already exist to avoid redundant writes.
"""
key_strs = [self._logical_anchor_key(key) for key in keys]
exist_result = self._batch_exist(key_strs)
results = [state == 1 for state in exist_result]

# Mooncake put() rejects zero-length payloads, so use one byte
marker = b"1"
for i, key in enumerate(key_strs):
if results[i]:
continue
results[i] = self.store.put(key, marker) == 0
return results

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The self.store.put call is executed in a loop and can raise exceptions if there are network or storage issues. Since the backup thread does not catch general exceptions, any unhandled exception here will crash the backup thread. Wrapping the put call in a try-except block ensures robustness.

    def _batch_set_logical_anchor(self, keys: List[str]) -> List[bool]:
        """Set marker keys for logical anchor pages.

        Uses 1-byte markers since Mooncake put() rejects zero-length payloads.
        Only writes keys that don't already exist to avoid redundant writes.
        """
        key_strs = [self._logical_anchor_key(key) for key in keys]
        exist_result = self._batch_exist(key_strs)
        results = [state == 1 for state in exist_result]

        # Mooncake put() rejects zero-length payloads, so use one byte
        marker = b"1"
        for i, key in enumerate(key_strs):
            if results[i]:
                continue
            try:
                results[i] = self.store.put(key, marker) == 0
            except Exception as e:
                logger.error("Failed to put logical anchor key %s: %s", key, e)
                results[i] = False
        return results

Comment on lines +76 to +77
mock_draft_pool.allocator = Mock()
mock_draft_pool.allocator.__class__.__name__ = "HostTensorAllocator"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

In Python, the __name__ attribute of a class/type object is read-only. Attempting to write to mock_draft_pool.allocator.__class__.__name__ will raise an AttributeError: attribute '__name__' of 'type' objects is not writable and cause the test to fail. Using a simple dummy class instead of a mock avoids this issue.

Suggested change
mock_draft_pool.allocator = Mock()
mock_draft_pool.allocator.__class__.__name__ = "HostTensorAllocator"
class HostTensorAllocator:
pass
mock_draft_pool.allocator = HostTensorAllocator()

Comment on lines +90 to +98
def is_mooncake_registerable(self) -> bool:
"""Whether this pool's buffer can be zero-copy registered with Mooncake.

Returns True if the allocator produces Mooncake-compatible tensors.
Returns False for:
- Logical anchors (no physical buffer, only page indices)
- Pools using non-Mooncake allocators (e.g., mmap for draft pools)
"""
return self.allocator.is_mooncake_compatible

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To prevent potential AttributeError if self.allocator is None or does not have the is_mooncake_compatible attribute, it is safer to use getattr with a default value of False.

Suggested change
def is_mooncake_registerable(self) -> bool:
"""Whether this pool's buffer can be zero-copy registered with Mooncake.
Returns True if the allocator produces Mooncake-compatible tensors.
Returns False for:
- Logical anchors (no physical buffer, only page indices)
- Pools using non-Mooncake allocators (e.g., mmap for draft pools)
"""
return self.allocator.is_mooncake_compatible
@property
def is_mooncake_registerable(self) -> bool:
"""Whether this pool's buffer can be zero-copy registered with Mooncake.
Returns True if the allocator produces Mooncake-compatible tensors.
Returns False for:
- Logical anchors (no physical buffer, only page indices)
- Pools using non-Mooncake allocators (e.g., mmap for draft pools)
"""
return getattr(self.allocator, "is_mooncake_compatible", False)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant