-
Notifications
You must be signed in to change notification settings - Fork 4
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
Remove build isolation in wheel builds #108
Comments
Contributes to rapidsai/build-planning#108 Contributes to rapidsai/build-planning#111 Proposes building `libcuspatial` wheels with `--no-build-isolation`, to improve the rate of `sccache` cache hits and therefore reduce CI times. Also proposes printing `sccache` stats to CI logs after wheel and conda builds. ## Notes for Reviewers # Authors: - James Lamb (https://github.com/jameslamb) - Bradley Dice (https://github.com/bdice) Approvers: - Mike Sarahan (https://github.com/msarahan) - Kyle Edwards (https://github.com/KyleFromNVIDIA) - Vyas Ramasubramani (https://github.com/vyasr) - Bradley Dice (https://github.com/bdice) URL: #1473
I've updated the description to reflect our decision from an offline conversation:
And proposed documenting that in the |
Contributes to rapidsai/build-planning#108 This is a pure Python project, so it doesn't need configuration about CMake or `sccache`. This proposes removing them to simplify build scripts a bit. It also proposes updating the `rapids-dependency-file-generator` pre-commit hook to it's latest version, something I'm trying to roll out across RAPIDS as part of rapidsai/build-planning#108. Authors: - James Lamb (https://github.com/jameslamb) Approvers: - Jake Awe (https://github.com/AyodeAwe) URL: #1400
We shouldn't need to do this for any of the packages that haven't switched to using C++ wheels from other libraries yet, i.e. anything that is still statically linking everything. cugraph, cuml, cuvs, and raft (that may be exhaustive, or I may have missed some) don't have relevant changes yet. |
…792) Contributes to rapidsai/build-planning#108 Contributes to rapidsai/build-planning#111 Proposes some small packaging/CI changes, matching similar changes being made across RAPIDS. * removes the use of build isolation in wheel builds * printing `sccache` stats to CI logs * reducing `pip`'s verbosity in wheel building scripts * using the RAPIDS conventions `py_build_{project}` and `py_rapids_build_{project}` in `dependencies.yaml` * updating to the latest `rapids-dependency-file-generator` (v1.16.0) Authors: - James Lamb (https://github.com/jameslamb) Approvers: - Bradley Dice (https://github.com/bdice) URL: #792
…uilding scripts (#641) Contributes to rapidsai/build-planning#108 Proposes removing `sccache` configuraiton in CI scripts ... this is a pure Python project, so it doesn't need that. It also proposes updating the `rapids-dependency-file-generator` pre-commit hook to it's latest version (something I'm trying to roll out across RAPIDS as part of rapidsai/build-planning#108), and reducing the verbosity of wheel-building scripts. Authors: - James Lamb (https://github.com/jameslamb) Approvers: - Bradley Dice (https://github.com/bdice) URL: #641
Oh interesting. I'd expected that all of them were at least getting RMM headers from the evidence of that (click me)Looking at recent nightly wheel builds, it seems like all those projects you mentioned are getting rmm via CPM.
I'm comparing that to other projects like
And any other build-time dependencies those projects are getting via wheels appear to only be reference in Cython code, which we've already said we don't want to use build isolation for (#108 (comment)). I'll update the issue description here and add notes about using build isolation to the corresponding issues that track the work for delivering more C++ dependencies in those projects via wheels. |
Yeah we've been following a depth-first rather than breadth-first approach to migrating packages over to use C++ wheels i.e. instead of updating everything to use librmm wheels once they existed we switched kvikio to C++ wheels (using rmm), then cudf (using kvikio and rmm), etc. It makes more sense that way since there's very little benefit to doing this partially until you get to the leaf nodes in our tree anyway (cugraph/cuml using raft). |
By this additional standard of "only use
Some of those have e.g. Have I understood this correctly @vyasr ? |
Contributes to rapidsai/build-planning#108 Contributes to rapidsai/build-planning#111 Proposes some small packaging/CI changes, matching similar changes being made across RAPIDS. * building `libcudf` wheels with `--no-build-isolation` (for better `sccache` hit rate) * printing `sccache` stats to CI logs * updating to the latest `rapids-dependency-file-generator` (v1.16.0) * always explicitly specifying `cpp` / `python` in calls to `rapids-upload-wheels-to-s3` # Authors: - James Lamb (https://github.com/jameslamb) Approvers: - Bradley Dice (https://github.com/bdice) URL: #17088
Yup, that is correct. AFAIK right now we should really only need to make changes to cudf, cuspatial, and kvikio (rmm would need this if it had any dependencies, but since it's our root node it doesn't). For the other libraries we'll have to remember to set this up correctly when we get around to making the C++ wheels. |
Great, thank you.
Agreed. I'm going to fill out the remaining issues tracking each individual dynamic-wheels thing (e.g. https://github.com/rapidsai/rapids-wheels-planning/issues/66 for
I think we want ucxx as well... And I think we don't want |
You're right about both ucxx and kvikio, thanks. |
Contributes to rapidsai/build-planning#111 Proposes some small packaging/CI changes, matching similar changes being made across RAPIDS. * printing `sccache` stats to CI logs * reducing `pip`'s verbosity in wheel building scripts * updating to the latest `rapids-dependency-file-generator` (v1.16.0) * always explicitly specifying `cpp` / `python` in calls to `rapids-upload-wheels-to-s3` * modifying `dependencies.yaml` to match RAPIDS-wide naming conventions ## Notes for Reviewers This originally also ran wheel builds with `--no-build-isolation`, but I reverted that based on rapidsai/build-planning#108 (comment). Authors: - James Lamb (https://github.com/jameslamb) Approvers: - Bradley Dice (https://github.com/bdice) URL: #413
Contributes to rapidsai/build-planning#111 Proposes some small packaging/CI changes, matching similar changes being made across RAPIDS. * printing `sccache` stats to CI logs * updating to the latest `rapids-dependency-file-generator` (v1.16.0) * always explicitly specifying `cpp` / `python` in calls to `rapids-upload-wheels-to-s3` ## Notes for Reviewers This originally also ran wheel builds with `--no-build-isolation`, but I reverted that based on rapidsai/build-planning#108 (comment). Authors: - James Lamb (https://github.com/jameslamb) Approvers: - Kyle Edwards (https://github.com/KyleFromNVIDIA) URL: #4719
Contributes to rapidsai/build-planning#108 Contributes to rapidsai/build-planning#111 Proposes some small packaging/CI changes, matching similar changes being made across RAPIDS. * building `libucxx` wheels with `--no-build-isolation` (for better `sccache` hit rate) * printing `sccache` stats to CI logs * updating to the latest `rapids-dependency-file-generator` (v1.16.0) * always explicitly specifying `cpp` / `python` in calls to `rapids-upload-wheels-to-s3` * moving more one-wheel-specific logic into `build_wheel_{project}.sh` scripts, mimicking how `cudf` has structured its scripts Authors: - James Lamb (https://github.com/jameslamb) Approvers: - Bradley Dice (https://github.com/bdice) URL: #301
Contributes to rapidsai/build-planning#111 Proposes some small packaging/CI changes, matching similar changes being made across RAPIDS. * printing `sccache` stats to CI logs * reducing `pip`'s verbosity in wheel building scripts * updating to the latest `rapids-dependency-file-generator` (v1.16.0) * always explicitly specifying `cpp` / `python` in calls to `rapids-upload-wheels-to-s3` ## Notes for Reviewers This originally also ran wheel builds with `--no-build-isolation`, but I reverted that based on rapidsai/build-planning#108 (comment). Authors: - James Lamb (https://github.com/jameslamb) Approvers: - Bradley Dice (https://github.com/bdice) URL: #6111
Contributes to rapidsai/build-planning#111 Proposes some small packaging/CI changes, matching similar changes being made across RAPIDS. * printing `sccache` stats to CI logs * reducing `pip`'s verbosity in wheel building scripts * updating to the latest `rapids-dependency-file-generator` (v1.16.0) * always explicitly specifying `cpp` / `python` in calls to `rapids-upload-wheels-to-s3` ## Notes for Reviewers This originally also ran wheel builds with `--no-build-isolation`, but I reverted that based on rapidsai/build-planning#108 (comment). Authors: - James Lamb (https://github.com/jameslamb) Approvers: - Bradley Dice (https://github.com/bdice) URL: #2470
I've added notes about this to the "Approach" sections in all the rest of the C++ wheels issues, so I think this can now be closed. |
Description
Currently, RAPIDS projects build wheels with build isolation, like
pip wheel .
.With build isolation, the build tool (in our case,
pip wheel
), creates a virtual environment and installs of the package's build-time dependencies into it. That virtual environment is created at a random location on each build, which means source files included from other wheels (like the rmm headers from thelibrmm
wheel) are found at a different path on each build in CI.Path changes lead to cache misses with
sccache
, and therefore longer-running and more memory-intensive builds.As more of RAPIDS build-time dependencies are provided wheels (e.g. #33), this problem is going to get worse (see #33 (comment)).
This issue tracks the work of removing build isolation in RAPIDS wheel-building CI jobs.
Benefits of this work
Better
sccache
hit rate = faster, cheaper CI (contributes to #95).Stronger guarantees that packages built in CI are actually installed at test time (i.e. would close #79).
Acceptance Criteria
--no-build-isolation
Approach
Follow the approach from rapidsai/cuspatial#1473, and documented in #112.
Notes
These can be done in any order.
Updates
Do the GNN repos last, as some of the
cugraph
repos are currently being shuffled around. See e.g. rapidsai/cugraph-gnn#58 (comment).GNN repos
The text was updated successfully, but these errors were encountered: