feat(pip): support specifying requirements per (os, arch) #1885
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
aignas
force-pushed
the
feat/os-arch-requirement-files
branch
2 times, most recently
from
20d2045
to
a1a90d2
Compare
aignas
changed the title
rickeylev
approved these changes
May 18, 2024
There was a problem hiding this comment.
Ah, ok. I think I see how this is working. Just making sure I got the gist of it:
- Input is basically a list of (filename, csv-string-platforms)
- It expands that into (filename, platform-pattern)
- It reads each file and populates a distribution->list-of-platforms map
- That map eventually gets to whl_library
There's a bunch of surrounding code to handle some edge cases and error checking, but I think that's the gist of it.
Yeah, this makes sense. Looking at the requirements parser, it's pretty simple and doesn't have any support for env markers, which is...yeah, we'd only be able to do limited support given what env markers can do compared to bazel constraints.
docs/sphinx/pip.md
Outdated
| (per-os-arch-requirements)= | ||
| ## Requirements for a specific OS/Architecture | ||
|
|
||
| IN some cases you may need to use different requirements files for different OS, Arch combinations. This is enabled via the `requirements_by_platform` attribute in `pip.parse` extension and the `pip_parse` repository rule. The keys of the dictionary are labels to the file and the values are a list of comma separated target (os, arch) tuples. |
There was a problem hiding this comment.
Suggested change
| IN some cases you may need to use different requirements files for different OS, Arch combinations. This is enabled via the `requirements_by_platform` attribute in `pip.parse` extension and the `pip_parse` repository rule. The keys of the dictionary are labels to the file and the values are a list of comma separated target (os, arch) tuples. | |
| In some cases you may need to use different requirements files for different OS, Arch combinations. This is enabled via the `requirements_by_platform` attribute in `pip.parse` extension and the `pip_parse` repository rule. The keys of the dictionary are labels to the file and the values are a list of comma separated target (os, arch) tuples. |
| doc = """\ | ||
| The requirements files and the comma delimited list of target platforms as values. | ||
|
|
||
| The keys are the requirement files and the values are coma-separated platform |
There was a problem hiding this comment.
Suggested change
| The keys are the requirement files and the values are coma-separated platform | |
| The keys are the requirement files and the values are comma-separated platform |
|
|
||
| """Requirements parsing for whl_library creation. | ||
|
|
||
| Usecases that the code needs to cover: |
There was a problem hiding this comment.
Suggested change
| Usecases that the code needs to cover: | |
| Use cases that the code needs to cover: |
|
Yeah, you got the gist correctly. The map of the platforms gets to
|
This change implements the necessary selection of the requirement files based on the (os, arch) for both - bzlmod and legacy workspace code paths. As part of this addition I have moved some tests from integration tests to unit tests as now all of the logic on selecting the right requirement file, ensuring that there are no duplicate requirement lines and that the user knows what to provide is done in a single place. This should be a non-breaking change for most users unless they have been passing `requirements_linux` together with `extra_pip_args = ["--platform=manylinux_2_4_x86_64"]`, in which case they would have to change their code to use `requirements_lock` attribute which itself is a trivial change.
aignas
force-pushed
the
feat/os-arch-requirement-files
branch
from
dfabf80
to
6e31814
Compare
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.
This PR implements a better way of specifying the requirements files for
different (os, cpu) tuples. It allows for more granular specification than what
is available today and allows for future extension to have all of the sources
in the select statements in the hub repository.
This is replacing the previous selection of the requirements and there are a
few differences in behaviour that should not be visible to the external user.
Instead of selecting the right file which we should then use to create
whl_libraryinstances we parse all of the provided requirements files andmerge them based on the contents. The merging is done based on the blocks
within the requirement file and this allows the starlark code to understand if
we are working with different versions of the same package on different target
platforms.
Fixes #1868
Work towards #1643, #735