Skip to content

[Platform] Thread vendor KV pool classes through the in-tree builders; add PlatformCapabilities - #35594

Merged
alexnails merged 32 commits into
sgl-project:mainfrom
alexnails:kv-platform-seam
Oct 8, 2026
Merged

alexnails merged 32 commits into
sgl-project:mainfrom
alexnails:kv-platform-seam

Conversation

@alexnails

@alexnails alexnails commented Aug 20, 2026 •

Copy link
Copy Markdown
Collaborator

folding in musa, npu, and oot kv contract


CI States

Latest PR Test (Base): ✅ Run #37523252004
Latest PR Test (Extra): ✅ Run #37523251744
Latest PR Test (AMD ROCm 10): ❌ Run #37523251848

…class hooks to Optional

Replaces the scattered platform selection for KV pools with a single
request-passing seam, and converts the remaining get_*_cls hooks from
raise-NotImplementedError (OOT-only) to Optional-returning ("None = no
platform opinion") consulted unconditionally.

Why a request seam for pools: the class-getter hooks had two structural
defects — the call site instantiates the platform's class with a fixed
kwargs set, coupling every vendor pool __init__ to call-site churn
(sgl-project#30568), and every construction site must remember to consult the hook
(sgl-project#27679: SWATokenToKVPoolAllocator bypassed get_paged_allocator_cls).
With build_kv_pool(request) the platform sees the full request (kind,
sizes, layout, dtype, attention backend) and constructs the pool itself.
This also resolves the in-tree-override ambiguity flagged on sgl-project#29445: the
platform now sees the caller-selected layout instead of clobbering it
through an or-chain priority.

Changes:
- platforms/interface.py: new build_kv_pool(request) -> Optional[KVCache];
  get_{mha,mla,dsa}_kv_pool_cls deprecated (still honored for OOT),
  get_paged_allocator_cls / get_graph_runner_cls /
  get_piecewise_backend_cls now Optional with in-tree fallbacks.
- mem_cache/kv_pool_request.py: KVPoolRequest (msgspec.Struct) carrying
  plain scalars; index_head_dim is set only for DSA-indexed models.
- kv_cache_configurator.py: single resolution point
  _build_platform_kv_pool (platform -> legacy OOT hooks -> in-tree),
  plus _build_platform_hybrid_full_kv_pool resolving the full-attention
  leaf of hybrid-linear composites (absorbs the NPU/OOT selection that
  lived inside HybridLinearKVPool.__init__); paged-allocator hook
  consulted unconditionally.
- memory_pool.py: HybridLinearKVPool.__init__ no longer makes platform
  decisions — a platform pool arrives pre-built via full_kv_pool.
- allocator/swa.py: consult get_paged_allocator_cls inside the SWA
  composite (the sgl-project#27679 fix); _is_npu fallback stays until NpuSRTPlatform
  lands.
- cuda_graph_setup.py / compilation/backend.py: platform hook first,
  device-keyed in-tree defaults unchanged as fallback.

Platform-neutral by construction: every in-tree platform returns None
from every new/changed hook, and the in-tree NPU (_is_npu / ascend)
branches are untouched pending NpuSRTPlatform. OOT platforms keep
working through the deprecated hooks (warn-once). One deliberate
contract change: an OOT platform that never overrode a hook now falls
back to the in-tree default instead of crashing with NotImplementedError.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…fallback

Five fixes on top of the build_kv_pool seam.

1. DSA startup crash, all platforms. _make_kv_pool_request called
   calculate_mla_kv_cache_dim(..., server_args=...), but main dropped that
   parameter while this branch was open. The seam is consulted
   unconditionally, so every DeepSeek-V3.2 (DSA) launch raised TypeError --
   on CUDA too, not just out-of-tree. The new test exercises the real
   request builder; every seam test previously replaced
   _make_kv_pool_request with a MagicMock, which is why CI stayed green.

2. Out-of-tree regression. "None means use the in-tree default" is right
   for in-tree platforms but silently hands a device main does not know
   about the CUDA implementation: graph_runners is a defaultdict whose
   default factory is the CUDA runner, make_backend falls through to
   CUDAPiecewiseBackend, and the KV path lands on MHATokenToKVPool. Those
   sites previously raised NotImplementedError for out-of-tree platforms.
   require_out_of_tree_impl restores an actionable error naming the hook,
   and is a no-op in-tree, so the flattened Optional hooks keep their
   in-tree meaning.

3. Hybrid-SWA sizing. The request advertised is_hybrid_swa and swa_size but
   carried size=max_total_num_tokens, while the in-tree SWAKVPool sizes its
   full side with full_max_total_num_tokens. Added full_size.

4. Layout over-reporting. layout came from the global page-major flag
   regardless of kind, so MLA/DSA and mxfp8 MHA requests ordered a
   page-major pool that the in-tree default never builds. _page_major_applies
   restricts it to plain MHA. Same class of fix for post_capture_active,
   which now mirrors _build_mha_kv_pool's fp4 gating, and for
   kv_cache_dtype_str, declared str but fed the configurator's Optional[str].

5. The sgl-project#27679 fix was unreachable. For out-of-tree platforms the top-level
   allocator preemption returns a flat paged allocator before any SWA
   composite is built, and in-tree platforms return None from the hook, so
   the new consult inside SWATokenToKVPoolAllocator could never fire
   (UnifiedSWATokenToKVPoolAllocator skips that __init__ deliberately). The
   preemption now stands down when the platform pool is itself a
   BaseSWAKVPool, routing that pair through the composite -- which consults
   the same hook per sub-pool. Flat platform pools keep today's behavior.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@alexnails alexnails added the run-ci CI: run the baseline test suite on this PR label Sep 1, 2026
alexnails and others added 2 commits September 1, 2026 16:57
…ass it

Design follow-ups from review of the seam itself, all platform-neutral.

**Closed domains.** `kind`, `layout` and the new `quant_scheme` are `Literal`s
instead of bare `str`. A platform that matches exhaustively now gets a signal
when a new member lands, rather than an `else` arm that serves a new family as
an old one.

**MiniMax-sparse no longer arrives as "mha".** The in-tree dispatcher routes it
to `MiniMaxSparseKVPool` *after* the seam, so a platform answering the `"mha"`
request with a plain MHA pool was building something the sparse attention
backend then rejects with an isinstance assert. It is now `"mha_sparse_index"`;
platforms that cannot serve it return None and fall through to the in-tree
builder. The deprecated class hooks keep mapping it to `get_mha_kv_pool_cls`,
so migrated and unmigrated out-of-tree platforms behave as before.

**`quant_scheme`** makes the block-scaled pools (fp4, mxfp8) explicit. mxfp8 is
not inferable from `dtype` alone -- it was only reachable by string-matching
`kv_cache_dtype_str` -- and the fp4 post-capture gate now reads the scheme
instead of re-deriving the dtype check.

**Paths with no seam.** Unified memory returns before `_build_token_to_kv_pool`
entirely, and DSV4 has a dedicated builder that runs ahead of the seam. Both
silently built in-tree CUDA-assuming pools for an out-of-tree platform.
`reject_out_of_tree_path` fails them loudly, naming the subsystem. Same defect
class as the `require_out_of_tree_impl` sites, different cause: there is no hook
to override, so the message says so rather than pointing at one.

**`allocator/hisparse.py`** constructed `PagedTokenToKVPoolAllocator` directly at
three sites -- the same bug as sgl-project#27679, one file over from the fix. It now
consults `get_paged_allocator_cls()`.

**The real contract is wider than `KVCache`.** In-tree callers (PD transfer,
hicache, the attention backends) reach for concrete attributes and gate on
`isinstance` against the in-tree pool classes -- `disaggregation/utils.py`
hardcodes `NPUMLATokenToKVPool`, and 7 of 8 attention backends check concrete
`SWAKVPool` rather than `BaseSWAKVPool`. `build_kv_pool`'s docstring now says to
subclass `MHATokenToKVPool`/`MLATokenToKVPool`/`DSATokenToKVPool` instead of
implementing `KVCache` directly. Narrowing that surface is follow-up work, but
vendors should not discover it by breakage.

**Hybrid-SWA is documented as not-yet-buildable.** `is_hybrid_swa`/`full_size`/
`swa_size` describe the budget but not the sliding/full layer-id split a
composite needs, so the struct says to return None rather than implying a
capability the seam does not yet have.

**Docs.** `plugin.mdx` still documented the class getters as
`raise NotImplementedError` and never mentioned the seam.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Sep 2, 2026
@alexnails

alexnails commented Sep 2, 2026 •

Copy link
Copy Markdown
Collaborator Author

Design review of the seam itself — findings and what changed

Two follow-up commits on this PR. Recording the reasoning so reviewers can push back on the shape, not just the diff.

The concern that drove this

KVPoolRequest is bad integration point. Its shape fields (size, dtype, head_num, layer_num) describe what the pool must hold — legitimately the vendor's problem. But layout, attention_backend, is_hybrid_swa, post_capture_active describe how the configurator decided, i.e. promises about our own selector. Two bugs found in review were symptoms rather than independent defects:

  • layout was reported as page_major for MLA/DSA, where the in-tree path never builds page-major.
  • size was the combined budget for hybrid-SWA, where in-tree sizes the full pool from full_max_total_num_tokens.

Same failure both times: the struct claimed to mirror a decision it was not mirroring. "Adding a field never breaks a platform that ignores it" is true for additive change and irrelevant here — a field that lies is worse than a missing field, because a conscientious vendor honors it.

What is fixed

  • Closed domains. kind, layout, and the new quant_scheme are Literals. Exhaustive matching now gets a signal when a member is added, instead of an else arm serving a new family as an old one.
  • MiniMax-sparse no longer arrives as "mha". The dispatcher routes it to MiniMaxSparseKVPool after the seam, so a platform answering the "mha" request built something minimax_sparse_backend.py then rejects with an isinstance assert — and the request gave it no way to decline. Now "mha_sparse_index". Deprecated class hooks still map it to get_mha_kv_pool_cls, so migrated and unmigrated OOT platforms are unchanged.
  • quant_scheme makes fp4/mxfp8 explicit; mxfp8 was only reachable by string-matching kv_cache_dtype_str.
  • Paths with no seam now fail loud for OOT. Unified memory returns before _build_token_to_kv_pool entirely; DSV4 has a dedicated builder ahead of the seam. Both silently built CUDA-assuming pools for a device main does not know about. reject_out_of_tree_path names the subsystem — distinct from require_out_of_tree_impl because there is no hook to point at.
  • allocator/hisparse.py constructed PagedTokenToKVPoolAllocator directly at three sites. Same bug as Fix SWA allocator factory selection for OOT platforms #27679, one file over from the fix.
  • Hybrid-SWA documented as not-yet-buildable. is_hybrid_swa/full_size/swa_size give the budget but not the sliding/full layer-id split a composite needs.
  • Docs. plugin.mdx still documented the class getters as raise NotImplementedError and never mentioned the seam.

The bigger issue this PR does not fix

The real contract is wider than KVCache. The ABC declares four abstract methods, but in-tree callers reach for concrete attributes and gate on isinstance against concrete classes:

  • disaggregation/utils.py:1206,1218 hardcodes NPUMLATokenToKVPool in otherwise generic code — the precedent a new vendor copies is one core PR per vendor per feature.
  • 58 isinstance gates on concrete pool classes across srt/.
  • 7 of 8 attention backends check concrete SWAKVPool; only triton_backend.py uses BaseSWAKVPool. A vendor's own SWA composite is treated as a flat pool.

So this PR decouples the construction side and leaves the consumption side untouched. Honest framing: it moves ctor-kwarg coupling (fails at startup, TypeError, loud) to method/attribute/identity coupling (fails at first PD transfer or hicache attach, late). Better failure timing, same class of problem. build_kv_pool's docstring now says to subclass the concrete pools rather than implement KVCache, so vendors do not discover this by breakage.

Proposed follow-ups (stacked, alongside the NPU PR)

  1. Dogfood the seam — factor the in-tree dispatcher into build_in_tree_kv_pool(request) and have in-tree platforms consume the same request. Nothing in-tree reads KVPoolRequest today, which is exactly why layout and size could be wrong unnoticed. Dogfooding makes drift impossible by construction and lets a vendor delegate (pool = build_in_tree_kv_pool(req); return MyWrapper(pool)) instead of re-deriving things like enable_alt_stream=not get_disagg().enable_pdmux. It is also the step that makes RFC [Feature] Introduce hardware plugin system #20372's end state mechanical: in-tree platforms become real implementors, Optional goes away, and is_out_of_tree() leaves the configurator.
  2. SWA leaf injection — give SWAKVPool full_kv_pool=/swa_kv_pool= parameters and issue two leaf requests, the way HybridLinearKVPool already works in this PR. Principle: vendors build leaves, core builds composites.
  3. Narrow the consumption surface — migrate isinstance gates to BaseSWAKVPool/protocols; drop NPUMLATokenToKVPool from disaggregation/utils.py.
  4. A conformance kit vendors can run in their own CI, driving _make_kv_pool_request over a model matrix and asserting against real in-tree pool attributes. That is the test that would have caught both review bugs mechanically.

Deliberate non-changes

  • Kept the frozen scalar struct rather than a live view object — a copy is right at a plugin boundary; dogfooding, not aliasing, is what stops it drifting.
  • Did not split into a tagged union (MHAPoolRequest | MLAPoolRequest | ...). Literal + explicit quant_scheme gets most of the value at a fraction of the diff. Worth doing while there is still exactly one consumer, if reviewers prefer it.
  • require_out_of_tree_impl's message is over-broad for get_paged_allocator_cls: PagedTokenToKVPoolAllocator is pure index arithmetic, so a vendor can legitimately return it. Kept for consistency with pre-PR behavior; happy to soften.

Comment thread python/sglang/srt/mem_cache/allocator/hisparse.py Outdated
The merge broke every CPU, GPU and jit-kernel shard with

    ImportError: cannot import name 'MultiPlatformOp'
    from 'sglang.srt.layers.utils'

This branch drops that re-export on purpose: it was the only reason
sglang.srt.layers.utils -- imported by dozens of model files for
PPMissingLayer / get_layer_id -- pulled in sglang.kernels.fused_op. The
supported spelling is the module path, and the alias module stays.

Since then main grew two importers of the old spelling, dsa_indexer_kpool
(sgl-project#39273) and qsa_indexer (sgl-project#39721). Neither conflicts textually, so the merge
took both sides cleanly and the break only showed up in CI.

The blast radius was wider than the two files because
import_model_classes catches every exception and logs a warning, so any
model whose chain reached either one silently vanished from the arch table
and surfaced much later as "KimiLinearForCausalLM has no SGlang
implementation" in the unified-memory e2e test.

Verified: every sglang.srt module now imports except for absent optional
deps (polars, numba, ray, vllm, cutlass), and all 36 attributes that
sglang.srt.platforms consumers read off current_platform resolve on
SRTPlatform.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@alexnails alexnails added bypass-fail-fast CI: a failing job no longer aborts its siblings (lint still gates) and removed bypass-fastfail labels Oct 3, 2026
alexnails and others added 2 commits October 3, 2026 09:48
The previous commit hoisted the num_new_pages check above the kernel
launch to match alloc_decode. That silently gave up a property the code
documents: get_num_new_pages works on CPU tensors, its docstring says,
"to avoid blocking kernel launch". Putting it first puts that CPU work on
the critical path ahead of every extend launch instead of overlapping it.

The check is only needed when the pool might actually be short, and the
function already computes a quantity that answers that:
extend_num_tokens // page_size + bs + 1, the existing defrag trigger, is a
strict upper bound on num_new_pages -- one request's page count grows by
less than extend_len / page_size + 1, so the sum stays under it. When that
bound fits in free_pages the allocation is provably safe and the launch
goes first, exactly as before. Only when it does not do we pay for the
exact count up front, which is the case the review comment was about: a
short pool makes the Triton kernel read past free_pages and the native one
clamp to duplicate rows.

Verified the bound by enumeration: exhaustive over page sizes 1-16 and one
and two requests, plus 400k random batches up to bs=64, tightest slack
exactly 1, matching bound = exact + 1 in the worst case.

Whether this ordering is what moved test_vlm_serving_flashinfer (5734 and
5748 against a 5940 floor, twice) is unproven -- that lane has no main
baseline since 2026-09-24, and its threshold was already cut from 6900 to
5940 for spread. The change was wrong on its own terms regardless.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The merge that brought main's MpsSRTPlatform in beside this branch's
MusaSRTPlatform left two pre-commit failures in platforms/__init__.py:
the mps import sorted after musa, and only one blank line before
_is_mps_available. isort and ruff-format respectively.

Formatting only; the probes and the fallback order are untouched.

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

@yeahdongcn yeahdongcn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

MUSA changes look good to me. Thanks!

One conflict, in platforms/interface.py, where main re-added two methods
this branch had replaced and added a third that is new.

Kept support_mamba_cache_extra_buffer: it is live, read by
arg_groups/mamba_hook.py and covered by test_mamba_extra_buffer_platform
and test_platform_interface.

Dropped the re-added support_cuda_graph and support_piecewise_cuda_graph.
This branch migrated every consumer of those to capabilities.graph_capture
and capabilities.piecewise_graph, and a grep over python/ and test/ finds
no caller of either outside the definition, so re-adding them would only
restore a second way to ask the same question.

Checked the merge for the symbol one side drops while the other starts
using it: no stale caller of the removed hooks, no new importer of the
dropped MultiPlatformOp re-export, every sglang.srt module imports except
for absent optional deps, and all 38 attributes read off current_platform
resolve on SRTPlatform -- 38 now, up from 37, the new one being
support_mamba_cache_extra_buffer.

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

Copy link
Copy Markdown
Collaborator

/rerun-failed-ci

@alexnails
alexnails merged commit c892301 into sgl-project:main Oct 8, 2026
382 of 420 checks passed
Shunkangz added a commit to Shunkangz/sglang that referenced this pull request Oct 8, 2026
The main merge brought in sgl-project#35594, which threads the resolved MLA pool
class into _build_mla_kv_pool. Expect that kwarg in the fallback-path test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
vincentzed added a commit to vincentzed/sglang that referenced this pull request Oct 8, 2026
main passes the DSA pool class into _build_dsa_kv_pool (sgl-project#35594). The DCP
pool sizing keeps its branch and uses that class, and the DCP pool test
passes its CPU pool through the new argument.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

amd bypass-fail-fast CI: a failing job no longer aborts its siblings (lint still gates) documentation Improvements or additions to documentation hicache Hierarchical Caching for SGLang jit-kernel memory-pool mthreads npu piecewise-cuda-graph run-ci CI: run the baseline test suite on this PR run-ci-extra CI: also run the extra suite (requires run-ci)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants