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 @@ -507,7 +507,7 @@
- { tp: 8, conc-start: 4, conc-end: 64 }

kimik2.5-int4-mi300x-vllm:
image: vllm/vllm-openai-rocm:v0.18.0
image: vllm/vllm-openai-rocm:v0.20.2

Check warning on line 510 in .github/configs/amd-master.yaml

View check run for this annotation

Claude / Claude Code Review

PR title/description claims v0.21.0 but diff bumps to v0.20.2

PR title says 'Update ... to v0.21.0' and the description says 'from v0.18.0 to v0.21.0', but the actual diff bumps the image to `v0.20.2` (and the new perf-changelog entry also describes it as v0.20.2). Please reconcile — either fix the title/description to say v0.20.2 (most likely, since the diff and changelog agree), or bump the YAML further if v0.21.0 was actually intended.
model: moonshotai/Kimi-K2.5
model-prefix: kimik2.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:
- kimik2.5-int4-mi300x-vllm
description:
- "Update vLLM ROCm image from v0.18.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 uses placeholder pull/XXX

The new perf-changelog entry adds `pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX` — a literal `XXX` placeholder that was never substituted. Every other entry in this file uses the real numeric PR ID (e.g., `pull/1310` immediately above). Since this is PR #1404, the link should be `https://github.com/SemiAnalysisAI/InferenceX/pull/1404`.

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 at line 2351 has pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX — the literal XXX placeholder from the PR template was not replaced with the actual PR number. This will produce a 404 link in the changelog; fix by replacing XXX with 1350.

Extended reasoning...

What's wrong

perf-changelog.yaml line 2351 reads:

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

The XXX is a stub from the "Updating Docker Images" section of AGENTS.md, where the template literally shows pull/XXX as a placeholder for authors to fill in. The author/generator of this PR copied the template but didn't substitute the actual PR number.

Why this is the only such case

Every other entry in perf-changelog.yaml uses a real numeric PR id — e.g. line 2332 → pull/1305, line 2338 → pull/1308, line 2345 → pull/1310. A grep for non-numeric pr-link values turns up exactly one hit: the new entry added by this PR. So this is clearly an authoring oversight, not a convention.

Impact

When this is merged, the changelog entry for the kimik2.5-int4-mi300x-vllm v0.18.0 → v0.20.2 image bump will link to github.com/SemiAnalysisAI/InferenceX/pull/XXX, which 404s. Anyone using the changelog to trace a performance change back to its PR for this entry will hit a dead link, breaking changelog→PR traceability. No runtime/benchmark impact — purely a metadata/documentation issue, hence nit severity.

Step-by-step proof

  1. This PR is Update kimik2.5-int4-mi300x-vllm vLLM image to v0.20.2 #1350 (per PR metadata: <pr number="1350">).
  2. The diff adds a new perf-changelog.yaml entry whose pr-link ends in /pull/XXX (diff hunk at perf-changelog.yaml:2346–2351).
  3. XXX is not a valid GitHub PR number; navigating to https://github.com/SemiAnalysisAI/InferenceX/pull/XXX returns a 404.
  4. All other recent entries (lines 2332, 2338, 2345) use real numeric ids, confirming the expected format.
  5. Replacing XXX with 1350 (this PR's number) yields a working link to the merge commit, restoring traceability.

Fix

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

Loading