[DO NOT MERGE] document why and when --no-build-isolation is used in wheel-building CI #112
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jameslamb
added
DO NOT MERGE
jameslamb
requested review from
msarahan,
trxcllnt,
vyasr,
bdice,
KyleFromNVIDIA and
robertmaynard
vyasr
requested changes
Oct 24, 2024
Comment on lines
+53
to
+55
| By default, this will create an isolated build environment... a temporary directory containing all of the build-time dependencies of the package. | ||
| That's convenient because it means that you don't have to manually install the build dependencies ahead of time or deal with conflicts | ||
| between those dependencies and other things in your development environment. |
There was a problem hiding this comment.
In keeping with the detail provided elsewhere in this section, we should mention rapids-build-backend here. Conversely, I don't think that we need to explain build isolation since it's a Python default. The old version of this file wasn't explaining what build isolation was, but rather why we couldn't use it. I think it's fair to expect that knowledge, or if not, to link out to pip/build docs rather than explaining it here ourselves.
Comment on lines
+28
to
+29
| By default, `pip wheel` or `python -m build` will install such build-time dependencies into a temporary directory (see ["Python Packages"](build.md#python-packages) for details). | ||
| The path to that directory is randomly chosen, and so the path that those build dependencies are installed to changes on every build. |
There was a problem hiding this comment.
As above, I would be comfortable rephrasing this assuming the reader knows what build isolation is and simply providing the informational link for them if they need it.
| This pattern is followed to balance these two competing concerns: | ||
|
|
||
| * improved cache hit rates for `sccache`, and therefore faster and less resource-intensive builds | ||
| * testing that `rapids-build-backend` correctly generates package metadata |
There was a problem hiding this comment.
Suggested change
| * testing that `rapids-build-backend` correctly generates package metadata | |
| * testing that `rapids-build-backend` correctly generates package metadata | |
| * ensuring that the default approach for local builds from source (with isolation) will work out of the box for the non-C++ packages |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contributes to #108
--no-build-isolationis used in wheel-building CIrapids-build-backend-now😀 )pre-commitconfig, to fix file endings and trailing whitespaceNotes for Reviewers
I've marked this
DO NOT MERGEbecause it's describing as current state changes that haven't actually been made yet... it shouldn't be merged until most of #108 is done.But it's ready for review.