Repository navigation
Conversation
|
Verified the head of this PR against the issue chain. Your correction to my proposal is the right one: One thing missing: nothing pins the derivation this fix stands on. A small unit test next to the other runtime_context tests would do it — feel free to take this verbatim: def test_derive_attention_widths_matches_dp_attention_arithmetic():
from sglang.srt.runtime_context import derive_attention_widths
# DP attention on: attention shards by 1 under tp16/dp16 (#38202).
assert derive_attention_widths(
tp_size=16, attn_cp_size=1, dp_size=16, enable_dp_attention=True
) == (16, 1)
# DP attention off: the full tp width is the attention width.
assert derive_attention_widths(
tp_size=16, attn_cp_size=1, dp_size=16, enable_dp_attention=False
) == (1, 16)
# Context parallelism stacks on top of the DP split.
assert derive_attention_widths(
tp_size=16, attn_cp_size=2, dp_size=8, enable_dp_attention=True
) == (8, 1)I can't push to your fork, so committing the test yourself or applying the patch above is equally good; happy to open a PR onto your branch instead if you prefer that route. [AI-assisted] |
|
Thank you for your complement ! Just open a PR to my branch if you like. |
|
I couldn't push here directly (no write access to the fork), so the commits are on my fork at What my commit changes, and why: The reservation now reads The bigger reason is drift, not style. Regression test ( Verification: the whole |
|
The file moved again since my last note (cell-size resolution now sits behind You can pull it straight in: or cherry-pick |
|
Heads-up on the CI state: the branch predates the MIN_BASE_SHA maintenance gate (8854857), so check-changes will fail until it is rebased onto current main. I do not have push access to the fork branch anymore, so that rebase needs the author's hand. The fix itself targets #38202: the draft KV pool reservation scales by attn_tp_size instead of tp_size, removing the attn_dp_size-fold over-allocation that OOMed DP attention with DFLASH/DSPARK enabled, verified against the reporter's repro at the time. Note #39225 attacks the same bug; whichever lands first is fine. Could a maintainer add the run-ci label once the branch is rebased so CI can actually exercise it? |
|
Thanks for the diagnosis and the first fix here. Since this branch has been quiet for a few weeks and main has moved on (the |
Fixes #38202
Problem
With DP attention on, the target KV pool eats essentially the whole
--mem-fraction-staticbudget and the draft worker OOMs allocating its own pool when serving Kimi-K3.A DFLASH/DSPARK draft never sizes itself:
KVCacheConfigurator.configureshort-circuits for draft workers and reuses the target's resolved
MemoryPoolConfig, then allocates its owntoken_to_kv_poolat the target'smax_total_num_tokens. Room for it is reserved by inflating the target'sper-token cell size in
pool_configurator.py, which bottoms out in a singlehead-count input:
Every KV pool builder, however, shards by
attn_tp_size:kv_cache_configurator._build_mha_kv_poolkv_cache_configurator._build_hybrid_linear_kv_poolpool_configurator._compute_cell_sizeWith
--tp-size 16 --dp-size 16 --enable-dp-attention,attn_tp_sizeis16 // 16 // 1 = 1while the reservation still divides by 16, so the draft termis under-counted by
min(attn_dp_size, total_kv_heads). Without DP attention thetwo are equal, which is why this went unnoticed. This is the only place in the KV
sizing path (
model_executor/,mem_cache/) that reads the rawtp_size.Fix
Derive the attention width from the configured leaves via
derive_attention_widths, and threadserver_argsin to reach them.Reading
get_parallel().attn_tp_sizedirectly does not work here, and failsin a way worth calling out: this runs in
ModelRunner.init_spec_aux_hidden_state(),which is invoked before
init_torch_distributed(), so the attention TP group doesnot exist yet. The property raises
attention tensor model parallel group is not initialized, the surroundingexcept Exceptionswallows it, and the budget silently degrades to thelayer-count fallback — turning a wrong reservation into no reservation at all.
derive_attention_widthsis a pure function over the config leaves and isalready used this way by
dp_attention.compute_dp_attention_world_info.Test plan
Manually verified on a PD decode node (Kimi-K3 target + DSPARK draft,
--tp-size 16 --dp-size 16 --enable-dp-attention,--mem-fraction-static 0.90):before the fix the draft pool OOMs during
alloc_memory_pool; after it, the nodestarts cleanly.
Not addressed here
The
except Exceptionin_resolve_dflash_draft_cell_sizedegrades to azero-byte draft reservation on the hybrid-SWA path:
HybridSWAPoolConfigurator'slayer-count fallback only covers EAGLE/STANDALONE, so a hybrid-SWA DFLASH/DSPARK
target reserves nothing when resolution fails. That is what turned the bug above
into an OOM rather than an error. Happy to fold a fallback (or a loud failure)
into this PR if reviewers prefer.
CI States
Latest PR Test (Base): ❌ Run #34024742578
Latest PR Test (Extra): ❌ Run #34024742435
Latest PR Test (AMD ROCm 10): ❌ Run #34024742593