Using venvs_site_packages=yes results in overlapping action output paths error

[hartikainen]

🐞 bug report

Affected Rule

The issue is caused by the rule: py_test/py_binary/py_library when using venvs_site_packages=yes

Is this a regression?

No, I don't think so. Indeed, I'm aware that vens_site_packages=yes is still experimental, meaning that feature request might be a more fitting category than bug for this.

Description

I'm unable to run py_{test,binary,library} targets with simple requirements.txt containing just mujoco and mujoco-mjx. I get:

ERROR: One of the output paths 'bazel-out/k8-fastbuild/bin/_mujoco_test.venv/lib/python3.12/site-packages/mujoco' (belonging to //:mujoco_test) and 'bazel-out/k8-fastbuild/bin/_mujoco_test.venv/lib/python3.12/site-packages/mujoco/mjx' (belonging to //:mujoco_test) is a prefix of the other. These actions cannot be simultaneously present; please rename one of the output files or build just one of them

🔬 Minimal Reproduction

main...hartikainen:mujoco-example

🔥 Exception or Error

$ bazel test //:mujoco_test
WARNING: Build option --incompatible_default_to_explicit_init_py has changed, discarding analysis cache (this can be expensive, see https://bazel.build/advanced/performance/iteration-speed).
ERROR: One of the output paths 'bazel-out/k8-fastbuild/bin/_mujoco_test.venv/lib/python3.12/site-packages/mujoco' (belonging to //:mujoco_test) and 'bazel-out/k8-fastbuild/bin/_mujoco_test.venv/lib/python3.12/site-packages/mujoco/mjx' (belonging to //:mujoco_test) is a prefix of the other. These actions cannot be simultaneously present; please rename one of the output files or build just one of them
Target //:mujoco_test failed to build
Use --verbose_failures to see the command lines of failed build steps.
ERROR: One of the output paths 'bazel-out/k8-fastbuild/bin/_mujoco_test.venv/lib/python3.12/site-packages/mujoco' (belonging to //:mujoco_test) and 'bazel-out/k8-fastbuild/bin/_mujoco_test.venv/lib/python3.12/site-packages/mujoco/mjx' (belonging to //:mujoco_test) is a prefix of the other. These actions cannot be simultaneously present; please rename one of the output files or build just one of them
INFO: Elapsed time: 0.177s, Critical Path: 0.00s
INFO: 1 process: 1 internal.
ERROR: Build did NOT complete successfully
ERROR: No test targets were found, yet testing was requested

🌍 Your Environment

Operating System:

$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 24.04.2 LTS
Release:        24.04
Codename:       noble

Output of bazel version:

$ bazel --version
bazel 8.2.1

Rules_python version:

fe45faa

Anything else relevant?

Related to #2156.

[hartikainen]

Maybe this has something to do with the pkgutil.extend_path line that exists in [mujoco].whl/mujoco/__init__.py but not in [mujoco-mjx].whl/mujoco/__init__.py: https://github.com/google-deepmind/mujoco/blob/4cf5c37211821538c5de31ab25ae7af444e3b417/python/mujoco/__init__.py#L27-L34

# Extend the path to enable multiple directories to contribute to the same
# package. Without this line, the `mujoco-mjx` package would not be able to
# be discovered by import. For more information, see: https://packaging.python.org/guides/packaging-namespace-packages/#pkgutil-style-namespace-packages
# NOTE: As per the Python Packaging User Guide linked above, the preferable way
# of declaring the namespace package is to use the native namespace packages.
# This seems non-trivial at the current state of the project, however.
# For more information, see: https://github.com/google-deepmind/mujoco/issues/2119
__path__ = __import__('pkgutil').extend_path(__path__, __name__)

This line was actually originally contributed by myself to make mujoco and mujoco-mjx work with Bazel. (See google-deepmind/mujoco#2119 and https://github.com/google-deepmind/mujoco/pull/2121/files for rationale why I decided to use the legacy namespace package instead of the native namespace packages). Maybe that solution is not the right one and maybe it confuses the bazel site-packages layout somehow? Not sure. I know this worked with the old layout with venvs_site_packages=no and also with other package managers like uv venv+uv pip install ... and just vanilla python -m venv + pip install ....

Probably what's missing right now is the support for Legacy namespace packages. I'm not sure if those should be supported or not. Either way, this issue seems more like a feature request than a bug.

[hartikainen]

The Legacy namespace packages documentation says that:

The __init__.py file for the namespace package needs to contain the following:

__path__ = __import__('pkgutil').extend_path(__path__, __name__)

Every distribution that uses the namespace package must include such an __init__.py. If any distribution does not, it will cause the namespace logic to fail and the other sub-packages will not be importable. Any additional code in __init__.py will be inaccessible.

mujoco-mjx is indeed missing this. I tried patching my original example with the following:

Details

modified   examples/mujoco/MODULE.bazel
@@ -28,4 +28,9 @@ pip.parse(
     requirements_lock = "requirements.txt",
 )
 
+pip.override(
+    file = "mujoco_mjx-3.3.5-py3-none-any.whl",
+    patch_strip = 0,
+    patches = ["//:mjx-namespace.patch"],
+)
 use_repo(pip, "pypi")
new file   examples/mujoco/mjx-namespace.patch
@@ -0,0 +1,11 @@
+--- mujoco/__init__.py
++++ mujoco/__init__.py
+@@ -0,0 +1,8 @@
++# Extend the path to enable multiple directories to contribute to the same
++# package. Without this line, the `mujoco-mjx` package would not be able to
++# be discovered by import. For more information, see: https://packaging.python.org/guides/packaging-namespace-packages/#pkgutil-style-namespace-packages
++# NOTE: As per the Python Packaging User Guide linked above, the preferable way
++# of declaring the namespace package is to use the native namespace packages.
++# This seems non-trivial at the current state of the project, however.
++# For more information, see: https://github.com/google-deepmind/mujoco/issues/2119
++__path__ = __import__('pkgutil').extend_path(__path__, __name__)

This gets me a bit further but still doesn't work because ModuleNotFoundError: No module named 'mujoco.introspect':

Details

$ bazel run //:mujoco_test
...
Executing tests from //:mujoco_test
-----------------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/user/.cache/bazel/_bazel_user/70727693f486c45dbb2d227c77c6f897/execroot/_main/bazel-out/k8-fastbuild/bin/mujoco_test.runfiles/_main/_mujoco_test_stage2_bootstrap.py", line 474, in <module>
    main()
  File "/home/user/.cache/bazel/_bazel_user/70727693f486c45dbb2d227c77c6f897/execroot/_main/bazel-out/k8-fastbuild/bin/mujoco_test.runfiles/_main/_mujoco_test_stage2_bootstrap.py", line 468, in main
    _run_py_path(main_filename, args=sys.argv[1:])
  File "/home/user/.cache/bazel/_bazel_user/70727693f486c45dbb2d227c77c6f897/execroot/_main/bazel-out/k8-fastbuild/bin/mujoco_test.runfiles/_main/_mujoco_test_stage2_bootstrap.py", line 284, in _run_py_path
    runpy.run_path(main_filename, run_name="__main__")
  File "<frozen runpy>", line 287, in run_path
  File "<frozen runpy>", line 98, in _run_module_code
  File "<frozen runpy>", line 88, in _run_code
  File "/home/user/.cache/bazel/_bazel_user/70727693f486c45dbb2d227c77c6f897/execroot/_main/bazel-out/k8-fastbuild/bin/mujoco_test.runfiles/_main/mujoco_test.py", line 4, in <module>
    from mujoco import mjx
  File "/home/user/.cache/bazel/_bazel_user/70727693f486c45dbb2d227c77c6f897/execroot/_main/bazel-out/k8-fastbuild/bin/mujoco_test.runfiles/_main/_mujoco_test.venv/lib/python3.12/site-packages/mujoco/mjx/__init__.py", line 18, in <module>
    from mujoco.mjx._src.collision_driver import collision
  File "/home/user/.cache/bazel/_bazel_user/70727693f486c45dbb2d227c77c6f897/execroot/_main/bazel-out/k8-fastbuild/bin/mujoco_test.runfiles/_main/_mujoco_test.venv/lib/python3.12/site-packages/mujoco/mjx/_src/collision_driver.py", line 47, in <module>
    from mujoco.mjx._src import support
  File "/home/user/.cache/bazel/_bazel_user/70727693f486c45dbb2d227c77c6f897/execroot/_main/bazel-out/k8-fastbuild/bin/mujoco_test.runfiles/_main/_mujoco_test.venv/lib/python3.12/site-packages/mujoco/mjx/_src/support.py", line 23, in <module>
    from mujoco.introspect import mjxmacro
ModuleNotFoundError: No module named 'mujoco.introspect'

It seems like mujoco is excluded from the site-packages altogether and shadowed by just mujoco-mjx:

$ ls ./bazel-bin/mujoco_test.runfiles/_main/_mujoco_test.venv/lib/python3.12/site-packages/mujoco/
__init__.py  mjx  __pycache__

[rickeylev]

I can't remember to what extent we implemented conflicting/overlapping file logic in the site-packages building code. I think only partially? I don't recall writing/reviewing the full "merge two trees that overlap" logic. Just the basic "give preference to first seen path" logic (which I thought also handled path prefixes, but that code is a bit subtle, so a bug is likely)

[aignas]

What should we do in that case? Merge the files or use one of them? Print a warning? Allow selecting which one will be included?

Ideally the mujoco package should have the init file and the plugins don't have it. That way there is no ambiguity here. If I remember correctly that is the approach airflow has taken.

EDIT: actually, looking at this we should do the same as pip does - installing the packages in topological order. https://pip.pypa.io/en/stable/cli/pip_install/#installation-order

We can also issue a warning if there is desire, but since pip is not doing that we probably should avoid that as well.

[hartikainen]

The google api python client libraries have the same issue:

Details

py_test(
    name = "google_test",
    srcs = ["google_test.py"],
    deps = [
        "@pypi//google_api_core",
        "@pypi//google_api_python_client",
        "@pypi//google_auth",
        "@pypi//google_auth_httplib2",
        "@pypi//google_cloud_aiplatform",
        "@pypi//google_cloud_bigquery",
        "@pypi//google_cloud_core",
        "@pypi//google_cloud_resource_manager",
        "@pypi//google_cloud_storage",
        "@pypi//google_crc32c",
        "@pypi//google_genai",
        "@pypi//google_pasta",
        "@pypi//google_resumable_media",
    ],
)

"""A simple `google.api` test."""

import google.api

ERROR: One of the output paths 'bazel-out/darwin_arm64-fastbuild/bin/_google_test.venv/lib/python3.12/site-packages/google/cloud' (belonging to //:google_test) and 'bazel-out/darwin_arm64-fastbuild/bin/_google_test.venv/lib/python3.12/site-packages/google/cloud/aiplatform_v1beta1' (belonging to //:google_test) is a prefix of the other. These actions cannot be simultaneously present; please rename one of the output files or build just one of them
ERROR: One of the output paths 'bazel-out/darwin_arm64-fastbuild/bin/_google_test.venv/lib/python3.12/site-packages/google/cloud' (belonging to //:google_test) and 'bazel-out/darwin_arm64-fastbuild/bin/_google_test.venv/lib/python3.12/site-packages/google/cloud/storage' (belonging to //:google_test) is a prefix of the other. These actions cannot be simultaneously present; please rename one of the output files or build just one of them
ERROR: One of the output paths 'bazel-out/darwin_arm64-fastbuild/bin/_google_test.venv/lib/python3.12/site-packages/google/cloud' (belonging to //:google_test) and 'bazel-out/darwin_arm64-fastbuild/bin/_google_test.venv/lib/python3.12/site-packages/google/cloud/aiplatform' (belonging to //:google_test) is a prefix of the other. These actions cannot be simultaneously present; please rename one of the output files or build just one of them
ERROR: One of the output paths 'bazel-out/darwin_arm64-fastbuild/bin/_google_test.venv/lib/python3.12/site-packages/google/cloud' (belonging to //:google_test) and 'bazel-out/darwin_arm64-fastbuild/bin/_google_test.venv/lib/python3.12/site-packages/google/cloud/resourcemanager_v3' (belonging to //:google_test) is a prefix of the other. These actions cannot be simultaneously present; please rename one of the output files or build just one of them
ERROR: One of the output paths 'bazel-out/darwin_arm64-fastbuild/bin/_google_test.venv/lib/python3.12/site-packages/google/cloud' (belonging to //:google_test) and 'bazel-out/darwin_arm64-fastbuild/bin/_google_test.venv/lib/python3.12/site-packages/google/cloud/resourcemanager' (belonging to //:google_test) is a prefix of the other. These actions cannot be simultaneously present; please rename one of the output files or build just one of them
ERROR: One of the output paths 'bazel-out/darwin_arm64-fastbuild/bin/_google_test.venv/lib/python3.12/site-packages/google/cloud' (belonging to //:google_test) and 'bazel-out/darwin_arm64-fastbuild/bin/_google_test.venv/lib/python3.12/site-packages/google/cloud/aiplatform_v1' (belonging to //:google_test) is a prefix of the other. These actions cannot be simultaneously present; please rename one of the output files or build just one of them
ERROR: One of the output paths 'bazel-out/darwin_arm64-fastbuild/bin/_google_test.venv/lib/python3.12/site-packages/google/cloud' (belonging to //:google_test) and 'bazel-out/darwin_arm64-fastbuild/bin/_google_test.venv/lib/python3.12/site-packages/google/cloud/bigquery' (belonging to //:google_test) is a prefix of the other. These actions cannot be simultaneously present; please rename one of the output files or build just one of them
ERROR: One of the output paths 'bazel-out/darwin_arm64-fastbuild/bin/_google_test.venv/lib/python3.12/site-packages/google/cloud' (belonging to //:google_test) and 'bazel-out/darwin_arm64-fastbuild/bin/_google_test.venv/lib/python3.12/site-packages/google/cloud/bigquery_v2' (belonging to //:google_test) is a prefix of the other. These actions cannot be simultaneously present; please rename one of the output files or build just one of them
Target //:google_test failed to build

[rickeylev]

Ideally the mujoco package should have the init file and the plugins don't have it. That way there is no ambiguity here. If I remember correctly that is the approach airflow has taken.

With how pkgutil-style namespace packages work, such is not the case. Each distribution is supposed to have the namespace-establishing __init__.py file. This ensures that, regardless of the order of sys.path (i.e which distribution is first imported), and regardless of their install location (separate directories or the same site-packages directory) the namespace package is setup appropriately.

TIL pip defines an install order, that's good to know. Are we able to replicate that, though? We switched off topological ordering because it didn't give the "closer target wins" semantics we wanted, which is fine; I think giving up that goal for a better defined "install" sequencing makes sense.

[groodt]

TIL pip defines an install order

I do not believe this is the case and I don't believe it's a spec that would be replicated across installers. Do you have any links to any references that claim pip has an installation order?

edit: never mind, I see it in the thread 😂

[groodt]

Ah, I see the issue. These packages are attempting to use Legacy Namespace Packages incorrectly. The fact that they work is likely accidental.

For a pkg-util style of namespace packages the __init__.py can ONLY contain the following:

__path__ = __import__('pkgutil').extend_path(__path__, __name__)

It should not include any other code: https://github.com/pypa/sample-namespace-packages/tree/master/pkgutil

Mujoco and mujoco-mjx do not follow that guidance:
https://pypi-browser.org/package/mujoco/mujoco-3.3.5-cp310-cp310-macosx_10_16_x86_64.whl/mujoco/__init__.py#L34

[groodt]

It looks like mujoco should consider if it wants to use native namespace packages as defined in PEP420 (the modern style) or the legacy pkgutil style. Right now, it's using a mix.

The Packaging Implications section outlines how the namespace packages should be installed and removed.

[rickeylev]

mujoco is using a mix of native and pkgutil namespace packages
mujoco's __init__.py has a bunch of extra non-namespace package initializing code in it

Ahhh, yeah, that's problematic. The only way this can work is if, somehow, it's known that mujoco's file is supposed to have precedence. pip using topological order is a good heuristic, but not a guarantee, of the right order occurring.

I think the best we can come to Just Works is to replicate the topological ordering. Otherwise, we simply have to pick one file or the other and hope for the best. A half measure might be file size 😅 ? (larger file means higher precedence). Maybe we could preserve the ordering information from requirements.txt? (the order of packages in requirements.txt is the order things get installed in, right?)

In any case, I think the first step is the better conflict and merge logic Ignas and I have previously discussed. That would prevent this error. For well formed packages (e.g. all the init files are the same pkgutil init), it'll work regardless of ordering. For mujoco (i.e. pkgutil with real code, plugins omitting init.py entirely), it'll also end up working.

[aignas]

To whomever is doing this, I think we would have to:

• Pull out some of the code that is creating the venv from py_executable.bzl (and maybe py_library.bzl) - right now the code is internal and hard to test other than integration tests.
• We should have simple unit tests for all the behaviour:
  • No overlap in dirs - we symlink top level dirs
  • Overlap in dirs but no conflicting files - we symlink non conflicts subtrees
  • Overlap in files - we use topological sorting when merging the files.
• maybe we should have venv_runfiles.bzl file that has all of the code related to this?

[rickeylev]

I started working on this last night. Have something almost working, mostly just needs some cleanup in py_library. It implements basic merging; no topological. It works for the mujoco case (top-level is pkgutil, sub-packages are native namespace).

I did find that the immediate cause of the KeyError should be easy to fix. The code is basically doing if key in map: map.pop(key + "/") -- the logic just has to be updated to pop the key prior to appending "/".

That said, that wouldn't fix the problem of a mixture of pkgutil and native namespace packages. That's the part that requires merging and is the logic I've implemented.

create venv_runfiles.bzl and test that

Yeah, that's a good idea. There's basically just two functions to test: py_library's PyInfo.venv_symlinks construction, and py_executable's _build_link_map function. IIRC, neither uses ctx, so factoring them out and unit testing them should be pretty straight forward.