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

pip.parse is installing a Py3.12 version of a (private-index) wheel even though we're using Py3.11 #1930

Closed
dougthor42 opened this issue May 29, 2024 · 2 comments · Fixed by #1917

Comments

@dougthor42
Copy link
Contributor

dougthor42 commented May 29, 2024

🐞 bug report

Affected Rule

  • pip.parse

Is this a regression?

I don't think so. Nothing on the Bazel or rules_python side of things have changed when this appeared.

The bug appeared when our internal foobar package was updated from 2.2.10 to 2.2.12.

Description

pip.parse is installing a Py3.12 version of a (private-index) wheel even though we're using Py3.11.

How does rules_python figure out which Python wheel to install if there are multiple wheels for different python versions?

We have an package on our internal private index. It has wheels for two python versions:

foobar-2.2.12-py311-none-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
foobar-2.2.12-py312-none-manylinux_2_17_x86_64.manylinux2014_x86_64.whl

In MODULE.bazel, I set python_version=3.11:

python.toolchain(
    configure_coverage_tool = True,
    python_version = "3.11",
)
...
pip.parse(
    experimental_extra_index_urls = ["https://[redacted]/simple"],
    experimental_index_url = "https://pypi.org/simple",
    hub_name = "pypi",
    python_version = "3.11",
    requirements_lock = "//:requirements.txt",
)

However, the Py3.12 wheel gets installed:

$ cat bazel-bin/.../decoding_test.runfiles/rules_python~~pip~pypi_311_foobar/site-packages/foobar-2.2.12.dist-info/WHEEL 
Wheel-Version: 1.0
Generator: bazel-wheelmaker 1.0
Root-Is-Purelib: true
Tag: py312-none-manylinux_2_17_x86_64
Tag: py312-none-manylinux2014_x86_64
Root-Is-Purelib: False

My requirements.txt has:

foobar==2.2.12 \
    --hash=sha256:b7d2f441650f075a91f80d131673791f2b7face5722105f528916226dbd8e9e4 \
    --hash=sha256:f8eac44fb8f2ca05d2335085651d5471f5f87647fbf1159b566923b7524eff55
  • The first hash b7d2...e9e4 is for the 3.11 wheel, as verified by python -m pip hash ./foobar-2.2.12-py311-none-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
  • The 2nd hash f8ea...ff55 is for the 3.12 wheel, as verified by python -m pip hash ./foobar-2.2.12-py312-none-manylinux_2_17_x86_64.manylinux2014_x86_64.whl

🔬 Minimal Reproduction

Working on this. Need to find a public pypi package that has separate 3.11 and 3.12 wheels. Know any?

🔥 Exception or Error

bazel build doesn't complain at all. During bazel test, Python throws this:

...
src/.../circuits_for_snake_circuit.py:9: in 
    import foobar
../rules_python~~pip~pypi_311_foobar/site-packages/foobar/__init__.py:4: in 
    from foobar import circuit_generation
E   ImportError: Python version mismatch: module was compiled for Python 3.12, but the interpreter version is incompatible: 3.11.9 (main, Apr 15 2024, 18:24:23) [Clang 17.0.6 ].

🌍 Your Environment

Operating System:

$ lsb_release -a
No LSB modules are available.
Distributor ID: Debian
Description:    Debian GNU/Linux rodete
Release:        n/a
Codename:       rodete

Output of bazel version:

$ bazel version
Bazelisk version: v1.19.0
Starting local Bazel server and connecting to it...
Build label: 7.2.0rc1
Build target: @@//src/main/java/com/google/devtools/build/lib/bazel:BazelServer
Build time: Wed May 15 15:57:57 2024 (1715788677)
Build timestamp: 1715788677
Build timestamp as int: 1715788677

Rules_python version:

bazel_dep(name = "bazel_skylib", version = "1.6.1")
bazel_dep(name = "rules_python", version = "0.31.0")

bazel_dep(name = "buildifier_prebuilt", version = "6.4.0", dev_dependency = True)

