Repository navigation
[mem_cache][11/N] refactor: extract KVCache and BaseSWAKVPool into pool/base.py - #35647
alphabetc1 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6bbc4efc0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| from sglang.srt.mem_cache.pool.base import ( | ||
| unwrap_write_loc, | ||
| ) |
There was a problem hiding this comment.
Update the mocked module tree for the new pool import
When test/manual/minimax_m3/test_npu_memory_pool.py runs _load_npu_memory_pool_module(), it stubs sglang.srt.mem_cache as a plain ModuleType and provides only the old memory_pool.unwrap_write_loc. Executing this newly added import therefore raises ModuleNotFoundError: 'sglang.srt.mem_cache' is not a package before any of the standalone NPU pool tests run. The loader needs to stub sglang.srt.mem_cache.pool.base and its unwrap_write_loc symbol, or otherwise load the real package.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
e6bbc4e to
2e551e7
Compare
Motivation
Part of #25371. Mechanical Move. First PR of the
mem_cache/pool/layer.pool/is the last of the three layer packages and the only one still getting worse.Since the restructure was agreed,
memory_pool.pywent from 2257 to 5042 lines and from11 to 17 classes, because there was nowhere else for a device pool to go. This creates
the package and moves the base layer into it, so every later family move has a target.
Modifications
New
mem_cache/pool/base.py, holding only what every device pool derives from:KVWriteLoc,unwrap_write_locmemory_pool.pyKvBufferDescmemory_pool.pyKVCache(ABC)memory_pool.pyBaseSWAKVPool(ABC)base_swa_memory_pool.py(deleted)GBmemory_pool.pybase_swa_memory_pool.pyis deleted: it was a 29-line file holding one cross-familyABC, sitting under the SWA feature's name while
SWAKVPool,DeepSeekV4TokenToKVPooland the disagg paths all depend on it. It belongs next to
KVCache.No re-export shim: all 44 call sites are updated in this PR, matching how
allocator.pywas deleted outright rather than left forwarding.
RadixAttentionmoves underTYPE_CHECKINGin the new module. It is only ever anannotation (
layer: RadixAttention), and the file hasfrom __future__ import annotations; keeping it at runtime madepool/base.py->layers.radix_attention->... ->
forward_context->pool/base.pya genuine import cycle, becausepool/base.pyis imported far earlier in the chain than
memory_pool.pywas. This mirrors howLayerDoneCounterwas already handled.Two now-unused imports (
abc,KVWriteLoc) drop out ofmemory_pool.py. Both remainingKVWriteLocmentions there are in comments.Accuracy Test
Mechanical move, so the bar is byte-level equality:
KVWriteLoc/unwrap_write_loc/KvBufferDesc/KVCacheblock isbyte-identical to the original
memory_pool.pyregion.BaseSWAKVPoolclass body is byte-identical tobase_swa_memory_pool.py.Verified on an H200 devbox (
PYTHONPATHshadowing the image's copy):Identical command (
test/registered/unit/mem_cache/plusspec/test_resolve_swa_kv_pool.py,--ignoreontest_umbp_store.pywhich needs themoripackage that is absent from the image) run against both trees on the same box --zero delta, so the 1074 skips are pre-existing and none were introduced here. MRO was
asserted intact (
KVCache in MHATokenToKVPool.__mro__,BaseSWAKVPool in SWAKVPool.__mro__), and the re-run after the ruff import cleanup gavethe same counts.
Benchmark and Profiling Results
Not applicable -- no runtime behavior changes.
Checklist
Independent of the other in-flight #25371 PRs -- touches no file that #35306, #35643 or
#35644 touch. Once this lands, #35638 can enable its
poolrole and move the devicepools from its
_SHRINKING_MODULESpin into_LEGACY_HOMES.🤖 Generated with Claude Code
CI States
Latest PR Test (Base): ❌ Run #34434260912
Latest PR Test (Extra): ✅ Run #34439738060
Latest PR Test (AMD ROCm 10): ❌ Run #34434260836