-
Notifications
You must be signed in to change notification settings - Fork 7.1k
[ci] removing requirement file and constraint file build args #60151
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: elliot-barn <[email protected]>
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.
Code Review
This pull request refactors the Docker image build process by moving build arguments like REQUIREMENTS_FILE and CONSTRAINTS_FILE into the Dockerfiles as ARG with default values. This is a good simplification of the CI configuration. However, I've found several critical issues that will likely cause build failures, including a missing build argument declaration, incomplete source file lists, and an incorrect build argument that would override a default with an empty value. I've also pointed out some medium-severity issues where unused files are included in the build context, which should be cleaned up.
docker/base-extra/cuda.wanda.yaml
Outdated
| - python/deplocks/base_extra/ray_base_extra_py${PYTHON_VERSION}.lock | ||
| build_args: | ||
| - BASE_IMAGE=cr.ray.io/rayproject/$IMAGE_TYPE-py$PYTHON_VERSION-cu$CUDA_VERSION-base$ARCH_SUFFIX | ||
| - PYTHON_DEPSET=$REQUIREMENTS_FILE |
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.
| FROM "$BASE_IMAGE" | ||
|
|
||
| ARG PIP_REQUIREMENTS | ||
| ARG PIP_REQUIREMENTS="python/deplocks/base_extra_testdeps/ray_base_extra_testdeps_py${PYTHON_VERSION}.lock" |
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 default value for PIP_REQUIREMENTS uses ${PYTHON_VERSION}, but PYTHON_VERSION is not declared as a build argument in this Dockerfile. This will cause ${PYTHON_VERSION} to be an empty string, leading to a build failure when trying to copy the requirements file. You should declare PYTHON_VERSION as an ARG before it's used.
ARG PYTHON_VERSION="3.10"
ARG PIP_REQUIREMENTS="python/deplocks/base_extra_testdeps/ray_base_extra_testdeps_py${PYTHON_VERSION}.lock"
| froms: ["ubuntu:22.04"] | ||
| dockerfile: docker/base-deps/Dockerfile | ||
| srcs: | ||
| - python/requirements_compiled.txt |
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.
This srcs entry for python/requirements_compiled.txt appears to be unnecessary. The corresponding docker/base-deps/Dockerfile has been updated to use the $CONSTRAINTS_FILE build argument, which defaults to python/requirements_compiled_py${PYTHON_VERSION}.txt, and no longer directly copies python/requirements_compiled.txt. To keep the build context clean and avoid confusion, this line can be removed.
| froms: ["nvidia/cuda:$CUDA_VERSION-devel-ubuntu22.04"] | ||
| dockerfile: docker/base-deps/Dockerfile | ||
| srcs: | ||
| - python/requirements_compiled.txt |
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.
| froms: ["nvidia/cuda:$CUDA_VERSION-runtime-ubuntu22.04"] | ||
| dockerfile: docker/base-slim/Dockerfile | ||
| srcs: | ||
| - python/requirements_compiled.txt |
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.
Signed-off-by: elliot-barn <[email protected]>
Signed-off-by: elliot-barn <[email protected]>
Signed-off-by: elliot-barn <[email protected]>
Signed-off-by: elliot-barn <[email protected]>
|
|
||
| ARG PYTHON_DEPSET | ||
| ARG PYTHON_VERSION=3.10 | ||
| ARG PYTHON_DEPSET="python/deplocks/base_extra/ray_base_extra_py${PYTHON_VERSION}.lock" |
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.
when this is copied in, this will overwrite the one that is copied in from base-deps?
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.
correct. Should overwrite the existing base-deps lock file
Signed-off-by: elliot-barn <[email protected]>
Signed-off-by: elliot-barn <[email protected]>
| dockerfile: release/ray_release/byod/byod.Dockerfile | ||
| srcs: | ||
| - $REQUIREMENTS_FILE | ||
| - python/deplocks/base_extra_testdeps/ray_base_extra_testdeps_cuda_py{{matrix.python}}.lock |
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.
Ray-ML testdeps build uses wrong lock file path
High Severity
The ray-mlcudabaseextra-testdeps build previously used REQUIREMENTS_FILE: "python/deplocks/base_extra_testdeps/ray_ml_base_extra_testdeps_cuda_py..." with the ray_ml_ prefix. This was removed without providing an alternative mechanism. The hardcoded srcs path in cuda.wanda.yaml and the PIP_REQUIREMENTS default in byod.Dockerfile both reference ray_base_extra_testdeps_cuda_py... (without ray_ml_ prefix), but separate lock files exist for ray-ml as shown in the depsets configuration. The ray-ml CUDA testdeps build will fail.
Additional Locations (1)
Signed-off-by: elliot-barn <[email protected]>
Signed-off-by: elliot-barn <[email protected]>
Signed-off-by: elliot-barn <[email protected]>
Signed-off-by: elliot-barn <[email protected]>
Signed-off-by: elliot-barn <[email protected]>
Signed-off-by: elliot-barn <[email protected]>
Signed-off-by: elliot-barn <[email protected]>
Signed-off-by: elliot-barn <[email protected]>
Signed-off-by: elliot-barn <[email protected]>
Signed-off-by: elliot-barn <[email protected]>
Signed-off-by: elliot-barn <[email protected]>
removing requirement file and constraint file build args from the following images
base-deps
base-extra
base-extra-test-deps
base-slim (add constraints file as build arg)
defaulting PYTHON_DEPSET & CONSTRAINTS_FILE args in the dockerfile
Renaming ray-llm, ray-gpu & ray base extra testdeps lock files. IMAGE_TYPE defined on the BK jobs will determine which lock file to copy to the image
hello world release test run: https://buildkite.com/ray-project/release/builds/75854