Update minimaxm2.5-fp4-mi355x-vllm vLLM ROCm image to v0.21.0#1409
Conversation
Ref #1154 Co-authored-by: Klaud Cold <Klaud-Cold@users.noreply.github.com>
|
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. |
| - minimaxm2.5-fp4-mi355x-vllm | ||
| description: | ||
| - "Update vLLM ROCm image from v0.19.1 to v0.21.0" | ||
| pr-link: XXX |
There was a problem hiding this comment.
🔴 The new perf-changelog entry at line 2501 has pr-link: XXX instead of an actual PR URL — the placeholder from the changelog template was left in. Every other entry in the file uses pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/<number>; this should be https://github.com/SemiAnalysisAI/InferenceX/pull/1409.
Extended reasoning...
What is the bug
perf-changelog.yaml line 2501 commits the literal placeholder XXX as the pr-link value:
- config-keys:
- minimaxm2.5-fp4-mi355x-vllm
description:
- "Update vLLM ROCm image from v0.19.1 to v0.21.0"
pr-link: XXXThis is the template placeholder shown in AGENTS.md (the documented template form is pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX). The author bumped the image tag in .github/configs/amd-master.yaml and updated the changelog but never substituted the actual PR number (1409) into pr-link. Every other entry in the file uses a full URL (see e.g. lines 2482, 2488, 2495).
Why existing code does not prevent it
There is no schema/format validation on pr-link in the diff or surrounding code that would reject a bare XXX. YAML parsing happily produces the string "XXX".
Impact — concrete tooling failure
This is more than a cosmetic documentation issue. utils/merge_with_reuse.sh has logic that specifically expects the /pull/XXX URL form, not a bare XXX:
- The filter
if "XXX" not in link and not link.endswith(f"/pull/{pr}"): continuedoes keep the entry (because"XXX" in "XXX"is true), so the entry is selected as a contribution. - The substitution
block.replace("/pull/XXX", f"/pull/{pr}")then looks for the literal substring/pull/XXX. Since the value here is justXXX(no/pull/prefix), the replace is a no-op and the placeholder is silently retained in the merged changelog. - Immediately after, the script asserts
last["pr-link"].endswith("/$PR"). Withpr-link: XXX,"XXX".endswith("/1409")is false, so the assertion fails and the merge-with-reuse path errors out.
Step-by-step proof
Concrete walk-through with PR #1409:
pr-linkin YAML is parsed to Python string"XXX".- In the contribution loop:
link = "XXX";"XXX" in "XXX"isTrue, so the entry is not skipped (good — it is supposed to be picked up). block.replace("/pull/XXX", "/pull/1409")is called on the raw YAML block. The block containspr-link: XXX, notpr-link: ...pull/XXX, so no match is found. The block is appended unchanged, still withpr-link: XXX.- The script then parses the rewritten file and runs
assert entries[-1]["pr-link"].endswith("/1409")."XXX".endswith("/1409")isFalse, so the assertion fires and the merge script exits with anAssertionError.
Result: if merge_with_reuse.sh is used for this PR, it will fail on the assertion. Even if a maintainer bypasses that path (e.g. squash-merges directly), the changelog entry remains untraceable — XXX is not a URL, so any downstream tooling or reader that consumes perf-changelog.yaml cannot link the entry to its PR.
How to fix
Replace line 2501 with the proper URL:
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1409This matches every other entry in the file and satisfies both the /pull/XXX substitution and the endswith("/<pr>") assertion in utils/merge_with_reuse.sh.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=25956504657 |
|
/reuse-sweep-run |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=25976989308 |
Summary
minimaxm2.5-fp4-mi355x-vllmfrom v0.19.1 to v0.21.0.Ref #1154
Generated with Claude Code