[AMD/ROCM] Update minimaxm2.5-fp8-mi355x-atom config#1194
[AMD/ROCM] Update minimaxm2.5-fp8-mi355x-atom config#1194
Conversation
Signed-off-by: seungrokj <seungrok.jung@amd.com>
…55x-vllm Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
|
||
| - config-keys: | ||
| - minimaxm2.5-fp8-mi355x-atom | ||
| description: | ||
| - "" |
There was a problem hiding this comment.
🔴 The new perf-changelog entry for minimaxm2.5-fp8-mi355x-atom (lines 1822–1826) is incomplete: description contains only an empty string, pr-link is the bare https://github.com/SemiAnalysisAI/InferenceX/pull/ with no PR number (404s), and the file no longer ends with a trailing newline. Please populate the description with the changes (image bump to rocm7.2.2_ubuntu24.04_py3.12_pytorch_release_2.10.0_atom0.1.2.post, conc-end expanded to 256, removal of tp8/ep8 configs), set pr-link to https://github.com/SemiAnalysisAI/InferenceX/pull/1194, and add a final newline.
Extended reasoning...
What the bug is
This PR adds a perf-changelog entry whose explicit purpose (per the PR description: "Add perf-changelog entry for minimaxm2.5-fp8-mi355x-atom") is to document the FP8 Atom changes. However, the entry is structurally present but semantically empty:
- config-keys:
- minimaxm2.5-fp8-mi355x-atom
description:
- ""
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/Three concrete defects sit on adjacent lines:
descriptionis a single-element list whose only element is the empty string.pr-linkends in/pull/with no number — visiting that URL on GitHub returns a 404.- The diff trailer shows
\ No newline at end of file, so the file no longer ends with a newline.
Why existing code doesn't prevent it
perf-changelog.yaml is YAML documentation; nothing in the repo schema-validates description non-emptiness, pr-link well-formedness, or trailing newlines. Every other recent entry in the file consistently includes a real description and a real PR number (e.g. /pull/1042, /pull/1129, /pull/1148). A handful use placeholder /pull/XXX//pull/XXXX, but none use a bare /pull/ with nothing after the slash. So this entry is the only one that's actively broken at the URL level.
Notably, the previous commit on this branch (d613138) was titled "Fill in perf-changelog description for minimaxm2.5-fp4-mi355x-atom" and addresses the FP4 sibling — the FP8 entry was simply left in its placeholder state.
Impact
Any tool that consumes the changelog (release notes generators, dashboards, link checkers) will hit a 404 on pr-link. Human readers will see an entry that says nothing about what changed, which directly contradicts the stated PR deliverable. The missing trailing newline is a minor hygiene issue: it produces noisy diffs on the next edit (the next addition will appear as a 2-line change rather than a 1-line addition) and can confuse cat-style concatenation.
Step-by-step proof
- Open
perf-changelog.yamland scroll to lines 1822–1826 (the last entry). - Read the
description:field — it contains exactly one list item, the empty string"". - Read the
pr-link:field — it ishttps://github.com/SemiAnalysisAI/InferenceX/pull/(no digits after the trailing slash). - Open that URL in a browser → GitHub returns a 404 / redirects to the PR list.
- Run
tail -c 1 perf-changelog.yaml | xxd(or check the diff hunk's\ No newline at end of filemarker) — the file ends on the/ofpull/, not on\n. - Compare with the FP4 sibling entry just above (lines 1816–1821, PR [WIP] [AMD/ROCM] atom minimaxm2.5 fp4 on mi355x #1042): it has a populated 2-item description and a real PR number, which is the format this entry should match.
How to fix
Replace the entry with something like:
- config-keys:
- minimaxm2.5-fp8-mi355x-atom
description:
- "Update Atom image to rocm/atom:rocm7.2.2_ubuntu24.04_py3.12_pytorch_release_2.10.0_atom0.1.2.post"
- "Search-space: expand conc-end to 256 for tp2/tp4 (1k/1k and 8k/1k); remove tp8/ep8 configs"
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1194…and ensure the file ends with a newline. This single edit resolves all three defects.
| model: amd/MiniMax-M2.5-MXFP4 | ||
| model-prefix: minimaxm2.5 | ||
| runner: mi355x | ||
| precision: fp4 | ||
| framework: atom | ||
| multinode: false | ||
| seq-len-configs: | ||
| - isl: 1024 | ||
| osl: 1024 | ||
| search-space: | ||
| - { tp: 1, conc-start: 4, conc-end: 1024 } | ||
| - { tp: 2, conc-start: 4, conc-end: 1024 } | ||
| - { tp: 4, conc-start: 4, conc-end: 128 } | ||
| - { tp: 8, ep: 8, conc-start: 32, conc-end: 256 } | ||
| - { tp: 8, conc-start: 4, conc-end: 16 } | ||
| - isl: 8192 | ||
| osl: 1024 | ||
| search-space: | ||
| - { tp: 2, conc-start: 4, conc-end: 128 } | ||
| - { tp: 1, conc-start: 4, conc-end: 1024 } | ||
| - { tp: 2, conc-start: 4, conc-end: 1024 } | ||
| - { tp: 4, conc-start: 4, conc-end: 128 } | ||
| - { tp: 8, ep: 8, conc-start: 32, conc-end: 256 } | ||
| - { tp: 8, conc-start: 4, conc-end: 16 } | ||
|
|
||
| minimaxm2.5-fp4-mi355x-vllm: | ||
| image: vllm/vllm-openai-rocm:v0.19.1 |
There was a problem hiding this comment.
🟡 PR title/description only mention the fp8 image/search-space update, but the diff also adds an entirely new minimaxm2.5-fp4-mi355x-atom config block in amd-master.yaml plus a brand-new benchmarks/single_node/minimaxm2.5_fp4_mi355x_atom.sh launcher script and a perf-changelog entry attributing the fp4 work to PR #1042. Either expand the description to disclose the fp4 additions, or rebase off PR #1042 once it merges so this PR is purely the fp8 update — branch name srok/atom_minimaxm2.5_fp4 suggests the fp4 work was meant to land separately.
Extended reasoning...
What the issue is
The PR is titled "Update minimaxm2.5-fp8-mi355x-atom config" and its Summary bullets only describe three fp8-scoped changes:
- Update the Atom image
- Update fp8 search-space (expand conc-end to 256, remove tp8/ep8)
- Add a perf-changelog entry for
minimaxm2.5-fp8-mi355x-atom
But the diff actually contains a second, unrelated piece of work that is never mentioned:
- A brand-new
minimaxm2.5-fp4-mi355x-atomconfig block in.github/configs/amd-master.yaml(24 lines, modelamd/MiniMax-M2.5-MXFP4, TP1–TP8 sweep over 1k/1k and 8k/1k). - A brand-new
benchmarks/single_node/minimaxm2.5_fp4_mi355x_atom.shlauncher script (80 lines). - A separate
perf-changelog.yamlentry forminimaxm2.5-fp4-mi355x-atomwhosepr-linkpoints to PR [WIP] [AMD/ROCM] atom minimaxm2.5 fp4 on mi355x #1042, not this PR.
How it manifests / why this isn't just noise
The branch name srok/atom_minimaxm2.5_fp4 and commit history corroborate that the fp4 work is the original purpose of the underlying branch and the fp8 changes were layered on later:
d2e4cb5— Update minimaxm2.5-fp4-mi355x-atom config and add minimaxm2.5-fp4-mi355x-vllmd613138— Fill in perf-changelog description for minimaxm2.5-fp4-mi355x-atom0ba47da— Update minimaxm2.5-fp8-mi355x-atom config and add perf-changelog entry
So the fp4 commits predate the fp8 commit on this branch, and the fp4 changelog already attributes itself to a different PR (#1042), strongly implying the fp4 work belongs in #1042 and is riding along here only because #1042 has not yet merged.
Step-by-step proof
- Check the parent commit (
3d416ba) of this branch:amd-master.yamlcontainsminimaxm2.5-fp8-mi355x-atom,minimaxm2.5-fp4-mi355x-vllm, etc., but nominimaxm2.5-fp4-mi355x-atomblock — so this PR genuinely introduces the fp4-atom config. - Read the PR's Summary bullets — only the fp8 image bump, fp8 search-space change, and a fp8 changelog entry are mentioned.
- Read the diff: a 24-line fp4-atom YAML block and an 80-line
minimaxm2.5_fp4_mi355x_atom.share added with no corresponding mention in the description. - Read the new fp4 changelog entry:
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1042— it credits a different PR.
Impact
This is a documentation/scope-disclosure issue, not a code defect. The fp4 config block and shell script themselves look well-formed and consistent with sibling configs. But:
- A reviewer reading only the PR description will be surprised by the fp4 additions and may merge content they did not intend to review.
- If PR [WIP] [AMD/ROCM] atom minimaxm2.5 fp4 on mi355x #1042 lands separately with the same fp4 changelog entry / config block / script, you'll get a merge conflict or duplicate entries.
- The fp8-only changelog entry added at the bottom of
perf-changelog.yamlis also missing both a description and a realpr-link(pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/), and the file ends without a trailing newline — minor hygiene issues that suggest this commit was rushed.
How to fix
Pick one:
- Preferred: rebase this branch onto PR [WIP] [AMD/ROCM] atom minimaxm2.5 fp4 on mi355x #1042 once [WIP] [AMD/ROCM] atom minimaxm2.5 fp4 on mi355x #1042 merges, so this PR's diff contains only the fp8 image/search-space update plus the fp8 changelog entry.
- Otherwise: update the PR title and Summary to explicitly call out that this PR also adds
minimaxm2.5-fp4-mi355x-atom(config + benchmark script + changelog entry), explain the relationship to PR [WIP] [AMD/ROCM] atom minimaxm2.5 fp4 on mi355x #1042, and either drop the duplicate fp4 changelog entry here or update itspr-linkto this PR.
…e fp4 atom config Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Hi, This is just a minor change. Will do sweep after high priority dsv4 stuffs.
Summary
rocm7.2.2_ubuntu24.04_py3.12_pytorch_release_2.10.0_atom0.1.2.postTest plan
🤖 Generated with Claude Code