feat(pypi): generate filegroup with all extracted wheel files #3011
Conversation
| @@ -14,6 +14,7 @@ | |||
|
|
|||
| """Constants used by parts of pip_repository for naming libraries and wheels.""" | |||
|
|
|||
| EXTRACTED_WHEEL_FILES = "extracted_whl_files" | |||
There was a problem hiding this comment.
You still need to modify the pkg_aliases macro to include the extra alias.
| DIST_INFO_LABEL: ["site-packages/*.dist-info/**"], | ||
| DATA_LABEL: ["data/**"], | ||
| EXTRACTED_WHEEL_FILES: dict( | ||
| include = ["**"], |
There was a problem hiding this comment.
nit, in the repo phase we could parse the RECORD file and include those files instead? In the case where the whl is built from an sdist, this may include the extra sdist file under some configurations. Maybe we should exclude these things? The wheel patching thing has some logic to exclude it correctly.
There was a problem hiding this comment.
Ah, I forgot about sdists. And didn't realize they get extracted out into here (and then built into a wheel?).
Rather than parse RECORD, how about making the glob conditional? e.g. if name.endswith(".whl"): glob-include **
The RECORD is just a simple manifest of everything in the wheel file, right? i.e. glob(**) and RECORD should expand to the same set of files?
There was a problem hiding this comment.
name will always end with .whl because the sdist gets built into a wheel. Maybe a better way would be to remove the sdist or just add it to the excludes list.
| DATA_LABEL: ["data/**"], | ||
| EXTRACTED_WHEEL_FILES: dict( | ||
| include = ["**"], | ||
| exclude = ["*.whl"], |
There was a problem hiding this comment.
I think the name attribute is the wheel filename, so we can be precise with the exclude. If we can pass the sdist filename to this macro, then it would be good as well.
| exclude = ["*.whl"], | |
| exclude = [name], |
There was a problem hiding this comment.
Is this true for sdists? I guess it doesn't matter -- we'd want to exclude the sdist anyways.
Addresses review comments from PR bazel-contrib#3011: - Adds `EXTRACTED_WHEEL_FILES` to `pkg_aliases` in `python/private/pypi/pkg_aliases.bzl`. - Changes the `exclude` attribute for `EXTRACTED_WHEEL_FILES` in `python/private/pypi/whl_library_targets.bzl` to use the specific wheel filename. - Adds a TODO comment in `python/private/pypi/whl_library_targets.bzl` to consider parsing the `RECORD` file in the future. - Updates `CHANGELOG.md` to include the new feature. Note: Encountered a persistent internal Bazel error while attempting to run tests. The changes could not be verified with automated tests.
…into feat.pypi.all.files
Of branch 'feat-pypi-all-files-extracted' into feat.pypi.all.files
| DIST_INFO_LABEL: ["site-packages/*.dist-info/**"], | ||
| DATA_LABEL: ["data/**"], | ||
| EXTRACTED_WHEEL_FILES: dict( | ||
| include = ["**"], |
There was a problem hiding this comment.
Ah, I forgot about sdists. And didn't realize they get extracted out into here (and then built into a wheel?).
Rather than parse RECORD, how about making the glob conditional? e.g. if name.endswith(".whl"): glob-include **
The RECORD is just a simple manifest of everything in the wheel file, right? i.e. glob(**) and RECORD should expand to the same set of files?
| DATA_LABEL: ["data/**"], | ||
| EXTRACTED_WHEEL_FILES: dict( | ||
| include = ["**"], | ||
| exclude = ["*.whl"], |
There was a problem hiding this comment.
Is this true for sdists? I guess it doesn't matter -- we'd want to exclude the sdist anyways.
| @@ -14,6 +14,7 @@ | |||
|
|
|||
| """Constants used by parts of pip_repository for naming libraries and wheels.""" | |||
|
|
|||
| EXTRACTED_WHEEL_FILES = "extracted_whl_files" | |||
| @@ -151,6 +152,7 @@ def pkg_aliases( | |||
| WHEEL_FILE_PUBLIC_LABEL: WHEEL_FILE_IMPL_LABEL if group_name else WHEEL_FILE_PUBLIC_LABEL, | |||
| DATA_LABEL: DATA_LABEL, | |||
| DIST_INFO_LABEL: DIST_INFO_LABEL, | |||
| EXTRACTED_WHEEL_FILES: EXTRACTED_WHEEL_FILES, | |||
There was a problem hiding this comment.
todo: add a test for this. Just need to have some that references it.
Adds a filegroup with all the files that came from the extracted wheel.
This has two benefits over using
whl_filegroup: it avoids copying the wheeland makes the set of files directly visible to the analysis phase.
Some wheels are multiple gigabytes in size (e.g. torch, cuda, tensorflow), so
avoiding the copy and archive processing saves a decent amount of time.
Knowing the specific files at analysis time is generally beneficial. The
particular case I ran into was the CC rules were unhappy with a TreeArtifact
of header files because they couldn't enforce some check about who was
properly providing headers that were included (layering check?).
Another example is using the unused_inputs_list optimization, which allows
an action to ignore inputs that aren't actually used. e.g. an action could
take all the wheel's files as inputs, only care about the headers, and then
tell bazel all the non-header files aren't relevant, and thus changes to
other files don't re-run the thing that only cares about headers.