[AMD] [AgentX] Add MI300X Qwen3.5 FP8 SGLang MTP - #2563
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 关于重新运行失败任务的文档 |
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 关于重新运行失败任务的文档 |
cquil11
added
AMD
agentx
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| - "Collect and require SGLang Prometheus metrics from the aggregate engine endpoint" | ||
| - "Image: lmsysorg/sglang-rocm:v0.5.16-rocm720-mi30x-20260730" | ||
| pr-link: TBD |
There was a problem hiding this comment.
🔴 The new changelog entry uses pr-link: TBD, but the only accepted placeholder is XXX (or the canonical .../pull/<number> link) per utils/validate_perf_changelog.py's PR_LINK_PLACEHOLDERS. This will make utils/merge_with_reuse.sh's call to prepare_perf_changelog_merge.py raise ChangelogValidationError and block the merge-with-reuse flow — change TBD to XXX so it auto-rewrites to the real PR link at merge time.
Extended reasoning...
The appended entry at the end of perf-changelog.yaml sets pr-link: TBD. Every other historical entry in this file uses either the XXX placeholder (pre-merge) or a canonical https://github.com/SemiAnalysisAI/InferenceX/pull/<number> link (post-merge, auto-rewritten). TBD has never been used before and is not a recognized value anywhere in the validation pipeline.
utils/validate_perf_changelog.py defines PR_LINK_PLACEHOLDERS = {"XXX", "https://github.com/SemiAnalysisAI/InferenceX/pull/XXX"} (lines 24-27) and validate_added_pr_link() (lines 144-160) checks any newly-added entry's pr-link against it. On a PR run (pr_number set), the check is if link not in PR_LINK_PLACEHOLDERS and link != expected: raise ChangelogValidationError(...). TBD is neither in the placeholder set nor equal to the expected .../pull/2563 link, so this always raises. On a main-branch run (pr_number is None), the check instead requires CANONICAL_PR_LINK.fullmatch(link), which TBD also fails. So there's no code path in which TBD validates successfully.
This function is reached via compare_entries() (line 205-206), which is invoked by utils/prepare_perf_changelog_merge.py's canonicalize_appended_links at merge time. utils/merge_with_reuse.sh (lines 174 and 187-188) calls prepare_perf_changelog_merge.py --pr-number "$PR" as the standard artifact-reuse merge path (also used by the merge-prs / find-mergeable-claude-prs skills). With the real PR number supplied, validate_added_pr_link('TBD', 2563) raises ChangelogValidationError, aborting the merge before the entry's link can be canonicalized.
Concretely: had the author used XXX (the standard placeholder every other entry uses pre-merge), canonicalize_appended_links would recognize it as a placeholder and rewrite it to https://github.com/SemiAnalysisAI/InferenceX/pull/2563 automatically. Instead, TBD fails the placeholder check outright and the merge-with-reuse script errors out, requiring manual intervention to fix the changelog before merging can proceed.
The fix is a one-line change: replace pr-link: TBD with pr-link: XXX at the end of perf-changelog.yaml (around line 5807), consistent with the placeholder convention test_prepare_perf_changelog_merge.py and every other new entry in this file exercises.
| exit 1 | ||
| fi | ||
| TOTAL_CPU_DRAM_GB="$REQUESTED_HICACHE_TOTAL_GB" | ||
|
|
||
| # Qwen3.5 allocates target KV and Mamba host pools per rank. Native MTP | ||
| # adds a one-attention-layer draft KV pool (1/15 of target KV), so enforce | ||
| # H * 31/15 per rank against the workflow's node-total DRAM budget. | ||
| HICACHE_ALIGNMENT_RESERVE_GB=$TP | ||
| HICACHE_USABLE_TOTAL_GB=$((TOTAL_CPU_DRAM_GB - HICACHE_ALIGNMENT_RESERVE_GB)) | ||
| if [ "$HICACHE_USABLE_TOTAL_GB" -lt 1 ]; then | ||
| echo "Error: insufficient DRAM after HiCache alignment reserve" >&2 | ||
| exit 1 | ||
| fi | ||
| MAX_HICACHE_SIZE_GB=$((HICACHE_USABLE_TOTAL_GB * 15 / TP / 31)) | ||
| HICACHE_SIZE_GB="${HICACHE_SIZE_GB:-$MAX_HICACHE_SIZE_GB}" | ||
| if [ "$HICACHE_SIZE_GB" -lt 1 ] || [ "$HICACHE_SIZE_GB" -gt "$MAX_HICACHE_SIZE_GB" ]; then | ||
| echo "Error: HICACHE_SIZE_GB=$HICACHE_SIZE_GB outside 1..$MAX_HICACHE_SIZE_GB" >&2 | ||
| exit 1 | ||
| fi | ||
| PROJECTED_HICACHE_TOTAL_GB=$(((HICACHE_SIZE_GB * TP * 31 + 14) / 15 + HICACHE_ALIGNMENT_RESERVE_GB)) | ||
| if [ "$PROJECTED_HICACHE_TOTAL_GB" -gt "$TOTAL_CPU_DRAM_GB" ]; then | ||
| echo "Error: projected HiCache use ${PROJECTED_HICACHE_TOTAL_GB} GB exceeds configured capacity ${TOTAL_CPU_DRAM_GB} GB" >&2 | ||
| exit 1 | ||
| fi | ||
| echo "HiCache CPU pools: ${HICACHE_SIZE_GB} GB target + Mamba + 1/15 draft per rank across TP=${TP}; projected node total ${PROJECTED_HICACHE_TOTAL_GB} GB <= ${TOTAL_CPU_DRAM_GB} GB" | ||
|
|
There was a problem hiding this comment.
🟡 sweep:HICACHE_ALIGNMENT_RESERVE_GB=$TP
The ~25-line HiCache CPU-pool sizing block (lines 51-76) is byte-for-byte identical (only comments differ) to the block already duplicated in qwen3.5_fp8_b200_sglang_mtp.sh, qwen3.5_fp8_b300_sglang_mtp.sh, qwen3.5_fp4_b200_sglang_mtp.sh, and qwen3.5_fp4_b300_sglang_mtp.sh, making this the fifth hand-maintained copy. Per AGENTS.md ("Shared benchmark Bash behavior belongs in benchmark_lib.sh, with parameters passed through environment variables"), this should be a benchmark_lib.sh helper parameterized by TP and TOTAL_CPU_DRAM_GB.
Extended reasoning...
This PR introduces a new benchmark script, qwen3.5_fp8_mi300x_mtp.sh, whose HiCache CPU-pool sizing block (lines 51-76: HICACHE_ALIGNMENT_RESERVE_GB, HICACHE_USABLE_TOTAL_GB, MAX_HICACHE_SIZE_GB, HICACHE_SIZE_GB, PROJECTED_HICACHE_TOTAL_GB, plus the three associated bounds checks) is byte-for-byte identical arithmetic to the block already present in at least four sibling scripts: qwen3.5_fp8_b200_sglang_mtp.sh, qwen3.5_fp8_b300_sglang_mtp.sh, qwen3.5_fp4_b200_sglang_mtp.sh, and qwen3.5_fp4_b300_sglang_mtp.sh. Diffing this file against the b200 copy shows the divisor (15/TP/31), the projection formula ((X*TP*31+14)/15 + reserve), the alignment reserve (= TP), and the three error messages all match exactly — only the surrounding comment wording differs.
The reason existing code does not prevent this is structural: each new single-node recipe script is written from a copy of the most similar existing script, and the HiCache sizing math travels along with the copy rather than being pulled from the shared benchmark_lib.sh that this file already sources (source "$(dirname "$0")/../../benchmark_lib.sh"). AGENTS.md states explicitly (line 17): "Shared benchmark Bash behavior belongs in benchmark_lib.sh, with parameters passed through environment variables." Both inputs this block needs — TP and TOTAL_CPU_DRAM_GB (or HICACHE_TOTAL_CPU_DRAM_GB) — are already environment variables by the time this block runs, so the block is a clean, drop-in candidate for a shared function such as compute_hicache_size_gb that takes those two values and returns/exports HICACHE_SIZE_GB and PROJECTED_HICACHE_TOTAL_GB, with the bounds-check exit 1 calls folded in.
Concretely, the impact is that the 31/15 draft-KV ratio and the TP alignment-reserve formula are now maintained by hand in five separate files instead of one. If a future change is needed — e.g. the MTP draft pool ratio changes from 1/15 to something else as more speculative-num-steps values are exercised, or the alignment reserve needs adjusting for a new SKU — a maintainer must remember to find and edit all five copies identically. Missing one silently reintroduces stale sizing logic in that script, which would only surface later as an inconsistent capacity check or an incorrect projected-DRAM figure on that specific SKU, likely well after this PR has merged.
As a concrete walk-through: today, all 5 copies compute MAX_HICACHE_SIZE_GB=$((HICACHE_USABLE_TOTAL_GB * 15 / TP / 31)) identically. Suppose a later PR bumps MTP draft depth such that the draft pool becomes 2/15 of target KV instead of 1/15 — the ratio in the formula needs to change from 15/31 to 15/32 (or similar) in every file. A maintainer patching only the file they are actively working on (say, the B300 script) leaves this MI300X script, and the FP4 B200/B300 scripts, silently using the stale 31-based ratio. The four unpatched scripts would then either over-reserve HiCache pool (wasting DRAM budget) or under-reserve it (risking an OOM at runtime) without any error, since the arithmetic itself remains internally consistent — it is just consistent with the wrong ratio.
The fix is to add a helper to benchmark_lib.sh, e.g. compute_hicache_size_gb TP TOTAL_CPU_DRAM_GB [HICACHE_SIZE_GB], that performs the alignment-reserve, usable-total, max-size, and projected-total derivations plus the three bounds checks, and have this script (and ideally the four existing siblings, in a follow-up) call it instead of inlining the block. This is a pure code-quality/DRY finding — the math itself is correct and the PR functions as intended — so it should not block merging, but flagging it now avoids the copy growing to a sixth or seventh occurrence in the next MI300X/MI355X recipe.
cquil11
changed the title
Prune the completed fast-discovery grid to representative EP1 and EP8 points spanning the latency floor, throughput knee, and first post-knee regression. Remove the nonfunctional HiCache path rejected by the pinned SGLang hybrid-cache layout.
cquil11
removed
full-sweep-enabled
agentx-fast
3f8b29a to
4292294
Compare
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31544070583 |
2 participants
Summary
Adds
qwen3.5-fp8-mi300x-sglang-agentic-mtp, an aggregate single-node AgentX recipe for Qwen3.5-397B-A17B FP8 on 8×MI300X with SGLang native EAGLE MTP.agentx-fastdiscovery compared TP8/EP1 and TP8/EP8 at c1–c64 plus HiCache c32–c76. The official matrix keeps c4/c16/c20/c24/c32 for both topologies: latency floor, rise, balanced point, c24 throughput knee, and the first post-knee regression at c32.http://localhost:$PORT/metrics, and the result is rejected unlesssglang:metrics are present. Disaggregation is not applicable.Discovery evidence: 40-point canonical fast run.
Validation
bash -npasses.test-configgenerates exactly 10 one-hour AgentX throughput rows, all MTP, oncluster:mi300x-amds.utils/matrix_logic/test_generate_sweep_configs.py: 105 passed.utils/validate_perf_changelog.pypasses.git diff --checkpasses.