[WIP][NV] update glm5-fp4-b200 sglang image#1031
[WIP][NV] update glm5-fp4-b200 sglang image#1031hshrivastava-droid wants to merge 2 commits intomainfrom
Conversation
hshrivastava-droid
requested review from
jgangani and
kedarpotdar-nv
as code owners
|
Thanks for the contribution! For vLLM & SGLang, please ensure that your recipes is similar to the official vLLM recipes and/or the SGLang cookbook If it is not, please create a PR first before we can merge your PR into the master branch. Let's ensure that the documentation is first class such that the entire ML community can benefit from your hard work! Thank you |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| - glm5-fp4-b200-sglang | ||
| description: | ||
| - "Update SGLang image from nightly-dev-cu13-20260328-a27651d5 to v0.5.10.post1-cu130" | ||
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1031 |
There was a problem hiding this comment.
🟡 The new perf-changelog.yaml entry uses a placeholder pr-link of pull/XXX instead of the actual PR number 1031. Update line 1356 to pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1031 before merging.
Extended reasoning...
What the bug is: The new changelog entry for glm5-fp4-b200-sglang added at the bottom of perf-changelog.yaml uses the placeholder string pull/XXX in its pr-link field rather than the actual PR number.
The specific code path: Line 1356 of perf-changelog.yaml reads:
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXXThe PR being reviewed is clearly PR #1031 (visible in the PR metadata), so the correct value is https://github.com/SemiAnalysisAI/InferenceX/pull/1031.
Why existing practices don't prevent it: The file already contains several other entries with pull/XXX (e.g., glm5-fp8-mi355x-sglang, minimaxm2.5-fp8-h200-vllm, qwen3.5-bf16-mi325x-sglang, qwen3.5-fp8-mi325x-sglang), indicating this placeholder pattern has slipped through in prior merges. However, those past failures to update the link do not make it acceptable to repeat the mistake in a new entry, especially when the correct PR number is definitively known.
Addressing the refutation: The refutation argues this is an accepted practice because other XXX entries exist and the PR is marked [WIP]. Neither argument holds: the presence of prior mistakes doesn't make them correct, and [WIP] status means the author should update the PR number before requesting final merge, not that the placeholder is intentionally permanent. The refutation also conflates this with bug_002—but this is an independently reportable, actionable issue on a new entry introduced by this PR.
Impact: Purely documentation/metadata. The pr-link field is informational only; no CI or runtime behavior is affected. However, a broken/placeholder link makes the changelog less useful for traceability.
How to fix: Replace line 1356 with:
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1031Step-by-step proof:
- PR [WIP][NV] update glm5-fp4-b200 sglang image #1031 is titled "[NV] update sglang image" and modifies
perf-changelog.yaml. - The diff shows a new entry appended with
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX. - The PR number is 1031 by definition (it is the PR under review).
- Therefore the correct link is
pull/1031, notpull/XXX.
Audit against glm5_fp4_b200.sh benchmark script (SemiAnalysisAI/InferenceX#1031): --ep 1 was used in benchmark but missing from recipe; --enable-dp-lm-head was in recipe but absent from the validated benchmark configuration. Signed-off-by: Faradawn Yang <73060648+faradawn@users.noreply.github.com>
hshrivastava-droid
changed the title
2 participants
No description provided.