Skip to content

feat!: add support for conditional dependencies #136

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

Merged
merged 30 commits into from
Jul 23, 2025

Conversation

baszalmstra
Copy link
Contributor

@baszalmstra baszalmstra commented May 8, 2025

This PR adds support for conditional dependencies. A conditional dependency is a dependency on a package that is only active if another requirement is satisfied. E.g. A requires B if C. Conditions can be combined with and and or. This allows depending on packages that are only active in certain scenarios, e.g.

  • Depending on the platform (using conda virtual packages). E.g. linux-specific; if __linux
  • Depending on the presence of other packages (e.g. numpy <=1.22; if python<3.12, numpy >1.22; if python >=3.13).

Care is taken that conditional dependencies only incur an overhead when used. So this PR should not affect the performance of problems without conditional dependencies. Even when they are used the overhead should be minimal.

I've added a lot of documentation that describes how conditional dependencies are implemented.

Todo

  • Benchmark
  • Cpp bindings

@baszalmstra baszalmstra marked this pull request as ready for review May 9, 2025 20:21
@baszalmstra
Copy link
Contributor Author

@prsabahrami @jjerphan If you have time I would appreciate a few more eyes on this PR! 👍

@baszalmstra baszalmstra requested review from Copilot, tdejager and wolfv May 9, 2025 20:22
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements support for conditional dependencies by introducing new types and methods to capture conditions and their conversion into SAT clauses. Key changes include:

  • Adding new data structures (e.g. ConditionalSpec, ConditionalRequirement, Condition, LogicalOperator) to encode conditions.
  • Extending the solver internals to handle conditional requirements via disjunctions and auxiliary variables.
  • Updating C++ bindings and tests to account for the new conditional dependency support.

Reviewed Changes

Copilot reviewed 59 out of 59 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/solver/bundle_box/conditional_spec.rs New test support for conditional dependencies.
tests/snapshots/solver__root_constraints.snap Snapshot changes reflecting updated solver behavior.
src/solver/variable_map.rs Added variable variant and allocation function for at-least-one semantics.
src/solver/mod.rs Updated API and internal state to carry ConditionalRequirement and disjunctions.
src/solver/encoding.rs Major changes to encode conditional requirements including disjunction conversion and clause registration.
src/solver/conditions.rs New module implementing condition conversion to DNF and related types.
src/solver/clause.rs Modified clause representation to support optional condition disjunctions.
Other files (binary_encoding.rs, conditional_requirement.rs, C++ bindings, Cargo.toml, etc.) Adapted to support conditional dependencies and updated language features.
Comments suppressed due to low confidence (2)

src/solver/encoding.rs:292

  • [nitpick] The variable name 'conditions' is used to store disjunction IDs produced from the DNF conversion of a requirement condition. To improve clarity, consider renaming it to 'disjunction_ids' or similar.
let mut conditions = Vec::with_capacity(condition.as_ref().map_or(0, |(_, dnf)| dnf.len()));

cpp/src/lib.rs:646

  • [nitpick] Using .cloned() instead of .copied() is appropriate here because ConditionalRequirement may not implement Copy; verify that this change aligns with the intended type traits for ConditionalRequirement.
.cloned()

Copy link
Contributor

@tdejager tdejager left a comment

Choose a reason for hiding this comment

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

Phew! That was a read, but what excellent excellent work! Can't wait for this to land :) Just a few nits.

@wolfv
Copy link
Member

wolfv commented Jul 21, 2025

I added some benchmarking here: conda/rattler#1541

Copy link
Contributor Author

@baszalmstra baszalmstra left a comment

Choose a reason for hiding this comment

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

can you also post the benchmark results here?

conditions: Arena<ConditionId, Condition>,

/// Map from condition to its id
pub(crate) condition_to_id: FrozenCopyMap<Condition, ConditionId, ahash::RandomState>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this pub crate needed?

Copy link
Member

Choose a reason for hiding this comment

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

I just kinda copied it from other fields, but probabbly it's not needed. Let me remove it for now.

Image

Copy link
Member

Choose a reason for hiding this comment

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

Made it non-pub.

@wolfv
Copy link
Member

wolfv commented Jul 23, 2025

Benchmark results:

image image image

@baszalmstra baszalmstra enabled auto-merge (squash) July 23, 2025 09:07
@baszalmstra baszalmstra merged commit 105fb4f into prefix-dev:main Jul 23, 2025
9 checks passed
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