-
Notifications
You must be signed in to change notification settings - Fork 208
[Klaud Cold] Update dsv4-fp4-mi355x-vllm vLLM ROCm image to v0.22.0 #1624
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
Merged
Merged
Changes from all commits
Commits
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
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.
🟡 Nit: the comment block above this entry (lines 2118-2127) describes the image as a 'nightly image' and contains an 'IMPORTANT' block instructing maintainers to pin to a digest-suffixed nightly rather than the floating
:nightly(becauselaunch_mi355x-amds.shcaches enroot squashfs files by image string). After the bump tov0.22.0(a stable versioned tag), both the 'nightly image' wording and the digest-vs-floating rationale are stale and will mislead future maintainers. Consider updating the comment block to reflect the stable-tag pin, or dropping the nightly-pinning paragraph entirely.Extended reasoning...
What's stale
The diff changes the image at line 2138 from
vllm/vllm-openai-rocm:nightly-4f940896a32c9e2a0eba7f50d521bf5f6b4de458tovllm/vllm-openai-rocm:v0.22.0. The 10-line comment block immediately above the entry (lines 2118-2127), which was written to explain why the previous nightly pin was needed, now contradicts the actual image:v0.22.0is a stable release tag, not a nightly.:nightly. launch_mi355x-amds.sh caches enroot squashfs files keyed on the image string and short-circuits re-import if the file already exists, so the floating tag silently keeps a stale build even after Docker Hub updates:nightly." — the digest-vs-floating concern is specific to mutable tags.v0.22.0is an immutable stable release, so this entire rationale is moot for the currently pinned image.Why this matters (and why it's still just a nit)
The staleness is introduced directly by this PR — the comment was accurate when the image was
nightly-4f940896...and becomes inaccurate the moment the image becomesv0.22.0. A future maintainer looking at this entry to understand why the tag is structured this way will read an IMPORTANT block that doesn't describe what they're seeing, which is the canonical recipe for stale-comment confusion.That said, there is no functional impact: the sweep config itself is correct, and the enroot squashfs caching behavior described in the comment is still true in general (it's just no longer the reason this particular tag is pinned the way it is). For that reason this is filed as a nit, not a blocking issue.
Addressing the refutation
One verifier refuted this as a non-egregious nit not worth posting. I think it's worth flagging because (a) the comment block is unusually detailed — it isn't a one-line label, it's an
IMPORTANT:paragraph with a specific operational rationale; (b) the staleness was created by this PR rather than being pre-existing drift, so it's directly in scope for an image-bump cleanup; and (c) the wording is actively misdirecting ("nightly image" / "digest-suffixed nightly tag") rather than merely incomplete. The IMPORTANT-block cache-keying guidance is still generally accurate as a reference, but its placement above a stable-tag entry implies it explains this pin, which it no longer does.Suggested fix
Either:
launch_mi355x-amds.shcaches enroot squashfs files by image string and so any tag (nightly or stable) must be replaced rather than re-pushed; orv0.22.0tag is immutable on Docker Hub and the caching concern doesn't apply to it in practice.