[AMD] Add MI355X DeepSeek-V4-Pro Sglang Single Node AgentX recipes - #2600
Conversation
billishyahao
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good overall — a large but mechanical new recipe (script + config + changelog) that closely follows the pattern of sibling AMD MI355X AgentX entries (glm5.2, kimik3, dsv4 disagg). The two inline findings are both nits (a stale placeholder/image tag in the append-only changelog entry, and duplicated GPU-drain-wait logic that already exists as a helper) — neither affects benchmark correctness or safety, so I'm not blocking on them.
Extended reasoning...
Overview
This PR adds a new single-node SGLang AgentX benchmark recipe for DeepSeek-V4-Pro FP4 on MI355X (dsv4-fp4-mi355x-sglang-agentic-mtp): a new 273-line launch script, a new amd-master.yaml config block with two search-space arms (TP8 no-offload and TP8+HiCache DRAM offload), and an append-only perf-changelog.yaml entry. It restores single-node SGLang AgentX coverage that PR #2531 previously deleted, drawing its flags (EAGLE/MTP depth 3, HiCache ratios, chunked-prefill sizing, DSv4 kernel routing env vars) from several sibling recipes already validated on this cluster (glm5.2 sglang agentic-mtp, dsv4 sglang disagg agentic-hicache-mtp, dsv4 vllm agentic-mtp).
Security risks
None. This is benchmark/config infrastructure only — no auth, crypto, network-facing surface, or user-controlled input parsing. The script runs on internal CI/cluster runners with fixed, repo-controlled arguments.
Level of scrutiny
Medium. This is not production-critical code, but it is a new 273-line bash script with nontrivial branching (DP-attention dormant path, HiCache arg construction, router launch) rather than a pure config tweak, so a full read-through was warranted rather than a rubber-stamp. The change follows an extremely well-established pattern (same repo has ~5 near-identical sibling recipes), which lowers the risk of a genuinely novel bug versus a typo/copy-paste slip — which is exactly what both inline findings turned out to be.
Other factors
No prior reviews exist on this PR in the timeline, so this is a first pass. Both findings from the bug-hunting pass are nits: one is a documentation-only mismatch in the append-only perf-changelog.yaml (worth fixing since it's permanent post-merge, but has zero effect on benchmark execution), and the other is code duplication of an already-extracted benchmark_lib.sh helper (a maintainability nit, functionally correct as written). Neither rises to the level of blocking merge, and I did not find additional correctness issues in the script's argument construction, env var handling, or config search-space definitions beyond what's already flagged inline.
| - "Add DeepSeek-V4-Pro FP4 single-node SGLang AgentX recipe on MI355X with EAGLE/MTP (num-steps 3, num-draft-tokens 4) and SGLANG_SIMULATE_ACC_LEN=2.49 from the committed thinking_on golden AL curve" | ||
| - "Restores the single-node SGLang agentic coverage deleted in PR #2531; TP8 arms with and without HiCache DRAM offload" | ||
| - "Image: lmsysorg/sglang-rocm:v0.5.15.post1-rocm720-mi35x-20260719" | ||
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX |
There was a problem hiding this comment.
🟡 The new perf-changelog.yaml entry for dsv4-fp4-mi355x-sglang-agentic-mtp has two stale/placeholder values that can't be corrected post-merge since this file is append-only: (1) pr-link is left as the literal .../pull/XXX instead of .../pull/2600, and (2) the Image: line cites v0.5.15.post1-rocm720-mi35x-20260719 (copied from the sibling disagg entry) while configs/amd-master.yaml actually pins v0.5.17-rocm720-mi35x-20260813 for this key. Please fill in the real PR number and correct the image tag before merging.
Extended reasoning...
What's wrong
The perf-changelog.yaml entry added by this PR (lines 5925-5928) for the new dsv4-fp4-mi355x-sglang-agentic-mtp config key contains two factual errors that were evidently introduced by copy-pasting the sibling dsv4-fp4-mi355x-sglang-disagg-agentic-hicache-mtp entry without updating the copied fields:
- config-keys:
- dsv4-fp4-mi355x-sglang-agentic-mtp
description:
- "Add DeepSeek-V4-Pro FP4 single-node SGLang AgentX recipe on MI355X with EAGLE/MTP ..."
- "Restores the single-node SGLang agentic coverage deleted in PR #2531; TP8 arms with and without HiCache DRAM offload"
- "Image: lmsysorg/sglang-rocm:v0.5.15.post1-rocm720-mi35x-20260719"
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXXIssue 1 — placeholder PR link. pr-link is left as the literal template value .../pull/XXX rather than the real PR number. This PR is #2600, and every other entry in the file (the three preceding entries alone reference #2554, #2570, #2550) has a concrete numeric PR link filled in — this is the only unfilled placeholder in the file.
Issue 2 — wrong image tag. The Image: note says v0.5.15.post1-rocm720-mi35x-20260719, but configs/amd-master.yaml's actual new entry for this exact key pins:
dsv4-fp4-mi355x-sglang-agentic-mtp:
image: lmsysorg/sglang-rocm:v0.5.17-rocm720-mi35x-20260813v0.5.15.post1-...-20260719 is the image used by the sibling dsv4-fp4-mi355x-sglang-disagg-agentic-hicache-mtp key elsewhere in amd-master.yaml, and the new entry's own description text ("The image matches the already-green disagg AgentX entry") confirms the author intended to reference that sibling, but the config was subsequently bumped to v0.5.17-...-20260813 without updating the changelog note to match.
Why this isn't caught elsewhere
perf-changelog.yaml is purely descriptive metadata: it is never parsed by the benchmark launcher or config loader, so nothing validates that pr-link is a real URL or that the Image: free-text line matches the corresponding image: field in amd-master.yaml. There is no schema check or CI linter cross-referencing the two files.
Impact
Per this repo's conventions (AGENTS.md), perf-changelog.yaml is append-only and byte-sensitive — historical entries are not edited after merge. That means once this PR lands, both defects are permanent: the PR link will forever 404 (or point to whatever unrelated PR eventually claims #XXX literally, which is not a valid PR number), and anyone auditing which SGLang image this AgentX recipe uses will be misled into believing it runs the older 20260719 build with different kernel fixes than the 20260813 build actually configured.
Proof walkthrough
- Open
perf-changelog.yamlat the newly appended entry (end of file, lines 5925-5928). - Read
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX— compare to the PR metadata, which shows this is PR [AMD] Add MI355X DeepSeek-V4-Pro Sglang Single Node AgentX recipes #2600. The link is wrong. - Read
"Image: lmsysorg/sglang-rocm:v0.5.15.post1-rocm720-mi35x-20260719"in the same entry. - Open
configs/amd-master.yamland find thedsv4-fp4-mi355x-sglang-agentic-mtp:block (same config-key referenced by the changelog entry) — itsimage:field readslmsysorg/sglang-rocm:v0.5.17-rocm720-mi35x-20260813. - The two image strings differ, confirming the changelog documents the wrong image for the recipe it describes.
Fix
Before merging, update the appended entry to:
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2600and
- "Image: lmsysorg/sglang-rocm:v0.5.17-rocm720-mi35x-20260813"Since the file is append-only after merge, this is the only opportunity to fix both values.
| rocm-smi || true | ||
| amd-smi || true | ||
|
|
||
| # A server killed on this node minutes earlier (previous job, crashed run) | ||
| # can still be draining its HBM: KFD reclaim takes minutes, and booting into a | ||
| # half-drained node fails RCCL init with HIP 'unhandled cuda error' / | ||
| # 'invalid argument'. DeepSeek-V4-Pro is an 805 GiB checkpoint, so the drain | ||
| # window here is at the long end. Wait for the GPUs to come back before | ||
| # launching. Per-GPU threshold: idle nodes hold a small driver/firmware VRAM | ||
| # baseline (observed up to ~4%/GPU), while a draining or occupied GPU sits at | ||
| # 50-90%. Require every GPU <= 10%. | ||
| GPU_CLEAN=false |
There was a problem hiding this comment.
🟡 The 15-min GPU-drain-wait loop at lines 29-40 duplicates wait_for_amd_gpu_clean() in benchmark_lib.sh (lines 240-258) character-for-character, even though this script already sources that file. Replace the inline block with a call to wait_for_amd_gpu_clean || exit 1 so future threshold/timeout tuning only needs to happen in one place.
Extended reasoning...
What the bug is. Lines 29-40 of the new dsv4_fp4_mi355x_sglang_mtp.sh re-implement a 15-minute GPU-drain-wait poll loop:
GPU_CLEAN=false
for i in $(seq 1 90); do
VRAM_MAX=$(rocm-smi --showmemuse 2>/dev/null | grep -oE "GPU Memory Allocated \(VRAM%\): [0-9]+" | awk '{if ($NF > m) m = $NF} END {print m+0}')
if [ "${VRAM_MAX:-0}" -le 10 ]; then echo "GPUs clean (vram%max=$VRAM_MAX after $((i*10))s)"; GPU_CLEAN=true; break; fi
echo "waiting for prior-job GPU memory reclaim: vram%max=$VRAM_MAX"; sleep 10
done
[ "$GPU_CLEAN" = "true" ] || { echo "Error: GPUs still draining prior job's memory after 15min" >&2; exit 1; }This is copy-pasted verbatim (only the surrounding comment differs, mentioning 805 GiB for DeepSeek-V4-Pro vs. the ~1.4 TB comment in the GLM-5.2 sibling script) from glm5.2_fp4_mi355x_sglang_mtp.sh, which has the identical block at the same lines.
Why this is avoidable. benchmark_lib.sh already defines a helper, wait_for_amd_gpu_clean() (lines 240-258), whose body is a character-for-character match of the inlined logic here: same seq 1 90 loop, same rocm-smi --showmemuse | grep -oE "GPU Memory Allocated (VRAM%): [0-9]+" | awk pipeline, same <=10% threshold, same 90x10s = 15-minute budget, and the same 'GPUs still draining prior job's memory after 15min' error message. This isn't a hypothetical refactor target — the helper is already the canonical, adopted pattern: kimik3_fp4_mi355x_mtp.sh already calls wait_for_amd_gpu_clean instead of inlining the loop.
The code path that triggers it. The new script sources benchmark_lib.sh at line 8 (source "$(dirname "$0")/../../benchmark_lib.sh"), so wait_for_amd_gpu_clean is already in scope at the point where the inline block sits. Nothing prevents calling it directly.
Why existing code doesn't prevent it. AGENTS.md states: 'Shared benchmark Bash behavior belongs in benchmark_lib.sh, with parameters passed through environment variables.' Nothing enforces this at review time beyond that guidance, so it's easy for a new recipe branched from a sibling script (in this case glm5.2_fp4_mi355x_sglang_mtp.sh) to inherit an inlined pattern rather than the shared helper. Now three copies of this exact logic exist: benchmark_lib.sh's helper, and two duplicated inline copies in glm5.2_fp4_mi355x_sglang_mtp.sh and this new script.
Impact. No runtime failure — the inlined code is functionally correct and behaves identically to the helper. The cost is maintainability: any future tuning of the 10% threshold, the 90x10s timeout, or the rocm-smi parsing pipeline now has to be updated in up to three places instead of one, and they can silently drift (which has already started, per the differing comment wording).
Step-by-step proof of duplication:
- Read
benchmark_lib.sh:240-258— defineswait_for_amd_gpu_clean()with the loop, threshold, and error message described above. - Read
dsv4_fp4_mi355x_sglang_mtp.sh:29-40(this PR) — same loop, samerocm-smipipeline, same<= 10threshold, same error text, just inlined instead of calling the helper. - Read
glm5.2_fp4_mi355x_sglang_mtp.shat the equivalent lines — same inlined block again (source of the copy-paste). - Read
kimik3_fp4_mi355x_mtp.sh— this sibling script instead callswait_for_amd_gpu_clean, proving the helper is already the intended, working call site pattern.
Fix. Since the script runs under set -eo pipefail (line 2) and the helper returns 1 on failure, replace lines 29-40 with:
wait_for_amd_gpu_clean || exit 1(or simply wait_for_amd_gpu_clean, since set -e will already exit non-zero on failure).
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31762202350 |
|
/reuse-sweep-run |
There was a problem hiding this comment.
As a PR reviewer and CODEOWNER, I have reviewed this and have:
- Verified that as of the moment of typing this, this is the latest version of PR_REVIEW_CHECKLIST.md
- Verified that the general code quality meets the InferenceX standard and does not make the code quality any worse.
- Verified that this PR has passed PR validation. Please link to GitHub Action workflow that shows this.
- Verified that this PR passes evals. Please link to GitHub Action workflow that shows this.
- Verified that speculative decoding PRs uses chat templates to align the AL distribution to real world
- For agentic workloads: verified that speculative-decoding configs (EAGLE / MTP / draft models) run with simulated synthetic acceptance, with the acceptance-length value taken from the committed golden AL curve in golden_al_distribution/ for that model, thinking mode, and draft length. A submission may choose any supported draft length, but it may not substitute a different acceptance target.
- Verified against the current MODELS.md that this PR does not submit a deprecated model, scenario, or model-scenario combination.
- Verified that the model architecture isn't changed with benchmark hacks like using --hf-overrides to skipping indexer for every x layers on models that don't natively support this. As a general rule, we won't accept optimizations that reduces the number of model architecture FLOPs. Anything that makes that same computation run faster is fair game; FLOPs at lower precisions is fine, given that the config passes private evals. As an general north star princple, we should only use optimizations which is used in production by customers that care about accuracy
- If an company claims that they support vLLM/SGLang as first class LLM inference engines on their hardware, I have verified that the respective vLLM submission made using upstream https://hub.docker.com/u/vllm docker repo, upstream SGLang https://hub.docker.com/u/lmsysorg docker repo. The only exceptions are for new hardware, such as MI455X UALoE72, Vera Rubin NVL72, Rubin NVL8, etc., and for new model architectures where there is an actual reason why vLLM/SGLang does not fundamentally support them yet as supported by vLLM/SGLang community maintainers
- If an company claims that they support vLLM/SGLang as first class upstream in-tree LLM inference engines on their hardware, I have have verified that the respective vLLM/SGLang submission has been made before additional frameworks (TRT-LLM, ATOM, etc.). The only exceptions are for new hardware, such as MI455X UALoE72, Vera Rubin NVL72, Rubin NVL8, etc., and for new model architectures where there is an actual reason why vLLM/SGLang does not fundamentally support them yet.
- Verified that every single-node vLLM/SGLang recipe in this PR is documented in the official vLLM recipes and/or the SGLang cookbook:
- I linked the corresponding upstream PR in the vLLM recipe repo or SGLang repo and verified that it is MERGED before this InferenceX PR merges. An opened, draft, or closed-without-merge upstream PR does not satisfy this requirement. If the matching recipe was already published, I linked the published recipe/cookbook page in the additional detail section below.
- Verified that this PR does not patch the inference engine or serving stack — the pinned image must run as shipped. This covers .patch files / git apply / patch, inline patches embedded in benchmark scripts (e.g. a python3/sed heredoc that rewrites installed engine sources before serving), in-place edits of site-packages, monkey-patching, overwriting container files, and installing forked/rebuilt engine wheels on top of the pinned image. The only exception is a patch covered by a filled-out waiver at docs/waiver/
<PR_NUMBER>.md— named after the PR that introduces the patch and filed in that same PR, stating what is patched, why the unmodified upstream image cannot run this benchmark, the upstream PR/issue link, and the removal plan — which I have linked below in the additional detail section. - If any of the above criteria cannot reasonably be satisfied, I have provided additional reasoning below.
Additional detail section:
- insert any additional info here
Recipe at https://docs.sglang.io/cookbook/autoregressive/DeepSeek/DeepSeek-V4#hw=mi355x&variant=pro&quant=fp4&strategy=high-throughput&nodes=single
Signed: seungrokj
❌❌❌ REJECTED ❌❌❌@seungrokj — blocking: no upstream recipe link. The sign-off's additional detail section is the unmodified template placeholder ("insert any additional info here"), but this PR adds a single-node SGLang recipe, which requires a link to a MERGED sgl-project/sglang recipe PR or the published SGLang cookbook page for this DeepSeek-V4-Pro serve config. Please repost the sign-off with that link. ✅ Check 0 (CODEOWNER): PASS — @seungrokj is a listed owner of |
|
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 关于重新运行失败任务的文档 |
seungrokj
left a comment
seungrokj
left a comment
There was a problem hiding this comment.
As a PR reviewer and CODEOWNER, I have reviewed this and have:
- Verified that as of the moment of typing this, this is the latest version of PR_REVIEW_CHECKLIST.md
- Verified that the general code quality meets the InferenceX standard and does not make the code quality any worse.
- Verified that this PR has passed PR validation. Please link to GitHub Action workflow that shows this.
- Verified that this PR passes evals. Please link to GitHub Action workflow that shows this.
- Verified that speculative decoding PRs uses chat templates to align the AL distribution to real world
- For agentic workloads: verified that speculative-decoding configs (EAGLE / MTP / draft models) run with simulated synthetic acceptance, with the acceptance-length value taken from the committed golden AL curve in golden_al_distribution/ for that model, thinking mode, and draft length. A submission may choose any supported draft length, but it may not substitute a different acceptance target.
- Verified against the current MODELS.md that this PR does not submit a deprecated model, scenario, or model-scenario combination.
- Verified that the model architecture isn't changed with benchmark hacks like using --hf-overrides to skipping indexer for every x layers on models that don't natively support this. As a general rule, we won't accept optimizations that reduces the number of model architecture FLOPs. Anything that makes that same computation run faster is fair game; FLOPs at lower precisions is fine, given that the config passes private evals. As an general north star princple, we should only use optimizations which is used in production by customers that care about accuracy
- If an company claims that they support vLLM/SGLang as first class LLM inference engines on their hardware, I have verified that the respective vLLM submission made using upstream https://hub.docker.com/u/vllm docker repo, upstream SGLang https://hub.docker.com/u/lmsysorg docker repo. The only exceptions are for new hardware, such as MI455X UALoE72, Vera Rubin NVL72, Rubin NVL8, etc., and for new model architectures where there is an actual reason why vLLM/SGLang does not fundamentally support them yet as supported by vLLM/SGLang community maintainers
- If an company claims that they support vLLM/SGLang as first class upstream in-tree LLM inference engines on their hardware, I have have verified that the respective vLLM/SGLang submission has been made before additional frameworks (TRT-LLM, ATOM, etc.). The only exceptions are for new hardware, such as MI455X UALoE72, Vera Rubin NVL72, Rubin NVL8, etc., and for new model architectures where there is an actual reason why vLLM/SGLang does not fundamentally support them yet.
- Verified that every single-node vLLM/SGLang recipe in this PR is documented in the official vLLM recipes and/or the SGLang cookbook:
- I linked the corresponding upstream PR in the vLLM recipe repo or SGLang repo and verified that it is MERGED before this InferenceX PR merges. An opened, draft, or closed-without-merge upstream PR does not satisfy this requirement. If the matching recipe was already published, I linked the published recipe/cookbook page in the additional detail section below.
- Verified that this PR does not patch the inference engine or serving stack — the pinned image must run as shipped. This covers .patch files / git apply / patch, inline patches embedded in benchmark scripts (e.g. a python3/sed heredoc that rewrites installed engine sources before serving), in-place edits of site-packages, monkey-patching, overwriting container files, and installing forked/rebuilt engine wheels on top of the pinned image. The only exception is a patch covered by a filled-out waiver at docs/waiver/
<PR_NUMBER>.md— named after the PR that introduces the patch and filed in that same PR, stating what is patched, why the unmodified upstream image cannot run this benchmark, the upstream PR/issue link, and the removal plan — which I have linked below in the additional detail section. - If any of the above criteria cannot reasonably be satisfied, I have provided additional reasoning below.
Additional detail section:
- insert any additional info here
Recipe at https://docs.sglang.io/cookbook/autoregressive/DeepSeek/DeepSeek-V4#hw=mi355x&variant=pro&quant=fp4&strategy=high-throughput&nodes=single
Signed: seungrokj
✅✅✅ Verdict: PASS ✅✅✅✅ Check 0 (CODEOWNER): PASS — @seungrokj is a listed owner of |
|
@functionstackx @cquil11 can you review this ? |
|
/stage-results 31762202350 |
|
@cquil11 staged run 31762202350: https://inferencemax-app-git-staging-semianalysisai.vercel.app/inference?i_dates=2026-08-14~r31762202350 This run remains available across future |
4 participants
Add DeepSeek-V4-Pro FP4 single-node SGLang AgentX recipe on MI355X with EAGLE/MTP (num-steps 3, num-draft-tokens 4) and SGLANG_SIMULATE_ACC_LEN=2.49 from the committed thinking_on golden AL curve