Skip to content
Open
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 @@ -702,7 +702,7 @@ minimaxm2.5-fp8-mi325x-vllm:
- { tp: 8, ep: 8, conc-start: 4, conc-end: 256 }

gptoss-fp4-mi300x-vllm:
image: vllm/vllm-openai-rocm:v0.17.0
image: vllm/vllm-openai-rocm:v0.20.2
model: openai/gpt-oss-120b
model-prefix: gptoss
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:
- gptoss-fp4-mi300x-vllm
description:
- "Update vLLM ROCm image from v0.17.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

perf-changelog pr-link placeholder not replaced

The new perf-changelog entry for this image bump uses a placeholder pr-link `https://github.com/SemiAnalysisAI/InferenceX/pull/XXX` instead of the actual PR number. This produces a 404 link in the rendered changelog and breaks any tooling that parses `pr-link` to associate the entry with a real PR. Please replace `XXX` with `1349` (or the final PR number) before merging.
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 entry for this image bump uses a placeholder pr-link https://github.com/SemiAnalysisAI/InferenceX/pull/XXX instead of the actual PR number. This produces a 404 link in the rendered changelog and breaks any tooling that parses pr-link to associate the entry with a real PR. Please replace XXX with 1349 (or the final PR number) before merging.

Extended reasoning...

What the bug is

In perf-changelog.yaml at lines 2347-2351, the new changelog entry added by this PR ends with:

- config-keys:
    - gptoss-fp4-mi300x-vllm
  description:
    - "Update vLLM ROCm image from v0.17.0 to v0.20.2"
  pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX

The literal string XXX was never substituted with the actual PR number.

Why this is wrong

Every other entry in the file uses a real numeric PR ID. The immediately preceding entries reference PRs #1308 (line 2338) and #1310 (line 2345), and entries further up reference #1303, #1304, #1305, etc. The established convention — used consistently across the entire file — is that pr-link points at the merging PR with a real numeric ID.

Step-by-step proof

  1. The PR metadata above identifies this as PR Update gptoss-fp4-mi300x-vllm vLLM image to v0.20.2 #1349.
  2. The diff adds the following block to perf-changelog.yaml:
    - config-keys:
        - gptoss-fp4-mi300x-vllm
      description:
        - "Update vLLM ROCm image from v0.17.0 to v0.20.2"
      pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX
  3. Visiting https://github.com/SemiAnalysisAI/InferenceX/pull/XXX resolves to a 404 because XXX is not a valid GitHub PR number.
  4. Any consumer of perf-changelog.yaml that parses pr-link (e.g. to deep-link entries to their PR, or to cross-reference commits to PRs) will either fail to parse the URL as an integer or produce a dangling link.

Note on the referenced issue

The PR description references #1154 (the gptoss tracking issue). That reference belongs in the body of the PR, not in pr-linkpr-link in perf-changelog.yaml consistently points at the merging PR, not at related issues. The correct value here is https://github.com/SemiAnalysisAI/InferenceX/pull/1349.

How to fix

Replace XXX with 1349:

  pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1349

Loading