Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: constraining dependencies #124

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

baszalmstra
Copy link
Contributor

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
Copy link
Member

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

Copy link
Member

@Eh2406 Eh2406 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// [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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@mpizenberg mpizenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// [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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants