uv-resolver: fix marker propagation #5597

BurntSushi · 2024-07-30T12:42:26Z

This PR represents a different approach to marker propagation in an
attempt to unblock #5583. In particular, instead of propagating markers
when forks are created, we wait until resolution is complete to
propagate all markers to all dependencies in each fork. This ends up
being both more robust (we should never miss anything) and simpler to
implement because it doesn't require mutating a PubGrubPackage (which
was pretty annoying). I think the main downside here is that this can
sometimes add markers where they aren't needed.

This actually winds up making quite a few snapshot changes. I went
through each of them. Some of them look like legitimate bug fixes. Some
of them look like superfluous additions. And some of them look like they
would be removed if we had perfect marker normalization. But I don't
think any of the changes are wrong.

This PR represents a different approach to marker propagation in an attempt to unblock #4640. In particular, instead of propagating markers when forks are created, we wait until resolution is complete to propagate all markers to all dependencies in each fork. This ends up being both more robust (we should never miss anything) and simpler to implement because it doesn't require mutating a `PubGrubPackage` (which was pretty annoying). I think the main downside here is that this can sometimes add markers where they aren't needed. This actually winds up making quite a few snapshot changes. I went through each of them. Some of them look like legitimate bug fixes. Some of them look like superfluous additions. And some of them look like they would be removed if we had perfect marker normalization. But I don't think any of the changes are _wrong_.

BurntSushi · 2024-07-30T12:44:44Z

crates/uv/tests/branching_urls.rs

-        { name = "idna" },
-        { name = "sniffio" },
+        { name = "idna", marker = "python_version < '3.12'" },
+        { name = "sniffio", marker = "python_version < '3.12'" },


I think these are superfluous because the dependency on anyio from a above is already gated on marker expressions.

(I think this change overall brings us closer to "write out the full marker expressions for each distribution" that could potentially obviate the need for a DAG traversal.)

BurntSushi · 2024-07-30T12:45:32Z

crates/uv/tests/lock.rs

-                { name = "typing-extensions", marker = "(python_full_version <= '3.10' and python_version == '3.10') or (python_full_version <= '3.10' and python_version < '3.10') or (python_full_version <= '3.10' and python_version < '3.10' and python_version > '3.10') or (python_full_version <= '3.10' and python_version > '3.10') or (python_full_version > '3.10' and python_version == '3.10') or (python_full_version > '3.10' and python_version < '3.10') or (python_full_version > '3.10' and python_version < '3.10' and python_version > '3.10') or (python_full_version > '3.10' and python_version > '3.10')" },
+                { name = "attrs" },
+                { name = "iniconfig" },
+                { name = "typing-extensions", marker = "python_full_version <= '3.10' or python_full_version > '3.10'" },


I believe these are all correct simplifications from what was here previously.

BurntSushi · 2024-07-30T12:47:07Z

crates/uv/tests/lock_scenarios.rs

