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

Allow pip_repository (and pip_parse) to "select" requirements based on both OS and architecture #735

Closed
jiawen opened this issue Jun 23, 2022 · 10 comments

Comments

@jiawen
Copy link

jiawen commented Jun 23, 2022

🚀 feature request

Relevant Rules

pip_repository and by extension pip_parse.

Description

Because pip_repository is a repository rule, it somewhat awkwardly uses several attributes to override the default requirements file based exclusively on a limited selection of operating systems (linux, macos, windows).

Now that ARM is a more popular architecture and available on all three operating systems, we need a way to select among that axis as well. Otherwise, some wheels that have native dependencies may not be available on certain architectures. We may want to consider an "os version" as a third axis as well. For example, rawpy is available on MacOS but only on Intel, and possibly only MacOS 10.9 (I don't have a machine to test).

Describe the solution you'd like

Because it's awkward to add all possible combinations as new attributes, perhaps we should consider passing in a dictionary of tuples to match against fields in repository_os. The current implementation uses only .name. It's easy to add .arch to match against arch. It might be a little tricky to add "OS version" as it requires either adding a new field to repository_os (probably possible), or invoking a platform-dependent shell script.

Describe alternatives you've considered

Passing in a struct? It's more or less the same as a dictionary.

@jiawen
Copy link
Author

jiawen commented Jun 23, 2022

@likesum FYI.

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days.
Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_python!

@github-actions github-actions bot added the Can Close? Will close in 30 days if there is no new activity label Dec 20, 2022
@yk
Copy link

yk commented Jan 20, 2023

bump. generated requirements differ between architectures, thus making version control of e.g. lock files very difficult

@github-actions github-actions bot removed the Can Close? Will close in 30 days if there is no new activity label Jan 20, 2023
@github-actions
Copy link

This issue has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days.
Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_python!

@github-actions github-actions bot added the Can Close? Will close in 30 days if there is no new activity label Jul 20, 2023
@arrdem
Copy link
Contributor

arrdem commented Jul 21, 2023

In order to solve this problem we settled on using a workspace rule we call python_config, which is able to use the rctx to inspect the building host and template out a config.bzl file containing values such as a computed path to a requirements_lock.txt chosen based on the building host. This allows us to check in many platform+virtualenv combination lockfiles and pick "the right one" transparently within Bazel without modification to rules_python.

Our WORKSPACE then looks something like this -

python_version = "3.8"

load("//tools/rules:external_python.bzl", "external_python")

external_python(
    name = "local_py",
    version = python_version,
)

register_toolchains("@local_py//:python_toolchain")

# Refer to this workspace rule and associated template for how we're choosing
# the Python configuration.
load("//tools/rules:python_config.bzl", "python_config")

python_config(
    name = "pycfg",
    version = python_version,
)

load("@pycfg//:config.bzl", "environment", "extra_pip_args", "requirement_clusters", "requirements_lock")

pip_parse(
    name = "pypi",
    environment = environment,
    extra_pip_args = extra_pip_args,
    python_interpreter_target = "@local_py//:python3",
    requirement_clusters = requirement_clusters,
    requirements_lock = requirements_lock,
)

load("@pypi//:requirements.bzl", pypi_install_deps = "install_deps")

pypi_install_deps()

It would be nice if there was a better solution "in the box", but per platform+venv lockfiles appears the best way to go and it would be somewhat unreasonable to teach rules_python about all the relevant combinations. Having different requirements arguments for linux/darwin/windows is already IMO awkward.

@github-actions github-actions bot removed the Can Close? Will close in 30 days if there is no new activity label Jul 21, 2023
@aignas aignas self-assigned this Jan 22, 2024
@aignas
Copy link
Collaborator

aignas commented Jan 22, 2024

FYI: #1593 added whl_library ability to point to dependencies that are platform specific, now we need installing of dependencies based on the target platform, that is further away. I had a poc done for it in #1625, but I am not sure if that design is the best way to move forward.

@thomasegriffith
Copy link

thomasegriffith commented Feb 7, 2024

Hi @aignas - thanks for #1593! My understanding of this is that we can point to dependencies that are OS-specific, but not architecture specific. For example, one can do requirements_linux = "//linux:requirements_lock.txt" and requirements_windows = "//windows:requirements_lock.txt".

Is there a way to switch between architectures? If not, should the API expose something akin to requirements_linux_x86_64 and requirements_linux_aarch64? Perhaps that's a little messy, but something that captures the same point would be nice.

(FWIW I see the title of #1625 contains "multi-arch", but I'm not following how to actually switch between different architectures on say linux.)

@aignas
Copy link
Collaborator

aignas commented Feb 8, 2024

@thomasegriffith, thanks for the comment. What is your usecase for having different requirements files? I'd like to know a little bit more about how people use this feature.

I would expect that for a particular python version, one could just have the superset of all requirements files listing all of the dependencies that are needed by all platforms and then bazel will lazily download only the required ones. Or is it that the versions for packages are different for different platforms?

@thomasegriffith
Copy link

Or is it that the versions for packages are different for different platforms?

@aignas, exactly. I've had to use specific versions of packages for certain platforms, because sometimes a version of a particular package is buggy or even nonexistent for one platform but not for other platforms. I've also had to build wheels from source for certain platforms, in which case I end up pointing to a local wheel in my pip requirements, all while using the main repository for other platforms.

Another thing to consider is that sometimes even the OS / architecture combination isn't sufficient. For example, the picamera2 pip package will only work on raspberry pi. Raspberry pi is linux / aarch64, but other linux / aarch64 machines will fail when trying to pip compile with picamera2.

aignas added a commit to aignas/rules_python that referenced this issue Apr 17, 2024
With this change we can in theory have multi-platform libraries in the
dependency cycle and use the pip hub repo for the dependencies. With
this we can also make the contents of `whl_library` not depend on what
platform the actual dependencies are. This allows us to support the
following topologies:

* A platform-specific wheel depends on cross-platform wheel.
* A cross-platform wheel depends on cross-platform wheel.
* A whl_library can have `select` dependencies based on the interpreter
  version, e.g. pull in a `tomli` dependency only when the Python
  interpreter is less than 3.11.

Relates to bazelbuild#1663.
Work towards bazelbuild#735.
github-merge-queue bot pushed a commit that referenced this issue Apr 29, 2024
With this PR the code becomes more maintainable and easier to inspect.
Since bazel-skylib is already a dependency of rules_python, this is
a backwards compatible change.

Skipping the CHANGELOG notes because it should not be an externally
visible change.

Work towards #735.
github-merge-queue bot pushed a commit that referenced this issue Apr 30, 2024
…#1856)

With this change we can in theory have multi-platform libraries in the
dependency cycle and use the pip hub repo for the dependencies. With
this we can also make the contents of `whl_library` not depend on what
platform the actual dependencies are. This allows us to support the
following topologies:

* A platform-specific wheel depends on cross-platform wheel.
* A cross-platform wheel depends on cross-platform wheel.
* A whl_library can have `select` dependencies based on the interpreter
  version, e.g. pull in a `tomli` dependency only when the Python
  interpreter is less than 3.11.

Relates to #1663.
Work towards #735.
aignas added a commit to aignas/rules_python that referenced this issue May 4, 2024
…nency closures

We start using the recently introduced is_python_config_setting to make
it possible to have a working select statement when multiple python
version selection needs to happen in a whl_library

Work towards bazelbuild#735.
aignas added a commit to aignas/rules_python that referenced this issue May 18, 2024
We start using the recently introduced `is_python_config_setting` to make
it possible to have a working select statement when multiple python
version selection needs to happen in a `whl_library`.

This adds further fixes so that the correct dependencies are pulled in when the
`python_version` string flag is unset thus making this implementation suitable
for `bzlmod` use case where we would use a single `whl_library` instance for
multiple python versions within the hub.

Work towards bazelbuild#735.
github-merge-queue bot pushed a commit that referenced this issue May 19, 2024
…nency closures (#1875)

We start using the recently introduced `is_python_config_setting` to
make
it possible to have a working select statement when multiple python
version selection needs to happen in a `whl_library`.

This adds further fixes so that the correct dependencies are pulled in
when the
`python_version` string flag is unset thus making this implementation
suitable
for `bzlmod` use case where we would use a single `whl_library` instance
for
multiple python versions within the hub.

Work towards #735.
github-merge-queue bot pushed a commit that referenced this issue May 19, 2024
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_library` instances we parse all of the provided requirements files
and
merge 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
@aignas
Copy link
Collaborator

aignas commented Jun 3, 2024

This has been done in #1885

@aignas aignas closed this as completed Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants