-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat!: add support for conditional dependencies #136
Conversation
@prsabahrami @jjerphan If you have time I would appreciate a few more eyes on this PR! 👍 |
There was a problem hiding this 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()
There was a problem hiding this 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.
I added some benchmarking here: conda/rattler#1541 |
There was a problem hiding this 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?
src/utils/pool.rs
Outdated
conditions: Arena<ConditionId, Condition>, | ||
|
||
/// Map from condition to its id | ||
pub(crate) condition_to_id: FrozenCopyMap<Condition, ConditionId, ahash::RandomState>, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made it non-pub.
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 withand
andor
. This allows depending on packages that are only active in certain scenarios, e.g.linux-specific; if __linux
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