pip_parse does not respect requirement markers

[arrdem]

🐞 bug report

Affected Rule

The issue is caused by the rule: pip_parse (all versions)

Is this a regression?

It appears to be. Previously, rules_python did evaluate platform markers see nikhaldi@323e84e

In 0.16, python/pip_install/extract_wheels/parse_requirements_to_bzl.py will gladly parse a requirements file which uses markers, but does nothing with them. Nor does the new Starlark parser introduced in ba69aec.

It appears this functionality was simply never implemented in the new parser, and was deleted in 37e7e68 when the legacy pip_import implementation was removed.

Description

rules_python makes no attempt to evaluate requirement marker expressions, which leads to false build failures due to attempting to install packages marked as not viable because pip wheel succeeds with a no-op and an error message when presented with a requirement which will be ignored due to markers.

🔬 Minimal Reproduction

When handed a requirements.txt file which uses platform markers to select a different version of an artifact depending on markers everything appears okay. Consider

matplotlib==3.4.3 ; sys_platform != "darwin" or platform_machine != "arm64"
matplotlib==3.4.3.post1+abnormal ; sys_platform == "darwin" and platform_machine == "arm64"

The correct behavior for this requirements file should be to choose between these two versions. Note that the boolean selector condition is disjoint. However the actual behavior is to unconditionally attempt to install the first requirement clause listed in the file. This breaks both bazel fetch //... and any rules referencing the requirement("matplotlib").

The behavior should be to evaluate all the marker expressions and select matching versions/args.

It may be valid for no versions of a package to be selected. Consider tensorflow-macos==2.7.0 ; sys_platform == "darwin". Arguably this should be achieved with platform-specific lockfiles and requirements_darwin, but it shouldn't be an error.

If multiple requirements are selected, that should likely be an error.

🔥 Exception or Error

ERROR: $REPO/BUILD:1:11: $TARGET depends on @pypi_py38_darwin_arm64_matplotlib//:pkg in repository @pypi_py38_darwin_arm64_matplotlib which failed to fetch. no such package '@pypi_py38_darwin_arm64_matplotlib//': whl_library pypi_py38_darwin_arm64_matplotlib failed:
Ignoring matplotlib: markers 'sys_platform != "darwin" or platform_machine != "arm64"' don't match your environment
 (Traceback (most recent call last):
  File "/Users/arrdem/.pyenv/versions/3.8.16/lib/python3.8/runpy.py", line 194, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/Users/arrdem/.pyenv/versions/3.8.16/lib/python3.8/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/private/var/tmp/_bazel_arrdem/64fd9f51655edad1a031ed416b537465/external/rules_python/python/pip_install/extract_wheels/extract_single_wheel.py", line 105, in <module>
    main()
  File "/private/var/tmp/_bazel_arrdem/64fd9f51655edad1a031ed416b537465/external/rules_python/python/pip_install/extract_wheels/extract_single_wheel.py", line 92, in main
    whl = next(iter(glob.glob("*.whl")))
StopIteration
)

[arrdem]

While implementing support for marker evaluation back on 0.18.1 or before looks quite doable using the underlying pip machinery as before, I don't see a good way to add support for it into the new starlark parser as doing so would require teaching starlark how to both parse and evaluate the marker grammar or putting a Python interpreter back in the parsing flow which would defeat the apparent purpose of the starlark parser.

[aignas]

@arrdem, does your usecase work better if you have the os specific requirements files as implemented in #1123 and you keep the platform specific includes in the requirements.in as opposed to requirements.txt file?

[mvgijssel]

Just ran into this issue as well trying to install different versions of torch for different architectures (aarch64 vs x86_64)

A requirements lock file that looks like

torch @ https://download.pytorch.org/whl/cpu/torch-1.13.1%2Bcpu-cp39-cp39-linux_x86_64.whl ; platform_machine == "x86_64" and python_full_version == "3.9.10" \
    --hash=sha256:71636a5c21927236f4974d2355fb3f66a0b707c28219b0135ff65ed0f0e61287
torch @ https://download.pytorch.org/whl/torch-1.13.1-cp39-cp39-manylinux2014_aarch64.whl ; platform_machine == "aarch64" and python_full_version == "3.9.10" \
    --hash=sha256:2c3581a3fd81eb1f0f22997cddffea569fea53bafa372b2c0471db373b26aafc

works locally on a M1 Macbook pro with an arm architecture, but fails in the GitLab CI which is using x86_64 with the error

00:01:12 ERROR: /builds/oVEuX7FM/0/predict/BUILD.bazel:201:11: //predict:lib depends on @predict_torch//:pkg in repository @predict_torch which failed to fetch. no such package '@predict_torch//': whl_library predict_torch failed: Ignoring torch: markers 'platform_machine == "aarch64" and python_full_version == "3.9.10"' don't match your environment

Using the latest rules_python

http_archive(
    name = "rules_python",
    sha256 = "ffc7b877c95413c82bfd5482c017edcf759a6250d8b24e82f41f3c8b8d9e287e",
    strip_prefix = "rules_python-0.19.0",
    url = "https://github.com/bazelbuild/rules_python/releases/download/0.19.0/rules_python-0.19.0.tar.gz",
)

[arrdem]

does your usecase work better if you have the os specific requirements files as implemented in #1123 and you keep the platform specific includes in the requirements.in as opposed to requirements.txt file?

@aignas No, we have architecture specific backport builds which need to be selected in support of Apple ARM hardware and Amazon Graviton. In working through options for how to fit this into the existing rules_python infrastructure the #1123 per-OS requirements selectors helps, but it's not immediately obvious to me that teaching the pip_parse macro to do os+arch based selection would be a good pattern. It seems better to use markers for that, rather than adding full select() style support for arbitrary (kernel, arch) tuples which is the real need here.

[arrdem]

Slept on this a bit - I could see an argument that we should deploy consistent builds of Python artifacts across architectures so we don't have version differences. That'd get us close to being able to just leverage the current requirements_linux= etc. features.

The kicker is that we do have architecture-specific requirements within a given OS. @mvgijssel's example is legitimate, tensorflow for example also has architecture-dependent requirement within a given host platform/kernel. So I don't think we can get away from something markers-equivalent.

[arrdem]

Hacked around this by writing a python_config repository rule which can use the repository_ctx with access to the host arch and OS and starlark conditionals to generate a config.bzl template file with appropriate contents, simplifying all the multiple-python/pip setup malarky down to

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,
    requirement_clusters = requirement_clusters,
    requirements_lock = requirements_lock,
)

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

pypi_install_deps()

[OliverFM]

Any progress on this issue @arrdem – I couldn't see if you had merged this in, since it looks like, if it is merged, the name has changed.

If not could you share an example of this kind of rule?
In particular it would be great to change these outputs dependent on whether or not CUDA is present.

[github-actions]

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]

This issue was automatically closed because it went 30 days without a reply since it was labeled "Can Close?"

[keith]

I hit this case too, looks like I could duplicate the requirements files in order to pass different files to requirements_by_platform, but that's a lot of files to keep up to date

[aignas]

Note, that there are more comments/discussion on this in #1868.