feat: constraining dependencies #124

baszalmstra · 2022-10-28T15:15:14Z

This is an implementation of "constraining" dependencies or sometimes called "peer dependencies" as described in #120.

The implementation is fully functioning. I've added some regular tests as well as modified the proptests to handle constraints.

I did have to modify the API of DependencyProvider and of OfflineDependencyProvider. I'm not really happy with the names and stuff but I'm curious to get your feedback.

I've also already used this branch in my Conda package solver and it works like a charm there! 🥳

mpizenberg · 2022-10-29T08:48:37Z

I've also already used this branch in my Conda package solver and it works like a charm there!

Awesome! I can't review this weekend but I'll have some time during the week and will happily give you some feedback

Eh2406

Thank you! This is looking good!
I had a few comments and concerns. Allso tests and clippy are unhappy.
But we are very close.

Eh2406 · 2022-10-29T15:13:57Z

src/internal/incompatibility.rs

+                (package.clone(), Term::Positive(set1.clone())),
+                (p2.clone(), Term::Positive(set2.complement())),
+            ]),
+            kind: Kind::FromDependencyOf(package, set1, p2.clone(), set2.clone()),


Currently weather it is constrained or required does not appear in the kind at all. This does not affect the correctness of the algorithm, as kind is only used for error messages. Do we think it's important to have this distinction available when generating error messages?

Yes, you can't understand the proof argument created by this algorithm unless you know what the inputs are. Therefore we should add a boolean or enum to Kind::FromDependency to distinguish which kind of requirement it was.

No, they are not significantly different. If a resolver uses both of them and wants to distinguish it can look out this dependency in its source of truth and figure out which kind it had generated.

Allso, is FromDependencyOf still the correct terminology, Is FromRequirementOf better?

Eh2406 · 2022-10-29T15:17:46Z

src/solver.rs

