feat(pypi): add native local wheel override support to bzlmod pip.parse#3768
feat(pypi): add native local wheel override support to bzlmod pip.parse#3768meteorcloudy wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for overriding PyPI dependencies with local wheels located in a specified distribution folder, restricted to the root module. It adds new attributes to the pip extension for specifying the distribution folder and inclusion/exclusion patterns, implements logic to collect and filter local wheels based on ABI markers, and updates the wheel repository generation to use local file URLs. Feedback from the review highlights potential issues with ABI marker generation when patch versions are present and warns against using lexicographical sorting to resolve multiple matching wheels, suggesting instead to fail on ambiguity.
30e1a50 to
7083382
Compare
7083382 to
ace01f7
Compare
d8b2229 to
8310448
Compare
aignas
left a comment
There was a problem hiding this comment.
I have some concerns about this short-circuiting the wheel selection algorithm used in parse_requirements so left a few ideas.
|
|
||
| Overrides apply when bazel downloader is used and only take effect in the root module. | ||
|
|
||
| :::{versionadded} 2.1.0 |
There was a problem hiding this comment.
| :::{versionadded} 2.1.0 | |
| :::{versionadded} VERSION_NEXT_FEATURE |
| dirname = "/".join(path.split("/")[:-1]), | ||
| _path = path, | ||
| ) | ||
| path_str = str(path) |
There was a problem hiding this comment.
This is great to have, but having a few comments would be good.
| self._get_index_urls.get(python_version) != None, | ||
| ) | ||
|
|
||
| def _collect_local_wheels(module_ctx, pip_attr, is_root = False): |
There was a problem hiding this comment.
Please add a docstring with typing information
| if local_wheel: | ||
| repo_name += "_local_override" | ||
| path_str = local_wheel._path if hasattr(local_wheel, "_path") else str(local_wheel) | ||
| args["urls"] = ["file://" + path_str] |
There was a problem hiding this comment.
Is this path_str absolute? We could as well use whl_file attribute and pass a label if we were ever able to construct one.
Note to myself - because we are not getting this from pypi_cache.bzl, this will never go into the lock file, which means that whether it is absolute or not does not matter that much.
| if not candidate.basename.endswith(".whl"): | ||
| continue | ||
| if _wildcard_match(candidate.basename, pattern): | ||
| if not matched_wheel or matched_wheel.basename < candidate.basename: |
There was a problem hiding this comment.
This relies on lexicographic sorting, but it may be better to:
- parse the wheel name (with //python/private:parse_whl_name.bzl or similar)
- get the version component
- parse the version component (with //python/private:version.bzl)
- Do the version comparison.
What do we do if we match multiple abis, platforms? Should we use the select_whl function from parse_requirements.bzl to actually match the right wheel based on target platform?
Maybe the right way would be to actually override the get_index function in case it is a root module to replace the wheels found on the internet with the wheels found locally and then let the wheel selection algorithm do the right thing instead of doing the wild card matching here and this comparison.
This feature allows developers to easily test locally built Python wheels (e.g., wheels built locally before running integration tests) without needing to publish them to an index or manually edit
requirements_lock.txt.This is a proper implementation of the local_wheel_dist_folder feature used in JAX/TF/XLA.
How to Use
In your
MODULE.bazelfile, configurepip.parsewith the newlocal_wheelsdictionary mapping:Details
dist/libtpu-*.whl) and automatically selects the newest version if multiple matching wheels exist. Silently falls back to remote PyPI if the local file is missing.@@//:MODULE.bazelpip.parseis evaluated by the root module.index_urlis used (which is now by default)