Skip to content

[AMD][MI355X] Update MinimaxM3 configs #2262

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
+4 −0
Open

[AMD][MI355X] Update MinimaxM3 configs #2262

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
4 changes: 4 additions & 0 deletions benchmarks/single_node/fixed_seq_len/minimaxm3_fp4_mi355x_vllm.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,14 @@
SERVER_LOG=/workspace/server.log
export VLLM_ENGINE_READY_TIMEOUT_S=3600
export VLLM_USE_BREAKABLE_CUDAGRAPH=0
export VLLM_ROCM_USE_AITER=1
export VLLM_ROCM_USE_AITER_MOE=1
export VLLM_ROCM_USE_AITER_FUSION_SHARED_EXPERTS=1
export AMDGCN_USE_BUFFER_OPS=1
export VLLM_ROCM_QUICK_REDUCE_QUANTIZATION=INT4

if [ "${EVAL_ONLY}" = "true" ]; then
setup_eval_context

Check failure on line 42 in benchmarks/single_node/fixed_seq_len/minimaxm3_fp4_mi355x_vllm.sh

View check run for this annotation

Claude / Claude Code Review

Missing perf-changelog.yaml entry for env var / flag changes 

This PR adds new env vars (`AMDGCN_USE_BUFFER_OPS`, `VLLM_ROCM_QUICK_REDUCE_QUANTIZATION`) and `vllm serve` flags (`--max-num-batched-tokens`, `--kv-cache-dtype`) to this MI355X config but doesn't append a `perf-changelog.yaml` entry — the only changed file in the diff is this `.sh` script. Per AGENTS.md's "Updating Docker images" section, appending to `perf-changelog.yaml` is required because it's what triggers the CI benchmark sweep for a given config-key; without it, this perf-tuning change w

Check warning on line 42 in benchmarks/single_node/fixed_seq_len/minimaxm3_fp4_mi355x_vllm.sh

View check run for this annotation

Claude / Claude Code Review

PR title/description missing required Simplified Chinese translation 

This PR's title and description do not follow the repo's mandatory bilingual policy: the title '[AMD][MI355X] Update MinimaxM3 configs' has no '/ <中文标题>' segment, and the description 'Enabled Quick Reduce and tuned vLLM parameters' has no accompanying Chinese translation section. Please update the title to '<English title> / <中文标题>' format and add a Chinese translation (e.g. a  section) to the body.
Comment on lines 35 to 42

@claude claude Bot Jul 17, 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 adds new env vars (AMDGCN_USE_BUFFER_OPS, VLLM_ROCM_QUICK_REDUCE_QUANTIZATION) and vllm serve flags (--max-num-batched-tokens, --kv-cache-dtype) to this MI355X config but doesn't append a perf-changelog.yaml entry — the only changed file in the diff is this .sh script. Per AGENTS.md's "Updating Docker images" section, appending to perf-changelog.yaml is required because it's what triggers the CI benchmark sweep for a given config-key; without it, this perf-tuning change won't actually be exercised/validated by the sweep.

Extended reasoning...

The bug: The diff modifies benchmarks/single_node/fixed_seq_len/minimaxm3_fp4_mi355x_vllm.sh to add two new env vars (AMDGCN_USE_BUFFER_OPS=1, VLLM_ROCM_QUICK_REDUCE_QUANTIZATION=INT4) and two new vllm serve flags (--max-num-batched-tokens 65536, --kv-cache-dtype fp8) for the minimaxm3-fp4-mi355x-vllm config. The PR's changed-files list contains exactly one file — the script itself — with no corresponding entry appended to perf-changelog.yaml.

Why this matters: AGENTS.md's "Updating Docker images" section is explicit: "Update the image tag in the relevant configs/*-master.yaml and/or benchmarks/*.sh, update any related env vars / config params, and append a perf-changelog.yaml entry (required - triggers benchmarks)". Elsewhere the same doc reiterates: "Changes to perf-changelog.yaml trigger benchmark runs." In other words, the changelog isn't documentation-only — it's the mechanism the CI sweep infrastructure keys off of to decide which config to actually re-benchmark. A .sh-only change with no changelog entry means the sweep has no signal telling it this config needs to be re-validated.

Existing repo convention confirms this is the expected pattern for this exact config. perf-changelog.yaml already has a prior entry (around line 4344-4347) for minimaxm3-fp4-mi355x-vllm describing the same class of change — enabling AITER MoE via env vars (VLLM_ROCM_USE_AITER=1, etc.) plus a new --moe-backend aiter flag — which correctly appended a changelog entry documenting the toggle and citing the PR. This PR follows the identical pattern (env-var toggles + new serve flags) but omits the corresponding entry.

