Skip to content

[AMD/ROCM] Update minimaxm2.5-fp8-mi355x-atom config#1194

Open
seungrokj wants to merge 9 commits intomainfrom
srok/atom_minimaxm2.5_fp8
Open

[AMD/ROCM] Update minimaxm2.5-fp8-mi355x-atom config#1194
seungrokj wants to merge 9 commits intomainfrom
srok/atom_minimaxm2.5_fp8

Conversation

@seungrokj
Copy link
Copy Markdown
Collaborator

@seungrokj seungrokj commented Apr 27, 2026

Hi, This is just a minor change. Will do sweep after high priority dsv4 stuffs.

Summary

  • Update Atom image to rocm7.2.2_ubuntu24.04_py3.12_pytorch_release_2.10.0_atom0.1.2.post
  • Update search-space: expand conc-end to 256, remove tp8/ep8 configs
  • Add perf-changelog entry for minimaxm2.5-fp8-mi355x-atom

Test plan

  • CI benchmark run for minimaxm2.5-fp8-mi355x-atom

🤖 Generated with Claude Code

seungrokj and others added 6 commits April 16, 2026 08:22
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>
@seungrokj seungrokj changed the title Update minimaxm2.5-fp8-mi355x-atom config [AMD/ROCM] Update minimaxm2.5-fp8-mi355x-atom config Apr 27, 2026
@seungrokj seungrokj added the AMD label Apr 27, 2026
Comment thread perf-changelog.yaml Outdated
Comment on lines +1822 to +1826

- config-keys:
- minimaxm2.5-fp8-mi355x-atom
description:
- ""
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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:

  1. description is a single-element list whose only element is the empty string.
  2. pr-link ends in /pull/ with no number — visiting that URL on GitHub returns a 404.
  3. 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

  1. Open perf-changelog.yaml and scroll to lines 1822–1826 (the last entry).
  2. Read the description: field — it contains exactly one list item, the empty string "".
  3. Read the pr-link: field — it is https://github.com/SemiAnalysisAI/InferenceX/pull/ (no digits after the trailing slash).
  4. Open that URL in a browser → GitHub returns a 404 / redirects to the PR list.
  5. Run tail -c 1 perf-changelog.yaml | xxd (or check the diff hunk's \ No newline at end of file marker) — the file ends on the / of pull/, not on \n.
  6. 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.

Comment thread .github/configs/amd-master.yaml Outdated
Comment on lines 558 to 581
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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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:

  1. A brand-new minimaxm2.5-fp4-mi355x-atom config block in .github/configs/amd-master.yaml (24 lines, model amd/MiniMax-M2.5-MXFP4, TP1–TP8 sweep over 1k/1k and 8k/1k).
  2. A brand-new benchmarks/single_node/minimaxm2.5_fp4_mi355x_atom.sh launcher script (80 lines).
  3. A separate perf-changelog.yaml entry for minimaxm2.5-fp4-mi355x-atom whose pr-link points 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:

  • d2e4cb5Update minimaxm2.5-fp4-mi355x-atom config and add minimaxm2.5-fp4-mi355x-vllm
  • d613138Fill in perf-changelog description for minimaxm2.5-fp4-mi355x-atom
  • 0ba47daUpdate 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

  1. Check the parent commit (3d416ba) of this branch: amd-master.yaml contains minimaxm2.5-fp8-mi355x-atom, minimaxm2.5-fp4-mi355x-vllm, etc., but no minimaxm2.5-fp4-mi355x-atom block — so this PR genuinely introduces the fp4-atom config.
  2. Read the PR's Summary bullets — only the fp8 image bump, fp8 search-space change, and a fp8 changelog entry are mentioned.
  3. Read the diff: a 24-line fp4-atom YAML block and an 80-line minimaxm2.5_fp4_mi355x_atom.sh are added with no corresponding mention in the description.
  4. 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.yaml is also missing both a description and a real pr-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:

seungrokj and others added 2 commits April 27, 2026 21:15
…e fp4 atom config

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant