-
Notifications
You must be signed in to change notification settings - Fork 7.1k
[build] add ray-wheel for ray and ray-cpp, local wheel build scripts #59781
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
adds new Wanda + Dockerfile for building out ray and ray-cpp. Most of the challenge was in making sure all Wanda pre-req images were built out locally. build_targets.py declares various Wanda build targets, allowing for easy DEPS setting. It's pretty fragile in terms of naming, but resolves the issue of orchestrating all the builds for local development Additionally, once we enable building the Ray image, we can easily extend the file for those targets Signed-off-by: andrew <andrew@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 introduces a comprehensive local build system for ray and ray-cpp wheels using Wanda and Docker, which is a significant improvement for local development workflows. The code is well-structured, using Python classes to define build targets and their dependencies, making the system modular and extensible. The inclusion of a Makefile wrapper provides a convenient command-line interface. The test suite for the build targets is thorough and well-written.
I have identified one bug in the wheel extraction logic that would cause failures on non-default architectures, and one critical issue where a required Wanda spec file is missing, which would prevent the ray-cpp wheel from being built. After addressing these points, this will be a solid contribution.
| class RayCppWheel(BuildTarget): | ||
| """Builds the ray-cpp wheel (includes cpp-core).""" | ||
|
|
||
| SPEC = "ci/docker/ray-cpp-wheel.wanda.yaml" |
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.
| def extract_wheels( | ||
| python_version: str = "3.10", | ||
| target: str = "ray", | ||
| out_dir: Optional[Path] = None, | ||
| dry_run: bool = False, | ||
| ) -> None: | ||
| """Extract wheels from built images.""" | ||
| if out_dir is None: | ||
| out_dir = get_repo_root() / ".whl" | ||
|
|
||
| # Create minimal context just for image_name() calls | ||
| ctx = BuildContext(env={"PYTHON_VERSION": python_version, "ARCH_SUFFIX": ""}) |
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 extract_wheels function hardcodes ARCH_SUFFIX to "" when creating its BuildContext. This will cause an error when trying to extract wheels for a build that used a specific architecture (e.g., aarch64), as the image name will be constructed incorrectly.
The arch_suffix should be passed down from build_wheels and used here. You'll also need to update the call at line 105 to pass arch_suffix:
# in build_wheels()
extract_wheels(python_version, target, out_dir, dry_run, arch_suffix=arch_suffix)| def extract_wheels( | |
| python_version: str = "3.10", | |
| target: str = "ray", | |
| out_dir: Optional[Path] = None, | |
| dry_run: bool = False, | |
| ) -> None: | |
| """Extract wheels from built images.""" | |
| if out_dir is None: | |
| out_dir = get_repo_root() / ".whl" | |
| # Create minimal context just for image_name() calls | |
| ctx = BuildContext(env={"PYTHON_VERSION": python_version, "ARCH_SUFFIX": ""}) | |
| def extract_wheels( | |
| python_version: str = "3.10", | |
| target: str = "ray", | |
| out_dir: Optional[Path] = None, | |
| dry_run: bool = False, | |
| arch_suffix: str = "", | |
| ) -> None: | |
| """Extract wheels from built images.""" | |
| if out_dir is None: | |
| out_dir = get_repo_root() / ".whl" | |
| # Create minimal context just for image_name() calls | |
| ctx = BuildContext(env={"PYTHON_VERSION": python_version, "ARCH_SUFFIX": arch_suffix}) |
Signed-off-by: andrew <andrew@anyscale.com>
Signed-off-by: andrew <andrew@anyscale.com>
adds new Wanda + Dockerfile for building out ray and ray-cpp.
Most of the challenge was in making sure all Wanda pre-req images were built out locally.
build_targets.py declares various Wanda build targets, allowing for easy DEPS setting. It's pretty fragile in terms of naming, but resolves the issue of orchestrating all the builds for local development
Additionally, once we enable building the Ray image, we can easily extend the file for those targets