-
Notifications
You must be signed in to change notification settings - Fork 7.1k
DNM: A/B validating Wanda-generated Ray images #60186
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
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 building process in the CI pipeline. It separates the build and push steps, introducing a new wanda tool for building and a push_ray_image.py script for publishing images to Docker Hub. The changes include new Buildkite pipeline steps, wanda configuration files, a generic Dockerfile, and the new push script with corresponding tests.
The overall refactoring is well-structured and the new Python script is robust and well-tested. I have a few suggestions to improve the maintainability of the new Dockerfile and the debuggability of the push script.
| RUN <<EOF | ||
| #!/bin/bash | ||
| set -euo pipefail | ||
|
|
||
| WHEEL_FILES=(/tmp/ray-*.whl) | ||
| if [[ ${#WHEEL_FILES[@]} -ne 1 ]]; then | ||
| echo "Error: Expected 1 ray wheel file, but found ${#WHEEL_FILES[@]} in /tmp/." >&2 | ||
| ls -l /tmp/*.whl >&2 | ||
| exit 1 | ||
| fi | ||
| WHEEL_FILE="${WHEEL_FILES[0]}" | ||
|
|
||
| echo "Installing wheel: $WHEEL_FILE" | ||
|
|
||
| $HOME/anaconda3/bin/pip --no-cache-dir install \ | ||
| -c /tmp/requirements_compiled.txt \ | ||
| "${WHEEL_FILE}[all]" | ||
|
|
||
| $HOME/anaconda3/bin/pip freeze > /home/ray/pip-freeze.txt | ||
|
|
||
| echo "Ray version: $($HOME/anaconda3/bin/python -c 'import ray; print(ray.__version__)')" | ||
| EOF |
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.
| $HOME/anaconda3/bin/pip --no-cache-dir install \ | ||
| -c /tmp/requirements_compiled.txt \ | ||
| "${WHEEL_FILE}[all]" | ||
|
|
||
| $HOME/anaconda3/bin/pip freeze > /home/ray/pip-freeze.txt | ||
|
|
||
| echo "Ray version: $($HOME/anaconda3/bin/python -c 'import ray; print(ray.__version__)')" |
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 script uses hardcoded paths to pip and python (e.g., $HOME/anaconda3/bin/pip). If the anaconda bin directory is in the PATH in the base image (which is common practice), you can just use pip and python directly. This would make the script more robust against changes in the anaconda installation path.
pip --no-cache-dir install \
-c /tmp/requirements_compiled.txt \
"${WHEEL_FILE}[all]"
pip freeze > /home/ray/pip-freeze.txt
echo "Ray version: $(python -c 'import ray; print(ray.__version__)')"
a6d5c52 to
0d911cd
Compare
Adds Dockerfile and Wanda files for Ray images, pulling artifacts from previous steps. Buildkite steps also updated to use these, including both build and upload steps. Topic: ray-image Signed-off-by: andrew <[email protected]>
6c1d0fc to
e81b321
Compare
Validating #59936