Skip to content

[DCP] Take the draft pool's page granularity from its allocator, and assert it - #3

Closed
kpham-sgl wants to merge 5 commits into
dcp-reads-via-get-parallelfrom
kpham/proto-loc-space-from-allocator
Closed

kpham-sgl wants to merge 5 commits into
dcp-reads-via-get-parallelfrom
kpham/proto-loc-space-from-allocator

Conversation

@kpham-sgl

@kpham-sgl kpham-sgl commented Aug 7, 2026

Copy link
Copy Markdown
Owner

Draft, stacked on sgl-project#33925 (dcp-reads-via-get-parallel) so the diff shows only the delta. Two commits.

Why this belongs with sgl-project#33925

loc_space_scale re-derives, from dcp_size, a shape the allocator object already carries — and the draft is handed that allocator:

value
allocator built by the target size * dcp_size, page_size * dcp_size
draft pool via loc_space_scale size * dcp_size, page_size * dcp_size

Equal by construction. So the draft can read the shape instead of recomputing it:

return allocator.page_size // get_schedule().page_size

That matters for sgl-project#33925 specifically. On main today loc_space_scale reads server_args.dcp_size, which a get_parallel()-scoped override cannot reach. sgl-project#33925 moves it onto get_parallel() — after which anything that neutralizes DCP for a draft (e.g. a draft-scoped context, under discussion for sgl-project#32858) would silently compute page_size * 1 and desync the draft pool from the allocator it shares with the target.

