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

feat: support parsing whl METADATA on any python version #1693

Merged
merged 29 commits into from
Jan 25, 2024

Conversation

aignas
Copy link
Collaborator

@aignas aignas commented Jan 14, 2024

With this PR we can deterministically parse the METADATA and generate a
BUILD.bazel file using the config settings introduced in #1555. Let's
imagine we had a requirements.txt file that used only wheels, we could
use the host interpreter to parse the wheel metadata for all the target
platforms and use the version aware toolchain at runtime. This potentially
unlocks more clever layouts of the bzlmod hub repos explored in #1625
where we could have a single whl_library instance for all versions within
a single hub repo.

Work towards #1643.

@aignas aignas requested a review from rickeylev January 14, 2024 10:03
With this PR we prepare the dependency resolution machinery to
support other than host python versions. This may make it possible
to generate select statements based on the version but we target
this usecase as it requires little code to be changed and once
wired to the starlark code could be of use. Creating this PR
for discussion so that we can decide on a suitable interface.
@aignas aignas force-pushed the feat/python-impl-independent-2 branch from c8656bb to 8f6b8a3 Compare January 19, 2024 01:20
@aignas aignas changed the title feat: support resolving deps for a non-host interpreter version feat: support selecting on python version and target different versions than the interpreter Jan 19, 2024
@aignas aignas changed the title feat: support selecting on python version and target different versions than the interpreter feat: support parsing whl METADATA on any python version Jan 19, 2024
@aignas aignas marked this pull request as ready for review January 19, 2024 08:29
Copy link
Collaborator

@rickeylev rickeylev left a comment

Choose a reason for hiding this comment

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

Mostly just small things. Otherwise LGTM.

python/pip_install/tools/wheel_installer/wheel.py Outdated Show resolved Hide resolved
python/pip_install/tools/wheel_installer/wheel.py Outdated Show resolved Hide resolved
python/pip_install/tools/wheel_installer/wheel.py Outdated Show resolved Hide resolved
@aignas aignas force-pushed the feat/python-impl-independent-2 branch from 8f96824 to 65c5542 Compare January 22, 2024 08:20
@@ -8,3 +8,6 @@ test --test_output=errors --enable_runfiles
build --enable_runfiles

startup --windows_enable_symlinks

# Set the default version of python_version config
common --@rules_python//python/config_settings:python_version=3.9.18
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I got this this example to work by setting the default toolchain/python version using this and then version specific selects start working in practice with regular py_binary and py_test rules.

@rickeylev , what do you think about how we should set the default python_version value? I think there are several improvements (outside this PR) that we could make:

  1. Allow users to set 3.9 as the value to python_version. This would requires us to change the toolchain definition so that it uses select.match_any(3.9, 3.9.18) in the target_settings and update the is_python generation code.
  2. Move the definition of the python_version to the pythons_hub under bzlmod with setting the default value the same as the default toolchain. We can make an alias if we are using bzlmod to the pythons_hub string value. That way I can keep the default settings in the MODULE.bazel and would not need this line here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Allow users to set 3.9 as the value to python_version. This would requires us to change the toolchain definition so that it uses select.match_any(3.9, 3.9.18) in the target_settings and update the is_python generation code.

Yeah, I think that's fine. I was going to suggest a slightly different structure: two separate toolchain registrations, with 3.9 after any 3.9.X toolchains. This would allow multiple micro-versions. But, the rest of the everything doesn't really support a micro-version, so perhaps that is premature.

I'm mixed on allowing "3.9" as a flag value. It does seem very natural, so I do like that part. But with how flags work, "3.9" has no relation to "3.9.18", so I worry that's going to rear its head somehow. I guess our public interface here are the "is_XX" settings, though, so we have some freedom about the flag input format (e.g. "latest" as a possible value sounds convenient).

Anyways, yeah, i guess allowing 3.9 as valid value is fine.

I've heard rumor that config_common.FeatureFlagInfo can be used to allow a flag be implemented as a rule, and have the resulting value seen by config_setting. I haven't tried it myself, though, and was told this was "deprecated", so I'm not sure about that. In any case, my point in mentioning this is that would allow taking 3.9 as the input value and then transform it to 3.8.18 (by looking at MINOR_MAPPING), and everything should just work as before.

Move the definition of the python_version to the pythons_hub under bzlmod with setting the default value the same as the default toolchain.

What about just loading the default and setting it instead? e.g.

# python/config_settings/BUILD
load("@pythons_hub//:interpreters.bzl", "DEFAULT_PYTHON_VERSION")

define_python_version_flag(default=DEFAULT_PYTHON_VERSION)

