Conversation
|
@abrarsheikh this will change the base from common py3.9 base to multipy base, and probably will fix the npm not found issue. |
There was a problem hiding this comment.
Code Review
This pull request correctly updates the CI configuration to make the minbuild process multi-python aware. The changes are consistent across the affected rayci.yml files and the wanda.yaml build definition, and the logic appears sound. I have one suggestion to improve the maintainability of the min.build.wanda.yaml file by reducing duplication.
ci/docker/min.build.wanda.yaml
Outdated
| build_args: | ||
| - PYTHON_VERSION | ||
| - EXTRA_DEPENDENCY | ||
| - DOCKER_IMAGE_BASE_BUILD=cr.ray.io/rayproject/oss-ci-base_build-py$PYTHON_VERSION |
There was a problem hiding this comment.
This line duplicates the base image URL from the froms field on line 2. To improve maintainability and avoid potential inconsistencies, consider defining this URL only once. If the wanda tool supports it, you could use a YAML anchor to define the URL in one place and reference it in both froms and build_args.
.buildkite/ml.rayci.yml
Outdated
| label: "wanda: minbuild-ml-py{{matrix}}" | ||
| wanda: ci/docker/min.build.wanda.yaml | ||
| depends_on: oss-ci-base_build | ||
| depends_on: oss-ci-base_build-multipy |
There was a problem hiding this comment.
Bug: ML Group: Undeclared Build Dependency
The minbuild-ml step depends on oss-ci-base_build-multipy, but this dependency is not declared at the group level. The ml group's top-level depends_on should include oss-ci-base_build-multipy to ensure the build is available when minbuild-ml attempts to use it, matching the pattern in core.rayci.yml and serve.rayci.yml.
.buildkite/ml.rayci.yml
Outdated
| label: "wanda: minbuild-ml-py{{matrix}}" | ||
| wanda: ci/docker/min.build.wanda.yaml | ||
| depends_on: oss-ci-base_build | ||
| depends_on: oss-ci-base_build-multipy |
There was a problem hiding this comment.
Bug: ML Group: Standardize Build Dependencies
The ml group's top-level depends_on is missing oss-ci-base_build-multipy, even though the minbuild-ml step depends on it. This is inconsistent with the core and serve pipeline groups, which declare this dependency at the group level. The ml group should include oss-ci-base_build-multipy in its top-level depends_on to ensure proper dependency ordering and availability.
6dc6324 to
e111abf
Compare
e111abf to
01b1725
Compare
There was a problem hiding this comment.
Bug: minbuild-serve missing dependency on base multipy build
The minbuild-serve build step is missing a depends_on: oss-ci-base_build-multipy field. The min.build.wanda.yaml was updated to use multi-Python base images, and minbuild-ml was updated to explicitly depend on oss-ci-base_build-multipy. For consistency and correctness, minbuild-serve should have the same explicit dependency since it uses the same min.build.wanda.yaml template.
.buildkite/serve.rayci.yml#L26-L37
ray/.buildkite/serve.rayci.yml
Lines 26 to 37 in 01b1725
add `-f` flags, so that they will not fail silently Signed-off-by: Lonnie Liu <lonnie@anyscale.com>
69891be to
4209d32
Compare
fix serve minbuild ci base image Signed-off-by: Lonnie Liu <lonnie@anyscale.com>
adding `-f` flag in curl's, so that failure can be properly captured. Signed-off-by: Lonnie Liu <lonnie@anyscale.com> Signed-off-by: Aydin Abiar <aydin@anyscale.com>
adding `-f` flag in curl's, so that failure can be properly captured. Signed-off-by: Lonnie Liu <lonnie@anyscale.com>
adding `-f` flag in curl's, so that failure can be properly captured. Signed-off-by: Lonnie Liu <lonnie@anyscale.com> Signed-off-by: YK <1811651+ykdojo@users.noreply.github.com>
adding `-f` flag in curl's, so that failure can be properly captured. Signed-off-by: Lonnie Liu <lonnie@anyscale.com>
adding `-f` flag in curl's, so that failure can be properly captured. Signed-off-by: Lonnie Liu <lonnie@anyscale.com> Signed-off-by: peterxcli <peterxcli@gmail.com>
adding
-fflag in curl's, so that failure can be properly captured.