fix: incremental solving (rip issue #75) #15
Conversation
The original bug was caused by adding a conflicting `requires` clause, yet failing to detect and handle the conflict. This commit introduces a clearer separation between: adding new clauses, detecting conflicts, and handling said conflicts.
| // do not have watches) | ||
| // Negative assertions derived from other rules (assertions are clauses that consist of a | ||
| // single literal, and therefore do not have watches) | ||
| for &(solvable_id, clause_id) in &self.negative_assertions { |
There was a problem hiding this comment.
Propagation is done a lot, this now runs for all negative_assertions every time propagate is called. Would it make sense to only do this for a subset of the negative assertions?
There was a problem hiding this comment.
Should we rerun the benchmarks in rattler?
There was a problem hiding this comment.
I think running the benchmarks makes sense. It's been a long time since I wrote this code, but I was under the impression that this part of propagation is not run that often (only after an arbitrary decision has been made). Also, it's not clear to me how to select the right subset of negative assertions without introducing complexity. With a bit of luck, the perf impact will be negligible and we can get away with it.
There was a problem hiding this comment.
(For future reference: the benchmarks showed no significant difference / a slight improvement)
Note for reviewers: this PR assumes that #15 has been merged. Ideally, I'd take my `fix-nested-deps-issue` branch as a base, but then the PR would end up being created in my own repository. Fortunately the whole code is in a single commit, so you can have a look at the last commit's diff (or wait till #15 is merged).
|
@sumanth-manchala thanks for creating the test that allowed me to develop this fix! I credited you in my last blog article :) |
|
That was really unexpected!!! Thanks, means a ton❤️ and that int test was my first open source contribution!! |
|
Great to hear :) For me, this is the 10th year since my first contribution. You can have a look at my first PR and you will have a good laugh, because I didn't know git that well back then 😅 |
The original bug was caused by adding a conflicting
requiresclause, yet failing to detect and handle the conflict. This commit introduces a clearer separation between: adding new clauses, detecting conflicts, and handling said conflicts.Closes #13
Closes prefix-dev/rip#75