Always narrow markers by Python version by charliermarsh · Pull Request #6076 · astral-sh/uv

charliermarsh · 2024-08-14T01:00:35Z

Summary

Using #6064 as a motivating example: at present, on main, we're not properly propagating the Requires-Python simplifications. In that case, for example, we end up solving for a branch with python_version < 3.11, and a branch >= 3.11, even though Requires-Python is >=3.11. Later, when we get to the graph, we apply version simplification based on Requires-Python, which causes us to remove the python_version < 3.11 markers entirely, leaving us with duplicate dependencies for pylint.

This PR instead tries to ensure that we always apply this narrowing to requirements and forks, so that we don't need to apply the same simplification when constructing the graph at all.

Closes #6064.

Closes #6059.

charliermarsh · 2024-08-14T01:06:52Z

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

+                    let not_markers = simplify_python(markers.negate(), python_requirement);
                    let mut new_markers = markers.clone();
-                    new_markers.and(fork.markers.negate());
+                    new_markers.and(simplify_python(fork.markers.negate(), python_requirement));


Honestly, I can't figure out if the simplify_python here is necessary. It's definitely necessary on not_markers above... But I'm not sure about here. I don't think any tests change when I remove it.

Logically, I think this simplification here is right and even necessary. Because while fork.markers, by construction, should already be simplified, negating that here might wind up with a marker that includes (or is completely excluded by) requires-python. So re-doing the simplification does indeed seem correct.

I think that the same simplification ought to happen to markers too right? Specifically, before passing it to fork.intersect below.

I thought markers was already simplified because we simplify when generating PubGrubDependency.

I.e., here: https://github.com/astral-sh/uv/pull/6076/files#diff-6be6d80fe4821c47b70a372260f55e73b8da8182b8dcad7525d5cd3eb584532bR1523

Talking over DM, the markers here are already simplified.

ibraheemdev · 2024-08-14T02:11:12Z

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

