-
Notifications
You must be signed in to change notification settings - Fork 556
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for multi-platform pip download
to pip_parse
#510
Conversation
Please enter the commit message for your changes. Lines starting
pip download
to pip_parsepip download
to pip_parse
pip download
to pip_parse
pip download
to pip_parse
As an overall feature I think this is okay, but I'd be careful in the cross / multi platform wording. The general issue is that pip tools (the rule that powers I guess what I'm trying to say is, this is constrained by the locked requirements file as the input. |
Yes, you're 100% correct that that is a gotcha that you have to be on the lookout for. I think this can already affect users of Also I see that you're a collaborator, do you know how I can find out if I would be able to get this merged if I fix up the list of issues in the PR description? |
@@ -178,6 +183,11 @@ pip_repository_attrs = { | |||
default = False, | |||
doc = "Create the repository in incremental mode.", | |||
), | |||
"pip_platform_definitions": attr.label_keyed_string_dict( | |||
doc = """ | |||
A map of select keys to platform definitions in the form <platform>-<python_version>-<implementation>-<abi>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A map of select keys to platform definitions in the form <platform>-<python_version>-<implementation>-<abi>" | |
A map of select keys to Python platform definitions in the form <platform>-<python_version>-<implementation>-<abi>" |
Just a nit, but helpful to distinguish from a Bazel platform
.
) | ||
pip_args = [sys.executable, "-m", "pip", "--isolated"] | ||
if args.pip_platform_definition: | ||
platform, python_version, implementation, abi = args.pip_platform_definition.split("-") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: why this format instead of the format listed here in PEP 425?
{python tag}-{abi tag}-{platform tag}
The extra bit in this PR's string is {implementation}
, cp = CPython
, but this is covered by the {python tag}
from PEP 425.
The Python tag indicates the implementation and version required by a distribution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because pip download
takes four flags: --platform
, --python-version
, --implementation
, --abi
. I'm not sure why pip doesn't line up with the PEP 425 format.
Python packaging is hell, isn't it? Appreciate the PR and the context you've provided in "other information". I think the biggest roadblocks to supporting cross-platform this way are the sharp-edges/pitfalls users have to navigate:
But generally I think moving to a 'download only' install phase is solid. |
It is interesting that I've found this as this would be useful for me. Currently I am setting up a bazel repository and the setup is that most of the developers are using Macs where as our CI is running on Ubuntu20.04, so wheel only installation would be something that would be really good to be able to use. Otherwise, the non-hermetic My further comments:
|
poetry and pipenv-resolve, if not others, support a single lock file across platform for (most? all?) wheels... worth considering if you have wheel only dependencies. it'd be nice if pip-compile added an option for the same. https://github.com/soniaai/rules_poetry has Poetry rules for Bazel (note: I'm the original author). |
Thanks for the replies and sorry for the long delay on this!
This is a good point, but in practice I've found it not cause very many problems in practice with our repo. For the few cases where we have platform-specific transitive dependencies I've just added them as unconditional top level dependencies in our Do you think coming up with a better solution to this problem would block landing this or could we just note it as a caveat when documenting the feature?
I dug into how this would look, and I'm not really sure how much of a practical benefit there would be. Does that analysis seem correct? Is there a better way I'm missing? |
Decided to go ahead with this simpler approach, that has a similar outcome. #773 |
PR Checklist
Please check if your PR fulfills the following requirements:
.par
files. See CONTRIBUTING.md for infoPR Type
What kind of change does this PR introduce?
What is the current behavior?
There is no multi-platform support: #260
What is the new behavior?
Adds a new argument,
pip_platform_definitions
, topip_parse
. The value of the argument looks like this:Each key is a
config_setting
, and each value is a string of the fom<platform>-<python_version>-<implementation>-<abi>
.When
pip_platform_definitions
is specified, instead of runningpip wheel
, the rules runpip download
with constraints extracted from the values in the map values. This makes it possible to do cross-platform Python builds, e.g. on MacOS run bazel with--platforms=//platforms:macos_x86_64
and have linux dependencies fetched properly. In the above example, we would end with 3 targets for a pip package calledfoo
:@pip_pypi__macosx_10_14_x86_64_37_cp_cp37m__foo//:pkg
is thepy_library
for the MacOS version of the package.@pip_pypi__manylinux2014_x86_64_37_cp_cp37m__foo//:pkg
is thepy_library
for the Linux version of the package.@pip_pypi__foo
is analias
with itsactual
determined by aselect
over the providedconfig_settings
pointing at the above two targets. This is the target that therequirement
macro points at.Of course this only works if all of the required packages have wheels available which match the configured platform constraints, either in PyPI or another configured repository.
Does this PR introduce a breaking change?
Other information
In its current state, this PR is more of a proof of concept that is intended to start a conversation rather than something that is ready to merge. If I get feedback from the maintainers that this general approach is something that could be merged, then I would be happy to do the work to get it into a better state.
We've been successfully been using this at my company to build Docker images with native Python dependencies (e.g. pandas, gRPC) on MacOS. We have a reasonably large collection of Python deps (124), and in practice it has not been a large problem to obtain wheels for all of them. All except 3 are already in PyPI, and for the 3 that are missing it's possible to build universal wheels, which we just do manually outside of Bazel.
Things that I think likely need improvement before this is ready to merge are:
toolchain_type
for this purpose, and put the pip download constraints in a toolchain provider, but have not investigated this thoroughly.repo_names_and_requirements
is pretty confusing.Feedback on any of these items would be great!