Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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 .github/configs/amd-master.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,7 @@
- { tp: 4, conc-start: 4, conc-end: 64 }

minimaxm2.5-fp8-mi300x-vllm:
image: vllm/vllm-openai-rocm:v0.16.0
image: vllm/vllm-openai-rocm:v0.20.2

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

View check run for this annotation

Claude / Claude Code Review

PR metadata says v0.21.0 but image is bumped to v0.20.2

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.
model: MiniMaxAI/MiniMax-M2.5
model-prefix: minimaxm2.5
runner: mi300x
Expand Down
6 changes: 6 additions & 0 deletions perf-changelog.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2343,3 +2343,9 @@
description:
- "Add Qwen3.5-397B-A17B FP8 MI355X ATOM benchmark configs with and without MTP"
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1310

- config-keys:
- minimaxm2.5-fp8-mi300x-vllm
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 new perf-changelog.yaml entry has pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX with a literal XXX placeholder rather than the actual PR number. Please replace XXX with 1351 before merging so the changelog link resolves to this PR instead of 404.

Extended reasoning...

What's wrong

The new entry added to perf-changelog.yaml at the end of the file contains an unresolved placeholder in the pr-link field:

- config-keys:
    - 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

The literal string XXX was left in place where the PR number should be.

Why this matters

Every other entry in perf-changelog.yaml references a real PR number (e.g. /pull/1303, /pull/1304, /pull/1308, /pull/1310). The pr-link is the changelog's way of tracing a config change back to its motivating PR, so consumers (humans browsing the changelog, or any tooling that follows these links) will be sent to https://github.com/SemiAnalysisAI/InferenceX/pull/XXX — a 404. The provenance for this image bump is effectively lost the moment the PR is merged and someone tries to follow the link.

Step-by-step proof

  1. Open perf-changelog.yaml at line 2348 (the new block added by this PR).
  2. Inspect the pr-link: value — it is the verbatim string https://github.com/SemiAnalysisAI/InferenceX/pull/XXX.
  3. Scroll up to any prior entry, e.g. line 2345 (pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1310) or earlier entries pointing to /pull/1303, /pull/1304, /pull/1308 — all use real, numeric PR IDs.
  4. This PR is Update minimaxm2.5-fp8-mi300x-vllm vLLM image to v0.20.2 #1351 (per the PR metadata), so the intended value is /pull/1351.
  5. As shipped, the link resolves to GitHub's "Page not found" rather than to this PR.

Fix

Replace the trailing XXX with 1351:

-  pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX
+  pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1351

Severity

Calling this nit: perf-changelog.yaml is documentation/metadata, so there is no runtime impact, no failing CI, and no broken benchmark. But it is a guaranteed-broken link being added by this very PR, and the fix is a one-character-class substitution, so it is well worth catching before merge.

description:
- "Update vLLM ROCm image from v0.16.0 to v0.20.2"
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX

Check failure on line 2351 in perf-changelog.yaml

View check run for this annotation

Claude / Claude Code Review

Unresolved pr-link placeholder (pull/XXX) in perf-changelog

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