Skip to content

mi355x dsv4 retry #1160

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Oseltamivir merged 1 commit into from
Apr 25, 2026
Merged

mi355x dsv4 retry #1160

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions perf-changelog.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1826,3 +1826,11 @@
- "Drop --mem-fraction-static 0.88 and --max-total-tokens from dsv4_fp8_mi355x.sh"
- "Bump --chunked-prefill-size from 4096 to 8192"
- "Retrigger dsv4-fp8-mi355x-sglang"

- config-keys:
- dsv4-fp8-mi355x-sglang
description:
- "Drop --mem-fraction-static 0.88 and --max-total-tokens from dsv4_fp8_mi355x.sh"
- "Bump --chunked-prefill-size from 4096 to 8192"
- "Retrigger dsv4-fp8-mi355x-sglang"
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1159

Check warning on line 1836 in perf-changelog.yaml

View check run for this annotation

Claude / Claude Code Review

pr-link should reference this PR (#1160), not #1159 

The newly added perf-changelog entry sets `pr-link: .../pull/1159`, but this is PR #1160 ("mi355x dsv4 retry"); #1159 is the prior already-merged retrigger PR (commit d578b7e). The entry was copy-pasted from the unlinked block above without bumping the number — following the convention used for the analogous retrigger pattern at lines 1799-1805 (where #1148 self-references), this should point to `pull/1160`.
Comment on lines +1830 to +1836
Copy link
Copy Markdown
Contributor

@claude claude Bot Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The newly added perf-changelog entry sets pr-link: .../pull/1159, but this is PR #1160 ("mi355x dsv4 retry"); #1159 is the prior already-merged retrigger PR (commit d578b7e). The entry was copy-pasted from the unlinked block above without bumping the number — following the convention used for the analogous retrigger pattern at lines 1799-1805 (where #1148 self-references), this should point to pull/1160.

Extended reasoning...

What the bug is

The new perf-changelog.yaml entry added at lines 1830-1836 sets:

- config-keys:
    - dsv4-fp8-mi355x-sglang
  description:
    - "Drop --mem-fraction-static 0.88 and --max-total-tokens from dsv4_fp8_mi355x.sh"
    - "Bump --chunked-prefill-size from 4096 to 8192"
    - "Retrigger dsv4-fp8-mi355x-sglang"
  pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1159

But this PR is #1160 ("mi355x dsv4 retry"), not #1159. PR #1159 ("retrigger mi355x dsv4") is the prior, already-merged retrigger PR — see commit d578b7e in git log. The new entry was clearly copy-pasted from the unlinked sibling block at lines 1823-1828 (which belongs to #1159) and only had pr-link appended, without updating the PR number.

Why this is a bug (convention violation)

The established convention in this file is that each entry's pr-link points to its own PR. The directly analogous retrigger pattern is at lines 1799-1805:

- config-keys:
    - dsv4-fp8-mi355x-sglang
  description:
    - "Bump MI355X SLURM allocation from --time=180 to --time=300 ..."
    ...
    - "Retriggering dsv4-fp8-mi355x-sglang"
  pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1148

Per git show --stat 694f206, PR #1148 was a retrigger PR that only modified perf-changelog.yaml (no recipe changes — just adding the duplicate linked entry), and its pr-link self-references #1148 even though the actual recipe changes were made in PR #1147. This PR (#1160) is structurally identical — it's a retrigger of #1159's recipe changes that only adds a duplicate changelog entry — but uses pull/1159 instead of pull/1160, breaking the precedent.

Step-by-step proof

  1. PR metadata says this is mi355x dsv4 retry #1160 ("mi355x dsv4 retry"), opened 2026-04-25.
  2. git log shows commit d578b7e = "retrigger mi355x dsv4 (retrigger mi355x dsv4 #1159)" — already merged.
  3. The diff adds exactly one block at lines 1830-1836 with pr-link: ...pull/1159.
  4. Lines 1823-1828 (already on main, added by retrigger mi355x dsv4 #1159) contain the same config-keys/description as lines 1830-1836 but with no pr-link field — the new block is a copy-paste of that prior unlinked block.
  5. Following the Bump MI355X SLURM time-limit to 300m; retrigger dsv4-fp8-mi355x-sglang #1147 → Increase timeout #1148 precedent (retrigger PR self-references its own PR number), the correct value here is https://github.com/SemiAnalysisAI/InferenceX/pull/1160.

Impact and fix

Severity is nit: perf-changelog.yaml is documentation/metadata only — no runtime or functional impact. The misleading link still resolves to a real (related) PR, so a reader can still find their way, just not directly. But it does break traceability and the file's self-consistency. Fix: change 1159 → 1160 on line 1836.

Loading