Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
ccce69a to
6ef9d9f
Compare
Conflicts: python/sglang/kernels/ops/attention/triton_gdn_fused_proj.py
|
@amd-bot ci-status |
CI Status for PR #33068Merge verdict: ❌ Do not merge on this CI run — the signal is incomplete and the PR's own tests were largely not executed. All 12 red X's are the CI rate-limit gate (not code failures): user Caution PR CI is incomplete AND this PR's changed code is not verified. The rate-limit gate fast-failed 8 workflows (Base, Extra, MUSA, NPU, XPU, Xeon, Arm64, Extra-AMD) — those downstream jobs are not tested. The PR's two new PR-CI tests were affected:
Green here would prove nothing. After the cooldown expires (~12:18Z, 120 min from 10:18:34Z), re-run / update the branch so the full pipeline executes. For high-priority full signal, the Changed files: Executed CI failure attribution: AMD: 0 executed failures (all gate-blocked or pending) · Others: 0 executed failures (all gate-blocked). All 12 red checks = 1 root cause (rate-limit gate). Still running: Lint, PR Test (MLX), PR Test ROCm 7.2 (AMD) — do not call this run green. Gate / cascade failures (collapsed to one root cause)
(Verified the rate-limit message directly in the Base and MUSA gate logs; the rest show the same Details / what to do before merge
Generated by amd-bot using Claude Code CLI |
|
@amd-bot ci-status |
CI Status for PR #33068Merge verdict: This PR's changed code is well-exercised and green — all three new tests pass, including the mi35x e2e test that explicitly enables the default-off Note Changed paths ARE exercised by PR CI. The fused feature is default-off ( Changed files: Executed CI failure attribution: AMD: 4 failures (0 related) · Others: 3 failures (0 clearly related, 1 possibly) — AMD Executed Failures
Other Executed Failures
Details / what to do before merge
Generated by amd-bot using Claude Code CLI |
|
@amd-bot ci-status |
CI Status for PR #33068Merge verdict: No executed failure is attributable to this PR, and — most importantly — this PR's changed code is exercised and green on both NVIDIA and AMD (MI300 + MI35x). All 8 executed job failures are in unrelated backends (DeepSeek-V4, hicache, NIXL disagg, radix-cache, MLA/lean-attention, openai-completion) or are pure infra (docker-pull, UCX). However, PR CI is incomplete: the fast-fail cascade skipped every "extra" pipeline (AMD-extra, NVIDIA-extra, NPU base-c) and the NPU base-b jobs were cancelled — so full signal has not been obtained. Note The PR's own code paths are covered and all three new tests passed on this head SHA:
Caution PR CI is incomplete due to fast-fail. Because unrelated stage-b/base-b tests failed, these downstream pipelines were skipped / cancelled and are NOT tested: AMD extra ( Changed files: Executed CI failure attribution: AMD: 7 failures (0 related) · Others: 1 failure (0 related). Extra/NPU pipelines: skipped/cancelled by fast-fail (not counted). AMD Executed Failures
Other Executed Failures
Details / what to do before merge
Generated by amd-bot using Claude Code CLI |
|
@yuan-luo, could you help review this PR, thanks. |
|
@amd-bot ci-status |
CI Status for PR #33068Merge verdict: The PR's own changes are well-covered and green — the MI35X fused-path e2e gate and the NVIDIA kernel column-view test both ran and passed on this head SHA. All 15 executed failures are in unrelated subsystems (mem cache, radix cache, disaggregation, MoE-EP, DeepSeek-V4, quark-mxfp4, NPU perf/diffusion, MLX, CPU); none touch the Qwen3.5 GDN in_proj code, and the new fusion is off by default + HIP-gated, so it is inert everywhere it wasn't explicitly enabled. CI is complete — the Note This PR's changed code is exercised by PR CI and passes. The fused path ( Changed files: Executed CI failure attribution: AMD: 8 job-failures (0 related) · Others: 5 failures (0 related) · plus 9 AMD Executed Failures
Other Executed Failures
Cascade only (not independent failures): Details / what to do before merge
Generated by amd-bot using Claude Code CLI |
e1226df to
08d9735
Compare
|
@amd-bot ci-status |
CI Status for PR #33068Merge verdict: Do not merge yet — PR CI is incomplete. The entire NVIDIA pipeline ( Caution PR CI is incomplete and the kernel-level change is unverified. Note The core feature is covered on AMD and passed: the new e2e gate Changed files: Executed CI failure attribution: AMD: 11 failures (0 related) · Others (NPU/MLX/CPU): 4 failures (0 related) · NVIDIA AMD Executed Failures
Other Executed Failures
(Cascade Details / what to do before merge
Generated by amd-bot using Claude Code CLI |
Motivation
In Qwen3.5 GDN layers the input projection is issued as two separate GEMMs,
in_proj_qkvzandin_proj_ba. Both read the same activation, so on checkpoints that quantize both to the samescheme they can be served by one wider GEMM. Keeping them apart costs an extra kernel launch, an
extra activation-quantization pass and an extra epilogue per GDN layer per decode step — overhead
that is a significant share of decode time at low-to-mid concurrency.
This became actionable with the AMD Qwen3.5 MXFP4+AttnFP8 "V2" line, which quantizes
in_proj_bato FP8; on V2 all four shards resolve to one scheme and the merge is eligible.
The optimization is AMD-only and default-off. To keep that from becoming a correctness question for
everyone else reading
qwen3_5.py, all of it lives in its own module rather than inline in theshared model file — see below.
Modifications
New
python/sglang/srt/models/qwen3_5_gdn_in_proj_merge.pyholds the entire feature: theSGLANG_GDN_FUSE_QKVZBA+ HIP gate (ENABLED),build(),split_output(),stacked_params_mapping()andPACKED_MODULES_MAPPING.build()returnsNonewhenever themerge does not apply — flag off, non-HIP, LoRA enabled, or a checkpoint whose four shards
disagree on a quantization scheme — and the caller then constructs
in_proj_qkvzandin_proj_baexactly as before. Reviewers on other platforms can therefore establish that nothingchanges for them by reading one gate, and the module is dead code off ROCm.
build()assembles the four shards (qkv,z,b,a) into a singlein_proj_qkvzbaviaMergedColumnParallelLinear, padding each rank's N up to the next multiple of 128 for the aitera8w8 tile (correctness only needs
N % 16 == 0); the padding rows are zeroed and no checkpointtensor reaches them. A scheme mismatch raises
ValueErrorinside the layer construction, which iscaught and logged once before falling back. Mixed-precision checkpoints are therefore untouched —
on V1, where
in_proj_a/bstay bf16, the model loads exactly as before.python/sglang/srt/models/qwen3_5.pynow delegates instead of implementing.create_qkvzba_proj()is a thin wrapper over
build(),_forward_input_proj()callssplit_output(), the fourload_weights()implementations splice instacked_params_mapping(self), andpacked_modules_mappingsplices inPACKED_MODULES_MAPPING. That mapping is{}off HIP, sonon-AMD platforms never see the merged parameter name at all.
python/sglang/kernels/ops/attention/triton_gdn_fused_proj.py: parameterize thesplit/reshape/cat kernel by
QKVZ_STRIDE/BA_STRIDErather than deriving the row stride fromlogical widths, so it can read column slices of a wider, padded projection.
split_output()returns views rather than copies, so the alignment padding falls away without touching memory.
No
communicator.pychange, and_forward_input_proj_fused_quant_amdis untouched: the mergedGEMM is dispatched at the top of
_forward_input_proj, where_select_fused_ar_input_for_linearhandles a plain tensor and a fused-AR tuple alike, so one branch covers both entry paths.
Tests: a GSM8K A/B accuracy gate on MI35x (below), plus a case in the existing decode-fusion
kernel test covering the strided, offset column views the merge produces.
CI coverage
test/registered/e2e/models/test_qwen35_gdn_in_proj_fusion_mi35x.pyruns two TP2 servers inparallel on one 8-GPU MI35x host (
stage-c-test-large-8-gpu-amd-mi35x): the merged arm withSGLANG_GDN_FUSE_QKVZBA=1against the default separate-projection arm, and requires the merged armto hold GSM8K accuracy. Since the feature is default-off, this is the only CI that executes it at
all.
The fused arm's server log is checked for the fallback message, because a checkpoint whose shards
disagree would silently degrade the test into baseline-vs-baseline and pass. TP=2 rather than TP=4:
TP=4 narrows
in_proj_bato 32 columns, whichgemm_a8w8_bpreshufflehas no kernel for once Mreaches 256.
test/registered/kernels/ops/attention/test_gdn_decode_fused_proj_conv.pygainstest_padded_merged_projection_column_views_are_supported, which feeds the decode fusion a paddedmerged projection —
stride(0)wider than the slice, nonzero storage offset on thebaview — andrequires bitwise equality against densely packed inputs.
Accuracy Tests
gsm8k 5-shot, full 1319 questions,
lm_evallocal-chat-completionswith--apply_chat_templateagainst/v1/chat/completions, server run with--reasoning-parser qwen3and
--default-chat-template-kwargs '{"enable_thinking": false}'.Checkpoint: AMD Qwen3.5-397B-A17B-MXFP4-AttnFP8-V2 (PR#4 revision), 2× MI355X, TP=2.
SGLANG_GDN_FUSE_QKVZBA0(separate, today's behaviour)1(merged)The merged arm is 0.6 pp higher, well inside the ±0.4–0.5 pp standard error of each measurement,
i.e. the two arms are statistically indistinguishable. No accuracy regression.
Note for anyone reproducing: Qwen3.5 V2 is a thinking model. Grading it on
/v1/completionsscores the raw
<think>text and yields a spurious ~0.82–0.83 for both arms; the chat +thinking-off recipe above is the one that reflects real quality.
Speed Tests and Profiling
bench_serving, random dataset, OSL 1024,--random-range-ratio 0.8,--num-prompts 10×cc,request_rate=infwith--max-concurrency, seed 0. Same checkpoint/hardware as above. Server:--attention-backend aiter --kv-cache-dtype fp8_e4m3 --page-size 16 --chunked-prefill-size 8192 --disable-radix-cache --enable-aiter-allreduce-fusion --enable-dense-fp8(effectivemem_fraction_static0.578,max_running_requests636); the 1k regime adds--enable-mixed-chunk.TPOT figures are medians.
8k context (ISL 8192, OSL 1024)
1k context (ISL 1024, OSL 1024)
All 20 cells (2 regimes × 5 concurrencies × 2 arms) favour the merged arm on both TPOT and
throughput; tok/s columns are total input+output throughput, and output-only moves by the same
percentage. The gain is largest at low-to-mid concurrency, where the per-GEMM launch and epilogue
overhead removed by the merge is the biggest share of decode time, and shrinks as concurrency grows
and the GEMMs become compute-bound. An independent earlier repeat of this sweep reproduced the same
deltas within a few tenths of a percent.
Absolute numbers vs the previous V2 baseline
For context beyond the A/B, the same sweep against our July 28 V2 baseline (aiter #4017+#3986 on
the published
Qwen3.5-397B-A17B-MXFP4-AttnFP8-V2), run with an identical harness and identicalserver settings. Medians, matching how that baseline was recorded:
Every cell improves on every metric. Note these are medians; under
request_rate=infthe initialburst gives mean TTFT a long tail (at 8k/cc64 the baseline's mean TTFT is 1358 ms against a median
of 344 ms), so mean and median must not be compared across runs.
Merge engagement was verified, not assumed. At the time these sweeps were taken nothing logged
the merge, so both harnesses used the KV-cache token count as a signature: the padded merged
projection carries ~12 MB more weights per rank, so at a fixed
mem-fraction-staticthe merged armhas slightly less room for KV. The perf sweep reported 4,505,008 → 4,495,728 tokens and the accuracy
run 11,774,352 → 11,765,072 — the same −9,280 delta in two independently configured runs. Identical
counts would have meant the flag never took effect. The CI gate now checks the fallback log line
directly instead of relying on this signature.
Checklist
Review and Merge Process
/tag-and-rerun-ci,/tag-run-ci-label,/rerun-failed-ciCI States
Latest PR Test (Base): ❌ Run #34545848390
Latest PR Test (Extra): ❌ Run #34545847979
Latest PR Test (AMD ROCm 10): ❌ Run #34545848258