-
Notifications
You must be signed in to change notification settings - Fork 7k
[ci][deps] updating docgpubuild to run on python 3.10 - CI-UPGRADES(3/5) #58068
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: elliot-barn <elliot.barnwell@anyscale.com>
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
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 updates the CI configuration to add support for Python 3.10, particularly for the docgpubuild step. The changes involve adding 3.10 to various build matrices across several CI files and updating test steps to use the new Python version. While the intent is clear, I've found a critical issue in the configuration for docgpubuild in .buildkite/base.rayci.yml that will likely cause the build to fail due to a non-existent base image and a mismatched image name. Additionally, there's a missing test case in .buildkite/serve.rayci.yml for a newly introduced minbuild-serve-py3.10 image. Please see my detailed comments for suggestions on how to fix these issues.
| - name: docgpubuild | ||
| label: "wanda: docgpubuild-py3.10" | ||
| wanda: ci/docker/docgpu.build.wanda.yaml | ||
| depends_on: oss-ci-base_gpu | ||
| depends_on: oss-ci-base_gpu-multipy | ||
| env: | ||
| PYTHON: "3.10" | ||
| tags: cibase |
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 configuration for the docgpubuild step is incorrect and will likely cause the build to fail.
- Incorrect Base Image: The
wandafileci/docker/docgpu.build.wanda.yamlappears to rely on a base imagecr.ray.io/rayproject/oss-ci-base_gpu. However, this PR replaces the step that builds this image with a matrix buildoss-ci-base_gpu-multipy, which produces version-suffixed images (e.g.,...-py3.10). The original base image tag will no longer be available. - Mismatched Image Name: Test steps in
data.rayci.ymlandserve.rayci.ymlare updated to expect an image from a build nameddocgpubuild-py3.10. However,ci/docker/docgpu.build.wanda.yamlseems to be hardcoded to produce an image nameddocgpubuild. This mismatch will prevent tests from finding the correct Docker image.
To fix this, ci/docker/docgpu.build.wanda.yaml should be parameterized to use the PYTHON environment variable for the image name, tag, and base image, similar to other wanda files in this repository. Since that file is not part of this PR, I'm highlighting the issue here as it will break this build and dependent steps.
.buildkite/serve.rayci.yml
Outdated
| --build-name minbuild-serve-py3.9 | ||
| --test-env=RAY_DEFAULT=1 | ||
| --only-tags minimal | ||
| depends_on: minbuild-serve | ||
| depends_on: minbuild-multipy |
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 minbuild-multipy step is configured to build images for both Python 3.9 and 3.10 with the serve extra dependency (minbuild-serve-py3.9 and minbuild-serve-py3.10). However, this test step, :ray-serve: serve: serve minimal, only runs against the py3.9 image. The minbuild-serve-py3.10 image is built but remains untested.
Please consider adding a new test step to cover the Python 3.10 case to ensure complete test coverage for the newly built image.
|
the data doc gpu test is failing? |
aslonnie
left a 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.
seems that it needs some more work?
Yea requested review a little too early. I'll fix this ASAP |
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
nrghosh
left a 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.
thanks, lgtm pending tests
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
aslonnie
left a 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.
seems more than docbuild now.. could you update the PR's title and description?
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
…ject/ray into elliot-barn/updating-docgpubuild
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
ci/docker/docgpu.build.wanda.yaml
Outdated
| - python/requirements/ml/rllib-test-requirements.txt | ||
| build_args: | ||
| - DOCKER_IMAGE_BASE_BUILD=cr.ray.io/rayproject/oss-ci-base_gpu-py$PYTHON | ||
| - PYTHON |
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.
why need this build args?
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.
keeping docker image base build because its now multipy and the default BASE BUILD will be base gpu py3.10
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
…/5) (ray-project#58068) Updating docgpubuild to run on python 3.10 updating minbuild-multiply job name to minbuild-serve Post merge test that uses the docgpubuild image: https://buildkite.com/ray-project/postmerge/builds/14073 --------- Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com> Co-authored-by: Nikhil G <nrghosh@users.noreply.github.com>
…GRADES(3/5) " (ray-project#58266) Reverts ray-project#58068
…/5) (ray-project#58068) Updating docgpubuild to run on python 3.10 updating minbuild-multiply job name to minbuild-serve Post merge test that uses the docgpubuild image: https://buildkite.com/ray-project/postmerge/builds/14073 --------- Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com> Co-authored-by: Nikhil G <nrghosh@users.noreply.github.com>
…GRADES(3/5) " (ray-project#58266) Reverts ray-project#58068
…/5) (ray-project#58068) Updating docgpubuild to run on python 3.10 updating minbuild-multiply job name to minbuild-serve Post merge test that uses the docgpubuild image: https://buildkite.com/ray-project/postmerge/builds/14073 --------- Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com> Co-authored-by: Nikhil G <nrghosh@users.noreply.github.com> Signed-off-by: Aydin Abiar <aydin@anyscale.com>
…GRADES(3/5) " (ray-project#58266) Reverts ray-project#58068 Signed-off-by: Aydin Abiar <aydin@anyscale.com>
Updating docgpubuild to run on python 3.10
updating minbuild-multiply job name to minbuild-serve
Post merge test that uses the docgpubuild image: https://buildkite.com/ray-project/postmerge/builds/14073