[AMD] [AGENTX] GLM-5.2 FP4 MI355X ATOM Agentic MTP - #2576
Conversation
Add the MI355X GLM-5.2 FP4 ATOM AgentX recipe with MTP speculative decoding and LMCache DRAM offload, plus the amd-master.yaml config entry glm5.2-fp4-mi355x-atom-agentic-mtp. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add perf-changelog entry for glm5.2-fp4-mi355x-atom-agentic-mtp (PR #2576). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Correct the script header to describe GLM-5.2 FP4 ATOM MTP instead of the leftover MiniMax-M3 vLLM/EAGLE3 template text. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Rename VLLM_CMD to ATOM_CMD and correct the server label to "ATOM server" to reflect the atom.entrypoints.openai_server backend. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
seungrokj
added
AMD
full-sweep-fail-fast
agentx
functionstackx
left a comment
functionstackx
left a comment
There was a problem hiding this comment.
can u edit
to have the atom way of setting fair mtp| SIMULATE_ACC_LEN=2.99 | ||
| NUM_SPEC_TOKENS=3 | ||
| # spec-decode-acceptance-rate = (SIMULATE_ACC_LEN - 1) / NUM_SPEC_TOKENS | ||
| SPEC_ACCEPTANCE_RATE=$(awk "BEGIN{print ($SIMULATE_ACC_LEN-1)/$NUM_SPEC_TOKENS}") |
There was a problem hiding this comment.
can u edit
to have the atom way of setting fair mtpThere was a problem hiding this comment.
@functionstackx
when
--spec-decode-acceptance-rate
is specified then it will fall back to
https://github.com/ROCm/ATOM/blob/57143379cca658d5ae7a08ebb9be8787766d0104/atom/model_ops/rejection_sampler.py#L18-L25
and this will reject based on the synthetic decay factor
https://github.com/ROCm/ATOM/blob/57143379cca658d5ae7a08ebb9be8787766d0104/atom/model_ops/rejection_sampler.py#L395
and they will behave the same as sgl's match-expected behavior
but will cross check with the one who implemented this to make sure
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
configs/amd-master.yaml:1653-1655— Comment forglm5.2-fp4-mi355x-atom-agentic-mtpinconfigs/amd-master.yaml(lines 1653-1655) is truncated mid-sentence: it ends at "...to support" before jumping to the source-PR line. The sibling entryglm5.2-fp4-mi355x-sglang-agentic-mtphas the intended complete phrasing ("...to support long agentic context windows.") — please complete the sentence similarly.Extended reasoning...
The new comment block added just above
glm5.2-fp4-mi355x-atom-agentic-mtpreads:# GLM-5.2 FP4 agentic-coding benchmark on MI355X via ATOM with MTP speculative # decoding. TP=4, TP=8 with KV offloading to DRAM (lmcache backend) to support # Recipe is from PR https://github.com/ROCm/ATOM/pull/1877The second line ends on the dangling preposition "to support" with no object, and the very next comment line abruptly pivots to an unrelated topic (the source-PR attribution). This reads as an editing slip where a clause was cut off before the comment was finalized.
This is clearly a copy/paste-and-edit artifact from the sibling recipe added a few entries earlier in the same file,
glm5.2-fp4-mi355x-sglang-agentic-mtp(lines 1635-1637), whose equivalent comment is complete:# GLM-5.2 FP4 agentic-coding benchmark on MI355X via SGLang with MTP speculative # decoding. TP=4 EP=4 with KV offloading to DRAM (hicache backend) to support # long agentic context windows. Concurrency sweep [1, 2, 4, 8, 10].Here the rationale clause is finished ("...to support long agentic context windows.") before the sweep description. The new ATOM entry was evidently derived from this sibling block but the completing phrase was dropped when the KV-offload backend name was swapped from
hicachetolmcacheand the PR-attribution line was appended.There is no functional or parsing impact: YAML comments are inert, so this does not affect config validation, benchmark scheduling, or CI. The only cost is to a future reader trying to understand why DRAM KV offloading was chosen for this recipe — the rationale (supporting long agentic context windows, i.e. keeping large KV footprints off GPU HBM during long-horizon agentic-coding traces) is left unstated, and the truncation makes the comment look unintentionally broken rather than deliberately terse.
Proof: Reading lines 1653-1655 of
configs/amd-master.yamlas diffed in this PR shows the second comment line terminates immediately after "to support" with a trailing space and no punctuation, and line 1655 starts a grammatically unrelated sentence ("Recipe is from PR ..."). Diffing against the sibling block at lines 1635-1637 (unchanged in this PR, added previously) shows the intended completed form of the same clause structure, confirming what content is missing.Suggested fix: complete the sentence, e.g.:
# GLM-5.2 FP4 agentic-coding benchmark on MI355X via ATOM with MTP speculative # decoding. TP=4, TP=8 with KV offloading to DRAM (lmcache backend) to support # long agentic context windows. # Recipe is from PR https://github.com/ROCm/ATOM/pull/1877This is a comment-only defect with no behavioral effect, so it is a nit.
| @@ -0,0 +1,184 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
🔴 AGENTS.md declares a non-negotiable invariant: every recipe addition requires a new perf-changelog.yaml entry, but this PR adds the brand-new glm5.2-fp4-mi355x-atom-agentic-mtp recipe (script + configs/amd-master.yaml entry) without one. The directly analogous sibling PR (glm5.2-fp4-mi355x-sglang-agentic-mtp) added the required 7-line entry alongside its script/config changes — this PR needs the equivalent append at the tail of perf-changelog.yaml before merge.
Extended reasoning...
The bug: AGENTS.md lines 19-21 list "Non-negotiable benchmark invariants," the first of which states: "Every change that can affect benchmark performance and every recipe addition or modification requires a new perf-changelog.yaml entry. The file is append-only and byte-sensitive. Preserve all existing bytes and separator whitespace, and append only at the tail."
This PR adds a brand-new recipe key, glm5.2-fp4-mi355x-atom-agentic-mtp, to configs/amd-master.yaml, plus the corresponding new benchmark script benchmarks/single_node/agentic/glm5.2_fp4_mi355x_atom_mtp.sh. Both changes squarely fall under "recipe addition" per the invariant. Yet the PR diff touches only these two files — perf-changelog.yaml is not present in the diff at all.
Why this isn't caught elsewhere: There's no CI lint step visible in this repo that mechanically enforces the invariant (it's documented policy, not a script-enforced check), so a reviewer/author has to remember to append the entry manually. The immediately preceding entry in configs/amd-master.yaml, glm5.2-fp4-mi355x-sglang-agentic-mtp (the direct SGLang sibling of this ATOM recipe, same model/precision/SKU/scenario), did add its perf-changelog.yaml entry in its own commit — confirming this is the expected, followed convention for this exact recipe family, not a check nobody actually does in practice.
Impact: Without the changelog entry, there is no recorded baseline/description for this new recipe's expected performance characteristics, which is the entire purpose of the changelog per the AGENTS.md invariant — it's how the team tracks what performance-affecting changes were intentional versus regressions.
Proof / how to verify:
grep -n "atom-agentic-mtp" AGENTS.md configs/amd-master.yaml perf-changelog.yaml— the recipe key appears inconfigs/amd-master.yaml(this PR's diff) but not inperf-changelog.yaml.- Compare against the sibling:
grep -n "sglang-agentic-mtp" perf-changelog.yamlshows the SGLang sibling's entry exists. - The PR's own changed-files list (
benchmarks/single_node/agentic/glm5.2_fp4_mi355x_atom_mtp.sh,configs/amd-master.yaml) confirms only 2 files changed —perf-changelog.yamlis absent.
Fix: Append a new entry to the tail of perf-changelog.yaml (config-keys / description / pr-link, matching the format of the glm5.2-fp4-mi355x-sglang-agentic-mtp entry) for glm5.2-fp4-mi355x-atom-agentic-mtp, being careful to preserve existing bytes/whitespace and append-only per the invariant's own byte-sensitivity warning.
| LMCACHE_LOG="$RESULT_DIR/lmcache_server.log" | ||
| mkdir -p "$RESULT_DIR" | ||
|
|
||
| SERVER_PID="" | ||
| LMCACHE_PIDS=() | ||
| cleanup_agentic_services() { | ||
| local exit_code=$? | ||
| trap - EXIT INT TERM | ||
| set +e | ||
| stop_background_process_tree "$SERVER_PID" "vLLM server" 60 | ||
| local i | ||
| for i in "${!LMCACHE_PIDS[@]}"; do | ||
| stop_background_process_tree "${LMCACHE_PIDS[$i]}" "LMCache server $i" | ||
| done |
There was a problem hiding this comment.
🟡 Dead LMCache server-process scaffolding was copy-pasted from the vLLM script this recipe was adapted from: LMCACHE_LOG (line 52), LMCACHE_PIDS (line 56), and the cleanup loop over LMCACHE_PIDS (lines 63-65) are never written/populated here, since this ATOM recipe offloads KV in-process via the lmcache_offload connector rather than spawning a standalone LMCache server. Recommend deleting these three pieces to avoid implying an external LMCache server process is expected.
Extended reasoning...
This script was adapted from benchmarks/single_node/agentic/minimaxm3_fp4_mi355x_mtp.sh, which actually spawns a standalone lmcache server process in the background, appends its PID to LMCACHE_PIDS (LMCACHE_PIDS+=($!)), and redirects that server's stdout/stderr to LMCACHE_LOG (> "$LMCACHE_LOG"). In that sibling script the log path and PID array are load-bearing: the cleanup trap needs the PIDs to tear down the spawned server, and the log is useful for debugging server startup issues.
This GLM-5.2 ATOM recipe took a different architectural path for KV offload — it configures the in-process lmcache_offload kv_connector via --kv-transfer-config (see OFFLOAD_ARGS in the lmcache case of the KV_OFFLOAD_BACKEND switch), which runs inside the same vLLM/ATOM server process rather than as a separate process. No lmcache server command is ever invoked anywhere in the script. Despite that, the vLLM-script scaffolding for tracking an external server was carried over verbatim:
- Line 52:
LMCACHE_LOG="$RESULT_DIR/lmcache_server.log"— assigned but never referenced again; nothing ever writes to this path. - Line 56:
LMCACHE_PIDS=()— initialized empty and never appended to, since nolmcache serverprocess is spawned. - Lines 63-65: the cleanup loop
for i in "${!LMCACHE_PIDS[@]}"; do stop_background_process_tree "${LMCACHE_PIDS[$i]}" "LMCache server $i"; done— iterates a permanently empty array, so it is a runtime no-op on every invocation, forever.
Step-by-step proof: (1) LMCACHE_PIDS=() sets the array empty at line 56. (2) Grep the rest of the file for LMCACHE_PIDS+= or any other mutation — there is none; the lmcache case of the KV_OFFLOAD_BACKEND switch only sets OFFLOAD_ARGS (a --kv-transfer-config flag passed to the single VLLM_CMD/SERVER_PID process) and never runs lmcache server & or captures a $! into LMCACHE_PIDS. (3) Therefore at cleanup time "${!LMCACHE_PIDS[@]}" expands to nothing, and the for loop body executes zero iterations on every run, regardless of KV_OFFLOAD_BACKEND. (4) Similarly, LMCACHE_LOG is assigned once at line 52 and grep shows no other reference to it in the file — no > "$LMCACHE_LOG" redirection exists anywhere, unlike the vLLM script it was copied from.
This has no functional/runtime impact — an empty-array loop is harmless — but it is misleading dead code: a future maintainer reading this script would reasonably assume an external LMCache server process is started and tracked here, when in fact KV offload is entirely in-process. Recommend removing LMCACHE_LOG, LMCACHE_PIDS, and the associated cleanup loop (or, if a future revision needs the standalone LMCache server path, keep them but actually spawn/track that process as the vLLM sibling script does).
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31613244164 |
… TOTAL_CPU_DRAM_GB Set LMCACHE_MAX_LOCAL_CPU_SIZE to the harness-provided TOTAL_CPU_DRAM_GB instead of a hardcoded 200 GB, so the DRAM offload budget tracks the config's dram-utilization. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31615355795 |
…ation to 0.064 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ding for tp8 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31654183817 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31654311672 |
Signed-off-by: zhuyuhua-v <yuhzhu@amd.com>
Signed-off-by: zhuyuhua-v <yuhzhu@amd.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31711779471 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31746030024 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31765309673 |
1 similar comment
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31765309673 |
|
/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. https://github.com/semiAnalysisAI/InferenceX/actions/runs/31765309673
- Verified that this PR passes evals. Please link to GitHub Action workflow that shows this. https://github.com/semiAnalysisAI/InferenceX/actions/runs/31765309673
- 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: https://github.com/ROCm/ATOM/blob/main/recipes/Agentic-GLM-5.2.md
- 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
Signed: billishyahao
❌❌❌ REJECTED ❌❌❌@billishyahao — blocking issue: the sign-off leaves the "does not patch the inference engine / serving stack" checklist item unchecked ( ✅ Check 0 (CODEOWNER): PASS — |
billishyahao
left a comment
billishyahao
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. https://github.com/semiAnalysisAI/InferenceX/actions/runs/31765309673
- Verified that this PR passes evals. Please link to GitHub Action workflow that shows this. https://github.com/semiAnalysisAI/InferenceX/actions/runs/31765309673
- 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: https://github.com/ROCm/ATOM/blob/main/recipes/Agentic-GLM-5.2.md
- 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
Signed: billishyahao
✅✅✅ Verdict: PASS ✅✅✅Supersedes the earlier rejection for this SHA: the new sign-off checks the previously unchecked engine-patch item, which was the only blocker. ✅ Check 0 (CODEOWNER): PASS — |
|
@functionstackx @cquil11 can you plz review this? |
5 participants
Co-author @zhuyuhua-v
This PR contains this
#2591
Summary
benchmarks/single_node/agentic/glm5.2_fp4_mi355x_atom_mtp.sh) with MTP speculative decoding and LMCache DRAM offload.glm5.2-fp4-mi355x-atom-agentic-mtpconfig inconfigs/amd-master.yaml(TP4/TP8, lmcache DRAM offload, MTP, agentic-coding scenario).ptpc_fp8online quant,lmcache_offloadconnector,--method mtp, acceptance rate derived from the golden AL distribution).Test plan
bash -n benchmarks/single_node/agentic/glm5.2_fp4_mi355x_atom_mtp.shpassesglm5.2-fp4-mi355x-atom-agentic-mtp🤖 Generated with Claude Code