Skip to content

[AMD][MI355X] Update MinimaxM3 with TP2EP1 #2446

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ukannika wants to merge 1 commit into
base: main
Choose a base branch
from
+1 −1
Open

[AMD][MI355X] Update MinimaxM3 with TP2EP1 #2446

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion configs/amd-master.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1840,10 +1840,10 @@
multinode: false
scenarios:
fixed-seq-len:
- isl: 8192
osl: 1024
search-space:
- { tp: 4, conc-start: 1, conc-end: 512 }
- { tp: 2, conc-start: 1, conc-end: 512 }

Check failure on line 1846 in configs/amd-master.yaml

View check run for this annotation

Claude / Claude Code Review

Missing perf-changelog.yaml entry for search-space change 

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.

Check warning on line 1846 in configs/amd-master.yaml

View check run for this annotation

Claude / Claude Code Review

PR title missing required bilingual (Chinese) translation 

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 ("<English title> / <中文标题>"). Fix: append the Chinese title (and ensure the PR description carries the mirrored 中文说明).

Check warning on line 1846 in configs/amd-master.yaml

View check run for this annotation

Claude / Claude Code Review

Stale comment: fp4 entry comment claims it mirrors fp8 search space 

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.
Comment on lines 1843 to +1846

@claude claude Bot Aug 1, 2026

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.

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

Comment on lines 1843 to +1846

@claude claude Bot Aug 1, 2026

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.

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

Comment on lines 1843 to +1846

@claude claude Bot Aug 1, 2026

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 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.


# EAGLE3 speculative-decoding variant of minimaxm3-fp4-mi355x-vllm. Pair the
# amd/MiniMax-M3-MXFP4 target with Inferact/MiniMax-M3-EAGLE3 and three draft
Expand Down