[AMD][MI355X] Update MinimaxM3 with TP2EP1 - #2446
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 关于重新运行失败任务的文档 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deferring — a blocking issue was found (missing perf-changelog.yaml entry for this master-config search-space change), which the repo's own review policy treats as required before merge.
Extended reasoning...
Overview
This PR is a single-line change to configs/amd-master.yaml, dropping the minimaxm3-fp4-mi355x-vllm search-space tp from 4 to 2 (TP2EP1 tuning). No other files are touched.
Security risks
None. This is a benchmark configuration value with no security-sensitive surface (no auth, no secrets, no code execution paths altered).
Level of scrutiny
This would normally warrant light scrutiny as a mechanical one-line config tweak, but the repo's own claude-pr-review.yml workflow explicitly marks master-config edits without a paired perf-changelog.yaml entry as a 🔴 BLOCKING issue (lines 122-126), since perf-changelog.yaml is what triggers the dedicated benchmark sweep for the changed config. That rule is met here — only configs/amd-master.yaml changed — so per the repo's own written policy this should not be approved as-is. The PR title is also missing the bilingual (Chinese) title required by AGENTS.md.
Other factors
The underlying tp:4->tp:2 change itself looks reasonable (FP4 weights have a smaller footprint, and TP2 is plausible for that), but without a perf-changelog entry the sweep automation may not validate the new value before merge, which is the exact failure mode the blocking rule exists to prevent. This is a quick fix for the author (append one perf-changelog entry and add the Chinese title) rather than something requiring redesign.
| - isl: 8192 | ||
| osl: 1024 | ||
| search-space: | ||
| - { tp: 4, conc-start: 1, conc-end: 512 } | ||
| - { tp: 2, conc-start: 1, conc-end: 512 } |
There was a problem hiding this comment.
🔴 This PR changes minimaxm3-fp4-mi355x-vllm's search-space tp from 4 to 2 in configs/amd-master.yaml (line 1846) but doesn't add a matching perf-changelog.yaml entry, which the repo's own review workflow flags as a blocking issue. Please append an entry to the end of perf-changelog.yaml documenting the TP2EP1 change, since perf-changelog.yaml is what triggers the dedicated benchmark sweep for this config.
Extended reasoning...
The PR's only changed file is configs/amd-master.yaml, where the minimaxm3-fp4-mi355x-vllm search-space is changed from { tp: 4, conc-start: 1, conc-end: 512 } to { tp: 2, conc-start: 1, conc-end: 512 } (lines 1843-1846). No corresponding entry was added to perf-changelog.yaml.
This repository explicitly requires that master-config edits be paired with a perf-changelog entry. .github/workflows/claude-pr-review.yml (lines 118-126) states verbatim: 'If either master config file was edited AND perf-changelog.yaml was NOT edited in the same PR: This is a 🔴 BLOCKING issue... Comment that perf-changelog.yaml must also be updated when master config files are changed.' AGENTS.md corroborates this in multiple places: line 39 describes perf-changelog.yaml as the 'benchmark trigger log,' line 78 states master configs define benchmark configurations while perf-changelog.yaml 'triggers which configs to benchmark,' and line 154 says an appended changelog entry is 'required — triggers benchmarks' whenever a master config's image/params change.
The reason this matters functionally, not just procedurally: perf-changelog.yaml is the path-filter trigger that run-sweep.yml watches to decide which benchmark configs get a dedicated validation run. Editing amd-master.yaml alone, without touching perf-changelog.yaml, means this tp: 4 -> tp: 2 change for minimaxm3-fp4-mi355x-vllm may not get its own sweep validation before merge — exactly the failure mode this repo's review automation was built to catch.
Step-by-step proof: (1) Diff the PR — only configs/amd-master.yaml is touched, confirmed by the PR's changed-files count=1. (2) Grep perf-changelog.yaml for any reference to this PR/change — none exists, since the file wasn't touched. (3) Apply the exact BLOCKING rule from claude-pr-review.yml: master config edited (yes) AND perf-changelog.yaml not edited (yes, confirmed) => BLOCKING per the repo's own automated check. (4) Cross-check precedent: prior tp/search-space changes to master configs in this repo (e.g. PR #2213 for kimik2.5-fp4-mi355x-vllm) each paired the config edit with a perf-changelog entry, confirming this is the established, enforced convention and not merely aspirational guidance.
The fix is small: append an entry to the end of perf-changelog.yaml (never insert in the middle, per AGENTS.md line 78 and the whitespace-sensitivity warning at line 178) describing the minimaxm3-fp4-mi355x-vllm TP2EP1 search-space change and linking this PR. This both satisfies the repo's mandated review check and ensures the sweep actually re-validates the new tp:2 configuration.
| - isl: 8192 | ||
| osl: 1024 | ||
| search-space: | ||
| - { tp: 4, conc-start: 1, conc-end: 512 } | ||
| - { tp: 2, conc-start: 1, conc-end: 512 } |
There was a problem hiding this comment.
🟡 This PR's title, "[AMD][MI355X] Update MinimaxM3 with TP2EP1", is English-only and lacks the Simplified Chinese translation that AGENTS.md mandates for every PR (" / <中文标题>"). Fix: append the Chinese title (and ensure the PR description carries the mirrored 中文说明).
Extended reasoning...
AGENTS.md (linked from the repo's CLAUDE.md, so its rules apply to every PR opened against this repo) states verbatim: "PR and GitHub-issue titles & descriptions must be bilingual — include a Simplified Chinese version in addition to English. Title format: <English title> / <中文标题>. ... This applies to every PR and every issue." This is not a stylistic suggestion — it's phrased as a mandatory, repo-wide convention with a specified exact format.
The PR under review is titled [AMD][MI355X] Update MinimaxM3 with TP2EP1. There is no / <中文标题> segment anywhere in the title, so it plainly does not satisfy the documented format. The PR timeline (the two automated recipe-reminder comments) also doesn't supply the missing Chinese title — those are boilerplate bot comments, not an author-provided translation.
Why nothing else catches this: none of the automated checks visible in this PR (the recipe-reminder comment, sweep labels, CODEOWNER sign-off bot) enforce the bilingual-title rule — those gates cover the PR_REVIEW_CHECKLIST template and sweep labeling, not title language. So a PR can pass every automated check here while still violating the AGENTS.md bilingual convention, since nothing else in the pipeline is positioned to catch it.
Concrete proof: take the actual PR title string, [AMD][MI355X] Update MinimaxM3 with TP2EP1, and apply the required format <English title> / <中文标题>. Splitting the string on " / " yields only one segment (the English title) — the right-hand <中文标题> component is absent. Per the rule's own literal format check, the title fails validation.
Impact is limited to documentation/process compliance, not runtime behavior — the underlying config change in configs/amd-master.yaml (dropping minimaxm3-fp4-mi355x-vllm's TP from 4 to 2) works correctly regardless of the title. The fix is trivial: rename the PR title to append a Chinese translation, e.g. [AMD][MI355X] Update MinimaxM3 with TP2EP1 / 更新 MinimaxM3 为 TP2EP1 配置, and mirror the description with a ## 中文说明 section per the same AGENTS.md guidance.
| - isl: 8192 | ||
| osl: 1024 | ||
| search-space: | ||
| - { tp: 4, conc-start: 1, conc-end: 512 } | ||
| - { tp: 2, conc-start: 1, conc-end: 512 } |
There was a problem hiding this comment.
🟡 The design-rationale comment above minimaxm3-fp4-mi355x-vllm still says the search space should 'mirror the MXFP8 MI355X search space for a direct precision comparison,' but this PR drops the fp4 entry to tp:2 while minimaxm3-fp8-mi355x-vllm elsewhere in the file stays at tp:4, so the two no longer mirror on parallelism. The comment should be updated to note the fp4 entry now runs at TP2 (smaller weight footprint) rather than mirroring the fp8 TP4 config.
Extended reasoning...
The comment block directly above minimaxm3-fp4-mi355x-vllm (a few lines before line 1843) explains the rationale for the recipe's search space: "Use the text-only language-model path and mirror the MXFP8 MI355X search space for a direct precision comparison." Before this PR, both minimaxm3-fp4-mi355x-vllm and minimaxm3-fp8-mi355x-vllm used identical search spaces — { tp: 4, conc-start: 1, conc-end: 512 } — so the comment was literally accurate.
This PR's only change is on line 1846, dropping the fp4 entry's tp from 4 to 2 (per the PR title, "Update MinimaxM3 with TP2EP1"). The sibling minimaxm3-fp8-mi355x-vllm entry elsewhere in the same file is untouched and remains at tp: 4. As a result, the two search spaces now differ on the parallelism dimension (TP2 fp4 vs. TP4 fp8), even though the concurrency sweep range (1–512) still matches. The comment's claim that the fp4 entry "mirror[s] the MXFP8 MI355X search space for a direct precision comparison" is therefore no longer accurate for the TP dimension, and it overstates how apples-to-apples the fp4-vs-fp8 comparison now is (different GPU counts per replica implies a different per-GPU memory/compute profile).
Nothing in the existing code or config schema catches this kind of drift — the comment is free text next to the YAML entry, so there's no mechanism that would flag a TP mismatch against the stated intent. Anyone reading the two entries side-by-side to compare fp4 vs. fp8 throughput, using the comment as a guide, would incorrectly assume the sweeps are TP-matched.
To reproduce the mismatch: (1) open configs/amd-master.yaml and find minimaxm3-fp8-mi355x-vllm's search-space — { tp: 4, conc-start: 1, conc-end: 512 }; (2) find minimaxm3-fp4-mi355x-vllm's rationale comment, which says to mirror that fp8 search space; (3) look at its actual search-space after this PR — { tp: 2, conc-start: 1, conc-end: 512 }; (4) tp:2 vs tp:4 is not a mirror, so the comment's central claim no longer holds on that axis.
This is purely a documentation-staleness issue — no runtime behavior changes, the sweep still executes correctly, and dropping fp4 to TP2 is a deliberate, reasonable tuning choice (MXFP4 weights are roughly half the byte footprint of MXFP8, so TP2 is a sensible fit). The concurrency range still mirrors exactly. Given that, and per the review rubric, this doesn't rise to a blocking issue — it's a one-word comment fix (e.g., "...mirror the MXFP8 MI355X search space's concurrency sweep, now at TP2 for the smaller FP4 footprint...") that avoids misleading future readers about how comparable the two configs are on parallelism.
1 participant
No description provided.