Skip to content

[AMD] Add MI355X DeepSeek-V4-Pro Sglang Single Node AgentX recipes - #2600

Open
billishyahao wants to merge 3 commits into
mainfrom
amd/mi355x-agentx-dsv4-sgl-Aug12
Open

[AMD] Add MI355X DeepSeek-V4-Pro Sglang Single Node AgentX recipes #2600
billishyahao wants to merge 3 commits into
mainfrom
amd/mi355x-agentx-dsv4-sgl-Aug12

Conversation

@billishyahao

@billishyahao billishyahao commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

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

@billishyahao
billishyahao requested a review from a team August 14, 2026 01:46
@billishyahao billishyahao changed the title Add MI355X DeepSeek-V4-Pro Sglang Single Node AgentX recipes [AMD] Add MI355X DeepSeek-V4-Pro Sglang Single Node AgentX recipes Aug 14, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread perf-changelog.yaml Outdated
Comment on lines +5925 to +5928
- "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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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/XXX

Issue 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-20260813

v0.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

  1. Open perf-changelog.yaml at the newly appended entry (end of file, lines 5925-5928).
  2. 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.
  3. Read "Image: lmsysorg/sglang-rocm:v0.5.15.post1-rocm720-mi35x-20260719" in the same entry.
  4. Open configs/amd-master.yaml and find the dsv4-fp4-mi355x-sglang-agentic-mtp: block (same config-key referenced by the changelog entry) — its image: field reads lmsysorg/sglang-rocm:v0.5.17-rocm720-mi35x-20260813.
  5. 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/2600

and

    - "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.

Comment on lines +29 to +40
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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:

  1. Read benchmark_lib.sh:240-258 — defines wait_for_amd_gpu_clean() with the loop, threshold, and error message described above.
  2. Read dsv4_fp4_mi355x_sglang_mtp.sh:29-40 (this PR) — same loop, same rocm-smi pipeline, same <= 10 threshold, same error text, just inlined instead of calling the helper.
  3. Read glm5.2_fp4_mi355x_sglang_mtp.sh at the equivalent lines — same inlined block again (source of the copy-paste).
  4. Read kimik3_fp4_mi355x_mtp.sh — this sibling script instead calls wait_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).

@github-actions

Copy link
Copy Markdown
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant