fix(bzlmod): use X.Y select for the hub repo #1696

aignas · 2024-01-17T00:41:47Z

This allows better interoperability between toolchains and the wheels
built by the whl_library, as wheels are usually compatible across all
of the python patch versions.

This allows better interoperability between toolchains and the wheels built by the `whl_library`, as wheels are usually compatible across all of the python patch versions.

rickeylev · 2024-01-17T00:48:22Z

python/private/bzlmod/pip.bzl

@@ -31,6 +31,11 @@ load("//python/private:parse_whl_name.bzl", "parse_whl_name")
 load("//python/private:version_label.bzl", "version_label")
 load(":pip_repository.bzl", "pip_repository")

+def _major_minor_version(version):
+    version = full_version(version)


Is the full_version() call just kept for the error checking? The logic it incurs is basically:

version = "3.10" full_version = mapping[version] # "3.10.1" or error if unrecognized version major_minor, _, _ = fulL_version.rpartion(full_version) # back to 3.10

If so, I'm fine with that for now. Lets just drop a comment about that.

A version-parsing function would be better TBH. I had to do this same "parse the dots" game in the PR to generate the config settings. But that can go in a separate PR.

Yeah, just wanted to get something functional before I spend cleaning up the API.

Changed the function to do actual version parsing and if this is something that looks OK, we can move it to python/private/semver.bzl#parse_semver or something similar.

I think this is fine for what' we're doing here (X.Y.Z). We can flesh out the parsing the be more comprehensive another time. I just remembered -- don't we have a version parser as part of the wheel format code?

rickeylev · 2024-01-17T00:57:03Z

CI failure might be legit? it's a bit weird because it says there's a failure, but I don't see any test failure logs. Seems related to...the coverage command...?

It looks like env markers could have a micro-version level of specificity, but I don't think I've seen that in practice. Does a wheel's metadata have that detail?

I'm just musing out loud if this will ever, practically speaking, need to regain the ability to have a more specific version (e.g. select({":3.10": ..., ":3.10.1": ...})

python/config_settings/config_settings.bzl

aignas · 2024-01-17T14:08:12Z

CI failure might be legit? it's a bit weird because it says there's a failure, but I don't see any test failure logs. Seems related to...the coverage command...?

It looks like env markers could have a micro-version level of specificity, but I don't think I've seen that in practice. Does a wheel's metadata have that detail?

I'm just musing out loud if this will ever, practically speaking, need to regain the ability to have a more specific version (e.g. select({":3.10": ..., ":3.10.1": ...})

The wheels are built for py3 (pure python), abi3 (subset of cpython abi, which is binary compatible with all cpython 3 versions), and cpxy, where there is no micro specificity. I have not seen any micro level specificity out there. That said, I think I once saw a python metadata file that excluded a particular micro version as a supported version, but I cannot remember where it was and if my brain is not making stuff up.

…y function

python/config_settings/config_settings.bzl

rickeylev

I'll try to find some time tonight to poke that weird failure. If I can't find that time, then we'll merge anyways; we can fix the "they might match PY3 in a config setting" separately if necessary.

python/config_settings/config_settings.bzl

python/private/bzlmod/pip.bzl

rickeylev · 2024-01-18T01:46:52Z