-                        // Requires-Python specifier.
-                        requires_python = requires_python.unwrap()
+                let requirement = if let Some(requires_python) = python_requirement.target().and_then(|target| target.as_requires_python()).filter(|_| !requirement.marker.is_true()) {
+                    let marker = requirement.marker.clone().simplify_python_versions(


Hmm.. I'm surprised this works. It's important to note that calling simplify_python_versions creates a different marker tree than the same tree parsed from the lockfile. For example, if we restrict the range of python_version >= 3.12 given requires-python >= 3.9, we now get the marker tree [3.9,3.12)=>false,[3.12,inf)=>true. If you parse that same expression from the lockfile, you get [0,3.12)=>false,[3.12,inf)=>true, which is not equal. This can cause false negatives in a --locked operation. Current we solve this by reapplying the requires-python restriction after parsing the lockfile. However, I believe this PR now restricts markers to the narrowed python requirement, which we do not reapply, so I'm surprised we aren't seeing failing test cases. Is there something I'm missing and can we remove the extra code in Lock::from_toml now?

That is... interesting. If I remove that code from from_toml, lock_conditional_dependency_extra fails (for one).

It may just be that this doesn't work (for that particular problem) and we just don't have a proper test case for it.

Any other ideas on how we can solve this problem?

I think we could fix this by removing the whole idea of a restricted range with a special "rest" range, and then only apply simplification when displaying a marker, using disjointness checks instead for requires-python filtering. I was hoping we could use restriction to our advantage but it seems that may not be possible with narrowing.

I guess the other option is: try to write this to the lockfile.

Can we use environment-markers to try and enforce this narrowing when we deserialize the lockfile?

Maybe.. how would we map those to requirements?

Could we instead just retain the lower-bound, like >=3.7, <3.10 instead of using <3.10? Does that cause other problems?

Ok @ibraheemdev -- I added a failing test for this.

crates/uv/tests/lock.rs

charliermarsh · 2024-08-14T14:44:08Z

I think something like this is critical (it fixes a bunch of issues), but need some solution to the problem Ibraheem brought up. Let me try to write a test for it.

charliermarsh · 2024-08-14T15:04:41Z

crates/uv/tests/lock.rs

+    warning: `uv lock` is experimental and may change without warning
+    warning: `uv.sources` is experimental and may change without warning
+    Resolved 3 packages in [TIME]
+    error: The lockfile at `uv.lock` needs to be updated, but `--locked` was provided. To update the lockfile, run `uv lock`.


This instability is due to the problem @ibraheemdev mentioned.

charliermarsh · 2024-08-14T15:11:25Z

@ibraheemdev -- Maybe an alternative idea... Could we add requires-python to Dependency? And only set it when it differs from the top-level?

charliermarsh · 2024-08-14T15:13:30Z

Hmm, but that might not cover fork_markers, just dependency markers.

charliermarsh · 2024-08-14T22:27:55Z

I think we can safely do this if we merge #6091?

) ## Summary This PR changes the definition of `--locked` from: > Produces the same `Lock` To: > Passes `Lock::satisfies` This is a subtle but important difference. Previous, if `Lock::satisfies` failed, we would run a resolution, then do `existing_lock == lock`. If the two weren't equal, and `--locked` was specified, we'd throw an error. The equality check is hard to get right. For example, it means that we can't ship #6076 without changing our marker representation, since the deserialized lockfile "loses" some of the internal marker state that gets accumulated during resolution. The downside of this change is that there could be scenarios in which `uv lock --locked` fails even though the lockfile would actually work and the exact TOML would be unchanged. But... I think it's ok if `--locked` fails after the user modifies something?

T-256 · 2024-08-15T12:29:13Z

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

-    "python_version >= '3.12' and python_version < '3.13'",
+    "python_version < '3.12'",
+    "python_version < '3.13'",
    "python_version >= '3.13'",


shouldn't this remove too? since it doesn't fill in >=3.11, <3.12 required range.

We don't respect upper-bounds on requires-python.

BurntSushi

I think I found one possible additional simplification that should be done, but otherwise I think this LGTM assuming @ibraheemdev's concern is addressed. (I think it is now with #6102 merged?)

ibraheemdev

This looks good to me now that we no longer rely on marker equality for --locked.

## Summary This is no longer required since we no longer implement `Eq` on `Lock`. It will also sometimes be "wrong" as of #6076, since we now apply different `requires-python` filtering to different parts of the tree during resolution.

charliermarsh added bug Something isn't working preview Experimental behavior labels Aug 14, 2024

charliermarsh requested review from BurntSushi and ibraheemdev August 14, 2024 01:05

charliermarsh commented Aug 14, 2024

View reviewed changes

charliermarsh force-pushed the charlie/py-fork branch 3 times, most recently from 5f09f9b to 817ff9f Compare August 14, 2024 01:11

charliermarsh mentioned this pull request Aug 14, 2024

Propagate fork markers to extras #6065

Merged

ibraheemdev reviewed Aug 14, 2024

View reviewed changes

charliermarsh force-pushed the charlie/py-fork branch 3 times, most recently from 6542938 to a838135 Compare August 14, 2024 14:10

charliermarsh commented Aug 14, 2024

View reviewed changes

crates/uv/tests/lock.rs Outdated Show resolved Hide resolved

charliermarsh force-pushed the charlie/py-fork branch from a838135 to 07acef6 Compare August 14, 2024 14:33

charliermarsh commented Aug 14, 2024

View reviewed changes

charliermarsh force-pushed the charlie/py-fork branch from 8cf3649 to 3c213ed Compare August 14, 2024 15:06

charliermarsh force-pushed the charlie/py-fork branch from 3c213ed to 5c2ddaf Compare August 15, 2024 00:17

charliermarsh requested a review from ibraheemdev August 15, 2024 00:18

charliermarsh force-pushed the charlie/py-fork branch from d22125a to 8bf6ed1 Compare August 15, 2024 01:01

charliermarsh mentioned this pull request Aug 15, 2024

Change the definition of --locked to require satisfaction check #6102

Merged

T-256 reviewed Aug 15, 2024

View reviewed changes

BurntSushi approved these changes Aug 15, 2024

View reviewed changes

ibraheemdev approved these changes Aug 15, 2024

View reviewed changes

Always narrow markers by Python version

afccd61

charliermarsh added 2 commits August 15, 2024 11:28

Add failing test

044bf3e

Remove dupe

35aac88

charliermarsh force-pushed the charlie/py-fork branch from 8bf6ed1 to 35aac88 Compare August 15, 2024 15:36

charliermarsh mentioned this pull request Aug 15, 2024

Remove requires-python application in lock deserialization #6115

Merged

charliermarsh merged commit fe0b873 into main Aug 15, 2024

charliermarsh deleted the charlie/py-fork branch August 15, 2024 15:50

Conversation

charliermarsh commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibraheemdev Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

charliermarsh commented Aug 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charliermarsh commented Aug 14, 2024

Uh oh!

charliermarsh commented Aug 14, 2024

Uh oh!

charliermarsh commented Aug 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BurntSushi left a comment

Choose a reason for hiding this comment

Uh oh!

ibraheemdev left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

charliermarsh commented Aug 14, 2024 •

edited

Loading

ibraheemdev Aug 14, 2024 •

edited

Loading