-
Notifications
You must be signed in to change notification settings - Fork 7.1k
wanda python-agnostic ray-cpp wheel (py3-none tag)(x86_64) #59969
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
|
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 Wanda-based builds for Ray C++ wheels on x86_64, adding new CI configurations and Dockerfiles. The changes are well-structured, but there are opportunities to improve maintainability and ensure correctness across different Python versions. My review includes suggestions to refactor the Buildkite configuration to reduce duplication and to address a potential issue with hardcoded Python dependency files in the Wanda configuration.
| - name: ray-cpp-core-build | ||
| label: "wanda: cpp core py{{matrix}} (x86_64)" | ||
| wanda: ci/docker/ray-cpp-core.wanda.yaml | ||
| matrix: | ||
| - "3.10" | ||
| - "3.11" | ||
| - "3.12" | ||
| env: | ||
| PYTHON_VERSION: "{{matrix}}" | ||
| ARCH_SUFFIX: "" | ||
| HOSTTYPE: "x86_64" | ||
| MANYLINUX_VERSION: "260103.868e54c" | ||
| tags: | ||
| - release_wheels | ||
| - oss | ||
|
|
||
| - name: ray-cpp-wheel-build | ||
| label: "wanda: cpp wheel py{{matrix}} (x86_64)" | ||
| wanda: ci/docker/ray-cpp-wheel.wanda.yaml | ||
| matrix: | ||
| - "3.10" | ||
| - "3.11" | ||
| - "3.12" | ||
| env: | ||
| PYTHON_VERSION: "{{matrix}}" | ||
| ARCH_SUFFIX: "" | ||
| HOSTTYPE: "x86_64" | ||
| MANYLINUX_VERSION: "260103.868e54c" | ||
| tags: | ||
| - release_wheels | ||
| - linux_wheels | ||
| - oss | ||
| depends_on: | ||
| - ray-core-build | ||
| - ray-cpp-core-build | ||
| - ray-java-build | ||
| - ray-dashboard-build | ||
|
|
||
| # Upload cpp wheels to S3 | ||
| # Gating: only on releases/* OR (master AND nightly) | ||
| - label: ":s3: upload: cpp wheel py{{matrix}} (x86_64)" | ||
| key: linux_cpp_wheels_upload | ||
| instance_type: small | ||
| commands: | ||
| - ./ci/build/extract_wanda_wheels.sh ray-cpp-wheel-py{{matrix}} --upload | ||
| matrix: | ||
| - "3.10" | ||
| - "3.11" | ||
| - "3.12" | ||
| depends_on: | ||
| - ray-cpp-wheel-build | ||
| tags: | ||
| - release_wheels | ||
| - skip-on-premerge | ||
| - oss |
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.
There's significant duplication in the newly added build steps. The matrix for Python versions and the env configuration are repeated. To improve maintainability and reduce redundancy, consider using YAML anchors and aliases.
For example, you could define:
x-python-versions: &python-versions
- "3.10"
- "3.11"
- "3.12"
x-cpp-build-env: &cpp-build-env
PYTHON_VERSION: "{{matrix}}"
ARCH_SUFFIX: ""
HOSTTYPE: "x86_64"
MANYLINUX_VERSION: "260103.868e54c"And then reference them in your build steps:
- name: ray-cpp-core-build
...
matrix: *python-versions
env: *cpp-build-env
- name: ray-cpp-wheel-build
...
matrix: *python-versions
env: *cpp-build-env
- label: ":s3: upload: cpp wheel py{{matrix}} (x86_64)"
...
matrix: *python-versionsThis would make the configuration cleaner and easier to update in the future.
bf5b228 to
ed903b7
Compare
8a4bdf5 to
a0b03e9
Compare
21500cc to
cf7cab6
Compare
a0b03e9 to
32347fa
Compare
8d1057a to
d8901f7
Compare
32347fa to
9074421
Compare
|
Verifying cpp wheel found no diffs between the old and this method: Remote URL : f6c2b5f Local wheel: 029272617770.dkr.ecr.us-west-2.amazonaws.com/rayproject/citemp@sha256:922a6d572dc2508d00cf247487facb2cbe918d3c924bc00ff53fd99726af5eb3 |
d8901f7 to
9e43825
Compare
9074421 to
1b2c341
Compare
9e43825 to
6801f72
Compare
1b2c341 to
05785cb
Compare
6801f72 to
9834ea3
Compare
05785cb to
37410f9
Compare
9834ea3 to
25e0d03
Compare
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.
could you resolve merge conflict? probably due to the oss tag thing
|
well.. the oss tag thing has not merged yet.. |
7c1782d to
09f88fb
Compare
I’ve learned this is an expected artifact of stacked PRs with the tooling I use once ancestor PRs merge. Ray (correctly) squash-merges PRs, which rewrites the commit history. These changes land on master as the new squashed commit, but the original commits my downstream PRs were based on no longer exist. After the base PR merges, this creates the merge conflict. The rebase is usually straightforward because the content is the same, but it does lead to this behavior Hopefully GitHub can support stacked PRs more natively in the future 🙏 |
09f88fb to
c7f89bf
Compare
Currently seeing issues of crane not available in the uploading environment. Installing crane on forge image. https://buildkite.com/ray-project/postmerge/builds/15375/steps/canvas?jid=019bb99d-6f9e-45fa-92e3-a5a1d9373e8d#019bb99d-6f9e-45fa-92e3-a5a1d9373e8d/L198 Topic: crane-fix Signed-off-by: andrew <andrew@anyscale.com>
The ray-cpp wheel contains only C++ headers, libraries, and executables with no Python-specific code. Previously we built 4 identical wheels (one per Python version: cp310, cp311, cp312, cp313), wasting CI time and storage. This change produces a single wheel tagged py3-none-manylinux2014_* that works with any Python 3.x version. Changes: - Add ray-cpp-core.wanda.yaml and Dockerfile for cpp core - Add ray-cpp-wheel.wanda.yaml for cpp wheel builds - Add ci/build/build-ray-cpp-wheel.sh for Python-agnostic wheel builds - Add RayCppBdistWheel class to setup.py that forces py3-none tags (necessary because BinaryDistribution.has_ext_modules() causes bdist_wheel to use interpreter-specific ABI tags by default) - Update ray-cpp-wheel.wanda.yaml to build single wheel per architecture - Update .buildkite/build.rayci.yml to remove Python version matrix for cpp wheel build/upload steps Topic: ray-cpp-wheel Relative: crane-fix Signed-off-by: andrew <andrew@anyscale.com>
be2c5c4 to
9cb14c9
Compare
e2ebabb to
7b8a44a
Compare
c9d30e1 to
daea687
Compare
1410def to
64ac2fd
Compare
daea687 to
1950595
Compare
64ac2fd to
d5b11ff
Compare
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.
looks good in general. just some small things on styling.
| """Custom bdist_wheel that produces Python-agnostic wheels for ray-cpp. | ||
| PROBLEM | ||
| ------- | ||
| The ray-cpp wheel contains only C++ files (headers, libraries, executables) | ||
| with NO Python-specific code. It should work with any Python 3.x version. | ||
| However, by default, setuptools/bdist_wheel generates wheels tagged with | ||
| the specific Python interpreter used to build, e.g.: | ||
| ray_cpp-3.0.0-cp310-cp310-manylinux2014_x86_64.whl | ||
| ^^^^^-^^^^^ | ||
| Python 3.10 specific! | ||
| This class forces the wheel tag to be: | ||
| ray_cpp-3.0.0-py3-none-manylinux2014_x86_64.whl | ||
| ^^^-^^^^ | ||
| Works with ANY Python 3! | ||
| Tag meanings: | ||
| - "py3" = compatible with any Python 3.x | ||
| - "none" = no Python ABI dependency (no compiled .so using Python.h) | ||
| - "manylinux2014_x86_64" = platform tag (still needed for C++ binaries) |
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.
while this analysis is great, can we remove it from the source code? or make it more succinct? this should just briefly explain that ray-cpp wheel is python version agnostic, so based on python wheel standard, we return these value, etc, etc..
this paragraph feels a bit too AI-ish.. it is okay for leaving it in the comments, but a bit too much for having it in the docstring..
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.
I am merging this in as things are passing. could you follow up with a PR that cleans up this docstring a bit?
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.
I'll create a followup on this
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.
…ct#59969) The ray-cpp wheel contains only C++ headers, libraries, and executables with no Python-specific code. Previously we built 4 identical wheels (one per Python version: cp310, cp311, cp312, cp313), wasting CI time and storage. This change produces a single wheel tagged py3-none-manylinux2014_* that works with any Python 3.x version. Changes: - Add ray-cpp-core.wanda.yaml and Dockerfile for cpp core - Add ray-cpp-wheel.wanda.yaml for cpp wheel builds - Add ci/build/build-ray-cpp-wheel.sh for Python-agnostic wheel builds - Add RayCppBdistWheel class to setup.py that forces py3-none tags (necessary because BinaryDistribution.has_ext_modules() causes bdist_wheel to use interpreter-specific ABI tags by default) - Update ray-cpp-wheel.wanda.yaml to build single wheel per architecture - Update .buildkite/build.rayci.yml to remove Python version matrix for cpp wheel build/upload steps Topic: ray-cpp-wheel Relative: ray-wheel Signed-off-by: andrew <andrew@anyscale.com> --------- Signed-off-by: andrew <andrew@anyscale.com>
The ray-cpp wheel contains only C++ headers, libraries, and executables
with no Python-specific code. Previously we built 4 identical wheels
(one per Python version: cp310, cp311, cp312, cp313), wasting CI time
and storage.
This change produces a single wheel tagged py3-none-manylinux2014_*
that works with any Python 3.x version.
Changes:
(necessary because BinaryDistribution.has_ext_modules() causes
bdist_wheel to use interpreter-specific ABI tags by default)
for cpp wheel build/upload steps
Topic: ray-cpp-wheel
Relative: ray-wheel
Signed-off-by: andrew andrew@anyscale.com