Skip to content
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 option to use "pip download" instead of "pip wheel" to download wheels for other platforms #773

Merged
merged 7 commits into from
Aug 27, 2022

Conversation

jesseschalken
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Running pip_parse or whl_library on Linux with a requirements.txt for Windows downloads the Linux instead of the Windows version of wheels.

Adding extra_pip_args = ["--platform", "win_amd64"] doesn't work because --platform isn't a valid flag for pip wheel.

In the case that all pip dependencies are available as precompiled wheels, pip download can be used instead, which accepts the --platform flag.

Issue Number: N/A

What is the new behavior?

Add a new option download_only to pip_parse and whl_library to allow using pip download instead of pip wheel so that wheels for platforms other than the host platform can be downloaded by adding --platform ... to extra_pip_args.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@jesseschalken jesseschalken changed the title Add option to use "pip download" instead of "pip wheel" to download dependencies for other platforms Add option to use "pip download" instead of "pip wheel" to download wheels for other platforms Jul 29, 2022
@UebelAndre
Copy link
Contributor

@groodt would you be able to take a look here some time this week? This change would be very helpful!

(also thanks for all the support!)

@jesseschalken
Copy link
Contributor Author

It might be useful to document how this can be used to target multiple platforms. Example:

# WORKSPACE

load("@rules_python//python:pip.bzl", "pip_parse")
load("@python3_10//:defs.bzl", "interpreter")

# Repeat for each supported target platform
pip_parse(
    name = "pip_windows",
    python_interpreter_target = interpreter,
    requirements_lock = "requirements_windows.txt",
    download_only = True,
    extra_pip_args = ["--platform", "win_amd64"],
)
# BUILD

load("@rules_python//python:pip.bzl", "compile_pip_requirements")

# Repeat for each supported target platform
compile_pip_requirements(
    name = "requirements_windows",
    requirements_in = "requirements.in",
    requirements_txt = "requirements_windows.txt",
    target_compatible_with = [
        "@platforms//os:windows",
        "@platforms//cpu:x86_64",
    ],
)

# Repeat for each supported target platform
config_setting(
    name = "windows_x86_64",
    constraint_values = [
        "@platforms//os:windows",
        "@platforms//cpu:x86_64",
    ],
)

# Repeat for each pip dependency
alias(
    name = "numpy",
    actual = select({
        # Repeat for each target platform
        ":windows_x86_64": "@pip_windows_numpy//:pkg",
        # ...
    }),
    visibility = ["//visibility:public"],
)

# ...

Obviously can be simplified with macros and loops when there are multiple target platforms and pip dependencies.

@jesseschalken jesseschalken force-pushed the cross-platform-wheel-download branch from 833da00 to 1d09c29 Compare August 2, 2022 15:26
@schultetwin
Copy link
Contributor

I’m a big fan of this change, or something similar! Out of curiosity, why this PR vs. #510?

This is definitely simpler! But #510 comes with the aliasing for platforms already?

@groodt groodt requested review from groodt and removed request for brandjon, lberki and thundergolfer August 6, 2022 23:12
@groodt groodt self-assigned this Aug 6, 2022
@jesseschalken
Copy link
Contributor Author

@schultetwin I didn't know about #510 and I don't have a strong preference between either PR.

It would be nice if there was a properly supported cross-build workflow like #510 is aiming for but it has been open for a year and there appears to be some open design questions.

This PR is a simpler change to at least unblock the ability to do cross builds as detailed above in #773 (comment).

@schultetwin
Copy link
Contributor

This PR is a simpler change to at least unblock the ability to do cross builds as detailed above in #773 (comment).

Understood, thank you!

Copy link
Collaborator

@groodt groodt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

This PR is simple and minimally invasive compared to PR #510. It unblocks users who require cross-platform builds and who are able to stand-up their own pypi mirrors / devpi pre-populated with wheels for all platforms. This is something that we probably should add documentation around and recommend.

Just for the record, both this PR and #510 still have short-comings and the maintainers of these rules are thinking hard about moving away from so much reliance on repository rules.

@groodt groodt merged commit 6a43ebd into bazelbuild:main Aug 27, 2022
groodt added a commit that referenced this pull request Aug 27, 2022
…wnload wheels for other platforms (#773)"

This reverts commit 6a43ebd.
groodt added a commit that referenced this pull request Aug 27, 2022
#808)

Revert "Add option to use "pip download" instead of "pip wheel" to download wheels for other platforms (#773)"

This reverts commit 6a43ebd.
@jesseschalken jesseschalken deleted the cross-platform-wheel-download branch September 3, 2022 18:43
@sasha-wing
Copy link

Thank you for this change, it saved me

@kpark-hrp
Copy link

kpark-hrp commented May 20, 2023

@jesseschalken @groodt

Is there a way to make this work with Gazelle? It would be awesome if gazelle can understand the select({...}) to work with multiple repos.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants