-
Notifications
You must be signed in to change notification settings - Fork 704
ci: Use nvcr.io image for sglang base container #3996
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
Conversation
Signed-off-by: Dillon Cullinan <[email protected]>
WalkthroughIntroduces a Docker proxy mechanism by adding a new DOCKER_PROXY build argument. The GitHub Actions workflow passes this argument to the container build process, while the Dockerfile accepts and uses it to prefix the base image URL for framework fetching. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes
Poem
Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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 |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
container/Dockerfile.sglang (1)
42-43: Add documentation for the DOCKER_PROXY argument.The DOCKER_PROXY build argument enables pulling the framework base image through a Docker registry proxy to mitigate Docker Hub rate limits. Without clarification, the purpose and expected format (a registry URL with trailing slash, e.g.,
registry.example.com/dockerhub/) may not be obvious to future maintainers.Consider adding a brief comment above line 42 explaining the argument's purpose and format:
+# Docker registry proxy to fetch the CUDA framework image (e.g., ECR proxy for Docker Hub rate limiting) +# If not provided, defaults to empty string and pulls directly from Docker Hub ARG DOCKER_PROXY
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/actions/docker-build/action.yml(2 hunks)container/Dockerfile.sglang(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: trtllm (arm64)
- GitHub Check: sglang (amd64)
- GitHub Check: trtllm (amd64)
- GitHub Check: sglang (arm64)
- GitHub Check: operator (arm64)
- GitHub Check: vllm (arm64)
- GitHub Check: vllm (amd64)
- GitHub Check: operator (amd64)
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (2)
.github/actions/docker-build/action.yml (2)
97-97: Good: ECR_HOSTNAME is properly set as environment variable for build step.Setting ECR_HOSTNAME as an environment variable (line 97) makes it accessible to the build.sh invocation, enabling the DOCKER_PROXY to be constructed correctly. This follows a clean pattern where the environment variable is defined once and reused.
133-134: Now let me verify if there are any Dockerfile-level issues or if the DOCKER_PROXY argument is actually used:Now let me search for how DOCKER_PROXY is actually used in the Dockerfile:Based on my verification, I've found that the build.sh script correctly forwards theDOCKER_PROXYargument to the docker build command. However, the original review comment requests verification of three specific concerns. Let me check if there's anything in the Dockerfiles that uses DOCKER_PROXY:The build.sh script correctly forwards the DOCKER_PROXY argument to docker build.
Based on verification of the build.sh script:
Argument forwarding: The script properly handles
--build-argparameters via a case statement that appends them to theBUILD_ARGSvariable, which is then expanded in both docker build commands (for dev and framework images). The DOCKER_PROXY argument is forwarded as-is without modification.Trailing slash: Registry mirror URLs must be the root of a domain with no path components other than an optional trailing slash, so the
/dockerhub/suffix is valid syntax for Docker registry proxies.The review comment's verification concerns are satisfied at the build script level. The ECR registry configuration and how the Dockerfile consumes the DOCKER_PROXY variable are outside the scope of build.sh's argument passing.
Signed-off-by: Dillon Cullinan <[email protected]>
Signed-off-by: Dillon Cullinan <[email protected]>
Signed-off-by: Dillon Cullinan <[email protected]>
…o sglang-docker-proxy
Signed-off-by: Dillon Cullinan <[email protected]>
Signed-off-by: Tushar Sharma <[email protected]>
Signed-off-by: Tushar Sharma <[email protected]>
Signed-off-by: Dillon Cullinan <[email protected]> Signed-off-by: Tushar Sharma <[email protected]> Co-authored-by: Tushar Sharma <[email protected]>
This reverts commit df7753f.
Signed-off-by: Dillon Cullinan <[email protected]> Signed-off-by: Tushar Sharma <[email protected]> Co-authored-by: Dillon Cullinan <[email protected]>
Overview:
Using nvidia/cuda:12.9.1-cudnn-devel-ubuntu24.04 is hitting Rate limits in our CI. This PR updates to use nvcr.io/nvidia/cuda:12.9.1-cudnn-devel-ubuntu24.04 which will prevent hitting rate limits since we have an authorized service key. These are the exact same images after validating with docker scout compare.
Summary by CodeRabbit