@@ -582,7 +582,7 @@ fn fork_filter_sibling_dependencies() -> Result<()> {
        version = "1.0.0"
        source = { registry = "https://astral-sh.github.io/packse/PACKSE_VERSION/simple-html/" }
        dependencies = [
-            { name = "package-d", version = "2.0.0", source = { registry = "https://astral-sh.github.io/packse/PACKSE_VERSION/simple-html/" } },
+            { name = "package-d", version = "2.0.0", source = { registry = "https://astral-sh.github.io/packse/PACKSE_VERSION/simple-html/" }, marker = "sys_platform == 'darwin'" },


I think this one and the one above are superfluous (again, assuming a DAG traversal).

BurntSushi · 2024-07-30T12:51:42Z

crates/uv/tests/lock_scenarios.rs

-            { name = "package-foo", version = "1.0.0", source = { registry = "https://astral-sh.github.io/packse/PACKSE_VERSION/simple-html/" } },
-            { name = "package-foo", version = "2.0.0", source = { registry = "https://astral-sh.github.io/packse/PACKSE_VERSION/simple-html/" } },
+            { name = "package-foo", version = "1.0.0", source = { registry = "https://astral-sh.github.io/packse/PACKSE_VERSION/simple-html/" }, marker = "sys_platform == 'linux'" },
+            { name = "package-foo", version = "2.0.0", source = { registry = "https://astral-sh.github.io/packse/PACKSE_VERSION/simple-html/" }, marker = "sys_platform != 'linux'" },


This one looks like a legitimate bug fix, since previous we had unconditional dependencies on two different versions of package-foo here. I filed #5598 in reaction to this.

BurntSushi · 2024-07-30T12:52:10Z

crates/uv/tests/pip_compile.rs

@@ -6612,7 +6612,7 @@ fn universal_conflicting() -> Result<()> {
        # via trio
    outcome==1.3.0.post0 ; sys_platform == 'darwin' or sys_platform == 'win32'
        # via trio
-    pycparser==2.21 ; (implementation_name != 'pypy' and os_name == 'nt' and sys_platform == 'darwin') or (os_name == 'nt' and sys_platform == 'win32')
+    pycparser==2.21 ; (sys_platform == 'darwin' or sys_platform == 'win32') and ((implementation_name != 'pypy' and os_name == 'nt' and sys_platform == 'darwin') or (os_name == 'nt' and sys_platform == 'win32'))


I believe the extra term added here is strictly redundant.

BurntSushi · 2024-07-30T12:52:30Z

crates/uv/tests/pip_compile.rs

        # via
        #   -r requirements.in
        #   torchvision
-    torch==2.0.0+cu118
+    torch==2.0.0+cu118 ; platform_machine == 'x86_64'


This looks like another legitimate bug fix, where previously we could install two different versions of torch.

BurntSushi · 2024-07-30T12:52:48Z

crates/uv/tests/pip_compile.rs

        # via
        #   -r requirements.in
        #   example
-    cffi==1.17.0rc1
+    cffi==1.17.0rc1 ; os_name == 'Linux'


Another bug fix!

charliermarsh approved these changes Jul 30, 2024

View reviewed changes

konstin approved these changes Jul 30, 2024

View reviewed changes

BurntSushi mentioned this pull request Jul 30, 2024

add test infrastructure for ensuring none of our universal locks can produce a dependency tree with multiple versions of the same package for the same marker environment #5598

Closed

BurntSushi commented Jul 30, 2024

View reviewed changes

BurntSushi merged commit 4e74836 into main Jul 30, 2024
57 checks passed

BurntSushi deleted the ag/fix-marker-propagation branch July 30, 2024 13:16

This was referenced Jul 30, 2024

Re-enable requires-python narrowing in forks #5583

Merged

Diverging versions without markers #5344

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

uv-resolver: fix marker propagation #5597

uv-resolver: fix marker propagation #5597

BurntSushi commented Jul 30, 2024 •

edited

Loading

BurntSushi Jul 30, 2024

BurntSushi Jul 30, 2024

BurntSushi Jul 30, 2024

BurntSushi Jul 30, 2024

BurntSushi Jul 30, 2024

BurntSushi Jul 30, 2024

BurntSushi Jul 30, 2024

uv-resolver: fix marker propagation #5597

uv-resolver: fix marker propagation #5597

Conversation

BurntSushi commented Jul 30, 2024 • edited Loading

BurntSushi Jul 30, 2024

Choose a reason for hiding this comment

BurntSushi Jul 30, 2024

Choose a reason for hiding this comment

BurntSushi Jul 30, 2024

Choose a reason for hiding this comment

BurntSushi Jul 30, 2024

Choose a reason for hiding this comment

BurntSushi Jul 30, 2024

Choose a reason for hiding this comment

BurntSushi Jul 30, 2024

Choose a reason for hiding this comment

BurntSushi Jul 30, 2024

Choose a reason for hiding this comment

BurntSushi commented Jul 30, 2024 •

edited

Loading