Propagate fork markers to extras #6065

charliermarsh · 2024-08-13T18:01:38Z

Summary

When constructing the Resolution, we only propagated the fork markers to the package node, but not the extras node. This led to cases in which an extra could be included unconditionally or otherwise diverge from the base package version.

Closes #6062.

charliermarsh · 2024-08-13T18:05:09Z

This isn't quite doing the right thing in lock_python_version_marker_complement. I'm getting an instability there.

charliermarsh · 2024-08-13T18:07:35Z

I think my change is correct but now hitting the aforementioned instability.

charliermarsh · 2024-08-13T18:07:57Z

crates/uv-resolver/src/resolver/mod.rs

-                            // true), but is a more robust approach that should
-                            // capture all cases.
-                            marker: self.markers.fork_markers().cloned().unwrap_or_default(),
+                            marker: MarkerTree::TRUE,


I moved this into graph construction, since we're now AND-ing the fork markers with every edge.

charliermarsh · 2024-08-13T18:11:06Z

crates/uv/tests/lock_scenarios.rs

-            { name = "package-b", version = "1.0.0", source = { registry = "https://astral-sh.github.io/packse/PACKSE_VERSION/simple-html/" }, marker = "implementation_name == 'pypy'" },
-            { name = "package-b", version = "2.0.0", source = { registry = "https://astral-sh.github.io/packse/PACKSE_VERSION/simple-html/" }, marker = "implementation_name == 'cpython'" },
+            { name = "package-b", version = "1.0.0", source = { registry = "https://astral-sh.github.io/packse/PACKSE_VERSION/simple-html/" }, marker = "implementation_name == 'pypy' and sys_platform == 'darwin'" },
+            { name = "package-b", version = "2.0.0", source = { registry = "https://astral-sh.github.io/packse/PACKSE_VERSION/simple-html/" }, marker = "implementation_name == 'cpython' and sys_platform == 'darwin'" },


These may not be strictly necessary... Because you can only reach this node on sys_platform == 'darwin'.

(Yeah confirmed. Is it wrong to include then?)

I don't think it's wrong, but it does seem superfluous. But I'm okay to run with it for now.

BurntSushi

I think this looks right to me.

In terms of moving this PR forward, maybe I cherry pick this PR into my branch that will (hopefully) fix the instability issue? So it won't get merged right away, but it will be in an active branch somewhere.

charliermarsh · 2024-08-13T18:15:50Z

crates/uv/tests/snapshots/ecosystem__transformers-lock-file.snap

