Update minimaxm2.5-fp8-mi300x-vllm vLLM ROCm image to v0.21.0#1405
Conversation
…n\nCo-authored-by: Klaud Cold <Klaud-Cold@users.noreply.github.com>
Klaud-Cold
requested review from
billishyahao,
chunfangamd,
seungrokj and
yctseng0211
as code owners
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| minimaxm2.5-fp8-mi300x-vllm: | ||
| image: vllm/vllm-openai-rocm:v0.16.0 | ||
| image: vllm/vllm-openai-rocm:v0.20.2 |
There was a problem hiding this comment.
🔴 The PR title and description claim this updates the image to v0.21.0 ("from v0.16.0 to v0.21.0"), but the actual diff bumps vllm/vllm-openai-rocm to v0.20.2 (matching the perf-changelog entry). Please reconcile by either fixing the title/description to v0.20.2 or, if v0.21.0 was the intended target, bumping the image tag (and changelog) accordingly — otherwise reviewers and downstream consumers will be misled about which vLLM version is actually being benchmarked.
Extended reasoning...
What the bug is
The PR metadata and the code are out of sync:
- PR title: "Update minimaxm2.5-fp8-mi300x-vllm vLLM ROCm image to v0.21.0"
- PR description: "Updates the vLLM ROCm image tag for
minimaxm2.5-fp8-mi300x-vllmfrom v0.16.0 to v0.21.0." - Actual diff (
.github/configs/amd-master.yamlline 663):vllm/vllm-openai-rocm:v0.16.0→vllm/vllm-openai-rocm:v0.20.2 perf-changelog.yamlentry (line ~2350): "Update vLLM ROCm image from v0.16.0 to v0.20.2"
The YAML and the changelog agree internally (both say v0.20.2), so the title/description are the outliers.
Step-by-step proof
- Open the PR — title reads
Update minimaxm2.5-fp8-mi300x-vllm vLLM ROCm image to v0.21.0. - Read the body — it says the bump is
from v0.16.0 to v0.21.0. - Look at the diff for
.github/configs/amd-master.yaml:The new tag is v0.20.2, not v0.21.0.minimaxm2.5-fp8-mi300x-vllm: - image: vllm/vllm-openai-rocm:v0.16.0 + image: vllm/vllm-openai-rocm:v0.20.2
- Look at the diff for
perf-changelog.yaml:The changelog also says v0.20.2.- config-keys: - minimaxm2.5-fp8-mi300x-vllm description: - "Update vLLM ROCm image from v0.16.0 to v0.20.2"
So a reviewer skimming the title approves "v0.21.0" but the runner will actually pull vllm/vllm-openai-rocm:v0.20.2 when this config is benchmarked.
Why it matters
The YAML is the source of truth for what image the benchmark runs against, so benchmark execution is internally consistent (v0.20.2). The real harm is review/audit integrity:
- Reviewers using the title as the summary may approve under a false belief about what version is being deployed.
- Anyone later searching git history / the changelog for "v0.21.0" will not find this PR, and anyone searching for "v0.20.2" will be confused by a title claiming v0.21.0.
- Downstream consumers of the perf changelog vs. the PR metadata get conflicting answers about which vLLM version produced the published numbers.
How to fix
Pick one of:
- If v0.20.2 was the intended version (most likely — both the YAML and the perf-changelog agree on it): correct the PR title to
Update minimaxm2.5-fp8-mi300x-vllm vLLM ROCm image to v0.20.2and update the description to say "from v0.16.0 to v0.20.2". - If v0.21.0 was the intended target: bump the image tag in
.github/configs/amd-master.yamlline 663 tovllm/vllm-openai-rocm:v0.21.0and update the perf-changelog entry to match.
Also worth fixing while you are here: the perf-changelog entry has pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX — the XXX placeholder should be replaced with this PR's actual number.
| - minimaxm2.5-fp8-mi300x-vllm | ||
| description: | ||
| - "Update vLLM ROCm image from v0.16.0 to v0.20.2" | ||
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX |
There was a problem hiding this comment.
🔴 The new perf-changelog entry has pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX — XXX is an unfilled template placeholder that will 404 when followed. Please substitute the actual PR number (1405) before merging so the link matches the convention used by every other entry in the file.
Extended reasoning...
What the bug is
The diff at perf-changelog.yaml:2351 adds a new changelog entry whose pr-link is literally:
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXXXXX is a template placeholder that was supposed to be replaced with the real PR number before the PR was published. The correct value is 1405 (this PR's own number).
Why this is wrong / how it manifests
Every other entry in perf-changelog.yaml uses a real numeric PR ID. The four entries immediately preceding the new one are a clear example:
- line 2325 →
.../pull/1304 - line 2332 →
.../pull/1305 - line 2338 →
.../pull/1308 - line 2345 →
.../pull/1310
The newly added entry breaks that convention. Visiting https://github.com/SemiAnalysisAI/InferenceX/pull/XXX produces a GitHub 404 — XXX is not a valid PR identifier — so any reader who clicks the changelog link to discover the source of the image bump lands on a dead page instead of the change that introduced it.
Why existing code doesn't catch it
The ChangelogEntry Pydantic model in utils/matrix_logic/validation.py validates that pr-link is a string, but does not validate the URL structure or that the trailing path segment is numeric / resolves to a real PR. So CI happily accepts the placeholder. This is purely a content-correctness issue that has to be caught at review time.
Step-by-step proof
- Open the diff at
perf-changelog.yaml:2346-2351— the added entry literally containspull/XXX. - Open any of the surrounding entries (e.g. line 2345) — they contain
pull/1310, a real numeric PR ID. - Open
https://github.com/SemiAnalysisAI/InferenceX/pull/XXXin a browser → GitHub returns 404 becauseXXXis not a valid pull-request number. - Open
https://github.com/SemiAnalysisAI/InferenceX/pull/1405→ resolves to this PR, which is the link the changelog entry is supposed to point at.
How to fix
Replace XXX with 1405 on line 2351:
- pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX
+ pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1405That restores the file's convention and gives readers a working link back to the source change.
Note on the version mismatch (not this bug)
While reviewing this entry, note that the PR title says "v0.21.0" but the actual image bump and changelog description both say "v0.20.2". The title/description appear inconsistent, but the diff itself is internally consistent (config and changelog both name v0.20.2) — that's a separate concern from the placeholder XXX flagged here.
# Conflicts: # perf-changelog.yaml
…or --disable-log-requests removal The original commit set the image to v0.20.2 instead of v0.21.0. Merging main picks up commit 0552ead which removed the deprecated --disable-log-requests flag from the benchmark script (PR #1371). Both issues caused the CI sweep to fail with 'unrecognized arguments'.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=25983959701 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=25984514925 |
3 similar comments
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=25984514925 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=25984514925 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=25984514925 |
|
/reuse-sweep-run |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=26008798951 |
2 participants
Summary
minimaxm2.5-fp8-mi300x-vllmfrom v0.16.0 to v0.21.0.Ref #1154
Generated with Claude Code