Regardless, something that isn't clear to me is how to do the same for workspace builds. Is that even possible? I think python_register_multi_toolchains would need to generate something with the default value in it. Except that function isn't always called, so we can't guarantee it is called. Yeah, I dunno.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW I'm currently using python_version directly since I can mix it with constraints in a single config_setting, like

config_setting(
  name = "python_3.10.12_aarch64-apple-darwin_config",
  flag_values = {"@rules_python//python/config_settings:python_version": "3.10.12"},
  constraint_values = ["@platforms//os:macos", "@platforms//cpu:aarch64"],
)

Otherwise, to use the is_XX settings, I guess I'd have to use skylib's selects.config_setting_group?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that is what I noticed and using config_setting_group is a little unwieldy. I have created #1720 as a POC to potentially allow having "@rules_python//python/config_settings:python_version": "3.10" instead, but I am missing a few things there to get it fully working.

# * @platforms//cpu:{value}
# * @//python/config_settings:is_python_3.{minor_version}
# * {os}_{cpu}
# * cp3{minor_version}_{os}_{cpu}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI, I had to move the rendering of the config settings outside the main loop because it was getting complex. However, this PR can generate the right select for pylint:

    deps = [
        "@pip_39_astroid//:pkg",
        "@pip_39_dill//:pkg",
        "@pip_39_isort//:pkg",
        "@pip_39_mccabe//:pkg",
        "@pip_39_platformdirs//:pkg",
        "@pip_39_tomlkit//:pkg",
    ] + select(
        {
            "@@rules_python~override//python/config_settings:is_python_3.10": ["@pip_39_tomli//:pkg"],
            "@@rules_python~override//python/config_settings:is_python_3.9": [
                "@pip_39_tomli//:pkg",
                "@pip_39_typing_extensions//:pkg",
            ],
            ":is_cp310_windows_anyarch": [
                "@pip_39_colorama//:pkg",
                "@pip_39_dill//:pkg",
                "@pip_39_tomli//:pkg",
            ],
            ":is_cp311_windows_anyarch": [
                "@pip_39_colorama//:pkg",
                "@pip_39_dill//:pkg",
            ],
            ":is_cp39_windows_anyarch": [
                "@pip_39_colorama//:pkg",
                "@pip_39_dill//:pkg",
                "@pip_39_tomli//:pkg",
                "@pip_39_typing_extensions//:pkg",
            ],
            "//conditions:default": [],
        },
    ),

Disregard the fact that the conditions and the target repos should match - this can be done as a followup item. I am not sure yet what design would be best. This versioned select is only enabled if the user specifies multiple target python versions.

@aignas aignas enabled auto-merge January 24, 2024 23:52
@aignas aignas added this pull request to the merge queue Jan 25, 2024
@rickeylev rickeylev removed this pull request from the merge queue due to a manual request Jan 25, 2024
@rickeylev
Copy link
Collaborator

Sorry, removed from queue -- just want to double check something about the unit tests.

@rickeylev rickeylev added this pull request to the merge queue Jan 25, 2024
Merged via the queue into bazelbuild:main with commit 0aa5ea4 Jan 25, 2024
4 checks passed
@aignas aignas deleted the feat/python-impl-independent-2 branch January 25, 2024 03:35
aignas added a commit to aignas/rules_python that referenced this pull request Mar 18, 2024
The bazelbuild#1693 PR incorrectly assumed that the platform tag will be os-arch
specific if and only if the abi tag is of form cpxy. However, there are
many wheels that are not like this (e.g. watchdog, tornado, libclang).
This fixes the starlark code that is overriding the user platforms with
something that only the wheel supports by also taking into account the
ABI.

Fixes bazelbuild#1810.
aignas added a commit to aignas/rules_python that referenced this pull request Mar 18, 2024
The bazelbuild#1693 PR incorrectly assumed that the platform tag will be os-arch
specific if and only if the abi tag is of form cpxy. However, there are
many wheels that are not like this (e.g. watchdog, tornado, libclang).
This fixes the starlark code that is overriding the user platforms with
something that only the wheel supports by also taking into account the
ABI.

Fixes bazelbuild#1810.
github-merge-queue bot pushed a commit that referenced this pull request Mar 18, 2024
The #1693 PR incorrectly assumed that the platform tag will be os-arch
specific if and only if the abi tag is of form cpxy. However, there are
many wheels that are not like this (e.g. watchdog, tornado, libclang).
This fixes the starlark code that is overriding the user platforms with
something that only the wheel supports by also taking into account the
ABI.

Fixes #1810.
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.

3 participants