Skip to content

B200/300 test#1233

Open
Oseltamivir wants to merge 8 commits intomainfrom
b200-300-test
Open

B200/300 test#1233
Oseltamivir wants to merge 8 commits intomainfrom
b200-300-test

Conversation

@Oseltamivir
Copy link
Copy Markdown
Collaborator

No description provided.

@Oseltamivir Oseltamivir marked this pull request as ready for review April 30, 2026 00:37
@Oseltamivir Oseltamivir requested a review from a team April 30, 2026 00:37
# Conflicts:
#	.github/configs/nvidia-master.yaml
#	perf-changelog.yaml
Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Additional findings (outside current diff — PR may have been updated during review):

  • 🟡 perf-changelog.yaml:2017 — The new perf-changelog.yaml entry at line 2017 has pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX — a literal placeholder rather than the actual PR number. Every other entry in this file uses a real numeric PR id (e.g. the immediately preceding entry uses pull/1221); this should be updated to pull/1233 before merge so the changelog correctly traces back to its PR.

    Extended reasoning...

    What's broken. The newly added perf-changelog.yaml entry for dsv4-fp4-b200-trt and dsv4-fp4-b300-trt ends with:\n\nyaml\n pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX\n\n\nXXX is a literal placeholder string left over from authoring — not the PR number. The current PR (per the metadata in this review) is #1233, so this should read https://github.com/SemiAnalysisAI/InferenceX/pull/1233.\n\nWhy this is wrong. Every other entry in perf-changelog.yaml uses an actual numeric PR id. The three entries immediately preceding this one end with pull/1209, pull/1218, and pull/1221 respectively, and a quick scan of the rest of the file confirms the pattern holds throughout. Leaving XXX in place breaks the changelog's only mechanism for tracing a config-change entry back to the PR that introduced it.\n\nWhy existing tooling didn't catch it. perf-changelog.yaml appears to be free-form YAML metadata; there is no schema validator that requires pr-link to be a real GitHub URL. YAML parsers happily accept the placeholder as a string, and PR review is the last line of defense.\n\nImpact. Cosmetic / documentation only — no runtime behavior is affected. But once merged, anyone auditing the changelog (e.g., "which PR added the dsv4-fp4-b200-trt config?") will hit a 404 instead of landing on the correct PR. Fixing it post-merge requires another PR to touch the changelog, which is wasteful.\n\nHow to fix. Replace XXX with 1233 on line 2017 of perf-changelog.yaml before merging this PR.\n\nStep-by-step proof.\n1. Open perf-changelog.yaml at the bottom of the file (the new entry added by this PR).\n2. Observe the entry's last line: pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX.\n3. Compare with the immediately preceding entry (the dsv4-fp4-b300-vllm "Change image to vllm/vllm-openai:v0.20.0-cu130" entry): its pr-link is https://github.com/SemiAnalysisAI/InferenceX/pull/1221 — a real numeric id.\n4. Compare with the PR's own metadata: the current PR number is 1233.\n5. Conclusion: the new entry should use pull/1233, matching the convention used by every other entry in the file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant