Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
/tag-and-rerun-ci |
fzyzcjy
changed the title
Classification: CI infra (crates.io download failure), not a code failure. Root cause job: That failure tripped No test actually ran and failed. Next step: wait for the current run to complete, then Also note the |
|
/rerun-failed-ci |
|
🤖 Posted by an AI coding agent (Claude Code) on behalf of @fzyzcjy. CI triage for run 30529599065 (head Not related to this PR
Pre-existing breakage on
|
An idle DP rank reports global_num_tokens == [0], so max_len and sum_len
are both zero and the communication-cost heuristic evaluates 0 >= 0 and
returns MAX_LEN. Communication cost is identical either way for an empty
batch, but MAX_LEN additionally sends the rank through the idle -> extend
fabricated-row conversion in ForwardBatch, which builds a dummy request
with extend_seq_lens == [0]. LogitsProcessor then computes
last_index = cumsum([0]) - 1 = [-1] and indexes a zero-row hidden_states:
File "python/sglang/srt/layers/logits_processor.py", line 526
pruned_states = hidden_states[last_index]
IndexError: index is out of bounds for dimension with size 0
Reproduced on 4xH200 with
test/registered/ep/test_mooncake_ep_small.py::TestPureDP
(--tp 4 --dp 4 --enable-dp-attention --elastic-ep-backend mooncake):
every non-zero rank crashed during server warm-up. With this change the
same test passes (2 passed, 1 skipped).
The early return is placed after the max_len_with_idle branch so the
hybrid-SSM MAX_LEN path is unaffected.
Hybrid-SSM models reach the fabricated-row idle conversion in ForwardBatch unconditionally, and that conversion asserts the rank is empty. A non-empty extend batch that selects MAX_LEN therefore fails with "extend-idle conversion expects an empty rank" during server warm-up (observed on Qwen3-Next-80B-A3B-Instruct-FP8 with --tp 8 --dp 8). Restrict them to the pre-existing behaviour instead of blocking the heuristic for every model: MAX_LEN when a rank is idle (the path the conversion is written for), SUM_LEN otherwise. Everything else now uses the communication-cost heuristic. A TODO records what has to be fixed before hybrid-SSM can join.
|
/tag-and-rerun-ci extra |
|
/tag-and-rerun-ci extra |
The carve-out gated on dp.max_len_with_idle, which is only set when
hf_config exposes hybrid_override_pattern. ForwardBatch decides whether to
run the fabricated-row conversion with mambaish_config() instead, and the
two predicates disagree: Qwen3-Next, Qwen3.5, Kimi-Linear, LFM2 and the
other class-based families are mambaish but have no
hybrid_override_pattern, so they fell through to the communication-cost
heuristic, picked MAX_LEN for a mixed decode/extend global batch and hit
File "python/sglang/srt/model_executor/forward_batch_info.py", line 1331
self.seq_lens.shape[0] == 0
AssertionError: extend-idle conversion expects an empty rank
on the ranks that were decoding while another rank prefilled.
Materialize a dp.hybrid_ssm flag from the same predicate ForwardBatch uses
(as a superset of max_len_with_idle, so no hybrid model can lose its
previous mode) and gate on that. Inside the branch the mode choice is
unchanged, so every hybrid-SSM family keeps exactly its mainline
behaviour; only non-hybrid models get the heuristic.
MAX_LEN pads every DP rank up to the global max token count, and those pad rows are only handled on the paths explicitly written for them: the idle-rank fabricated-row conversion in ForwardBatch, mask_dp_pad_moe_topk_ids for MoE topk, and num_token_non_padded (which is None unless moe_ep_size > 1). Where none of those apply, the pad rows run the model and their outputs are not discarded. GLM-5.2-FP8 with --tp 8 --dp 8 --enable-dp-attention plus HiSparse scored 0.656 on gsm8k against a 0.94 threshold once extend batches were allowed to select MAX_LEN. This is the accuracy failure #10414 originally fixed; the earlier padding-mode experiments missed it because they never covered this runner/offload combination. Require min(global_num_tokens) == max(global_num_tokens) for extend batches instead. MAX_LEN then rewrites global_num_tokens to values it already had, so not a single pad row is materialized and the mode is a pure choice of collective (all_gather + reduce_scatter over symmetric memory instead of all_reduce) -- which is exactly the uniform-prefill case the 16% regression was measured on. Skewed batches keep SUM_LEN.
Cover the three cases that broke while reverting #10414: an all-zero batch, a skewed extend batch (idle rank or not), and the hybrid-SSM families whose fabricated-row conversion only accepts an empty rank. Also pin the two behaviours the revert is meant to deliver: a uniform extend batch selects MAX_LEN, and decode batches keep the communication-cost heuristic.
|
🤖 Posted by an AI coding agent (Claude Code) on behalf of @fzyzcjy. CI status on The two failures this PR did cause are fixed, and CI confirms itOpting into the extra workflow surfaced two real regressions from the first version of this branch:
Both come from the same mechanism: The fix restricts extend batches to On this head every
The four remaining CUDA failures
The non-CUDA reds ( |
# Conflicts: # python/sglang/srt/layers/dp_attention.py
The previous restriction required min(global_num_tokens) == max(global_num_tokens) for extend batches. Real prefill batches are almost never exactly uniform, so that condition selected SUM_LEN nearly always and gave back the throughput this branch is meant to recover. Measurements on Qwen3-8B / 8xH200 separate the two cases: padding an already-active rank up to the global max costs ~2% of extra rows, while padding an idle rank inflates every rank's gathered buffer to max_len * dp_size and multiplies prefill GEMM work by 6.5x. The idle rank is also the only case that reaches the fabricated-row conversion in ForwardBatch, which is where the extra-CI failures came from. So guard on min(global_num_tokens) == 0 and let merely uneven batches fall through to the communication-cost heuristic.
|
🤖 Posted by an AI coding agent (Claude Code) on behalf of @fzyzcjy. Writing down what a naive revert of #10414 actually breaks, with the evidence for each item, since this branch has now hit all of them in CI. A naive revert is deleting the guard so extend batches fall back to the communication-cost heuristic: if is_extend_in_batch:
return DpPaddingMode.SUM_LENThat heuristic then selects Why this was never caught by ordinary testing
rules = [
# MLA prefill takes a different attn-forward path under BCG.
("MLA attention", lambda: self.use_mla_backend()),
1. Uninitialized attention output on padded rowsThis is the bug #10414 was originally titled after: "Fix cutlass moe accuracy drop caused by attention UB from DP padding mode" (72dfa96). The mechanism is visible in the padding code. For a non-empty extend rank,
So The rows in Observed failure: One hypothesis that turns out not to apply: padded 2. Hybrid-SSM ranks hit the fabricated-row assert
assert self.seq_lens.shape[0] == 0, "extend-idle conversion expects an empty rank"Hybrid-SSM families enter that branch on every rank, not just idle ones. Once Observed failure: Worth noting the predicate matters here: 3.
|
| total GEMM across 8 ranks | input throughput | median TTFT | |
|---|---|---|---|
MAX_LEN behaviour |
2188–2191 ms | ~20.2k tok/s | ~827 ms |
SUM_LEN behaviour |
335–336 ms | ~31.6k tok/s | ~530 ms |
Ratio 6.52×, with <0.5% spread inside each group across four configurations and two independent runs. MAX_LEN eager with no CUDA graph at all lands in the same group as the graph-enabled MAX_LEN configs, so this is the padding mode itself and not a graph artifact. A device-side probe inside the prefill graph confirms the shape directly: seven ranks report real_local_tokens=0 padded_local_tokens=1024, i.e. 1024 real tokens driving 8×1024 rows of attention.
By contrast, on batches where no rank is idle, MAX_LEN padding costs 0.0–2.2% and is genuinely faster than SUM_LEN (AllReduce bytes roughly halve, and AllGather + symmetric memory become available). So the useful split is idle vs non-idle, not uniform vs non-uniform.
Summary
A naive revert is unsafe for three independent reasons (uninitialized attention rows, the hybrid-SSM assert, and the all-empty IndexError) and is also a throughput regression on any batch containing an idle rank. The parts of #10414 that can be given back safely are extend batches where every rank has work, which is where the measured win actually lives.
The underlying fix, for whoever picks it up: teach the fabricated-row conversion to handle a non-empty rank by appending one dummy request of length num_tokens - sum(extend_seq_lens) instead of asserting the rank is empty. That makes the padded rows defined without touching any attention backend, and would let both the hybrid-SSM special case and the idle-rank guard go away.
1 participant
CI States
Latest PR Test (Base): 🚫 Run #30590369825
Latest PR Test (Extra): 🚫 Run #30590369703