Implement marker trees using algebraic decision diagrams by ibraheemdev · Pull Request #5898 · astral-sh/uv

ibraheemdev · 2024-08-07T23:55:54Z

Summary

This PR rewrites the MarkerTree type to use algebraic decision diagrams (ADD). This has many benefits:

The diagram is canonical for a given marker function. It is impossible to create two functionally equivalent marker trees that don't refer to the same underlying ADD. This also means that any trivially true or unsatisfiable markers are represented by the same constants.
The diagram can handle complex operations (conjunction/disjunction) in polynomial time, as well as constant-time negation.
The diagram can be converted to a simplified DNF form for user-facing output.

The new representation gives us a lot more confidence in our marker operations and simplification, which is proving to be very important (see #5733 and #5163).

Unfortunately, it is not easy to split this PR into multiple commits because it is a large rewrite of the marker module. I'd suggest reading through the marker/algebra.rs, marker/simplify.rs, and marker/tree.rs files for the new implementation, as well as the updated snapshots to verify how the new simplification rules work in practice. However, a few other things were changed:

We now use release-only comparisons for python_full_version, where we previously only did for python_version. I'm unsure how marker operations should work in the presence of pre-release versions if we decide that this is incorrect.
Meaningless marker expressions are now ignored. This means that a marker such as 'x' == 'x' will always evaluate to true (as if the expression did not exist), whereas we previously treated this as always false. It's negation however, remains false.
Unsatisfiable markers are written as python_version < '0'.
The PubGrubSpecifier type has been moved to the new uv-pubgrub crate, shared by pep508-rs and uv-resolver. pep508-rs also depends on the pubgrub crate for the Range type, we probably want to move pubgrub::Range into a separate crate to break this, but I don't think that should block this PR (cc @konstin).

There is still some remaining work here that I decided to leave for now for the sake of unblocking some of the related work on the resolver.

We still use Option<MarkerTree> throughout uv, which is unnecessary now that MarkerTree::TRUE is canonical.
The MarkerTree type is now interned globally and can potentially implement Copy. However, it's unclear if we want to add more information to marker trees that would make it !Copy. For example, we may wish to attach extra and requires-python environment information to avoid simplifying after construction.
We don't currently combine python_full_version and python_version markers.
I also have not spent too much time investigating performance and there is probably some low-hanging fruit. Many of the test cases I did run actually saw large performance improvements due to the markers being simplified internally, reducing the stress on the old normalize routine, especially for the extremely large markers seen in transformers and other projects.

Resolves #5660, #5179.

BurntSushi

This PR is next-level awesome. Solves a huge problem we've been having with marker expressions and solves it well.

What do you think about making MarkerTree::and and MarkerTree::or smart constructors instead of routines that mutate trees in place? Or perhaps that's for another PR, in order to keep the amount of code changed outside of pep508_rs smaller.

crates/pep508-rs/Cargo.toml

BurntSushi · 2024-08-08T16:50:29Z

crates/pep508-rs/src/marker/algebra.rs

+//!   (> '3.7')  -> os_name:
+//!                   (> 'Linux')  -> FALSE
+//!                   (== 'Linux') -> TRUE
+//!                   (< 'Linux')  -> FALSE


How come this isn't (!= 'Linux') -> FALSE and (== 'Linux') -> TRUE? Why the inequalities?

If I had to guess, I'd say it's because the inequalities are more general and easier to merge with other expressions?

Yes, exactly. Even if we did merge the two FALSE children, we would have to represent the != 'Linux' as (< 'Linux') or (> 'Linux') with pubgrub ranges. We could merge them to reduce the size of the tree, but for now this is simpler.

crates/pep508-rs/src/marker/algebra.rs

BurntSushi · 2024-08-08T17:37:03Z

crates/pep508-rs/src/marker/algebra.rs

+        edges
+    }
+
+    /// Merge two [`Edges`], applying the given function to all disjoint, intersecting edges.


I had some trouble grok'ing this routine. The map2 name I think also confused me a bit. Perhaps expand the comment here some?

I added some more general documentation. Is it clearer now?

