[ROCm][CI] Build and publish TheRock nightly docker images - #55014
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe PR adds a ROCk x86_64 release pipeline. It caches and reuses the ROCk base image in ECR, builds the release image, pushes a ChangesROCK release flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change enables ROCk nightly image publishing and allows cold-cache base-image builds up to six hours. The current implementation has no identified merge-blocking risk. Sequence Diagram(s)sequenceDiagram
participant Buildkite
participant ECR
participant ROCkBuild
participant DockerHub
Buildkite->>ECR: Check or build cached ROCk base image
Buildkite->>ROCkBuild: Build ROCk release image
ROCkBuild->>ECR: Push commit-specific -rock image
Buildkite->>DockerHub: Publish ROCk nightly variants
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 unsupported.)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
901125f to
158cad8
Compare
Rohan138
changed the title
docker/Dockerfile.rock_base and docker/Dockerfile.rock were added in vllm-project#49925 but are not wired into any pipeline. Duplicate ROCm release-pipeline Jobs 1 and 6 for them so the release pipeline produces a base image and a vllm-openai image from TheRock wheels, and publishes them to Docker Hub as vllm/vllm-openai-rock:{base-,}nightly{,-<commit>} when NIGHTLY=1. push-nightly-builds-rocm.sh now takes the target Docker Hub repo and the ECR tag suffix as arguments, defaulting to the current ROCm values, so both variants share one script. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Rohan Potdar <rohan.potdar@amd.com>
158cad8 to
2eb7c2f
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
✅ @Rohan138, CI is now available for this PR.
|
|
/ci run |
|
✅ Triggered Buildkite CI #86986 for commit |
|
Looks good to me, but I'm not super familiar with these things. |
| else | ||
| echo "CACHE MISS - Building from scratch..." | ||
|
|
||
| DOCKER_BUILDKIT=1 docker buildx build \ |
There was a problem hiding this comment.
Do you need DOCKER_BUILDKIT=1 if using buildx?
There was a problem hiding this comment.
Yeah I think this is a remnant of the fact that the regular ROCm build stage does the same:
vllm/.buildkite/release-pipeline.yaml
Line 662 in 4ae6228
But I think you're right that it's unnecessary, maybe we can remove it in both places.
| # Save ECR tag for downstream jobs | ||
| buildkite-agent meta-data set "rock-base-image-tag" "$${ECR_CACHE_TAG}" | ||
| env: | ||
| DOCKER_BUILDKIT: "1" |
There was a problem hiding this comment.
Same here, is it necessary or is this just something you need for buildkite?
| echo " BUILDKITE_COMMIT: $${BUILDKITE_COMMIT}" | ||
| echo "========================================" | ||
|
|
||
| DOCKER_BUILDKIT=1 docker build \ |
|
|
||
| docker push public.ecr.aws/q9t5s3a7/vllm-release-repo:$${BUILDKITE_COMMIT}-rock | ||
| env: | ||
| DOCKER_BUILDKIT: "1" |
|
@Rohan138 I just added a small patch. Also I think Randy is right here. Could we test this with his suggestions integrated? |
Co-authored-by: OpenAI Codex <noreply@openai.com> Signed-off-by: Andreas Karatzas <Andreas.Karatzas@amd.com>
| echo "CACHE MISS - Building from scratch..." | ||
|
|
||
| DOCKER_BUILDKIT=1 docker buildx build \ | ||
| --file docker/Dockerfile.rock_base \ |
There was a problem hiding this comment.
One other question, do you want to copy build/requirements/rock.txt to build/requirements/rocm.txt before building? Not sure if necessary.
There was a problem hiding this comment.
Well currently rock.txt and rocm.txt are more or less identical (and also partially unused as long as we use the dockerfiles as our primary source of truth). I'll come back and clean this up in future PRs.
…y-rocm714 Instead of a separate vllm/vllm-openai-rock Docker Hub repo, publish the TheRock nightly into the existing vllm/vllm-openai-rocm repo under a tag variant (rocm714, from ROCM_SDK_VERSION=7.14.0). This leaves room for nightly-rocm714-ubuntu2404, nightly-rocm100, etc. as more TheRock variants land, and keeps a single repo for ROCm-family images. push-nightly-builds-rocm.sh now takes [ECR_TAG_SUFFIX TAG_VARIANT] rather than [DOCKERHUB_REPO ECR_TAG_SUFFIX]: the repo is fixed to vllm/vllm-openai-rocm and the second arg becomes the Docker Hub tag flavor. No args still publishes the ROCm apt build as :nightly, unchanged; "rock rocm714" publishes the TheRock build as :nightly-rocm714. The arg-count discipline, strict mode, BASE_ECR_IMAGE dry-run path and fail-hard metadata read are preserved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Rohan Potdar <rohan.potdar@amd.com>
Rohan138
changed the title
|
@AndreasKaratzas can I clean up DOCKER_BUILDKIT=1 in a follow-up? There's multiple other follow-ups to be done here on e.g. getting ROCm 7.14 wheels and artifacts up, enabling ub2404 builds, unifying the release and CI pipelines, etc, so I'm using this PR just to get an initial ROCm 7.14 nightly image out before we iterate. |
I think that's a good idea sure |
There was a problem hiding this comment.
LGTM. Am I understanding correctly that this PR will essentially be reverted once we fully move on from ROCm 7.2? If so, maybe we can add a TODO for that to make it clear.
Also, should we choose a different naming convention besides nightly-rocm714? Otherwise let's just make sure we remember to update it if/when we move to ROCm 10 previews ("nightly-rocm10").
rasmith
left a comment
rasmith
left a comment
There was a problem hiding this comment.
The things I had question about are harmless, just little cleanups.
|
/ci run |
|
✅ Triggered Buildkite CI #87150 for commit |
The seed (cold-cache) build of the ROCk base compiles AITER (PREBUILD_KERNELS, gfx942;gfx950) and flash-attention from scratch; both were still running at ~176 min when the 180-min limit hit in release-v2 vllm-project#6076. Raise the limit so the first build can populate the ECR image cache, after which later runs skip it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Rohan Potdar <rohan.potdar@amd.com>
|
/ci run |
|
✅ Triggered Buildkite CI #87171 for commit |
…ect#55014) Signed-off-by: Rohan Potdar <rohan.potdar@amd.com> Signed-off-by: Andreas Karatzas <Andreas.Karatzas@amd.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Andreas Karatzas <Andreas.Karatzas@amd.com> Co-authored-by: OpenAI Codex <noreply@openai.com> Signed-off-by: Jyotirmoy Roy <jyotirmoyroy649@gmail.com>
5 participants
Purpose
docker/Dockerfile.rock_baseanddocker/Dockerfile.rockwere added in #49925 but nothing references them. This duplicates ROCm release-pipeline Jobs 1 and 6 (plus the nightly publish) for them, producing onNIGHTLY=1, in the existingvllm/vllm-openai-rocmrepo:vllm/vllm-openai-rocm:base-nightly-rocm714/-<commit>—Dockerfile.rock_basevllm/vllm-openai-rocm:nightly-rocm714/-<commit>—Dockerfile.rock --target vllm-openaiThe TheRock images share the existing
vllm/vllm-openai-rocmrepo and are distinguished by a tag variant (rocm714, fromROCM_SDK_VERSION=7.14.0), leaving room fornightly-rocm714-ubuntu2404,nightly-rocm100, etc. as more TheRock variants land.push-nightly-builds-rocm.shtakes[ECR_TAG_SUFFIX TAG_VARIANT](both or neither): the repo is fixed tovllm/vllm-openai-rocm, no args publishes the ROCm apt build as:nightly(unchanged), androck rocm714publishes the TheRock build as:nightly-rocm714.Notes for review:
vllm-openaidoesn't consumedocker/context/base-wheels/, so the base step caches the ECR image only and skips the S3 wheel cache.startswith("nightly-"), which also matchesnightly-rocm714-<commit>. The two variants therefore share a single 14-tag retention budget in the repo. The rock step cleans its ownnightly-rocm714-/base-nightly-rocm714-prefixes, but until the plain-ROCm cleanup is tightened (or given a variant tag of its own), a burst of TheRock nightlies could evict plain ROCm ones early. Flagging rather than reworking the shared cleanup here.publish-release-images.shis intentionally untouched — nolatest/versioned TheRock release tag yet; that's a follow-up.Draft: the image builds haven't been through Buildkite yet, and there are no docs changes. This builds on @AndreasKaratzas's "Harden ROCm nightly image publishing" commit (strict mode,
BASE_ECR_IMAGEdry-run path, fail-hard metadata), which is preserved.Not a duplicate:
gh pr list --state open --search "Dockerfile.rock"/"vllm-openai-rock"/"TheRock"returns no PR wiring these Dockerfiles into a pipeline; #49925 that added them is merged, and the other open TheRock PRs (#47092, #53837) are test fixes.AI assistance was used (Claude Code); the diff and reasoning have been reviewed by me and I own defending them in review.
Test Plan
CI-only change — no vLLM runtime code is touched, so there are no model/accuracy implications.
Test Result
YAML parses, shellcheck clean. No-arg invocation emits
docker pull/tag/pushtargets identical to the pre-change ROCm behaviour (vllm/vllm-openai-rocm:{base-,}nightly{,-deadbeef}).rock rocm714emitsvllm/vllm-openai-rocm:{base-,}nightly-rocm714{,-deadbeef}from the-rockECR images. Arg-count guards reject 1 and 3 args with exit 2.Not yet run through Buildkite — neither Dockerfile has been built in CI; that is what this draft is for, and I will post the pipeline result here before marking it ready.