Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
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/nvidia-master.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3969,7 +3969,7 @@ gptoss-fp4-b200-trt:
- { tp: 8, conc-start: 4, conc-end: 4}

gptoss-fp4-b200-vllm:
image: vllm/vllm-openai:v0.15.1
image: vllm/vllm-openai:v0.20.2
model: openai/gpt-oss-120b
model-prefix: gptoss
runner: b200
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-b200-vllm
description:
- "Update vLLM image from v0.15.1 to v0.20.2"
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX

Check warning on line 2351 in perf-changelog.yaml

View check run for this annotation

Claude / Claude Code Review

Placeholder PR link 'pull/XXX' in perf-changelog.yaml

The new perf-changelog.yaml entry uses a literal 'XXX' placeholder in its pr-link (`https://github.com/SemiAnalysisAI/InferenceX/pull/XXX`) instead of the actual PR number. Since this is PR #1333, the link should be `.../pull/1333`; as written, anyone following the changelog cross-reference will hit a 404. Fix by replacing `XXX` with `1333`.
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 uses a literal 'XXX' placeholder in its pr-link (https://github.com/SemiAnalysisAI/InferenceX/pull/XXX) instead of the actual PR number. Since this is PR #1333, the link should be .../pull/1333; as written, anyone following the changelog cross-reference will hit a 404. Fix by replacing XXX with 1333.

Extended reasoning...

What the bug is\n\nThe diff adds a new entry to perf-changelog.yaml at line 2346–2351 documenting the vLLM image bump for gptoss-fp4-b200-vllm. The final field is:\n\nyaml\n pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX\n\n\nThe XXX is a literal placeholder string that was never substituted with the real PR number. This PR is #1333, so the URL should be https://github.com/SemiAnalysisAI/InferenceX/pull/1333.\n\nWhy existing code doesn'''t prevent it\n\nThe changelog is a free-form YAML document; nothing in the repo validates that pr-link URLs point to existing PRs, and YAML syntax accepts the placeholder string just fine. The convention is enforced socially — every other recent entry (lines 2332 → /pull/1304, 2338 → /pull/1305, 2345 → /pull/1310, etc.) uses the real PR number, so this one stands out as an oversight rather than a structural failure.\n\nImpact\n\nThis is metadata only and has no runtime effect — benchmarks, image selection, and CI all ignore the changelog. The harm is purely to changelog traceability: any reader (engineer, release manager, future PR author) who clicks through to understand the context behind the v0.15.1 → v0.20.2 vLLM bump will land on a GitHub 404, since /pull/XXX is not a valid PR. The change history breaks at exactly the point where it'''s being recorded.\n\nHow to fix\n\nReplace XXX with 1333 on line 2351:\n\nyaml\n pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1333\n\n\nStep-by-step proof\n\n1. Open the PR in GitHub — the URL bar shows .../pull/1333, so this PR'''s number is 1333.\n2. View the diff for perf-changelog.yaml. The added block ends with: pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX.\n3. Construct the resulting URL: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX. XXX is not a valid integer PR identifier, so GitHub returns a 404 Not Found.\n4. Compare against the entry immediately above (lines 2343–2345): pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1310 — a real PR number, which resolves correctly.\n5. The correct value for this entry, by the same convention, is 1333. Replacing XXX1333 restores changelog traceability.\n\nSeverity\n\nNit — the placeholder is clearly an unintentional oversight (a sed/replace step that was skipped) and trivially fixable before merge, but it does not affect runtime behavior.

Loading