crates/pep508-rs/src/marker/simplify.rs

BurntSushi · 2024-08-08T18:00:24Z

crates/pep508-rs/src/marker/tree.rs

+    /// expression.
+    ///
+    /// If the marker is `true`, this method will return `None`.
+    /// If the marker is `false`, the marker is represented as the normalized expression, `python_version < '0'`.


I think this was a nice API decision. Prevents one from emitting an "invalid" marker, but still makes it easy to print a debug repr.

ibraheemdev · 2024-08-08T19:45:32Z

What do you think about making MarkerTree::and and MarkerTree::or smart constructors instead of routines that mutate trees in place? Or perhaps that's for another PR, in order to keep the amount of code changed outside of pep508_rs smaller.

Yes, I also want to remove uses of Option<MarkerTree> now that MarkerTree::TRUE exists. Will do in a follow-up PR.

crates/pep508-rs/src/marker/algebra.rs

konstin

The ADDs are much better than my naive DNF idea. All the maths looks well-implemented.

crates/pep508-rs/src/marker/algebra.rs

konstin · 2024-08-09T11:07:09Z

crates/pep508-rs/src/marker/algebra.rs

+    /// For non-boolean variables, this is more complex. See `apply_ranges` for details.
+    ///
+    /// Note that the LHS and RHS must be of the same [`Edges`] variant.
+    fn apply(


Is this ever used with something other than AND?

No, but it's possible that we may want to reuse this for other operations in the future (such as XOR or IFF).

crates/pep508-rs/src/marker/algebra.rs

crates/pep508-rs/src/marker/simplify.rs

konstin · 2024-08-09T12:22:05Z

crates/uv/tests/lock.rs

-                "python_full_version <= '3.10' and python_version < '3.10'",
-                "python_full_version <= '3.10' and python_version < '3.10' and python_version > '3.10'",
-                "python_full_version <= '3.10' and python_version > '3.10'",
+                "python_full_version > '3.10' and python_version > '3.10'",


konstin · 2024-08-09T12:24:24Z

crates/uv/tests/pip_compile.rs

    # This file was autogenerated by uv via the following command:
    #    uv pip compile --cache-dir [CACHE_DIR] requirements.in --no-strip-markers --python-platform windows
-    attrs==23.2.0 ; python_version > '3.11' or sys_platform == 'win32'
+    attrs==23.2.0 ; sys_platform == 'win32' or python_version > '3.11'


How is the ordering defined now, is it the ordering of the marker variable flowing down? It looks like in conjunctions and disjunctions the ordering is now different. Not high priority as long as the ordering is deterministic and robust, but might be easy enough to justify changing

It is the ordering of the markers flowing down yeah. In this case the original expression would have been (python_version <= '3.11' and sys_platform == 'win32') or python_version > '3.11 before simplifying, which is why the order appears reversed despite python_version being the highest order variable.

I guess we could sort these the same way we used to?

FYI, doing a final sort on simplified markers is preferential for users not seeing a change in equivelent output if something about the resolution itself changes.

The resolution is still deterministic, but sorting may result in a more syntactically consistent order.

I meant in a scenario where the input changed, (e.g. the user changed the order, or added requirements that were previously transative dependencies, or new versions of packages changed the requirements in some similiar sense), but the output was equivelent (e.g. same packages, same versions, same markers), but perhaps the marker order could be affected.

That should never happen. The decision diagram is canonical for any functionally equivalent marker and the simplification routine is deterministic. All sorting could do is give us a different canonical output for a given marker, but it shouldn't affect whether or not it is canonical.

konstin · 2024-08-09T12:31:12Z

crates/uv-resolver/src/lock.rs

+
+        // Simplify all marker expressions based on the requires-python bound.
+        //
+        // This is necessary to ensure the a `Lock` deserialized from a lockfile compares


What is the difference between fresh and from lock? We always know the current requires-python in the resolver (we determine it on fork construction), so marker construction should already have this information.

Also note that requires-python is the bound for the entire lockfile, but individual forks can have stricter bounds, e.g. say overall is >=3.7, but there's one fork >=3.7,<3.8 and one fork >=3.8

This is emulating the call to simplify_python_versions when constructing the ResolutionGraph.

I agree that marker construction should always have this information, I would like to avoid simplifying requires-python and extras in a separate step.

konstin · 2024-08-09T12:35:48Z

crates/uv-requirements/src/source_tree.rs

                    .marker
-                    .and_then(|marker| marker.simplify_extras(extras)),
+                    .map(|marker| marker.simplify_extras(extras))
+                    .filter(|marker| !marker.is_true()),


nit: is_false

I think !marker.is_true() does not imply marker.is_false(). I do like the succinctness of the method names, but they do suggest a mutual exclusivity that isn't there. Perhaps is_always_true and is_always_false are better names? Not sure.

Good call, totally missed that

konstin · 2024-08-09T12:44:28Z

Could you also a top level doc comment sentence that we implement all or as and by demorgan and cheap negation?

@konstin

This PR rewrites the `MarkerTree` type to use algebraic decision diagrams (ADD). This has many benefits: - The diagram is canonical for a given marker function. It is impossible to create two functionally equivalent marker trees that don't refer to the same underlying ADD. This also means that any trivially true or unsatisfiable markers are represented by the same constants. - The diagram can handle complex operations (conjunction/disjunction) in polynomial time, as well as constant-time negation. - The diagram can be converted to a simplified DNF form for user-facing output. The new representation gives us a lot more confidence in our marker operations and simplification, which is proving to be very important (see #5733 and #5163). Unfortunately, it is not easy to split this PR into multiple commits because it is a large rewrite of the `marker` module. I'd suggest reading through the `marker/algebra.rs`, `marker/simplify.rs`, and `marker/tree.rs` files for the new implementation, as well as the updated snapshots to verify how the new simplification rules work in practice. However, a few other things were changed: - [We now use release-only comparisons for `python_full_version`, where we previously only did for `python_version`](https://github.com/astral-sh/uv/blob/ibraheem/canonical-markers/crates/pep508-rs/src/marker/algebra.rs#L522). I'm unsure how marker operations should work in the presence of pre-release versions if we decide that this is incorrect. - [Meaningless marker expressions are now ignored](https://github.com/astral-sh/uv/blob/ibraheem/canonical-markers/crates/pep508-rs/src/marker/parse.rs#L502). This means that a marker such as `'x' == 'x'` will always evaluate to `true` (as if the expression did not exist), whereas we previously treated this as always `false`. It's negation however, remains `false`. - [Unsatisfiable markers are written as `python_version < '0'`](https://github.com/astral-sh/uv/blob/ibraheem/canonical-markers/crates/pep508-rs/src/marker/tree.rs#L1329). - The `PubGrubSpecifier` type has been moved to the new `uv-pubgrub` crate, shared by `pep508-rs` and `uv-resolver`. `pep508-rs` also depends on the `pubgrub` crate for the `Range` type, we probably want to move `pubgrub::Range` into a separate crate to break this, but I don't think that should block this PR (cc @konstin). There is still some remaining work here that I decided to leave for now for the sake of unblocking some of the related work on the resolver. - We still use `Option<MarkerTree>` throughout uv, which is unnecessary now that `MarkerTree::TRUE` is canonical. - The `MarkerTree` type is now interned globally and can potentially implement `Copy`. However, it's unclear if we want to add more information to marker trees that would make it `!Copy`. For example, we may wish to attach extra and requires-python environment information to avoid simplifying after construction. - We don't currently combine `python_full_version` and `python_version` markers. - I also have not spent too much time investigating performance and there is probably some low-hanging fruit. Many of the test cases I did run actually saw large performance improvements due to the markers being simplified internally, reducing the stress on the old `normalize` routine, especially for the extremely large markers seen in `transformers` and other projects. Resolves #5660, #5179.

charliermarsh · 2024-08-10T04:01:02Z

Amazing work.

We don't currently combine python_full_version and python_version markers.

Does this mean we no longer compute unions, etc., to combine or markers of versions? (Or we don't combine python_version with python_full_version?)

ibraheemdev · 2024-08-10T04:02:10Z

We compute unions of python_version and python_full_version independently, but we don't combine them.

charliermarsh · 2024-08-10T04:02:51Z

Makes sense thank you.

## Summary Follow up to #5898. This should fix some of the failures in #5887 where `uv lock --locked` is failing due to `Some(true)` and `None` markers not comparing equal.

ibraheemdev added the lock Related to universal resolution and locking label Aug 7, 2024

ibraheemdev requested review from BurntSushi, charliermarsh and konstin August 7, 2024 23:55

ibraheemdev force-pushed the ibraheem/canonical-markers branch 5 times, most recently from f86e792 to 2b5c772 Compare August 8, 2024 17:00

BurntSushi approved these changes Aug 8, 2024

View reviewed changes

ibraheemdev force-pushed the ibraheem/canonical-markers branch 2 times, most recently from 5b18a9c to db25cf1 Compare August 8, 2024 19:39

sid-kap reviewed Aug 9, 2024

View reviewed changes

crates/pep508-rs/src/marker/algebra.rs Outdated Show resolved Hide resolved

konstin approved these changes Aug 9, 2024

View reviewed changes

ibraheemdev force-pushed the ibraheem/canonical-markers branch 2 times, most recently from 8ed4dab to 8cb5abe Compare August 9, 2024 15:11

ibraheemdev added 12 commits August 9, 2024 11:36

implement marker trees using algebraic decision diagrams

f29d961

simplify marker expressions when deserializing lockfile

d91850a

add tests for NP-complete marker simplification cases

d960838

update lockfile snapshots

c478bde

move new pep508-rs dependencies to workspace root

1200ce0

remove unnecessary pub

c6ca22a

document invariants of marker tree Edges

e961cf7

document marker operation cache semantics

cdd0482

document invariant of Edges::apply

6a599ff

document and clean up Edges::apply routine

efd0fd7

clarify depth-first traversal in collect_dnf

1d7fc07

apply suggestions from code review

cccbe8c

ibraheemdev force-pushed the ibraheem/canonical-markers branch from 8cb5abe to cccbe8c Compare August 9, 2024 15:36

typos

2f06d72

ibraheemdev merged commit ffd18cc into main Aug 9, 2024

ibraheemdev deleted the ibraheem/canonical-markers branch August 9, 2024 17:40

ibraheemdev mentioned this pull request Aug 9, 2024

Remove uses of Option<MarkerTree> #5978

Merged

notatallshaw-gts mentioned this pull request Aug 10, 2024

Another example of duplicate packages with different versions using --universal #4992

Closed

ibraheemdev mentioned this pull request Aug 10, 2024

Markers under universal resolution could have more concise representation #5992

Closed

ibraheemdev mentioned this pull request Aug 12, 2024

uv lock --locked false positive #5902

Closed

paulJordaan mentioned this pull request Aug 14, 2024

0.2.35 - regression. Resurfaces behaviour reported in #3675 #6085

Closed

BurntSushi mentioned this pull request Aug 16, 2024

audit universal resolver's robustness with respect to false positives/negatives with routines like "are these two marker expressions disjoint" #5562

Closed

A5rocks mentioned this pull request Aug 21, 2024

Less concise simplification of markers in uv 0.2.35 #6295

Closed

notatallshaw mentioned this pull request Aug 22, 2024

Requirements from underscored extras are accidentally included as package dependencies #6279

Closed

Comments

Conversation

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

Uh oh!

Summary

Uh oh!

BurntSushi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibraheemdev Aug 8, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

konstin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

notatallshaw Aug 9, 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!

ibraheemdev Aug 9, 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!

konstin commented Aug 9, 2024

Uh oh!

charliermarsh commented Aug 10, 2024

Uh oh!

ibraheemdev commented Aug 10, 2024

Uh oh!

ibraheemdev commented Aug 7, 2024 •

edited

Loading

ibraheemdev Aug 8, 2024 •

edited

Loading

ibraheemdev commented Aug 8, 2024 •

edited

Loading

notatallshaw Aug 9, 2024 •

edited

Loading

ibraheemdev Aug 9, 2024 •

edited

Loading