[AMD] Enable triton DSA backends and top-k v2 path for glm5.2-fp4 in MI355X - #2915
[AMD] Enable triton DSA backends and top-k v2 path for glm5.2-fp4 in MI355X#2915EricKing626 wants to merge 7 commits into
Conversation
|
Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase For PR verification, add the PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. See GitHub's docs on re-running failed jobs 感谢你的贡献！请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md，然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人，你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板，包括保留英文语句 如需进行 PR 验证，请为此 PR 添加 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档 |
EricKing626
changed the title
Resolve perf-changelog.yaml by keeping main byte-for-byte and re-appending this PR's entry at the tail, matching utils/prepare_perf_changelog_merge.py.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline nit, this run also examined two other candidate issues and ruled them out: the new rocm10 segment in the bumped image tag (every other MI35x entry in configs/amd-master.yaml uses rocm720), and the --dsa-decode-backend triton + --kv-cache-dtype fp8_e4m3 pairing, which the PR description itself flags as resembling a prior memory-fault combination — neither turned up evidence of an actual problem here.
Extended reasoning...
This run has one CONFIRMED inline finding already queued (a run-on comment at glm5.2_fp4_mi355x_sglang_mtp.sh:65, a pure nit), so a full approve/defer writeup is not warranted — the inline comment already signals a human should glance at the PR. Per the narrow exception for findings-present runs, I verified two additional candidate concerns from the ruled-out list against the actual diff: (1) the image tag's rocm10 naming departure from the file's otherwise-uniform rocm720 convention, matching the KLAUD_DEBUG.md warning about invented dated-nightly tags — I could not independently confirm registry existence of the tag due to sandboxed network access, but found no other red flag (the PR author's own description flags this exact same concern for reviewer attention, and the changelog documents it), so it stays an open provenance question rather than a demonstrable bug; (2) the fp8_e4m3 KV-cache + triton DSA-decode combination echoing a previously-hit GPU memory fault per the PR's own risk section — this is a runtime/functional risk that static diff review cannot rule in or out, and the PR's own test plan explicitly lists the smoke test and full sweep as still-pending (not yet green). I also confirmed the perf-changelog.yaml entry is a pure append (no lines removed in the diff) satisfying the append-only invariant, and that the model.container == image convention does not apply to this config schema (no container key exists in amd-master.yaml entries). Given the PR's own test plan shows the smoke test, full sweep, and eval run as unchecked TODOs, and the PR body itself asks a reviewer to confirm the tag family and to verify from server logs that triton/top-k-v2 are actually dispatched rather than silently falling back, a human review pass adding a concrete answer to the tag-provenance question is worthwhile alongside the queued inline nit.
| # The DSA indexer's top-k v2 kernel (default since v0.5.14) is JIT-compiled | ||
| # from CUDA-only source (cooperative_groups.h) and cannot build for gfx950; | ||
| # v1 dispatches to the precompiled HIP op in sgl-kernel (upstream MI355X CI | ||
| # runs DSA models the same way). | ||
| export SGLANG_OPT_USE_TOPK_V2=false | ||
| # PR #36684 & PR #36851 turned the v2 fused top-k on for GLM-5.x on ROCm |
There was a problem hiding this comment.
🟡 nit (optional): the diff replaces only the bottom 3 lines of the SGLANG_OPT_USE_TOPK_V2 comment, leaving the original first line ('...is JIT-compiled') directly followed by the new sentence, so the comment now reads as a broken run-on ('...is JIT-compiled\nPR #36684 & PR #36851 turned the v2 fused top-k on...') instead of a coherent explanation. Fix: rewrite the whole comment block as one coherent explanation of why v2 is now enabled, rather than splicing a new sentence onto a leftover fragment.
Extended reasoning...
Before: lines 64-67 formed one sentence explaining v2 couldn't build for gfx950 (CUDA-only cooperative_groups.h) so v1 was used. The diff deletes the middle/end of that sentence and appends a new sentence, but keeps the unchanged first line 'is JIT-compiled', producing two sentence fragments concatenated without punctuation. A future engineer reading this recipe gets a confusing, ungrammatical justification and may misjudge why/when it's safe to flip the flag back.
Verification: nit: cosmetic comment defect, no functional impact. The diff deleted only the trailing lines of the old SGLANG_OPT_USE_TOPK_V2 comment and appended a new sentence, leaving line 64 (# The DSA indexer's top-k v2 kernel (default since v0.5.14) is JIT-compiled) as an unfinished fragment directly followed by the independent line 65 (`# PR #36684 & PR #36851 turned the v2 fused top-k on for GLM-5.x…
EricKing626
changed the title
1 participant
Summary
--dsa-prefill-backend/--dsa-decode-backend:tilelang→triton(requires sgl-project/sglang PR #30575, commit8a6ab89bf0b90304b3b454a833fcdcaefb777c29)lmsysorg/sglang-rocm:v0.5.16-rocm720-mi35x-20260728→lmsysorg/sglang-rocm:v0.5.19-rocm10-mi35x-20260910SGLANG_OPT_USE_TOPK_V2:false→true(sgl-project/sglang PR #36684, PR #36851 add the gfx950 path) No impact on performance because upstream SGLang already enableModel, runner, and search space (TP4/EP4 HiCache vs TP8/EP1 GPU-resident arms), MTP speculative decoding with the committed golden acceptance length, and the concurrency grids are unchanged.
Unofficial local, MI355X TP4/EP4. Baseline is the
v0.5.19-rocm10-mi35x-20260908image-only bump from #2917, not the CI curve. Δ = this PR / 0908 baseline (higher is better).TODO: TP8/EP1 GPU-resident arm (conc 1/2/4/10) once the TP4 numbers land.
TODO: confirm from the server log that the triton DSA backends and top-k v2 are actually the dispatched paths in the 0910 image, not a silent fallback.
Test plan
validate_perf_changelog.pyagainstupstream/main--dsa-*-backend tritonaccepted, no decode-time memory fault)full-sweep-fail-fast)Risk
Low-to-moderate. This is a benchmark recipe change with no application logic, but unlike a pure image bump it flips three coupled switches at once: the top-k v2 kernel, the DSA prefill/decode backends, and the container. If the sweep regresses or crashes, bisect in that order — the DSA backend switch is the most likely culprit, since the previous attempt to move ROCm DSA decode off the default hit a GPU memory fault in the a8w8 qseqlen1 persistent kernel.
The
v0.5.19-rocm10-mi35x-20260910tag uses arocm10segment rather than therocm720naming every other MI355X image inconfigs/amd-master.yamlcarries; worth a reviewer confirming that is the intended tag family.Note
Medium Risk
Benchmark-only changes, but they flip DSA backends, top-k kernel path, and container together; prior ROCm DSA decode changes have caused GPU faults, so sweep stability needs validation.
Overview
Updates the GLM-5.2 FP4 MI355X agentic MTP benchmark recipe to use newer SGLang ROCm capabilities:
SGLANG_OPT_USE_TOPK_V2is set totrue(was forced off because the v2 fused top-k did not build on gfx950; upstream ROCm support is now assumed), and--dsa-prefill-backend/--dsa-decode-backendswitch fromtilelangtotriton.configs/amd-master.yamlpinsglm5.2-fp4-mi355x-sglang-agentic-mtptolmsysorg/sglang-rocm:v0.5.19-rocm10-mi35x-20260910(fromv0.5.16-rocm720-mi35x-20260728). TP/EP arms, HiCache vs GPU-resident KV, MTP settings, and concurrency grids are unchanged.perf-changelog.yamldocuments the env flag, DSA backend, and image changes for the agentic-coding scenario.Reviewed by Cursor Bugbot for commit a8a2f87. Bugbot is set up for automated code reviews on this repo. Configure here.