-
Notifications
You must be signed in to change notification settings - Fork 542
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
pip_parse does not respect requirement markers #1105
Comments
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. |
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
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
Using the latest rules_python
|
@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 |
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 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. |
Hacked around this by writing a 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() |
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? |
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. |
This issue was automatically closed because it went 30 days without a reply since it was labeled "Can Close?" |
I hit this case too, looks like I could duplicate the requirements files in order to pass different files to |
Note, that there are more comments/discussion on this in #1868. |
This is just a small PR to reduce the scope of bazelbuild#2059. Work towards bazelbuild#260, bazelbuild#1105, bazelbuild#1868.
…around This is extra preparation needed for bazelbuild#2059. Summary: - Create `pypi_repo_utils` for more ergonomic handling of Python in repo context. - Split the resolution of requirements files to platforms into a separate function to make the testing easier. This also allows more validation that was realized that there is a need for in the WIP feature PR. - Make the code more robust about the assumption of the target platform label. Work towards bazelbuild#260, bazelbuild#1105, bazelbuild#1868.
…around This is extra preparation needed for bazelbuild#2059. Summary: - Create `pypi_repo_utils` for more ergonomic handling of Python in repo context. - Split the resolution of requirements files to platforms into a separate function to make the testing easier. This also allows more validation that was realized that there is a need for in the WIP feature PR. - Make the code more robust about the assumption of the target platform label. Work towards bazelbuild#260, bazelbuild#1105, bazelbuild#1868.
…around (#2069) This is extra preparation needed for #2059. Summary: - Create `pypi_repo_utils` for more ergonomic handling of Python in repo context. - Split the resolution of requirements files to platforms into a separate function to make the testing easier. This also allows more validation that was realized that there is a need for in the WIP feature PR. - Make the code more robust about the assumption of the target platform label. Work towards #260, #1105, #1868.
This also changes the local_runtime_repo to explicitly check for supported platforms instead of relying on a `None` value returned by the helper method. This makes the behaviour exactly the same to the behaviour before this PR and we can potentially drop the need for the validation in the future if our local_runtime detection is more robust. This also makes the platform detectino in `pypi_repo_utils` not depend on `uname` and only use the `repository_ctx`. Apparently the `module_ctx.watch` throws an error if one attempts to watch files on the system (this is left for `repository_rule` it seems and one can only do `module_ctx.watch` on files within the current workspace. This was surprising, but could have been worked around by just unifying code. This splits out things from bazelbuild#2059 and makes the code more succinct. Work towards bazelbuild#260, bazelbuild#1105, bazelbuild#1868.
This also changes the local_runtime_repo to explicitly check for supported platforms instead of relying on a `None` value returned by the helper method. This makes the behaviour exactly the same to the behaviour before this PR and we can potentially drop the need for the validation in the future if our local_runtime detection is more robust. This also makes the platform detectino in `pypi_repo_utils` not depend on `uname` and only use the `repository_ctx`. Apparently the `module_ctx.watch` throws an error if one attempts to watch files on the system (this is left for `repository_rule` it seems and one can only do `module_ctx.watch` on files within the current workspace. This was surprising, but could have been worked around by just unifying code. This splits out things from #2059 and makes the code more succinct. Work towards #260, #1105, #1868.
Before this change the `all_requirements` and related constants will have packages that need to be installed only on specific platforms and will mean that users relying on those constants (e.g. `gazelle`) will need to do extra work to exclude platform-specific packages. The package managers that that support outputting such files now include `uv` and `pdm`. This might be also useful in cases where we attempt to handle non-requirements lock files. Note, that the best way to handle this would be to move all of the requirements parsing code to Python, but that may cause regressions as it is a much bigger change. This is only changing the code so that we are doing extra processing for the requirement lines that have env markers. The lines that have no markers will not see any change in the code execution paths and the python interpreter will not be downloaded. We also use the `*_ctx.watch` API where available to correctly re-evaluate the markers if the `packaging` Python sources for this change. Extra changes that are included in this PR: - Extend the `repo_utils` to have a method for `arch` getting from the `ctx`. - Change the `local_runtime_repo` to perform the validation not relying on the implementation detail of the `get_platforms_os_name`. - Add `$(UV)` make variable for the `uv:current_toolchain` so that we can generate the requirements for `sphinx` using `uv`. - Swap the requirement generation using `genrule` and `uv` for `sphinx` and co so that we can test the `requirement` marker code. Note, the `requirement` markers are not working well with the `requirement_cycles`. Fixes #1105. Fixes #1868. Work towards #260, #1975. Related #1663. --------- Co-authored-by: Richard Levasseur <rlevasseur@google.com>
…thon Before this PR the lockfile would become platform dependent when the `requirements` file would have env markers. This was not caught because we do not have MODULE.bazel.lock checked into the `rules_python` repository because the CI is running against many versions and the lock file is different, therefor we would not be able to run with `bazel build --lockfile_mode=error`. With this change we use the label to `BUILD.bazel` which is living next to the `python` symlink and since the `BUILD.bazel` is the same on all platforms, the lockfile will remain the same. Work towards bazelbuild#1105, bazelbuild#1868.
…thon Before this PR the lockfile would become platform dependent when the `requirements` file would have env markers. This was not caught because we do not have MODULE.bazel.lock checked into the `rules_python` repository because the CI is running against many versions and the lock file is different, therefor we would not be able to run with `bazel build --lockfile_mode=error`. With this change we use the label to `BUILD.bazel` which is living next to the `python` symlink and since the `BUILD.bazel` is the same on all platforms, the lockfile will remain the same. Work towards bazelbuild#1105, bazelbuild#1868.
…thon Before this PR the lockfile would become platform dependent when the `requirements` file would have env markers. This was not caught because we do not have MODULE.bazel.lock checked into the `rules_python` repository because the CI is running against many versions and the lock file is different, therefor we would not be able to run with `bazel build --lockfile_mode=error`. With this change we use the label to `BUILD.bazel` which is living next to the `python` symlink and since the `BUILD.bazel` is the same on all platforms, the lockfile will remain the same. Work towards bazelbuild#1105, bazelbuild#1868.
…thon Before this PR the lockfile would become platform dependent when the `requirements` file would have env markers. This was not caught because we do not have MODULE.bazel.lock checked into the `rules_python` repository because the CI is running against many versions and the lock file is different, therefor we would not be able to run with `bazel build --lockfile_mode=error`. With this change we use the label to `BUILD.bazel` which is living next to the `python` symlink and since the `BUILD.bazel` is the same on all platforms, the lockfile will remain the same. Work towards bazelbuild#1105, bazelbuild#1868.
…thon Before this PR the lockfile would become platform dependent when the `requirements` file would have env markers. This was not caught because we do not have MODULE.bazel.lock checked into the `rules_python` repository because the CI is running against many versions and the lock file is different, therefor we would not be able to run with `bazel build --lockfile_mode=error`. With this change we use the label to `BUILD.bazel` which is living next to the `python` symlink and since the `BUILD.bazel` is the same on all platforms, the lockfile will remain the same. Work towards bazelbuild#1105, bazelbuild#1868.
…thon Before this PR the lockfile would become platform dependent when the `requirements` file would have env markers. This was not caught because we do not have MODULE.bazel.lock checked into the `rules_python` repository because the CI is running against many versions and the lock file is different, therefor we would not be able to run with `bazel build --lockfile_mode=error`. With this change we use the label to `BUILD.bazel` which is living next to the `python` symlink and since the `BUILD.bazel` is the same on all platforms, the lockfile will remain the same. Work towards bazelbuild#1105, bazelbuild#1868.
…thon Before this PR the lockfile would become platform dependent when the `requirements` file would have env markers. This was not caught because we do not have MODULE.bazel.lock checked into the `rules_python` repository because the CI is running against many versions and the lock file is different, therefor we would not be able to run with `bazel build --lockfile_mode=error`. With this change we use the label to `BUILD.bazel` which is living next to the `python` symlink and since the `BUILD.bazel` is the same on all platforms, the lockfile will remain the same. Work towards bazelbuild#1105, bazelbuild#1868.
…thon Before this PR the lockfile would become platform dependent when the `requirements` file would have env markers. This was not caught because we do not have MODULE.bazel.lock checked into the `rules_python` repository because the CI is running against many versions and the lock file is different, therefor we would not be able to run with `bazel build --lockfile_mode=error`. With this change we use the label to `BUILD.bazel` which is living next to the `python` symlink and since the `BUILD.bazel` is the same on all platforms, the lockfile will remain the same. Work towards bazelbuild#1105, bazelbuild#1868.
…thon (#2135) Before this PR the lockfile would become platform dependent when the `requirements` file would have env markers. This was not caught because we do not have MODULE.bazel.lock checked into the `rules_python` repository because the CI is running against many versions and the lock file is different, therefore we would not be able to run with `bazel build --lockfile_mode=error`. With this change we use the label to `BUILD.bazel` which is living next to the `python` symlink and since the `BUILD.bazel` is the same on all platforms, the lockfile will remain the same. Summary * refactor(uv): create a reusable macro for using uv for locking reqs. * test(bzlmod): enable testing the MODULE.bazel.lock breakage across platforms. * test(bzlmod): use a universal requirements file for 3.9. This breaks the CI, because the python interpreter file hash is added to the lock file. * fix(bzlmod): keep the lockfile platform independent when resolving python Fixes #1105 and #1868 for real this time. Implements an additional helper for #1975.
🐞 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 becausepip 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. ConsiderThe 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 therequirement("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 andrequirements_darwin
, but it shouldn't be an error.If multiple requirements are selected, that should likely be an error.
🔥 Exception or Error
The text was updated successfully, but these errors were encountered: