[AMD][AgentX] Add MI300X MiniMax-M3 EAGLE3 MXFP8 - #2580
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 关于重新运行失败任务的文档 |
cquil11
added
AMD
agentx
cquil11
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approving — the two flagged issues are both nits (a duplicated LMCache spin-up block missing a pre-flight check, and a required-but-unconsumed DP_ATTENTION var) that don't affect correctness of this PR's actual search space; see inline comments for details.
Extended reasoning...
Overview
This PR adds a new AgentX benchmark recipe: a MiniMax-M3 MXFP8 EAGLE3-MTP discovery sweep on MI300X via vLLM v0.27.1. Changes are a new shell launch script (minimaxm3_fp8_mi300x_mtp.sh), a new amd-master.yaml search-space entry, and a perf-changelog.yaml record. The new script closely mirrors sibling recipes already in the repo (minimaxm3_fp8_mi300x.sh, minimaxm3_fp4_mi355x_mtp.sh) in structure: env var checks, model download, vLLM serve command construction, optional LMCache MP KV-offload spin-up, and eval/replay dispatch.
Security risks
None identified. This is CI benchmark infrastructure — env vars come from the sweep-config generator (not user-controlled input), there's no auth/crypto surface, and the LMCache/vLLM invocations use fixed, hardcoded flags rather than interpolating untrusted data into anything eval'd or interpreted unsafely.
Level of scrutiny
Low-to-moderate. This is a new discovery recipe following an extremely well-established pattern in this repo (many nearly-identical sibling scripts/configs exist), it doesn't touch shared library code (benchmark_lib.sh is unchanged), and it only affects a single isolated benchmark matrix entry — blast radius of any latent issue is a single job's CI results, not shared behavior.
Other factors
Both bug-hunter findings are explicitly scoped as Nits: one is a code-duplication/pre-flight-check quality gap in the LMCache branch (only exercised by one isolated gate arm), and the other is a required-but-unconsumed DP_ATTENTION env var that is harmless under the current search space (no dp-attn: true row exists in this recipe) and mirrors an existing pattern already present in other sibling scripts (glm5.2_fp8_mi325x_mtp.sh, minimaxm3_fp4_mi355x_mtp.sh). Neither affects the correctness of what this PR actually ships. The PR's own validation section shows bash -n, YAML parsing, and the sweep-config test suite (105 passed) all green.
|
|
||
| OFFLOAD_ARGS=() | ||
| case "$KV_OFFLOAD_BACKEND" in | ||
| "") | ||
| require_agentic_kv_offload_none | ||
| ;; | ||
| lmcache) | ||
| require_agentic_kv_offload_backend lmcache | ||
| LMCACHE_VERSION="0.5.3" | ||
| LMCACHE_ROCM_INDEX="https://github.com/LMCache/LMCache/releases/expanded_assets/v${LMCACHE_VERSION}-rocm" | ||
| agentic_pip_install --quiet --no-cache-dir --no-deps \ | ||
| "sortedcontainers==2.4.0" \ | ||
| "opentelemetry-exporter-prometheus==0.61b0" \ | ||
| "cupy-rocm-7-0==14.1.1" \ | ||
| "lmcache==${LMCACHE_VERSION}" --find-links "$LMCACHE_ROCM_INDEX" | ||
| python3 -c \ | ||
| "import cupy; import lmcache.integration.vllm.lmcache_mp_connector; import opentelemetry.exporter.prometheus" \ | ||
| >/dev/null | ||
|
|
There was a problem hiding this comment.
🟡 This LMCache MP shard spin-up block (lines 62-128) is a near-verbatim duplicate (~55 lines) of the one in minimaxm3_fp4_mi355x_mtp.sh, and unlike that sibling it drops the pre-flight check that verifies LMCACHE_L1_SIZE_GB doesn't exceed 90% of free /dev/shm before launching. Worth extracting a shared start_lmcache_mp_shards helper into benchmark_lib.sh (removing the duplication) and restoring the /dev/shm guard so an oversized DRAM budget fails with a clear error instead of an opaque lmcache OOM/crash.
Extended reasoning...
What's duplicated: Lines 62-128 of minimaxm3_fp8_mi300x_mtp.sh reimplement the per-TP-rank LMCache MP server spin-up loop that already exists in benchmarks/single_node/agentic/minimaxm3_fp4_mi355x_mtp.sh (its equivalent block runs roughly lines 79-165). Both use the identical port scheme (shard_port=5555+shard, shard_http_port=8080+shard), the identical lmcache CLI flag set (--l1-init-size-gb 10 --l1-read-ttl-seconds 7200 --chunk-size 256 --max-workers 2 --eviction-policy LRU --supported-transfer-mode lmcache_driven), the same append_command/background-launch pattern, and the same wait_for_ready healthcheck loop plus the same LMCacheMPConnector OFFLOAD_ARGS kv-transfer-config construction. benchmark_lib.sh already centralizes shared agentic helpers (require_agentic_kv_offload_backend, wait_for_ready, stop_background_process_tree, append_command), so this block is a natural candidate for a start_lmcache_mp_shards helper that both scripts could call, removing roughly 55 duplicated lines.\n\nWhat's missing: the mi355x sibling has a pre-flight guard (around its lines 104-111) that computes free /dev/shm via df -BG /dev/shm and exits with a clear error if LMCACHE_L1_SIZE_GB would exceed 90% of that free space. The mi300x copy in this PR omits that check entirely — it goes straight from LMCACHE_L1_SHARD_GB=$((TOTAL_CPU_DRAM_GB / TP)) to only a -lt 1 floor check (line ~68-71 here). Since LMCache MP's L1 tier is backed by shared memory, a TOTAL_CPU_DRAM_GB/TP budget that's too large for the runner's actual /dev/shm capacity won't be caught up front; it will instead surface later as an opaque lmcache server crash or OOM in lmcache_server_*.log, which is much harder to diagnose than the sibling script's explicit pre-flight error.\n\nConcrete walk-through: suppose the mi300x runner has 512GB of /dev/shm" but the job is launched with TOTAL_CPU_DRAM_GB=600andTP=4(e.g. a copy-pasted or misconfigureddram-utilization/config value). LMCACHE_L1_SHARD_GB computes to 150GB per shard × 4 shards = 600GB total requested, exceeding the 512GB actually available. The mi355x sibling's check would catch this immediately (600 > 512 * 0.9 = 460.8) and exit with "Error: LMCache L1 size exceeds ... /dev/shm" before any process starts. In this PR's mi300x script, the -lt 1check passes fine (150 ≥ 1), so all fourlmcache serverbackground processes launch, each trying to mmap a 150GB region in the same shared/dev/shm; they will either fail to allocate, get OOM-killed, or degrade in ways that only show up in the per-shard logs after the fact, all while the outer wait_for_ready loop times out after up to 600 seconds — a much slower and more confusing failure than the sibling's instant, actionable error.\n\n**Why this matters but isn't blocking:** this LMCache configuration is exercised by exactly one isolated gate arm (tp: 8, conc-list: [16]) in the new minimaxm3-fp8-mi300x-vllm-agentic-mtpmatrix entry, so the blast radius if it does misconfigure is a single functional-gate job failing loudly (if diagnosed) rather than a broader correctness or data-loss issue. It's a code-quality / parity gap relative to the sibling recipe, not a bug that breaks the documented behavior of this PR.\n\n**Fix:** extract the LMCache MP spin-up loop (including the/dev/shmpre-flight check) from the mi355x script into a sharedstart_lmcache_mp_shardsfunction inbenchmark_lib.sh, and have both minimaxm3_fp8_mi300x_mtp.shandminimaxm3_fp4_mi355x_mtp.sh` call it. This removes the ~55-line duplication and restores the missing capacity check in one change.
|
|
||
| export EVAL_FRAMEWORK="lm-eval" | ||
|
|
||
| check_env_vars MODEL TP CONC KV_OFFLOADING TOTAL_CPU_DRAM_GB RESULT_DIR DURATION EP_SIZE DP_ATTENTION PORT EVAL_ONLY |
There was a problem hiding this comment.
🟡 The new minimaxm3_fp8_mi300x_mtp.sh requires DP_ATTENTION via check_env_vars (line 11) but never branches on it — PARALLEL_ARGS only checks EP_SIZE. It's harmless today since every row in minimaxm3-fp8-mi300x-vllm-agentic-mtp defaults dp-attn to false, but sibling recipes with the same required var (e.g. minimaxm3_fp8_mi300x.sh, dsv4_fp4_mi355x_vllm_mtp.sh) do consume it to build DP-attention flags, so if a future dp-attn: true row is added here (as already exists for the sibling mi325x recipe), the server would silently launch plain TP/EP while the sweep still labels it DEP.
Extended reasoning...
The bug: benchmarks/single_node/agentic/minimaxm3_fp8_mi300x_mtp.sh:11 lists DP_ATTENTION in its check_env_vars call, asserting the script depends on that variable. But grepping the rest of the script shows it is never referenced again — PARALLEL_ARGS (around the EP_SIZE-gated --enable-expert-parallel block) only branches on EP_SIZE. There is no analogous branch that would add --data-parallel-size, bump the port, or start a router when DP_ATTENTION=true.
Contrast with siblings that do consume it: minimaxm3_fp8_mi300x.sh (lines 150/160/188), minimaxm3_fp8_mi325x.sh (150/165/198), and dsv4_fp4_mi355x_vllm_mtp.sh (70/365) all require the same variable and use it to switch between plain TP/EP and DP-attention serving (different parallel args, a different port, and launching vllm-router). Those scripts show what 'consuming' DP_ATTENTION is supposed to look like in this codebase, and the new MTP script has the requirement without the corresponding behavior.
Why this doesn't currently break anything: every search-space row under minimaxm3-fp8-mi300x-vllm-agentic-mtp in configs/amd-master.yaml omits dp-attn, which generate_sweep_configs.py defaults to false. With DP_ATTENTION=false, the correct behavior (plain TP/EP) is exactly what the script does anyway, so no current run is mislabeled.
Why it's still worth flagging (as a nit, not a blocker): it's a latent footgun rather than an active bug. dp-attn: true rows already exist for sibling recipes in the very same amd-master.yaml file (e.g. minimaxm3-fp8-mi325x-vllm-agentic, dsv4-fp4-mi355x-vllm-agentic-mtp). If a future row adds dp-attn: true to this recipe, the sweep would set DP_ATTENTION=true and record the point as a DEP config, but the script would silently serve plain TP/EP — corrupting the benchmark's performance-curve labeling with no error surfaced anywhere in the pipeline.
Step-by-step proof: (1) A maintainer adds { tp: 8, ep: 8, dp-attn: true, ... } to minimaxm3-fp8-mi300x-vllm-agentic-mtp in amd-master.yaml, mirroring the pattern already used for minimaxm3-fp8-mi325x-vllm-agentic. (2) generate_sweep_configs.py emits a job with DP_ATTENTION=true. (3) check_env_vars passes since the variable is set. (4) The script builds PARALLEL_ARGS=(--tensor-parallel-size "") plus --enable-expert-parallel from EP_SIZE alone — DP_ATTENTION is never read, so no --data-parallel-size flag is added and no router is started. (5) The server runs as plain TEP8, but the result is recorded and labeled as a DEP8 data point in the benchmark output, silently corrupting the recorded performance curve.
On the refutation: one verifier correctly points out that this exact required-but-unconsumed pattern already exists in glm5.2_fp8_mi325x_mtp.sh:11 and minimaxm3_fp4_mi355x_mtp.sh:16 (confirmed by inspection — both require DP_ATTENTION via check_env_vars and never branch on it, the latter only echoing it for logging). So this specific script isn't introducing a wholly novel anti-pattern into the repo, and removing DP_ATTENTION from the required-var list would arguably regress the intended uniform env-contract assertion across the agentic family. That's a fair point, and it's why this should stay a nit rather than escalate to normal — but it doesn't fully neutralize the finding: several other siblings in the same model family (minimaxm3_fp8_mi300x.sh, minimaxm3_fp8_mi325x.sh) do wire DP_ATTENTION into real behavior, and this recipe's own amd-master.yaml neighbors already ship dp-attn: true rows for other configs. The inconsistency is real even if the specific failure mode is speculative and not triggered by this PR's search space.
Suggested fix: either wire DP_ATTENTION into PARALLEL_ARGS (mirroring minimaxm3_fp8_mi300x.sh) so a future dp-attn: true row behaves correctly, or drop it from check_env_vars if DP-attention is intentionally out of scope for this MTP recipe. Either fix removes the silent-mislabeling risk.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31621138962 |
cquil11
removed
the
agentx-fast
77b9f55 to
d2ff4b6
Compare
cquil11
changed the title
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31657578316 |
2 similar comments
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31657578316 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31657578316 |
Add a broad TP4/TP8 resident discovery grid plus an isolated LMCache MP offload gate for MiniMax-M3 MXFP8 on MI300X. Use vLLM EAGLE3-GQA with the committed golden acceptance length and require the single aggregate vLLM metrics endpoint. 新增 MI300X MiniMax-M3 MXFP8 的 TP4/TP8 常驻发现矩阵及独立 LMCache MP 卸载验证点。使用 vLLM EAGLE3-GQA 与已提交的黄金接受长度，并要求采集单一聚合 vLLM 指标端点。
d2ff4b6 to
074ec53
Compare
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31672590562 |
1 participant
Summary
http://localhost:$PORT/metrics, with requiredvllm:metrics.Discovery evidence
Validation
bash -n benchmarks/single_node/agentic/minimaxm3_fp8_mi300x_mtp.shutils/matrix_logic/test_generate_sweep_configs.py: 105 passed.