Concrete walkthrough of the failure mode:

  1. PR [AMD][MI355X] Update MinimaxM3 configs #2262 changes minimaxm3_fp4_mi355x_vllm.sh to enable Quick Reduce and tune two vLLM serving parameters — the PR description states this is exactly the point of the change ("Enabled Quick Reduce and tuned vLLM parameters").
  2. The PR is merged (or a sweep label is added) without touching perf-changelog.yaml.
  3. Because the CI benchmark trigger keys off changes to perf-changelog.yaml (per AGENTS.md), and that file is untouched, there's no changelog-driven signal instructing the sweep to re-run/validate the minimaxm3-fp4-mi355x-vllm config with these new settings.
  4. The result: the tuning this PR is entirely about is never actually benchmarked or validated by the automated sweep — silently defeating the stated purpose of the PR.

Fix: Append an entry to the end of perf-changelog.yaml (entries are chronological and must be appended, never inserted/prepended) with config-keys: [minimaxm3-fp4-mi355x-vllm], a description of the new env vars/flags (mirroring the style of the existing entry at line ~4344), and a pr-link back to this PR.

Comment on lines 35 to 42

@claude claude Bot Jul 17, 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 and description do not follow the repo's mandatory bilingual policy: the title '[AMD][MI355X] Update MinimaxM3 configs' has no '/ <中文标题>' segment, and the description 'Enabled Quick Reduce and tuned vLLM parameters' has no accompanying Chinese translation section. Please update the title to ' / <中文标题>' format and add a Chinese translation (e.g. a section) to the body.

Extended reasoning...

AGENTS.md line 7 states, in mandatory language: "PR and GitHub-issue titles & descriptions must be bilingual — include a Simplified Chinese version in addition to English. Title format: <English title> / <中文标题>. In the PR/issue body, follow the English content with its Chinese translation (e.g. a ## 中文说明 section mirroring the summary...)". This is not a stylistic suggestion — it is phrased as a hard requirement ("must") that applies to "every PR and every issue."

The PR under review is titled [AMD][MI355X] Update MinimaxM3 configs. There is no / <中文标题> segment anywhere in the title, so the bilingual title format is not satisfied. The PR description reads only Enabled Quick Reduce and tuned vLLM parameters — a single English sentence with no accompanying 中文说明 section or Chinese translation of any kind.

The only carve-outs documented in AGENTS.md are narrow and don't apply here: agent-instruction files are explicitly English-only, the CODEOWNER sign-off template must stay English-verbatim (because the sign-off bot matches on the exact English phrase), and auto-generated bot comments follow their own workflow templates. The two timeline comments on this PR are indeed bot-generated (github-actions[bot], tagged <!-- recipe-reminder -->) and correctly fall under that bot-template exception — but they cannot satisfy the author's obligation to write a bilingual title/description, since they are static boilerplate unrelated to this specific change.

Step-by-step verification:

  1. Read AGENTS.md line 7 — confirms the bilingual requirement is mandatory and applies to "every PR and every issue."
  2. Read the PR metadata title: [AMD][MI355X] Update MinimaxM3 configs — no / followed by Chinese characters present.
  3. Read the PR description: Enabled Quick Reduce and tuned vLLM parameters — a single English sentence, no Chinese content, no 中文说明 heading.
  4. Cross-checked the two timeline comments — both are bot-authored recipe reminders (already bilingual themselves, by the bot's own template), not author-written content, so they don't fulfill the author's requirement.
  5. Checked AGENTS.md for exceptions — none of the listed exceptions (agent-instruction files, sign-off template, bot comments) cover a PR title or author-written description.

Impact: this is a documentation/process-compliance gap only — it does not affect the correctness of the benchmark script changes (Quick Reduce enablement, vLLM parameter tuning) and causes no build, test, or runtime failure. Fixing it is low-effort: rename the PR title to append / <中文标题>, and add a short ## 中文说明 section to the body summarizing the English description. Given the metadata-only nature and trivial fix, this should be treated as a nit rather than a blocking issue, even though the underlying rule is explicitly mandatory.

fi

PARALLEL_ARGS=(--tensor-parallel-size "$TP")
Expand All @@ -60,6 +62,8 @@
--block-size 128 \
--no-enable-prefix-caching \
--language-model-only \
--max-num-batched-tokens 65536 \
--kv-cache-dtype fp8 \
--max-model-len "$MAX_MODEL_LEN" \
--attention-backend TRITON_ATTN \
--moe-backend aiter \
Expand Down