[AMD][AgentX] Add DeepSeek-V4-Pro FP4 MI355X ATOM + MTP agentic-coding recipe#2346
[AMD][AgentX] Add DeepSeek-V4-Pro FP4 MI355X ATOM + MTP agentic-coding recipe#2346seungrokj wants to merge 3 commits into
Conversation
Add benchmarks/single_node/agentic/dsv4_fp4_mi355x_atom_mtp.sh (ATOM openai_server with --method mtp --num-speculative-tokens 3) and register the dsv4-fp4-mi355x-atom-agentic-mtp config in configs/amd-master.yaml. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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 关于重新运行失败任务的文档 |
1 similar comment
|
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 关于重新运行失败任务的文档 |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
seungrokj
changed the title
seungrokj
added
AMD
full-sweep-fail-fast
agentx
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| PARALLEL_ARGS=(-tp "$TP") | ||
| SPEC_ARGS=(--method mtp --num-speculative-tokens 3 ) | ||
| OFFLOAD_ARGS=() | ||
| if [ "$DP_ATTENTION" = "true" ]; then | ||
| PARALLEL_ARGS=(-tp "$TP" --enable-dp-attention) | ||
| fi | ||
| if [ "$EP_SIZE" -gt 1 ]; then | ||
| PARALLEL_ARGS+=(--enable-expert-parallel) | ||
| fi | ||
|
|
||
| # AgentX concurrency counts live session trees, not individual requests. |
There was a problem hiding this comment.
🔴 The ATOM_CMD server launch omits --trust-remote-code, which every other DeepSeek-V4-Pro launch script in the repo (both sibling fixed_seq_len/dsv4_fp4_mi355x_atom_mtp.sh and dsv4_fp4_mi355x_atom.sh on the same atom.entrypoints.openai_server entrypoint, plus the agentic vLLM/SGLang/H200 DSv4 scripts) passes. DeepSeek-V4-Pro requires custom remote modeling code, so without this flag the ATOM server will fail to load the model/tokenizer at startup, causing wait_for_server_ready to time out and the entire recipe to fail.
Extended reasoning...
The bug: benchmarks/single_node/agentic/dsv4_fp4_mi355x_atom_mtp.sh builds ATOM_CMD (lines 68-79 in the diff, the python3 -m atom.entrypoints.openai_server ... array) without the --trust-remote-code flag. This is the exact command that gets exec'd to start the model server, and its readiness is polled by wait_for_server_ready a few lines later.
Why this breaks the recipe: DeepSeek-V4-Pro is a custom architecture that ships remote modeling/tokenizer code on the Hugging Face Hub. Loaders (transformers/AITER/ATOM) require trust_remote_code=True (surfaced here via the --trust-remote-code CLI flag) to load such checkpoints; omitting it causes the loader to refuse to proceed at startup rather than silently falling back. This isn't speculative — every other launch script in the repo serving the exact same model through the exact same entrypoint passes the flag:
benchmarks/single_node/fixed_seq_len/dsv4_fp4_mi355x_atom_mtp.sh(sameatom.entrypoints.openai_server, same model, same--method mtp) — passes--trust-remote-codeat line 49 (and again at line 74 in a second invocation).benchmarks/single_node/fixed_seq_len/dsv4_fp4_mi355x_atom.sh— passes it at line 63 (and line 85).- The agentic vLLM, SGLang, and H200 DSv4-Pro launch scripts all pass their framework's equivalent flag as well.
I verified both fixed_seq_len sibling scripts directly and confirmed the flag is present at those exact lines, and it is conspicuously the only DSv4-Pro launch script in the repo missing it.
Why nothing else catches this: There's no schema/lint validation on these ATOM_CMD argument arrays, and set -euo pipefail only fails on non-zero exit of the foreground shell — the server is launched in the background ("${ATOM_CMD[@]}" > "$SERVER_LOG" 2>&1 &), so a crash-on-load failure surfaces only via wait_for_server_ready timing out after polling the port, not as an immediate script error. That makes the omission easy to introduce (e.g. by adapting a different framework's agentic launch script as a template) and easy to miss in review, since the script is otherwise syntactically valid and passes bash -n.
Step-by-step proof of impact:
- CI (or a manual run) invokes this script with
MODEL=deepseek-ai/DeepSeek-V4-Pro. - The script downloads the model, sets env vars, and builds
ATOM_CMDwithout--trust-remote-code. "${ATOM_CMD[@]}" > "$SERVER_LOG" 2>&1 &launches the ATOMopenai_serverprocess in the background.- Because DeepSeek-V4-Pro requires remote code execution to instantiate its model/tokenizer classes, the ATOM/transformers loader raises an error demanding
trust_remote_code=Trueand the process exits, logging the failure to$SERVER_LOG. wait_for_server_ready --port "$VLLM_BACKEND_PORT" --server-log "$SERVER_LOG" --server-pid "$SERVER_PID"polls the port, finds it never opens (and/or detects the dead PID), and eventually times out/fails.- The whole benchmark run for every entry in
dsv4-fp4-mi355x-atom-agentic-mtp's search-space (all 7 concurrency levels inconfigs/amd-master.yaml) fails at the server-start step — no benchmark data is ever collected.
Fix: Add --trust-remote-code to the ATOM_CMD array, matching the sibling fixed_seq_len/dsv4_fp4_mi355x_atom_mtp.sh script, e.g. inserting it alongside --kv_cache_dtype fp8 or near --served-model-name "$MODEL".
functionstackx
left a comment
functionstackx
left a comment
There was a problem hiding this comment.
hi @seungrokj thanks for the PR contribution!
it seems like in order to align with the agreed upon guidelines https://github.com/SemiAnalysisAI/InferenceX/blob/main/docs/PR_REVIEW_CHECKLIST.md for fair spec decode on AgentX, ATOM is missing an feature that all other major inferencing engines have
ROCm/ATOM#555
There was a problem hiding this comment.
LGTM — approving alongside the inline nit, which is metadata-only and explicitly non-blocking.
Extended reasoning...
Overview
This PR adds a new agentic-coding + MTP (speculative decoding) benchmark recipe for DeepSeek-V4-Pro FP4 on MI355X via ATOM: a new launch script (benchmarks/single_node/agentic/dsv4_fp4_mi355x_atom_mtp.sh), a new config-key registration in configs/amd-master.yaml, and a matching perf-changelog.yaml entry. It closely mirrors several existing sibling recipes already in the repo (dsv4-fp4-mi355x-atom-mtp, dsv4-fp4-mi355x-vllm-agentic, dsv4-fp4-mi355x-sglang-agentic-hicache), reusing the same benchmark_lib.sh helpers, env-var contract, and agentic-coding scenario schema.
Security risks
None. This is a benchmark configuration change — no auth, crypto, or permission-sensitive code is touched. The new shell script only downloads a public HF model and launches a local inference server; no user-controlled input or network-exposed surface is introduced.
Level of scrutiny
This warrants config/recipe-level scrutiny rather than production-code scrutiny: it's an isolated, self-contained addition (3 files) that doesn't touch shared benchmark infrastructure, CI workflows, or other recipes. The pattern (image, TP/EP, spec-decoding, KV-offload settings) is well-established by near-identical sibling entries, reducing the chance of a novel class of bug.
Other factors
The bug-hunting pass on this run surfaced only one finding, a Nit: the PR title/description is missing the repo-mandated Simplified Chinese counterpart (verified against AGENTS.md's bilingual-PR rule and against recent merged sibling PRs #2314/#2343, which do include it). That finding is explicitly metadata-only and does not affect the benchmark config or launch script's correctness, so it does not block merge on its own. Two other candidate issues (missing --trust-remote-code, unenforced KV_OFFLOADING) were investigated by verifier agents this run and refuted. The perf-changelog.yaml entry that a prior run had flagged as missing is now present in the diff. Given the nit-only outcome and the established, low-risk pattern this PR follows, I'm comfortable approving without further human review.
| dsv4-fp4-mi355x-atom-agentic-mtp: | ||
| image: rocm/atom-dev:nightly_202607211523 | ||
| model: deepseek-ai/DeepSeek-V4-Pro | ||
| model-prefix: dsv4 | ||
| runner: cluster:mi355x-amds | ||
| precision: fp4 | ||
| framework: atom | ||
| multinode: false | ||
| scenarios: | ||
| agentic-coding: | ||
| - dram-utilization: 0.60 | ||
| search-space: |
There was a problem hiding this comment.
🟡 This PR's title and description (Summary/Test plan) are English-only, but AGENTS.md mandates that every PR title and body include a Simplified Chinese counterpart (title format <English title> / <中文标题>, plus a Chinese section like ## 中文说明 in the body). Recent merged PRs (#2314, #2343) follow this convention, so please add the Chinese title suffix and a mirroring Chinese section to the description before merge.
Extended reasoning...
AGENTS.md (linked from the repo's root CLAUDE.md) states verbatim: "PR and GitHub-issue titles & descriptions must be bilingual — include a Simplified Chinese version in addition to English. Title format: <English title> / <中文标题>. In the PR/issue body, follow the English content with its Chinese translation (e.g. a ## 中文说明 section) ... This applies to every PR and every issue." This is an explicit, repository-mandated convention rather than a subjective style preference, which is why it is being flagged here.
This PR's title, [AMD][AgentX] Add DeepSeek-V4-Pro FP4 MI355X ATOM + MTP agentic-coding recipe, has no trailing / <中文标题> segment, and the body contains only English ## Summary and ## Test plan sections with no ## 中文说明 (or equivalent) mirroring them. Both the title and the description are fully English-only.
The convention is not aspirational — it is actively followed in this repository's recent history. git log on this branch shows PR #2343 titled "...docs: add MODELS.md model & scenario support matrix / 新增 MODELS.md 模型与场景支持矩阵 (#2343)" and PR #2314 titled "Refresh MiniMax-M3 NVFP4 B300 8k1k disaggregated EAGLE Dynamo-vLLM / 刷新 MiniMax-M3 NVFP4 B300 8k1k 分离式 EAGLE Dynamo-vLLM 配置 (#2314)", both carrying the bilingual title format required by AGENTS.md. This confirms the convention is enforced in practice for merged PRs in this exact repo, not a stale or unused guideline.
Step-by-step proof:
- Open AGENTS.md at the repo root — line 7 defines the bilingual title/description requirement, applying to "every PR and every issue."
- Compare against this PR's actual title:
[AMD][AgentX] Add DeepSeek-V4-Pro FP4 MI355X ATOM + MTP agentic-coding recipe" — no/ <中文标题>` suffix is present. - Compare against this PR's body: only
## Summaryand## Test plansections exist, both English; no## 中文说明section mirrors the summary. - Cross-check against recently merged sibling PRs (Refresh MiniMax-M3 NVFP4 B300 8k1k disaggregated EAGLE Dynamo-vLLM / 刷新 MiniMax-M3 NVFP4 B300 8k1k 分离式 EAGLE Dynamo-vLLM 配置 #2314, [Klaud Cold] docs: add MODELS.md model & scenario support matrix / 新增 MODELS.md 模型与场景支持矩阵 #2343) in
git log— both include the bilingual title suffix, confirming the convention is actively enforced rather than aspirational or abandoned. - Conclusion: this PR's title and description do not comply with the mandated bilingual format.
Fix: Rename the PR title to append a Chinese translation (e.g. [AMD][AgentX] Add DeepSeek-V4-Pro FP4 MI355X ATOM + MTP agentic-coding recipe / 新增 DeepSeek-V4-Pro FP4 MI355X ATOM + MTP agentic-coding 配方), and add a ## 中文说明 section to the PR body mirroring the English Summary/Test plan content.
This is a metadata/process issue only — it does not affect the benchmark config, the launch script, or CI behavior, so it should not block merge on its own, but it should be corrected to keep the repository's PR history consistent with its own documented convention.
…-remote-code Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30235103061 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30235761934 |
2 participants
Summary
benchmarks/single_node/agentic/dsv4_fp4_mi355x_atom_mtp.sh— MTP (speculative-decoding) variant of the DeepSeek-V4-Pro FP4 MI355X ATOM agentic-trace-replay benchmark, served via the ATOMopenai_serverentrypoint with--method mtp --num-speculative-tokens 3.dsv4-fp4-mi355x-atom-agentic-mtpconfig inconfigs/amd-master.yaml(imagerocm/atom-dev:nightly_202607211523, TP8/EP1, conc list [1,2,4,8,16,32,48],spec-decoding: mtp, no KV offload).Test plan
bash -n benchmarks/single_node/agentic/dsv4_fp4_mi355x_atom_mtp.shpassesdsv4-fp4-mi355x-atom-agentic-mtp🤖 Generated with Claude Code