[bzlmod] Silence `Ignoring toolchain` warning for duplicate toolchains with the same configuration

[keith]

With our project using bzlmod, rules_python, and grpc, with this setup in the MODULE.bazel:

python = use_extension("@rules_python//python/extensions:python.bzl", "python")
python.toolchain(
    ignore_root_user_error = True,
    is_default = True,
    python_version = "3.11",
)
use_repo(python, "python_3_11")

When building you see this warning:

DEBUG: .../python.bzl:45:10: WARNING: Ignoring toolchain 'python_3_11' from module 'grpc': Toolchain 'python_3_11' from module 'foo' already registered Python version 3.11 and has precedence

In this case it seems that grpc also registers a 3.11 toolchain when using it from bzlmod https://github.com/bazelbuild/bazel-central-registry/blob/5958c06f2695493509c6b8ad64c2774b2e25925c/modules/grpc/1.56.3.bcr.1/patches/python.patch#L22-L26

Which produces this through this code https://github.com/bazelbuild/rules_python/blob/c5c03b2477dd1ce0c06c9dc60bf816995f222bcf/python/private/bzlmod/python.bzl#L144

In this case it doesn't seem like it's a problem, or that I can do anything about it? So I think it would be nice if it didn't produce a warning. Looking at the condition I am wondering if it is meant to be an and instead, since first.module.is_root == True in this case, but I don't understand the intent enough.

🌍 Your Environment

Operating System: macOS Output of bazel version: 7.1.1 Rules_python version: HEAD

[aignas]

Going through old issues and it looks like we have two options:

• Not produce a warning.
• Ask the non-root modules to not register toolchains or register them in dev_dependency mode.

I am curious if the second option would be feasible here.

[keith]

I don't think the second is possible since if you didn't register one in the root modules your dependencies would have to exist.

another less fun option would just be to allow users to disable this warning to avoid the noise if they are ok with the duplication

[chandlerc]

I'd like to highlight this part of the original report, because that's true for the case I'm seeing as well:

In this case it doesn't seem like it's a problem, or that I can do anything about it? So I think it would be nice if it didn't produce a warning. Looking at the condition I am wondering if it is meant to be an and instead, since first.module.is_root == True in this case, but I don't understand the intent enough.

This would also basically let folks that have two dependencies that do this in a way that collide and cause the warning avoid it by taking over managing the python toolchain registration in their root module. In a way, it makes that the option for users to disable the warning.

[aignas]

Recently we have added a logger instance into our repo_utils and #2082 added the usage of it everywhere. I think we could downgrade the severity of the warning that we have right now to INFO as it is causing issues to our stakeholders and just document that users can increase the verbosity to understand what is going on in the extension evaluation phase when things end up in an unexpected state. @keith, @rickeylev what do you think about such a solution?

That way we would have better experience out of the box.

[tpudlik]

Warnings must be actionable: you must be able to do something to make the warning go away (either fix the root cause, or explicitly silence the warning). Otherwise, warning spam will simply accumulate, and you will miss new warnings. Currently, this warning isn't actionable---or at least, I as a user (and distributor of a non-root module) don't know what's the action I'm supposed to take to make it go away!

Should I not register a python_3_11 toolchain, and rely on the one from rules_python?

1. The comment makes it sound like that's unwise, because rules_python may stop registering a 3.11 toolchain in the future!
2. Even if I'm OK with that risk, what if I need my 3.11 toolchain to have ignore_root_user_error = True (which the rules_python toolchain does not have)?

So it looks like in some situations, I ought to register the toolchain. (For example, rules_fuzzing appears to fall in this bucket.) How then do I avoid all my downstream users receiving this confusing warning?

@aignas would downgrading this to INFO prevent it from being printed by default? That would be great in my opinion!

[keith]

as a quick workaround we've just patched out this warning to silence it

[chandlerc]

as a quick workaround we've just patched out this warning to silence it

Got a handy link to the patch out? Given silence here, interested in doing that too.

[keith]

