Warn when two packages write to the same module#13437
Conversation
| modules: Mutex<FxHashMap<OsString, WheelFilename>>, | ||
| } | ||
|
|
||
| impl Locks { | ||
| /// Warn when a module exists in multiple packages. | ||
| fn warn_module_conflict(&self, module: &OsStr, filename: &WheelFilename) { | ||
| if let Some(existing) = self | ||
| .modules | ||
| .lock() | ||
| .unwrap() | ||
| .insert(module.to_os_string(), filename.clone()) |
There was a problem hiding this comment.
This should get <1000 hits so I've implemented the naive approach.
|
An example that's vexed me in the past:
both install to |
1dce177 to
b052c16
Compare
|
I don't know that we should make this user-facing, to be honest. Isn't it going to trigger any time anyone installs Jupyter? |
|
I strongly think this should be user-facing, I linked some of the issues this would have prevented above (we had another one just today: #13550), especially since this is otherwise non-deterministic, silent and almost impossible to figure out if you don't know what you're looking for. It doesn't warn for |
|
Hey @charliermarsh, I'm the author of #13550. Thanks to @konstin I found the issue but I would strongly side with here that especially for less experienced users this can lead to very frustrating problems. A warning during the install would have most likely prevented it for me. Thanks! |
b052c16 to
8bcf72a
Compare
|
Added a test case |
|
One place where this could fail is the Python 2 |
|
(I'll review this) |
crates/uv/tests/it/pip_install.rs
Outdated
| ----- stderr ----- | ||
| Resolved 2 packages in [TIME] | ||
| Prepared 2 packages in [TIME] | ||
| warning: The module built_by_uv exists in two packages! This leads to a race condition and likely to a broken installation. Consider removing either built-by-uv (built_by_uv-0.1.0-py3-none-any.whl) or also-built-by-uv (also_built_by_uv-0.1.0-py3-none-any.whl). |
There was a problem hiding this comment.
Why do we show the wheel filenames here? Is that an important detail?
There was a problem hiding this comment.
For debugging, I usually need to go open the wheel and look inside of it, so I consider this a valuable detail. We should show at least the version though, otherwise we can't find where this is from.
There was a problem hiding this comment.
There was a problem hiding this comment.
I worry about this as a user-facing detail in the warning though, isn't the file name going to be in the verbose logs?
There was a problem hiding this comment.
Also, aren't the versions included in the install summary?
There was a problem hiding this comment.
The error should by itself contain enough information to track down the problem, so I would like it to at least contain the versions.
There was a problem hiding this comment.
I'm okay with the versions, styled as we do elsewhere, e.g., foo (v1.0.0). I think the filenames sound like a developer-facing rather than user-facing detail.
There was a problem hiding this comment.
The exclamation mark is a bit much, I don't think we do that anywhere else.
There was a problem hiding this comment.
I updated the error message
| (wheel_a, &wheel_b) | ||
| }; | ||
| warn_user!( | ||
| "The module {} exists in two packages! \ |
There was a problem hiding this comment.
I reworded this in #13889, curious for your thoughts
There was a problem hiding this comment.
Will this trigger if two packages include the same file?
There was a problem hiding this comment.
Yes, do we have a case where this is expected and valid to happen?
There was a problem hiding this comment.
Isn't it a feature that two packages could contain the same shared module, and could just vendor it into their own wheel?
There was a problem hiding this comment.
Is it? That seems pretty problematic from a versioning perspective.
There was a problem hiding this comment.
I haven't seen any vendoring cases where the vendored modules would overlap.
| && relative.components().next_back().unwrap().as_os_str() == "__init__.py" | ||
| { | ||
| // Modules must be UTF-8, but we can skip the conversion using OsStr. | ||
| locks.warn_module_conflict(relative.components().next().unwrap().as_os_str(), filename); |
There was a problem hiding this comment.
What does this look like for modules like foo.bar? Are we only going to show bar? Or are you only checking for the top-level modules because of relative.components.count() == 2?
There was a problem hiding this comment.
That's the TODO above, we're only looking for foo/__init__.py rn and don't track foo/bar/__init__.py
CodSpeed WallTime Performance ReportMerging #13437 will not alter performanceComparing Summary
|
|
Suggested solution: I think it should be possible for the user to define different packages under the same module name similar to the sources logic. Something similar to this: |
This comment was marked as outdated.
This comment was marked as outdated.
dcefc54 to
6f9e320
Compare
d31f63c to
f18ca49
Compare
|
I sort of thought I've seen pre-PEP-420 namespace packages recently enough. Debian Code Search for [edit: meant to write "pre-PEP-420", not "PEP-420"] |
|
Valid PEP 420 packages that don't clash are not affected by this change. |
In the previous iteration, conflict detection was based on top level modules. This would work if all namespace packages correctly omitted the `__init__.py`, but e.g. the nvidia packages include an empty `nvida/__init__.py`. Instead, we track overlapping top level modules during installation and perform conflict analysis after installation, recursing only as far as necessary. See #13437 for a list of reported conflicts. Before: ``` $ uv venv -c -q && uv pip install --preview nvidia-nvjitlink-cu12==12.8.93 nvidia-nvtx-cu12==12.8.90 Resolved 2 packages in 0.99ms ░░░░░░░░░░░░░░░░░░░░ [0/2] Installing wheels... warning: The module `nvidia` is provided by more than one package, which causes an install race condition and can result in a broken module. Consider removing your dependency on either `nvidia-nvtx-cu12` (v12.8.90) or `nvidia-nvjitlink-cu12` (v12.8.93). Installed 2 packages in 3ms + nvidia-nvjitlink-cu12==12.8.93 + nvidia-nvtx-cu12==12.8.90 ``` After: ``` $ uv venv -c -q && cargo run -q pip install --preview nvidia-nvjitlink-cu12==12.8.93 nvidia-nvtx-cu12==12.8.90 Resolved 2 packages in 3ms Installed 2 packages in 4ms + nvidia-nvjitlink-cu12==12.8.93 + nvidia-nvtx-cu12==12.8.90 ``` Still detected true positive: ``` $ uv venv -c -q && cargo run -q pip install --no-progress opencv-python opencv-contrib-python --no-build --no-deps --preview Resolved 2 packages in 5ms warning: The file `cv2/__init__.pyi` is provided by more than one package, which causes an install race condition and can result in a broken module. Packages containing the file: * opencv-contrib-python (opencv_contrib_python-4.13.0.90-cp37-abi3-manylinux_2_28_x86_64.whl) * opencv-python (opencv_python-4.13.0.90-cp37-abi3-manylinux_2_28_x86_64.whl) Installed 2 packages in 6ms + opencv-contrib-python==4.13.0.90 + opencv-python==4.13.0.90 ```
We regularly get confusing bug reports where a package sometimes works and sometimes doesn't and it's not clear to the user why. Ultimately, it turns out that two packages contain the same module and there is a race condition when installing the two packages. Usually, it's one of the opencv-python distributions, but recently it's been z3, too. These error are completely inscrutable to users.
opencv-python-headlessinstalled via uv but not via pip #11806uv pip#13435uv syncremoves necessary dependencies when two packages include the same module #15238uv syncremoves base dependencies when switching extras (at least impacts typer/typer-slim) #17645We now warn for top-level modules (pattern:
<identifier>/__init__.py) that collide in a single installation, naming the offending wheels. Checking for__init__.pyexcludes namespace packages.Test script:
We currently only catch conflicts in a single installation. Should we prime the lock database with the site-packages contents, and would that carry overhead?