python/config_settings/config_settings.bzl

    for micro_version in python_versions:
        minor, _, _ = micro_version.rpartition(".")
        minor_to_micro_versions.setdefault(minor, []).append(micro_version)
        allowed_flag_values.append(micro_version)

    string_flag(
        name = "python_version",
-        # TODO: The default here should somehow match the MODULE config
-        build_setting_default = python_versions[0],
+        # Default to the legacy value that non-version aware toolchains use


I don't think this comment is accurate? I thought the value here was arbitrary; it's just picking the first value from TOOLS.keys(), which I don't think corresponds to any thing in particular?

Ideally this value would be more intentional. Either the value to in rules_python (3.11 right now think) or whatever was configured as the default python version

Added back the TODO part, but kept the note about the value being set, revised to reflect using the empty string as the "unknown version" value.

python/config_settings/config_settings.bzl

rickeylev · 2024-01-18T06:23:32Z

Ok, I think I know what's going on. This exposed an interesting misconfiguration -> surprising behavior -> bug. The basic issue is the :is_3.8 condition is matching the default flag value (3.8.10), which causes the 3.8 select path to be followed, which picks websockets 3.8.

Why was this passing before but failing now? Because all of these were true:

The flag's default was 3.8.10
The version-unaware rules will inherit the flag value (the default in this case)
The MINOR_MAPPING maps 3.8 to 3.8.18
the select condition was looking for exactly 3.8.18.

The 3.8.10 value comes from TOOL_VERSIONS.keys()[0]. The 3.8.18 value comes from MINOR_MAPPING["3.8"].

Unrolling things, the code looked like this:

# config.bzl
string_flag(default="3.8.10", ...) # from TOOL_VERSIONS.keys()[0]
# MODULE
python.toolchain(python_version="3.8") # turns into 3.8.18 from MINOR_MAPPING
python.toolchain(python_version="3.9", is_default=True)

# bazel query --output=build @pypi//websockets:pkg
alias(..., select({
  "@rules_python//python/config_settings:is_python_3.8.18": "...websockets 38..."
  "//conditions:default": "..websockets 39..."
})

When you build a non-version aware target, it'll use --python_version=3.8.10 (the default). When you build version aware target, it'll turn into 3.8.18.

In the new code, those select conditions match all 3.8.x.

Each piece here is working as intended. The bug is really that the default flag value is set to a value that isn't the actual default python. Fixing that is out of scope for this PR (I'm also not sure we can fix this without bzlmod). I think the best we can do is just set the default to some value that won't match. Empty string seems fine? It's that or a special value like "default"; not sure which is better. Mild preference for empty string (seems closer to "undefined")

rickeylev · 2024-01-18T07:09:19Z

Thinking about it more, I think that bug is just in this PR. In order to trigger it prior to this PR, you would need to have somehow have a version-unaware build generate the 3.8.10 select condition -- but the MINOR_MAPPING would prevent that. Another way would be to have a 3.8.10 config condition, but being as how activating that version is rather hard (you'd have to patch I think), that seems unlikely. Plus its an older version of Python that is no longer supported.

rickeylev · 2024-01-18T06:43:15Z

python/config_settings/config_settings.bzl

    for micro_version in python_versions:
        minor, _, _ = micro_version.rpartition(".")
        minor_to_micro_versions.setdefault(minor, []).append(micro_version)
        allowed_flag_values.append(micro_version)

    string_flag(
        name = "python_version",
-        # TODO: The default here should somehow match the MODULE config
-        build_setting_default = python_versions[0],
+        # Default to the legacy value that non-version aware toolchains use


Added back the TODO part, but kept the note about the value being set, revised to reflect using the empty string as the "unknown version" value.

python/config_settings/config_settings.bzl

rickeylev · 2024-01-18T06:50:17Z

python/private/bzlmod/pip.bzl

@@ -31,6 +31,11 @@ load("//python/private:parse_whl_name.bzl", "parse_whl_name")
 load("//python/private:version_label.bzl", "version_label")
 load(":pip_repository.bzl", "pip_repository")

+def _major_minor_version(version):
+    version = full_version(version)


I think this is fine for what' we're doing here (X.Y.Z). We can flesh out the parsing the be more comprehensive another time. I just remembered -- don't we have a version parser as part of the wheel format code?

fix(bzlmod): use X.Y select for the hub repo

3b62fd5

This allows better interoperability between toolchains and the wheels built by the `whl_library`, as wheels are usually compatible across all of the python patch versions.

aignas requested a review from rickeylev as a code owner January 17, 2024 00:41

rickeylev approved these changes Jan 17, 2024

View reviewed changes

fix: set a different default for the python_version setting

8f86a22

aignas commented Jan 17, 2024

View reviewed changes

python/config_settings/config_settings.bzl Show resolved Hide resolved

comment: improve the version parsing, which can later become a utilit…

ef5c7e8

…y function

aignas commented Jan 18, 2024

View reviewed changes

python/config_settings/config_settings.bzl Outdated Show resolved Hide resolved

rickeylev reviewed Jan 18, 2024

View reviewed changes

wip: reproing bug

7c7ba68

rickeylev approved these changes Jan 18, 2024

View reviewed changes

remove PY3 as valid flag value

af5a51e

rickeylev enabled auto-merge January 18, 2024 07:28

rickeylev added this pull request to the merge queue Jan 18, 2024

Merged via the queue into bazelbuild:main with commit bd22419 Jan 18, 2024
3 of 4 checks passed

mering mentioned this pull request Jan 20, 2024

Add grpc v1.56.3 bazelbuild/bazel-central-registry#1246

Merged

aignas mentioned this pull request Feb 5, 2024

bzlmod pip extension generates a different repo if the python.toolchain version is specified in X.Y.Z format #1339

Closed

aignas deleted the refactor/use-minor-version-alias-in-pip branch May 13, 2024 06:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(bzlmod): use X.Y select for the hub repo #1696

fix(bzlmod): use X.Y select for the hub repo #1696

aignas commented Jan 17, 2024 •

edited by rickeylev

Loading

rickeylev Jan 17, 2024

aignas Jan 17, 2024

aignas Jan 18, 2024

rickeylev Jan 18, 2024

rickeylev commented Jan 17, 2024

aignas commented Jan 17, 2024

rickeylev left a comment

rickeylev Jan 18, 2024

rickeylev Jan 18, 2024

rickeylev commented Jan 18, 2024 •

edited

Loading

rickeylev commented Jan 18, 2024

rickeylev Jan 18, 2024

rickeylev Jan 18, 2024

fix(bzlmod): use X.Y select for the hub repo #1696

fix(bzlmod): use X.Y select for the hub repo #1696

Conversation

aignas commented Jan 17, 2024 • edited by rickeylev Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickeylev commented Jan 17, 2024

aignas commented Jan 17, 2024

rickeylev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickeylev commented Jan 18, 2024 • edited Loading

rickeylev commented Jan 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aignas commented Jan 17, 2024 •

edited by rickeylev

Loading

rickeylev commented Jan 18, 2024 •

edited

Loading