diff --git a/python/private/bzlmod/python.bzl b/python/private/bzlmod/python.bzl
index 5862f00..3acd1ed 100644
--- a/python/private/bzlmod/python.bzl
+++ b/python/private/bzlmod/python.bzl
@@ -141,7 +141,7 @@ def _python_impl(module_ctx):
                 # module, they should not be warned for choosing the same
                 # version that rules_python provides as default.
                 first = global_toolchain_versions[toolchain_version]
-                if mod.name != "rules_python" or not first.module.is_root:
+                if False:
                     _warn_duplicate_global_toolchain_version(
                         toolchain_version,
                         first = first,

[aignas]

I wanted to submit a PR, but then I thought of a few reasons against it and wanted to write things down, so hopefully the brain dump is useful for future me or other maintainers.

TLDR: I think we should remove the warning and make it a DEBUG or INFO level log, which can be enabled by:

env RULES_PYTHON_REPO_DEBUG_VERBOSITY=INFO bazel build //...

Firstly, the root module can do whatever and the default version should ideally be settable only by the root module or rules_python. However, I see some non-root modules setting the default and I am not quite sure this is the best practise. If it is only for testing of the non-root module itself, then dev_dependency = True should be passed to the python extension. That way when it is used as a non-root module, the warning would not be present.

Secondly, the pip extension right now needs a registered interpreter with some default version. That default version will be set by rules_python or by the root module. Currently it has code like:

        if python_name not in INTERPRETER_LABELS:
            fail((
                "Unable to find interpreter for pip hub '{hub_name}' for " +
                "python_version={version}: Make sure a corresponding " +
                '`python.toolchain(python_version="{version}")` call exists.' +
                "Expected to find {python_name} among registered versions:\n  {labels}"
            ).format(
                hub_name = hub_name,
                version = pip_attr.python_version,
                python_name = python_name,
                labels = "  \n".join(INTERPRETER_LABELS),
            ))

This suggests me that the initial pip extension design was done either disregarding the fact that a warning would be issued if non-root modules define dependencies for their pip.parse repos or disregarding the fact that pip in non-root modules would be executed at all without dev_dependency = True. I am inclined to think that it was just an oversight on the warning part.

We do need to pass the interpreter label to the whl_library rule if the user is using the hermetic toolchain and this is the reason why the code exists. In order to not have the warning we would have to fail either during the analysis or build phase:

1. rules_python would have to register all of the python versions to make this work, but then it is unclear how users could opt-out of this behaviour. The opt-out ability is probably only relevant to root modules, whereas the non-root modules will need to be able to work with anything that exists, but that is complicated, see below.

2. non-root modules would have to use the default version set by rules_python, which is 3.11 at the time of writing. However, we would get into a situation which is similar to #1791 if rules_python decided to change it. Suddenly an upgrade to rules_python would break all of the consumers and the root module would want to override the hub repos set up by non-root modules using the pip.parse extension.

   However, not all non-root modules need to use pip.parse, rules like rules_mypy need this functionality working, but protobuf might just work if they defined everything as dev_dependency.

3. if we fail during analysis phase or repo phase when the toolchains are absent, then the root module needing to register/setup all of the toolchains for any non-root module that may exist would likely result in MODULE.bazel with rules_python just becoming the WORKSPACE file as discussed in https://github.com/bazelbuild/bazel/discussions/22024.

So my thinking after writing all of the above is that we should fix the issue at hand by one of below:

1. Reduce the verbosity of the warning, this would make everyone in this thread happy, but there is a known issue with coverage.py sometimes shadowing the runfiles library, which could be hard to debug if the root module is overriding the toolchain. See #2009 for details.

2. Fix the pip.parse logic pasted above and setup the whl_library with any python interpreter that might be available and do not require having all of the interpreters available. This is bigger scope than above, but definitely feasible with the code we have today, we just need to do some wiring, which is basically part of #260.

   Then the correct fix to remove the warning would still be to ask the author of a non-root module to change the invocation of python extension to have dev_dependency = True, but we could document that in the warning itself and that would be a fairly small change. The problem is that this would force the changes upon users of rules_python and nobody likes being forced to change. One could argue that the mandating the dev_dependency usage in this case might be rather pedantic.

   I guess the main problem of having the warning at all is that there is nothing the observer of the warning can do - the non-root module author will not see the warning themselves when developing.

P.S. Sorry for the long comment, I did not have time to write a shorter one.