-
Notifications
You must be signed in to change notification settings - Fork 181
Improve dsv4-fp8-mi355x-vllm with vllm-project/recipes#433 MI355X recipe #1373
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.
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
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 at line 2455 has an unfilled placeholder
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXXXinstead ofpull/1373. Every other entry in the file uses a real PR number, so the link as-committed returns a 404 and breaks the changelog's audit trail. Trivial one-character fix: replaceXXXXwith1373.Extended reasoning...
What the bug is
The new entry appended to
perf-changelog.yamlfor thedsv4-fp8-mi355x-vllmrecipe change ends with an unfilled template placeholder:This is on line 2455 of the committed file. The placeholder
XXXXwas never substituted with the actual PR number (1373).How it manifests
Following the link in the changelog yields a GitHub 404 (no PR numbered
XXXXexists in the repo). Any tooling that consumesperf-changelog.yamlto build a changelog, navigate from a recipe key back to its motivating PR, or audit which PR introduced a given configuration change will either error out (URL validation) or silently emit a dead link.Why existing code doesn't prevent it
perf-changelog.yamlis hand-maintained — there is no automated check thatpr-linkresolves to a real PR. The PR description itself documents the intended link aspull/1373, and every other entry in the file uses a real numeric PR number (e.g. lines 2426, 2432, 2438, 2444 reference/pull/1329,/pull/1343,/pull/1344,/pull/1346). This is clearly an unfinished template — the author leftXXXXas a fill-me-in marker and forgot to update it before pushing.Note on what reviewers see
The PR-diff viewer renders the bottom of the file with the real PR number
1373filled in (since GitHub knows the PR number when rendering), butgit show dacf068 -- perf-changelog.yamland reading the on-disk file directly both confirm the committed content is literallyXXXX. Multiple independent verifiers confirmed this by reading the file and the git object directly.Step-by-step proof
pull/1373.perf-changelog.yamlappends a new entry whose final line ispr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXXX.git show dacf068:perf-changelog.yaml→ line 2455), the value is the literal stringXXXX, not a numeric PR id.https://github.com/SemiAnalysisAI/InferenceX/pull/XXXXreturns a 404 (GitHub rejects non-numeric PR identifiers).pr-linkentry in the file is a real numeric PR (e.g./pull/1329,/pull/1344,/pull/1346), so this is the only entry that 404s.Impact
No runtime effect — this is documentation/metadata only. But
perf-changelog.yamlis the audit trail tying recipe-key changes back to the PRs that introduced them; a dead link defeats that purpose for this entry.Fix
Replace
XXXXwith1373on line 2455 ofperf-changelog.yaml.