+
+/// An enum that defines the type of dependency.
+#[derive(Clone, Debug)]
+pub enum Requirement<VS: VersionSet> {


this will need to #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))], and it's going to need to be even a little more complicated than that. because it needs to deserialize the existing test files and benchmarks, which do not have a marker for which kind it is.

Or we could decide to break compatibility of serialized data? or start defining a serialized format more in line with the internal representation of incompatiblities (list of pairs of pkg, and +/- version set) which is not going to change soon as this would not be a "pubgrub" algorithm anymore? Then will have manually written serialize and deserializers.

Eh2406 · 2022-10-29T15:27:23Z

tests/sat_dependency_provider.rs

+                            all_versions_by_p.get(p1).unwrap_or(&empty_vec).iter()
+                        {
+                            if !range.contains(p1_version) {
+                                // Either this specific variant is not selected or non-matching dependency versions are not selected.


On my third time reading it. Yes, I think this is correct.

Eh2406 · 2022-10-29T15:42:43Z

src/solver.rs

+    /// [add_dependencies](OfflineDependencyProvider::add_dependencies),
+    /// [add_constraints](OfflineDependencyProvider::add_constraints), or
+    /// [add_requirements](OfflineDependencyProvider::add_requirements).
+    /// All subsequent calls to any of those functions for a given package version pair will replace the


This is no longer true. At a minimum these comments need to be updated. These calls now append items to an existing list.
@mpizenberg, do you have a strong preference for the all at once API?

Indeed, I'm not a big fan of making it easy for people to make mistakes. I'd say we probably make add_dependency and add_constraint private or remove them alltogether, and only keep the add_requirements function. And the new add_requirements function should use = instead of insert. It doesn't make sense to have both a required dep and a constraint for the same dependency package.

mpizenberg

I haven't looked at the testing part but added some comments on the rest. I'm very happy that adding constraints managed to unblock you @baszalmstra! I think the changes make sense for your needs.

My personal position is that we should make dependency definition and retrieval our focus for pubgrub v0.4 (after 0.3 which focuses on version sets). I'd like that we explore many different ways of changing the get_dependencies part of pubgrub API, all the way from what's currently possible to having full flexibility of providing incompatibilities directly. Evaluate that whole spectrum of solutions, including intermediate ones like this one with "constraints".

My reasoning is that

yesterday we needed to transform (p1, v1) depends on (p2, range2) into { p1: +singleton(v1), p2: -range2 }
today we also need to transform (p1, v1) is constrained (optionally) by (p2, range2) into { p1: +singleton(v1), p2: +(complement(range2) }
tomorrow we might need to transform (p1, v1) might be used with either (p2, range2) or (p3, range3) into ...
...

The list goes on, with all things considered valid incompatibilities by pubgrub. By the way, for example, your use case of saying "my package p1 is incompatible with p2" would be better served by a custom type transforming (p1, v1) incompat with (p2, *) into { p1: +singleton(v1), p2: +*} instead of the twisting exercise consisting in saying (p1, v1) is constrained by (p2, v<0) which is effectively the same, since complement(v<0) is *.

All that is to say, I think this PR and your use case is a very important step in an API and ergonomics work which could be the focus of pubgrub v0.4. But I don't see this as being merged before we do similar experiments on other designs.

mpizenberg · 2022-10-30T18:43:05Z

src/internal/core.rs

-            }));
+        let new_incompats_id_range =
+            self.incompatibility_store
+                .alloc_iter(deps.iter().map(|(dep, requirement)| match requirement {


I'd suggest rename dep into p or pkg here to fit with the type annotation

mpizenberg · 2022-10-30T19:12:46Z

src/solver.rs

+
+/// An enum that defines the type of dependency.
+#[derive(Clone, Debug)]
+pub enum Requirement<VS: VersionSet> {


Or we could decide to break compatibility of serialized data? or start defining a serialized format more in line with the internal representation of incompatiblities (list of pairs of pkg, and +/- version set) which is not going to change soon as this would not be a "pubgrub" algorithm anymore? Then will have manually written serialize and deserializers.

mpizenberg · 2022-10-30T19:23:47Z

src/solver.rs

+    /// [add_dependencies](OfflineDependencyProvider::add_dependencies),
+    /// [add_constraints](OfflineDependencyProvider::add_constraints), or
+    /// [add_requirements](OfflineDependencyProvider::add_requirements).
+    /// All subsequent calls to any of those functions for a given package version pair will replace the


Indeed, I'm not a big fan of making it easy for people to make mistakes. I'd say we probably make add_dependency and add_constraint private or remove them alltogether, and only keep the add_requirements function. And the new add_requirements function should use = instead of insert. It doesn't make sense to have both a required dep and a constraint for the same dependency package.

feat: add the ability to define constrains

1da794a

Eh2406 requested changes Oct 29, 2022

View reviewed changes

mpizenberg reviewed Oct 30, 2022

View reviewed changes

baszalmstra mentioned this pull request Oct 1, 2023

Cooperation with PubGrub-rs. mamba-org/resolvo#2

Open

This was referenced Nov 7, 2023

Future proof dependency provider. #148

Open

Allow an arbitrary, custom incompatibilities #152

Open

Eh2406 added this to the v0.4 Dependency constraints milestone Mar 20, 2024

Eh2406 mentioned this pull request Apr 18, 2024

Add a proxy layer for extras astral-sh/uv#3100

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: constraining dependencies #124

feat: constraining dependencies #124

baszalmstra commented Oct 28, 2022

mpizenberg commented Oct 29, 2022

Eh2406 left a comment

Eh2406 Oct 29, 2022

Eh2406 Oct 29, 2022

mpizenberg Oct 30, 2022

Eh2406 Oct 29, 2022

Eh2406 Oct 29, 2022

mpizenberg Oct 30, 2022

mpizenberg left a comment •

edited

mpizenberg Oct 30, 2022

mpizenberg Oct 30, 2022

mpizenberg Oct 30, 2022

feat: constraining dependencies #124

Are you sure you want to change the base?

feat: constraining dependencies #124

Conversation

baszalmstra commented Oct 28, 2022

mpizenberg commented Oct 29, 2022

Eh2406 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

mpizenberg left a comment • edited

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

mpizenberg left a comment •

edited