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

bzlmod pip.parse default is leaking across to the other_module #1548

Closed
aignas opened this issue Nov 9, 2023 · 2 comments · Fixed by #1549
Closed

bzlmod pip.parse default is leaking across to the other_module #1548

aignas opened this issue Nov 9, 2023 · 2 comments · Fixed by #1549
Labels
type: bzlmod bzlmod work

Comments

@aignas
Copy link
Collaborator

aignas commented Nov 9, 2023

In the bzlmod example, if you examine the @other_module_pip//absl_py:BUILD.bazel contents, you will see:

$ cat bazel-bzlmod/external/rules_python~override~pip~other_module_pip/absl_py/BUILD.bazel
package(default_visibility = ["//visibility:public"])

alias(
    name = "absl_py",
    actual = ":pkg",
)

alias(
    name = "pkg",
    actual = select(
        {
            "@@rules_python~override//python/config_settings:is_python_3.11.6": "@other_module_pip_311_absl_py//:pkg",
            "//conditions:default": "@other_module_pip_39_absl_py//:pkg",
        },
    ),
)

alias(
    name = "whl",
    actual = select(
        {
            "@@rules_python~override//python/config_settings:is_python_3.11.6": "@other_module_pip_311_absl_py//:whl",
            "//conditions:default": "@other_module_pip_39_absl_py//:whl",
        },
    ),
)

alias(
    name = "data",
    actual = select(
        {
            "@@rules_python~override//python/config_settings:is_python_3.11.6": "@other_module_pip_311_absl_py//:data",
            "//conditions:default": "@other_module_pip_39_absl_py//:data",
        },
    ),
)

alias(
    name = "dist_info",
    actual = select(
        {
            "@@rules_python~override//python/config_settings:is_python_3.11.6": "@other_module_pip_311_absl_py//:dist_info",
            "//conditions:default": "@other_module_pip_39_absl_py//:dist_info",
        },
    ),
)%

However, the other_module_pip does not have anything set up for 3.9 and is setting 3.11 as the default toolchain version for the sub-module. It seems that this line may be wrong and we need to get the default version per module that the extension is used in.

@aignas aignas added the type: bzlmod bzlmod work label Nov 9, 2023
@rickeylev
Copy link
Collaborator

This is mostly WAI.

The basic issue is there can only be one default python version for the entire build. When the default py_binary rule is used (//python:py_binary.bzl), then its basically saying "The root module decides what Python version I'll run as".

This doesn't feel ideal, but I'm not sure there's much we can do. If a sub-module wants to support multiple python versions while not picking a particular python version, then it should create requirements for each version.

Also see: https://github.com/bazelbuild/rules_python/blob/main/examples/bzlmod/other_module/MODULE.bazel#L34

The is_default line has a comment saying it's ignored (because its not the root module). Silently ignoring isn't ideal, but I'm not sure what better options we have available. Rename it to is_default_if_root_module=True? The sub module does want it to be the default when doing development of the sub module itself, but we can't honor it when another root module is depending on other_module.

we need to get the default version per module that the extension is used in.

Creating a per-module specific default doesn't fix it because it results in a miss-match between the runtime and selected libraries. e.g. given

alias(name="pkg", actual=select({default: "other_module_pip_311_absl_py"}))

Then what would happen is Python 3.9 will be used as the runtime (via toolchain resolution), while a Python 3.11 package will be used as a dependency. This might work, depending on the circumstances, but is incorrect.


This also might be a remnant of a bug where -- I forget the exact details -- but something about how sub modules were required to specify a default, or required to include the default version the root module used. We fixed that by removing that validation logic.


The fix I recommend (and you see other_module use) is for a submodule, if it is pikcy about the Python version, to use the version-aware rules to ensure it gets the Python version it needs.

aignas added a commit to aignas/rules_python that referenced this issue Nov 9, 2023
…repos

This fixes the cases where the 'default_version' is passed to the
'render_pkg_aliases' utility but the 'default_version' is not present
for the wheels. This usually happens when a sub-module is using the
'pip.parse' extension and the default_version can only be set by the
root module.

Fixes bazelbuild#1548.
@aignas
Copy link
Collaborator Author

aignas commented Nov 9, 2023

OK, then I think the change I propose in #1549 should make things more correct. I was thinking that maybe the render_pkg_aliases does not need to know about the default_version possible not being present in the versions, but given that the default can only be set by the root module, I think it makes sense to handle that edge case.

With #1549 the absl_py/BUILD.bazel now is:

$ cat bazel-bzlmod/external/rules_python~override~pip~other_module_pip/absl_py/BUILD.bazel
package(default_visibility = ["//visibility:public"])

_NO_MATCH_ERROR = """\
No matching wheel for current configuration's Python version.

The current build configuration's Python version doesn't match any of the Python
versions available for this wheel. This wheel supports the following Python versions:
    3.11.6

As matched by the `@rules_python~override//python/config_settings:is_python_<version>`
configuration settings.

To determine the current configuration's Python version, run:
    `bazel config <config id>` (shown further below)
and look for
    rules_python~override//python/config_settings:python_version

If the value is missing, then the "default" Python version is being used,
which has a "null" version value and will not match version constraints.
"""

alias(
    name = "absl_py",
    actual = ":pkg",
)

alias(
    name = "pkg",
    actual = select(
        {
            "@@rules_python~override//python/config_settings:is_python_3.11.6": "@other_module_pip_311_absl_py//:pkg",
        },
        no_match_error = _NO_MATCH_ERROR,
    ),
)

alias(
    name = "whl",
    actual = select(
        {
            "@@rules_python~override//python/config_settings:is_python_3.11.6": "@other_module_pip_311_absl_py//:whl",
        },
        no_match_error = _NO_MATCH_ERROR,
    ),
)

alias(
    name = "data",
    actual = select(
        {
            "@@rules_python~override//python/config_settings:is_python_3.11.6": "@other_module_pip_311_absl_py//:data",
        },
        no_match_error = _NO_MATCH_ERROR,
    ),
)

alias(
    name = "dist_info",
    actual = select(
        {
            "@@rules_python~override//python/config_settings:is_python_3.11.6": "@other_module_pip_311_absl_py//:dist_info",
        },
        no_match_error = _NO_MATCH_ERROR,
    ),
)

github-merge-queue bot pushed a commit that referenced this issue Nov 16, 2023
…1549)

This fixes the cases where the 'default_version' is passed to the
'render_pkg_aliases' utility but the 'default_version' is not present
for the wheels. This usually happens when a sub-module is using the
'pip.parse' extension and the default_version can only be set by the
root module.

Previously, such a case would generate a `select()` expression that
mapped
the default condition to a non-existent target (because the sub-module
didn't
call `pip.parse()` with that version). This would either result in
errors
due the target not existing, or silently using a target intended for a
different Python version (which may work, but isn't correct to so).

Now, it results in a error via `select.no_match_error`.

Fixes #1548.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bzlmod bzlmod work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants