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] Silence Ignoring toolchain warning for duplicate toolchains with the same configuration #1818

Closed
keith opened this issue Mar 22, 2024 · 9 comments · Fixed by #2111
Closed

Comments

@keith
Copy link
Member

keith commented Mar 22, 2024

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

if mod.name != "rules_python" or not first.module.is_root:

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
Copy link
Collaborator

aignas commented Jun 4, 2024

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
Copy link
Member Author

keith commented Jun 4, 2024

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
Copy link

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.

chandlerc added a commit to chandlerc/carbon-lang that referenced this issue Jul 15, 2024
This improve the toolchain's performance by about 10%.

It will also allow us to leverage TCMalloc's extensions to do heap
profiling and get other information about how efficiently we're using
the heap.

Note that currently this causes all of our builds to produce a warning
due to an issue with `rules_python` and multiple modules registering
python toolchains:
bazelbuild/rules_python#1818
chandlerc added a commit to chandlerc/carbon-lang that referenced this issue Jul 15, 2024
This improve the toolchain's performance by about 10%.

It will also allow us to leverage TCMalloc's extensions to do heap
profiling and get other information about how efficiently we're using
the heap.

Note that currently this causes all of our builds to produce a warning
due to an issue with `rules_python` and multiple modules registering
python toolchains:
bazelbuild/rules_python#1818

This is also only enabled on Linux as there is no support for other OSes
at the moment.
github-merge-queue bot pushed a commit to carbon-language/carbon-lang that referenced this issue Jul 17, 2024
This improve the toolchain's performance by about 10%.

It will also allow us to leverage TCMalloc's extensions to do heap
profiling and get other information about how efficiently we're using
the heap.

Note that currently this causes all of our builds to produce a warning
due to an issue with `rules_python` and multiple modules registering
python toolchains:
bazelbuild/rules_python#1818

This is also only enabled on Linux as there is no support for other OSes
at the moment.
@aignas
Copy link
Collaborator

aignas commented Jul 22, 2024

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
Copy link
Contributor

tpudlik commented Jul 30, 2024

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
Copy link
Member Author

keith commented Jul 30, 2024

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

@chandlerc
Copy link

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
Copy link
Member Author

keith commented Aug 5, 2024

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
Copy link
Collaborator

aignas commented Aug 6, 2024

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 Thirdparty pip dependencies override root project's pip dependencies #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
    module.is_root is turning bzlmod into yet another WORKSPACE bazel#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 Cannot use runfiles.python import path when running bazel coverage #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 "Cross compilation" of py_binary/py_image/py_library targets #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.

aignas added a commit to aignas/rules_python that referenced this issue Aug 6, 2024
Before this PR we would warn when non-root modules register interpreter
versions and the owners of root modules would have no way to silence the
warning except for patching rules_python.

This PR reduces the default verbosity of the warning to INFO to avoid
spamming users by default.

Fixes bazelbuild#1818.
github-merge-queue bot pushed a commit that referenced this issue Aug 6, 2024
Before this PR we would warn when non-root modules register interpreter
versions and the owners of root modules would have no way to silence the
warning except for patching rules_python.

This PR reduces the default verbosity of the warning to INFO to avoid
spamming users by default.

Fixes #1818.
brymer-meneses pushed a commit to brymer-meneses/carbon-lang that referenced this issue Aug 15, 2024
…-language#4133)

This improve the toolchain's performance by about 10%.

It will also allow us to leverage TCMalloc's extensions to do heap
profiling and get other information about how efficiently we're using
the heap.

Note that currently this causes all of our builds to produce a warning
due to an issue with `rules_python` and multiple modules registering
python toolchains:
bazelbuild/rules_python#1818

This is also only enabled on Linux as there is no support for other OSes
at the moment.
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.

4 participants