After this change the draft pool path reads no DCP value at all. The only remaining attn_dcp_size reads in the configurator are in allocator construction, which runs on the target (if token_to_kv_pool_allocator is None; the draft's is passed in).

The assert

PagedTokenToKVPoolAllocator.clear() skips page 0, so the free list ends at page num_pages whose base is exactly size — it hands out slots up to size + allocator.page_size. A pool over-allocates only its own page_size past size. A replicated draft indexes those locs untranslated, so if its pool pages smaller than the allocator does, that final page's tail addresses rows which do not exist:

allocator page N spans   [size, size + allocator.page_size)
pool is valid to          size + pool.page_size
                          |---- exists ----||---- does not ----|

Silent, and only reachable at near-full occupancy — which is why this class of fault reads as stochastic. Under an 8-way DCP group with a 64-slot base page that is 448 nonexistent rows per request that reaches the last page.

The target is safe by arithmetic accident, not by design. It translates (loc // dcp_size), which scales the allocator's overshoot down by exactly the same factor. Only the raw-indexing draft is exposed.

Commit 1 makes the mismatch unreachable by construction; commit 2 asserts it at boot so a future independent derivation of pool_page_size fails loudly instead of corrupting KV again. Layer 1 alone is only as durable as nobody re-deriving — and the original defect was a derivation that looked right.

Behaviour change to review

SWATokenToKVPoolAllocator and HiSparseTokenToKVPoolAllocator are built with the unscaled page_size; only PagedTokenToKVPoolAllocator is widened. So for a draft sharing one of those, loc_space_scale now returns 1 where it previously returned dcp_size. That follows the allocator's actual shape, so I believe it is more correct — the old code scaled against an unwidened allocator — but it is a real change and I have not established whether hybrid-SWA + DCP + draft is reachable.

Verified on 8-GPU DCP8 hardware

Red-then-green, on a TP8/DCP8 deployment with a speculative draft worker.

REDpool_page_size reverted to the unwidened get_schedule().page_size:

AssertionError: DRAFT_POOL_PAGE_MISMATCH pool=64 allocator=512 reaches=<N+512> valid_to=<N+64>

Fires at draft pool allocation, before any request is served.

GREEN — the change as written: assert silent, draft pool derived from the allocator matches the value the dcp_size derivation produced, and a sustained high-occupancy serving benchmark completed all requests with zero server errors, at throughput in line with the previously validated fix. The run held at token usage 0.97 — the occupancy regime where this fault appears — for most of its duration.

CPU suitetest_dcp_layout_unit.py: 10 passed, 0 failed.

Running the tests caught two bugs that reading did not:

  1. Mine. pool_page_size composes loc_space_scale, so the SimpleNamespace stand-in raised AttributeError. loc_space_scale(draft) == 4 had already passed — production logic right, harness wrong. Fixed in 2da12ff985.
  2. Pre-existing, from config: route DCP topology reads through get_parallel() sgl-project/sglang#33925. test_configurator_scales_only_the_virtual_dcp_allocator failed 1024 != 4096: 1a45b4dd5d moved allocator construction to get_parallel().attn_dcp_size while the test injects only a server_args stand-in, so with no DCP group it built an unwidened allocator. Reproduced at the migration tip with this PR's commits absent. Fixed in 2d3be98773; that fix belongs in config: route DCP topology reads through get_parallel() sgl-project/sglang#33925. Worth asking why CI missed it — the file is registered base-a-test-cpu.

Still not verified

  • Whether hybrid-SWA + DCP + draft is reachable (the SWA/HiSparse behaviour change above is unexercised).
  • The hardware runs used the deployment's own base commit, not this branch's base — the two pin different sglang-kernel minimums.

🤖 Generated with Claude Code

kpham-sgl and others added 5 commits August 6, 2026 19:23
loc_space_scale re-derived, from dcp_size, a shape the allocator object already
carries -- and the draft is handed that allocator. The target builds it widened
(size * dcp_size, page_size * dcp_size), so allocator.page_size //
get_schedule().page_size is the same number by construction.

Effect: the draft pool path no longer reads DCP at all. The only remaining
attn_dcp_size reads in the configurator are in allocator construction, which
runs on the target (the draft's allocator is passed in). So "DCP is off for
drafts" becomes a complete ground truth for pool shape, and a draft-scoped DCP
context cannot desync the pool from the allocator it shares.

Also follows an allocator that widens differently: SWA / HiSparse are built with
the unscaled page_size, where the old dcp_size derivation scaled anyway.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The allocator's free list ends at page num_pages, whose base is exactly `size`
(clear() skips page 0), so it hands out slots up to size + allocator.page_size.
A pool over-allocates only its own page_size past size. A replicated draft
indexes those locs untranslated, so the two page sizes must agree or the last
page's tail addresses rows that do not exist -- silent, and only at near-full
occupancy: with an 8-way DCP group and a 64-slot base page that is 448
nonexistent rows.

Reading the shape off the allocator (previous commit) makes the mismatch
unreachable by construction; this asserts it at boot so a future independent
derivation of pool_page_size fails loudly instead of corrupting KV again.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pool_page_size composes loc_space_scale, so a SimpleNamespace only satisfies the
leaf reads and raises AttributeError on the composition. Verified on hardware:
loc_space_scale already returned 4 -- the production logic was right, the
harness was not.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1a45b4d moved allocator construction from self.server_args.dcp_size to
get_parallel().attn_dcp_size, but this test injects a server_args stand-in only.
With no DCP group attn_dcp_size is 1, so the dcp_size=4 iteration built an
unwidened allocator: "AssertionError: 1024 != 4096", reproducible at
the migration tip with the later commits absent.

Override the cause instead of the effect, per the runtime-context idiom.

Belongs in sgl-project#33925; carried here so this branch's suite is green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The invariant and the arithmetic are what a reader needs; the originating
deployment is not.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@kpham-sgl
kpham-sgl force-pushed the kpham/proto-loc-space-from-allocator branch from 2d3be98 to ca419d9 Compare August 7, 2026 05:52
@kpham-sgl kpham-sgl closed this Aug 7, 2026
@kpham-sgl

Copy link
Copy Markdown
Owner Author

Superseded by sgl-project#33955 — same commits, opened in the main repo and stacked on the upstream mirror of sgl-project#33925's branch.

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