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/nvidia-master.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1913,7 +1913,7 @@ dsr1-fp4-b200-trt-mtp:
- { tp: 8, ep: 8, dp-attn: true, conc-start: 64, conc-end: 256, spec-decoding: mtp }

dsr1-fp8-b200-sglang:
image: lmsysorg/sglang:v0.5.9-cu130
image: lmsysorg/sglang:v0.5.11-cu130
model: deepseek-ai/DeepSeek-R1-0528
model-prefix: dsr1
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:
- dsr1-fp8-b200-sglang
description:
- "Update SGLang image from v0.5.9-cu130 to v0.5.11-cu130"
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 XXX in perf-changelog entry

The new perf-changelog entry has `pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX` where `XXX` is an unfilled placeholder rather than this PR's number. Since this is PR #1322, the link should be `pull/1322` — as written, it resolves to a 404 and breaks the changelog cross-reference. Trivially fixable by replacing `XXX` with `1322`.
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 has pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX where XXX is an unfilled placeholder rather than this PR's number. Since this is PR #1322, the link should be pull/1322 — as written, it resolves to a 404 and breaks the changelog cross-reference. Trivially fixable by replacing XXX with 1322.

Extended reasoning...

The bug

The new entry appended to perf-changelog.yaml (line 2351) sets:

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

The XXX is clearly a template placeholder that was never substituted with this PR's real number. Every other entry in the file uses a real numeric PR id, for example the immediately preceding entries:

  • pull/1304
  • pull/1305
  • pull/1308
  • pull/1310

Why it matters

The changelog is a discoverability/audit aid — given a config-keys row, a reader follows pr-link to see why the configuration changed (here: the SGLang image bump from v0.5.9-cu130 to v0.5.11-cu130). With XXX left in place, that cross-reference is permanently broken: GitHub will return a 404 for /pull/XXX, so the rationale becomes unreachable from the changelog.

Why existing code doesn't prevent it

perf-changelog.yaml is free-form YAML. There is no schema check or lint that validates pr-link points at an existing PR (or even that the path component is numeric), so a placeholder slips through both human review and CI.

Step-by-step proof

  1. Open perf-changelog.yaml at the bottom of the file (the diff hunk in this PR).
  2. The new entry, lines 2347–2351, reads:
    - config-keys:
        - dsr1-fp8-b200-sglang
      description:
        - "Update SGLang image from v0.5.9-cu130 to v0.5.11-cu130"
      pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX
  3. The PR metadata for this change is #1322 (title: "Update dsr1-fp8-b200-sglang SGLang image to v0.5.11-cu130").
  4. Compare with the entry immediately above (lines 2340–2345):
    pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1310
    — numeric, resolves to a real PR.
  5. Visiting https://github.com/SemiAnalysisAI/InferenceX/pull/XXX returns a 404 (no such PR id), confirming the link is dead.

Fix

Replace XXX with 1322:

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

Severity is nit: this is a broken doc link, not a runtime/functional issue, but it should be fixed before merge so the changelog stays internally consistent.

Loading