bazel_dep(name = "rules_python_gazelle_plugin", version = "0.31.0")  # Same vers as rules_python
bazel_dep(name = "gazelle", version = "0.35.0", repo_name = "bazel_gazelle")

git_override(
    module_name = "rules_python",
    commit = "8791cbbaa2336e24555a6b577ed6e19df24d7d88",
    remote = "https://github.com/bazelbuild/rules_python",
)

git_override(
    module_name = "rules_python_gazelle_plugin",
    commit = "8791cbbaa2336e24555a6b577ed6e19df24d7d88",
    remote = "https://github.com/bazelbuild/rules_python",
    strip_prefix = "gazelle",
)

Anything else relevant?

This appeared when we updated foobar from 2.2.10 to 2.2.12.

For 2.2.10, the requirements.txt also had hashes for both Py3.11 and Py3.12.

@aignas
Copy link
Collaborator

aignas commented May 29, 2024

Yup, this is a know issue that would be fixed by #1917. Could you please try that one?

I would also expect the wheel to be cp312 rather than py312 because the version numbers are specific to an implementation I think. I wonder if our implementation should error out in those cases.

@dougthor42
Copy link
Contributor Author

Oh sweet! I'll be able to test things tomorrow morning.

I would also expect the wheel to be cp312 rather than py312

(Forgive me if you already know all of this - I'm writing it down mostly for my own sanity.)

TL;DR: I too would expect the wheel to be cp312 rather than py312. However, I don't know the full build stack of our internal foobar package, so it's possible that it wasn't built with cpython. 🤷

The wheel filename is defined by PEP 491 as {distribution}-{version}(-{build tag})?-{python tag}-{abi tag}-{platform tag}.whl and the various tags defined in PEP 425. The tags are implemented in wheel.bdist_wheel.bdist_wheel.get_tag(). Specifically, L343-L352 determine the "language implementation and version tag" {python tag}.

For pure wheels (which foobar is1, see the cat .../WHEEL results in OP), these lines call python_tag() which simply returns the "py" plus the major version (eg: "py3"). The bdist_wheel class is a subclass of setuptools.Command, which itself is a subclass of the distutils.cmd.Command ABC, both of which state that the values set by initialize_options() may be overwritten during the build (such as by other Commands).

It's possible that whatever build process we're using to make foobar wheels ends up making such modifications to include the minor version "py3" --> "py312".

For non-pure wheels, these lines end up calling functions from a vendored version of the packaging package, specifically packaging.tags.interpreter_name() and packaging.tags.interpreter_version().

interpreter_name() calls sys.implementation.name which typically returns "cpython". This value is then looked up in the tags.INTERPRETER_SHORT_NAMES mapping and converted to the short-form "cp".

...

Whoo, that was fun! I spent way too much time on that though, and while I learned a lot, I didn't find anything to say exactly why we're getting py312 instead of cp312 🤣.


I wonder if our implementation should error out in those cases.

Error out in cases where the python tag is py instead of cp? Definitely not - pyXYZ is still technically a valid tag.

Or are you talking about something else?

Footnotes

  1. At least I think it is. There are two conflicting Root-Is-Purelib: lines so... 🤷

aignas added a commit to aignas/rules_python that referenced this issue May 30, 2024
github-merge-queue bot pushed a commit that referenced this issue Jun 1, 2024
This moves some of the code out of the `pip.bzl` extension and changes
the layout of the code to prepare for multi-platform whl support.

Summary:

* parse_requirements: add whls and sdists attribute, so that we can use
  a function to populate the lists. Not sure if there is a better way to
  do this.
* parse_requirements: add an extra code to ensure that we are handling
  the target platform filtering correctly.
* select_whl: split the `select_whl` into `select_whls`, which filters
  the whls (this can be used later in multi-platform selects) and
  select_whl , which just is used get the most appropriate whl for the
  host platform.
* Additionally fix the logic in `select_whl`, which would result in
  Python 3.12 wheels being selected on Python 3.11 interpreters because
  we were not taking into account the interpreter tag when doing the
  filtering.

Fixes #1930
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 a pull request may close this issue.

2 participants