@@ -1901,8 +1897,8 @@ dependencies = [
    { name = "packaging" },
    { name = "regex" },
    { name = "rich" },
-    { name = "tensorflow-text", version = "2.7.3", source = { registry = "https://pypi.org/simple" }, marker = "platform_system != 'Darwin'" },
-    { name = "tensorflow-text", version = "2.15.0", source = { registry = "https://pypi.org/simple" }, marker = "platform_system != 'Darwin'" },


Ouch, not good!

charliermarsh · 2024-08-13T18:15:58Z

crates/uv/tests/snapshots/ecosystem__transformers-lock-file.snap

-    { name = "tensorflow-text", version = "2.7.3", source = { registry = "https://pypi.org/simple" }, marker = "platform_system != 'Darwin'" },
-    { name = "tensorflow-text", version = "2.15.0", source = { registry = "https://pypi.org/simple" }, marker = "platform_system != 'Darwin'" },
+    { name = "tensorflow-text", version = "2.7.3", source = { registry = "https://pypi.org/simple" }, marker = "python_version < '3.13' and platform_system != 'Darwin'" },
+    { name = "tensorflow-text", version = "2.15.0", source = { registry = "https://pypi.org/simple" }, marker = "python_version >= '3.13' and platform_system != 'Darwin'" },


So that's a bugfix.

Yup! ANd there's no extra here.

charliermarsh · 2024-08-13T18:18:10Z

crates/uv/tests/snapshots/ecosystem__warehouse-lock-file.snap

@@ -3144,7 +3144,7 @@ name = "redis"
 version = "5.0.8"
 source = { registry = "https://pypi.org/simple" }
 dependencies = [
-    { name = "async-timeout", marker = "python_full_version < '3.11.[X]'" },
+    { name = "async-timeout", marker = "python_full_version < '3.11.[X]' and python_version < '3.12'" },


This looks wrong. I don't know where it's coming from.

Debugging this.

I haven’t figured this out yet, I think I should understand why this is happening before we merge @BurntSushi though you’re welcome to cherry-pick.

Oh... I guess this is actually fine.

It's just redundant.

I needed to see it with fresh eyes.

charliermarsh · 2024-08-13T22:19:54Z

Very hard to explain but regarding the instability: it has to do with the fact that when we resolve without a lockfile, we create the fork on the dependencies of the project. So we iterate over all dependencies, then create forks.

When we resolve with a lockfile, we fork on the project itself... So when we go to solve for that project's dependencies, we already have a requires-python marker which we then apply to filter out certain dependencies.

charliermarsh · 2024-08-13T22:23:03Z

That might need to be solved holistically, though fixing the normalization to detect disjointness for these would also help.

charliermarsh · 2024-08-13T22:23:59Z

Basically, we end up with this bad fork due to the confusion:

DEBUG skipping iniconfig ; python_version >= '3.10' because of context resolver markers python_full_version > '3.10' and python_version < '3.10'
DEBUG skipping iniconfig ; python_version < '3.10' because of Requires-Python python_full_version > '3.10' and python_version > '3.10'

charliermarsh · 2024-08-14T01:13:23Z

Rebasing on #6076 does fix one of the instabilities, but github_wikidata_bot is still unstable...

BurntSushi · 2024-08-14T13:37:58Z

but github_wikidata_bot is still unstable

Is this the only issue? If so, I'd say flip this test to non-deterministic (like transformers) via lock_ecosystem_package_non_deterministic and merge it. That way we're working from a common base and because this PR fixes bugs where there are multiple versions of the same package for the same marker environment in the lock file.

charliermarsh · 2024-08-14T13:40:41Z

Yeah will do. lock_python_version_marker_complement and lock_conditional_dependency_extra also became non-deterministic, but some of those are fixed upstream I think. I'm gonna document and merge.

charliermarsh added bug Something isn't working resolver Related to the package resolver preview Experimental behavior labels Aug 13, 2024

charliermarsh requested a review from BurntSushi August 13, 2024 18:07

charliermarsh commented Aug 13, 2024

View reviewed changes

charliermarsh force-pushed the charlie/ex branch from 20be943 to d705164 Compare August 13, 2024 18:08

charliermarsh commented Aug 13, 2024

View reviewed changes

BurntSushi approved these changes Aug 13, 2024

View reviewed changes

charliermarsh commented Aug 13, 2024

View reviewed changes

charliermarsh mentioned this pull request Aug 13, 2024

uv lock sometimes changes the lock file when run for a second time #6063

Closed

charliermarsh force-pushed the charlie/ex branch 2 times, most recently from 99abf46 to fbefe60 Compare August 14, 2024 01:52

charliermarsh changed the base branch from main to charlie/py-fork August 14, 2024 01:52

Propagate fork markers to extras

9573fa9

charliermarsh changed the base branch from charlie/py-fork to main August 14, 2024 13:37

charliermarsh force-pushed the charlie/ex branch from fbefe60 to b540a6a Compare August 14, 2024 13:41

Simple fix

acb3f31

charliermarsh force-pushed the charlie/ex branch from b540a6a to acb3f31 Compare August 14, 2024 13:46

charliermarsh merged commit 4a902a7 into main Aug 14, 2024
57 checks passed

charliermarsh deleted the charlie/ex branch August 14, 2024 13:55

charliermarsh mentioned this pull request Aug 14, 2024

Remove same-graph merging in resolver #6077

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate fork markers to extras #6065

Propagate fork markers to extras #6065

charliermarsh commented Aug 13, 2024

charliermarsh commented Aug 13, 2024

charliermarsh commented Aug 13, 2024

charliermarsh Aug 13, 2024

charliermarsh Aug 13, 2024

charliermarsh Aug 13, 2024

BurntSushi Aug 13, 2024

BurntSushi left a comment

charliermarsh Aug 13, 2024

charliermarsh Aug 13, 2024

BurntSushi Aug 13, 2024

charliermarsh Aug 13, 2024

charliermarsh Aug 13, 2024

charliermarsh Aug 13, 2024

charliermarsh Aug 13, 2024

charliermarsh Aug 13, 2024

charliermarsh Aug 13, 2024

charliermarsh commented Aug 13, 2024

charliermarsh commented Aug 13, 2024

charliermarsh commented Aug 13, 2024

charliermarsh commented Aug 14, 2024 •

edited

Loading

BurntSushi commented Aug 14, 2024

charliermarsh commented Aug 14, 2024

Propagate fork markers to extras #6065

Propagate fork markers to extras #6065

Conversation

charliermarsh commented Aug 13, 2024

Summary

charliermarsh commented Aug 13, 2024

charliermarsh commented Aug 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BurntSushi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliermarsh commented Aug 13, 2024

charliermarsh commented Aug 13, 2024

charliermarsh commented Aug 13, 2024

charliermarsh commented Aug 14, 2024 • edited Loading

BurntSushi commented Aug 14, 2024

charliermarsh commented Aug 14, 2024

charliermarsh commented Aug 14, 2024 •

edited

Loading