refactor!(bzlmod): introduce pypi.install extension #2278
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
With this PR we are introducing a new extension for managing PyPI dependencies and it seems that we would fix bazelbuild#2268 in doing so. The code has been sitting around for long enough so that we have ironed out most of the bigger bugs already and the maintainers have agreed for long enough that pip.parse is not the best named, so I have chosen pypi.install. This should not actively break the code and will make some of the experimental parameters noop. This could yield to weird failures in cases where people are building docker containers, so I am thinking that we should potentially add a helpful error message prompting the users to migrate. However I am not sure how that works out when multiple module versions are at play in the same module graph.
|
I don't know if I am super happy about the breaking change here, but I do think fixing the bug here takes the priority. Let me know what you think. I think we have discussed enough times and agree on the new better name here. |
rickeylev
reviewed
Oct 9, 2024
There was a problem hiding this comment.
re: new extension and backwards compatibility:
The main thing I have in mind is how we transition from pip.parse to pypi.install. Some specific questions:
- If pypi.install is a drop-in replacement, why does it need to be a separate extension?
- Why can't we transition pip.parse to the new semantics under-the-hood? I know the
pip.parsename isn't as nice aspypi.install, but that's just aesthetics. - Is there something pip.parse can do that pypi.install can't do, or vice versa?
| @@ -27,6 +27,10 @@ A brief description of the categories of changes: | |||
| ### Changed | |||
| * (toolchains) `py_runtime.implementation_name` now defaults to `cpython` | |||
| (previously it defaulted to None). | |||
| * (bzlmod) **BREAKING** The `experimental_index_url` feature has been removed | |||
There was a problem hiding this comment.
nit: move the breaking entries to the top of the changed section
| incompatible ways if major design flaws are found. | ||
| ::: | ||
|
|
||
| `pypi.install` provides almost a drop-in replacement for the `pip.parse` extension: |
There was a problem hiding this comment.
Have this link to the api doc
| to set an environment variable `RULES_PYTHON_OS_ARCH_LOCK_FILE=0` which will | ||
| become default in the next release. | ||
|
|
||
| Fetching the distribution information from the PyPI allows `rules_python` to |
There was a problem hiding this comment.
Suggested change
| Fetching the distribution information from the PyPI allows `rules_python` to | |
| Fetching the distribution information from PyPI allows `rules_python` to |
|
|
||
| Fetching the distribution information from the PyPI allows `rules_python` to | ||
| know which `whl` should be used on which target platform and it will determine | ||
| that by parsing the `whl` filename based on [PEP600], [PEP656] standards. This |
There was a problem hiding this comment.
Suggested change
| that by parsing the `whl` filename based on [PEP600], [PEP656] standards. This | |
| that by parsing the `whl` filename based on [PEP600] and [PEP656] standards. This |
| @@ -461,7 +464,7 @@ def _pip_impl(module_ctx): | |||
| is_extension_reproducible = True | |||
|
|
|||
| for mod in module_ctx.modules: | |||
| for pip_attr in mod.tags.parse: | |||
| for pip_attr in (getattr(mod.tags, "parse", None) or getattr(mod.tags, "install")): | |||
There was a problem hiding this comment.
Suggested change
| for pip_attr in (getattr(mod.tags, "parse", None) or getattr(mod.tags, "install")): | |
| for pip_attr in (getattr(mod.tags, "parse", None) or mod.tags.install): |
|
Taking a step back a little as I am not sure that this should necessarily be merged like this.
|
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.
With this PR we are introducing a new extension for managing PyPI dependencies
and it seems that we would fix #2268 in doing so. The code has been sitting
around for long enough so that we have ironed out most of the bigger bugs
already and the maintainers have agreed for long enough that pip.parse is
not the best named, so I have chosen pypi.install.
This should not actively break the code and will make some of the experimental
parameters noop. This could yield to weird failures in cases where people are
building docker containers, so I am thinking that we should potentially add
a helpful error message prompting the users to migrate. However I am not
sure how that works out when multiple module versions are at play in the same
module graph.
Work towards #260