refactor(whl_library): split wheel downloading and extraction into separate executions#1487
Merged
aignas merged 1 commit intobazel-contrib:mainfrom Oct 17, 2023
Merged
Conversation
…parate executions Before the PR the downloading/building of the wheel and the extraction would be done as a single step, which meant that for patching of the wheel to happen, we would need to do it within the python script. In order to have more flexibility in the approach, this PR splits the process to two separate invocations of the wheel_installer, which incidentally also helps in a case where the downloading of the wheel file can happen separately via http_file.
Collaborator
Author
|
Note, that with this we could in theory set up a cyclic dependency test with:
|
groodt
reviewed
Oct 16, 2023
| """ | ||
|
|
||
| def generate_whl_library_build_bazel( | ||
| *, |
Collaborator
There was a problem hiding this comment.
nit: can we call it *args?
I'm not entirely following why we need this? Can you elaborate?
Collaborator
Author
There was a problem hiding this comment.
This is to force all of the arguments to be specified as keyword arguments. this function has lots of arguments and supporting positional arguments may be a little bit error prone.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Oct 20, 2023
Before that the users had to rely on patching the actual wheel files and uploading them as different versions to internal artifact stores if they needed to modify the wheel dependencies. This is very common when breaking dependency cycles in `pytorch` or `apache-airflow` packages. With this feature we can support patching external PyPI dependencies via pip.override tag class to fix package dependencies and/or a broken `RECORD` metadata file. Overall design: * Split the `whl_installer` CLI into two parts - downloading and extracting. Merged in #1487. * Add a starlark function which extracts the downloaded wheel applies patches and repackages a wheel (so that the extraction part works as before). * Add a `override` tag_class to the `pip` extension and allow users to pass patches to be applied to specific wheel files. * Only the root module is allowed to apply patches. This is to avoid far away modules modifying the code of other modules and conflicts between modules and their patches. Patches have to be in `unified-diff` format. Related #1076, #1166, #1120
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Before the PR the downloading/building of the wheel and the extraction
would be done as a single step, which meant that for patching of the
wheel to happen, we would need to do it within the python script. In
order to have more flexibility in the approach, this PR splits the
process to two separate invocations of the wheel_installer, which
incidentally also helps in a case where the downloading of the wheel
file can happen separately via http_file.
Related issues #1076, #1357