Update Minimax M3 B300 FP4 vllm#1994
Conversation
wzhao18
requested review from
Ankur-singh,
jgangani and
kedarpotdar-nv
as code owners
|
Thanks for the contribution! For vLLM & SGLang, please ensure that your recipes is similar to the official vLLM recipes and/or the SGLang cookbook If it is not, please create a PR first before we can merge your single node PR into the master branch. Let's ensure that the documentation is first class such that the entire ML community can benefit from your hard work! Thank you 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. If re-running failed jobs is attempted, PR authors are responsible for ensuring it passes. See GitHub's docs on re-running failed jobs: https://docs.github.com/en/actions/how-tos/manage-workflow-runs/re-run-workflows-and-jobs#re-running-failed-jobs-in-a-workflow As a rule of thumb, generally, PR authors should request a review & get a PR approval from the respective companies' CODEOWNERS before requesting a review from core maintainers. If additional help is needed, PR authors can reach out to core maintainers over Slack. 感谢你的贡献！对于 vLLM 与 SGLang，请确保你的 recipe 与官方 vLLM recipes 和/或 SGLang cookbook 保持一致 如果不一致，请先创建一个 PR，之后我们才能将你的单节点 PR 合并到 master 分支。让我们确保文档保持一流水准，使整个 ML 社区都能从你的辛勤工作中受益！谢谢 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。如果选择重新运行失败的任务，PR 作者有责任确保其最终通过。参见 GitHub 关于重新运行失败任务的文档：https://docs.github.com/en/actions/how-tos/manage-workflow-runs/re-run-workflows-and-jobs#re-running-failed-jobs-in-a-workflow 一般而言，PR 作者应先向相应公司的 CODEOWNERS 请求审阅并获得 PR 批准，然后再请求核心维护者审阅。 如需更多帮助，PR 作者可通过 Slack 联系核心维护者。 |
2 similar comments
|
Thanks for the contribution! For vLLM & SGLang, please ensure that your recipes is similar to the official vLLM recipes and/or the SGLang cookbook If it is not, please create a PR first before we can merge your single node PR into the master branch. Let's ensure that the documentation is first class such that the entire ML community can benefit from your hard work! Thank you 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. If re-running failed jobs is attempted, PR authors are responsible for ensuring it passes. See GitHub's docs on re-running failed jobs: https://docs.github.com/en/actions/how-tos/manage-workflow-runs/re-run-workflows-and-jobs#re-running-failed-jobs-in-a-workflow As a rule of thumb, generally, PR authors should request a review & get a PR approval from the respective companies' CODEOWNERS before requesting a review from core maintainers. If additional help is needed, PR authors can reach out to core maintainers over Slack. 感谢你的贡献！对于 vLLM 与 SGLang，请确保你的 recipe 与官方 vLLM recipes 和/或 SGLang cookbook 保持一致 如果不一致，请先创建一个 PR，之后我们才能将你的单节点 PR 合并到 master 分支。让我们确保文档保持一流水准，使整个 ML 社区都能从你的辛勤工作中受益！谢谢 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。如果选择重新运行失败的任务，PR 作者有责任确保其最终通过。参见 GitHub 关于重新运行失败任务的文档：https://docs.github.com/en/actions/how-tos/manage-workflow-runs/re-run-workflows-and-jobs#re-running-failed-jobs-in-a-workflow 一般而言，PR 作者应先向相应公司的 CODEOWNERS 请求审阅并获得 PR 批准，然后再请求核心维护者审阅。 如需更多帮助，PR 作者可通过 Slack 联系核心维护者。 |
|
Thanks for the contribution! For vLLM & SGLang, please ensure that your recipes is similar to the official vLLM recipes and/or the SGLang cookbook If it is not, please create a PR first before we can merge your single node PR into the master branch. Let's ensure that the documentation is first class such that the entire ML community can benefit from your hard work! Thank you 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. If re-running failed jobs is attempted, PR authors are responsible for ensuring it passes. See GitHub's docs on re-running failed jobs: https://docs.github.com/en/actions/how-tos/manage-workflow-runs/re-run-workflows-and-jobs#re-running-failed-jobs-in-a-workflow As a rule of thumb, generally, PR authors should request a review & get a PR approval from the respective companies' CODEOWNERS before requesting a review from core maintainers. If additional help is needed, PR authors can reach out to core maintainers over Slack. 感谢你的贡献！对于 vLLM 与 SGLang，请确保你的 recipe 与官方 vLLM recipes 和/或 SGLang cookbook 保持一致 如果不一致，请先创建一个 PR，之后我们才能将你的单节点 PR 合并到 master 分支。让我们确保文档保持一流水准，使整个 ML 社区都能从你的辛勤工作中受益！谢谢 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。如果选择重新运行失败的任务，PR 作者有责任确保其最终通过。参见 GitHub 关于重新运行失败任务的文档：https://docs.github.com/en/actions/how-tos/manage-workflow-runs/re-run-workflows-and-jobs#re-running-failed-jobs-in-a-workflow 一般而言，PR 作者应先向相应公司的 CODEOWNERS 请求审阅并获得 PR 批准，然后再请求核心维护者审阅。 如需更多帮助，PR 作者可通过 Slack 联系核心维护者。 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=28624280386 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=28624757873 |
wzhao18
force-pushed
the
wzhao/minimax-m3-b300-fp4
branch
from
bb7d76a to
cc531c2
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
perf-changelog.yaml:4437-4441— The perf-changelog entry for this PR omits three runtime knobs added inbenchmarks/single_node/fixed_seq_len/minimaxm3_fp4_b300.shthat directly affect measured throughput/latency:VLLM_FLASHINFER_ALLREDUCE_BACKEND=trtllm,--gpu-memory-utilizationbumped 0.90→0.95, and new--kv-cache-dtype fp8. AGENTS.md line 126-134 documents that env-var / config-param changes should be listed as separate bullets alongside the image bump. Adding bullets for these three preserves the historical record so future regressions/gains can be attributed to the specific knob.Extended reasoning...
What's missing
The new perf-changelog entry added by this PR (
perf-changelog.yamlline 4437-4441) currently reads:- config-keys: - minimaxm3-fp4-b300-vllm description: - "Update Minimax M3 b300 vllm image tag" - "Update search space to cover more configs" pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1994
But
git diff ae1c88a..HEAD -- benchmarks/single_node/fixed_seq_len/minimaxm3_fp4_b300.shshows three additional material runtime changes that are not called out in the description:+ export VLLM_FLASHINFER_ALLREDUCE_BACKEND=trtllm(line 41) — new env var, swaps the allreduce collective path.--gpu-memory-utilization 0.90 -> 0.95(line 60) — aggressive tuning, changes KV-cache footprint.+ --kv-cache-dtype fp8(line 62) — changes KV cache dtype, affects both throughput and accuracy.
Why this is the expected pattern
AGENTS.mdline 126 explicitly says: "Update the image tag in the relevantconfigs/*-master.yamland/orbenchmarks/*.sh, update any related env vars / config params, and append aperf-changelog.yamlentry", and the template on lines 128-134 shows env-var changes listed as distinct bullets alongside the image bump ("Add VLLM_MXFP4_USE_MARLIN=1 environment variable"). Comparable recent entries follow this pattern — see PR #1962 (lines 4413-4418) and PR #1975 (lines 4423-4427) which enumerate flags/env vars alongside image/search-space bumps.Impact
CI will not fail — the validator (
utils/validate_perf_changelog.py) only checks structural properties, not description completeness. But the changelog is the historical record used to attribute future perf shifts to a specific PR knob. With three material runtime knobs undocumented, a future regression triage (or gain investigation) onminimaxm3-fp4-b300-vllmwill have to diff the shell script by hand to figure out what changed.Suggested fix
Extend the description to something like:
description: - "Update Minimax M3 b300 vllm image tag" - "Update search space to cover more configs" - "Add VLLM_FLASHINFER_ALLREDUCE_BACKEND=trtllm environment variable" - "Bump --gpu-memory-utilization from 0.90 to 0.95" - "Add --kv-cache-dtype fp8"
Step-by-step proof
- Check out HEAD of PR #1994 and run
git diff ae1c88a..HEAD --stat. - Output shows three files changed, including
benchmarks/single_node/fixed_seq_len/minimaxm3_fp4_b300.sh. git diff ae1c88a..HEAD -- benchmarks/single_node/fixed_seq_len/minimaxm3_fp4_b300.shshows the three additions/changes above.sed -n '4437,4441p' perf-changelog.yamlshows only the two bullets (image tag + search space) — the shell-script knobs are not mentioned.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=28633304540 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=28664771389 |
Updated image reference and modified search-space parameters for fixed-seq-len scenarios.
Removed deprecated model configuration for MiniMax-M3-NVFP4.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=28671242557 |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
configs/nvidia-master.yaml:12606-12615— The edit tominimaxm3-fp4-b300-vllmat lines 12606–12614 accidentally duplicates the whole header block (model,model-prefix,runner,precision,framework,multinode,scenariosall appear twice) and inserts a strayimage:as a sibling offixed-seq-len:inside the firstscenariosmapping. PyYAML silently keeps the last occurrence so the sweep runs today, but the file has 7 duplicate mapping keys that violate the YAML spec and any strict parser (ruamel.yaml, yamllint duplicate-keys, IDE validators) would reject. Fix by deleting the duplicated block on lines 12606–12614 — the intended search-space edits are already present in the second block.Extended reasoning...
What the bug is
The diff for
minimaxm3-fp4-b300-vllminconfigs/nvidia-master.yamlinserts a duplicated copy of the config header before the intended search-space edits. The on-disk result at lines 12598–12615 is:minimaxm3-fp4-b300-vllm: image: vllm/vllm-openai:nightly-93d... model: nvidia/MiniMax-M3-NVFP4 model-prefix: minimaxm3 runner: b300 precision: fp4 framework: vllm multinode: false scenarios: # duplicate #1 (12606) fixed-seq-len: # null value image: vllm/vllm-openai:... # STRAY: sibling of fixed-seq-len, not a top-level key model: nvidia/MiniMax-M3-NVFP4 # DUPLICATE (of 12600) model-prefix: minimaxm3 # DUPLICATE runner: b300 # DUPLICATE precision: fp4 # DUPLICATE framework: vllm # DUPLICATE multinode: false # DUPLICATE scenarios: # duplicate #2, this one has the real list fixed-seq-len: - isl: 1024 ...
Why it doesn't blow up today
utils/matrix_logic/validation.pyusesyaml.safe_load, which silently keeps the LAST occurrence for duplicate mapping keys. So the secondscenarios:block wins, the misplacedimage:inside the firstscenariosis discarded, and Pydantic accepts the file. Verifiers confirmed by loading the file — it parses to a functional dict. That's why the sweep is running.Why it's still a real bug worth flagging
- The YAML spec requires unique mapping keys — this file has 7 duplicates in one mapping, so it's malformed by spec. Any strict loader (ruamel.yaml strict-mode, yamllint duplicate-keys rule, most IDE YAML validators) will reject it.
- Maintainability footgun: a future maintainer editing the FIRST header block (e.g. bumping the
image:tag) will believe their change is active when it's silently overwritten by the second block. - The stray
image:at 4-space indent inside the firstscenariosmapping violates the scenarios schema shape (scenarios expects sequence-category keys likefixed-seq-len). If the secondscenarios:block were ever removed during a later edit, the misplaced key would surface and Pydantic'sextra='forbid'would fail — or worse, silently corrupt the sweep matrix. - The insertion is clearly an unintended copy/paste of the header; the diff intent was only three search-space edits (
tp:4conc-endextended2→64, drop the redundanttp:4, 64–64point, add the newtp:2, ep:2, dp-attn: true, 4096–4096scenario). All three are present in the second block.
Addressing the refutation
One verifier refuted
bug_002as a duplicate ofbug_001. That's correct — they describe the exact same defect at the exact same lines. The synthesis agent has correctly merged them into a single finding, and only one comment is being filed. No independent claim is lost.Step-by-step proof
- Read
configs/nvidia-master.yamlat line 12606 and count top-level keys underminimaxm3-fp4-b300-vllm:. You will seemodel,model-prefix,runner,precision,framework,multinode,scenarioseach appearing twice. - Look at line 12608:
image: vllm/vllm-openai:...at 4-space indent, sibling tofixed-seq-len:on line 12607. - Run
python3 -c "import yaml; d=yaml.safe_load(open('configs/nvidia-master.yaml')); print(d['minimaxm3-fp4-b300-vllm']['scenarios'])". It succeeds — because PyYAML picked the LASTscenarios:value, not both. - Run any strict loader (
ruamel.yaml.YAML(typ='safe').load(...)with duplicate-key detection, oryamllintwithduplicate-keys: enable) and the file is rejected. - Delete lines 12606–12614 (the whole
scenarios:/fixed-seq-len:/image:/model:.../multinode: falseaccidentally-duplicated block). The remaining diff is exactly the three intended search-space edits.
Fix
Delete lines 12606–12614. Everything the PR actually wants to change is already in the second block below.
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2001 | ||
|
|
There was a problem hiding this comment.
🔴 The new perf-changelog entry uses a separator line with two trailing spaces ( \n) at line 4436 instead of a truly-empty line. This will abort utils/merge_with_reuse.sh at the prepare_perf_changelog_merge.py canonicalize step with ChangelogValidationError: new changelog entries must be separated from history by one empty line and appended at the end. Strip the two trailing spaces on line 4436 so the separator is just \n.
Extended reasoning...
What's wrong
Between the previous entry (ending at line 4435 with pull/2001) and the new entry (starting at line 4437 with - config-keys:), the separator on line 4436 is \n (two spaces then newline) instead of a truly-empty line \n. Every other separator between entries in this file (e.g. lines 4403, 4411, 4418, 4425) is a bare empty line — this new one is the odd one out.
Why PR-time CI does not catch this
PR CI runs utils/validate_perf_changelog.py (see .github/workflows/run-sweep.yml:102), whose CLI main() routes to validate_matrix_compatible_change → validate_generated_config only. That path checks for a final newline and that process_changelog.py accepts the file, but it does not call validate_raw_change — so raw-byte-level whitespace rules are not enforced at PR time. The byte-level check is a deliberate merge-time-only concern.
Why merge-time fails
utils/merge_with_reuse.sh (the documented reuse-assisted merge path — see .github/workflows/README.md:185 and codeowner-signoff-verify.yml:281,371) invokes prepare_perf_changelog_merge.py canonicalize at lines 181-186. That in turn calls canonicalize_appended_links → validate_raw_change (prepare_perf_changelog_merge.py:97). In validate_raw_change (utils/validate_perf_changelog.py:211-235), because base_raw ends with pull/2001\n (single newline, not \n\n), expected_start is set to b"\n- config-keys:". The check suffix.startswith(expected_start) then fails and raises ChangelogValidationError('new changelog entries must be separated from history by one empty line and appended at the end'). merge_with_reuse.sh runs under set -euo pipefail with no error handler on this call, so the non-zero exit aborts the merge.
Step-by-step proof
base_raw(fromorigin/main/1f234a6) ends with the bytes...pull/2001\n— verified viaod. Base does NOT end with\n\n.head_rawat that offset continues with the bytes\n- config-keys:\n - minimaxm3-fp4-b300-vllm\n...— the- In
validate_raw_change, sincebase_raw.endswith(b'\n\n')is False,expected_start = b'\n- config-keys:'. suffix = head_raw[len(base_raw):]— this suffix starts withb' \n- config-keys:'.suffix.startswith(b'\n- config-keys:')→False(suffix starts with two spaces, not a newline).- Raises
ChangelogValidationErrorwith the message quoted above. prepare_perf_changelog_merge.pyreturns exit code 1; underset -euo pipefailmerge_with_reuse.shaborts before push.
Empirically reproduced by running canonicalize_appended_links(base_raw, head_raw, 1994, 'SemiAnalysisAI/InferenceX') on the current bytes — it raises the exact error.
Impact
Blocks the standard supported merge path. This is distinct from the earlier inline comment on this PR (which flagged the missing 1994 PR number and missing trailing newline — both since fixed on head); the trailing-space separator remains.
Fix
Delete the two trailing spaces on line 4436 so the separator is a truly-empty line (\n only).
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=28671299958 |
wzhao18
changed the title
|
/reuse-sweep-run |
|
Hi @adibarra, could you help review this simple PR which only adds some more configs to the search space? Thanks |
|
@Ankur-singh @kedarpotdar-nv Could you approve and merge? |
2 participants
No description provided.