Use larger conflict set when solver chooses wrong version for package in link group

[grayjay]

This commit replaces a call to lgBlame with a call to lgConflictSet. lgConflictSet adds the members of the link group to the conflict set. The other members are required, for example, when a linked package's version doesn't match a constraint, and the solver must try other versions for the linked-to package.

I merged #3221, #3234, #3237, and @edsko's branch off of #3268 into master to try to find any remaining failures in the solver quickcheck test. The log below is from that branch. This commit adds 0.D to the conflict set when the solver rejects 1.D-1.

I'm still not sure if the conflict set is large enough. Should it also contain the variables from the GoalReasonChains for 0.D and 1.D?

 1  targets: A, B
 2        indepGoals:                                       constraints:
 3    stanzas A test (unknown source)
 4    stanzas B test (unknown source)
 5    stanzas C test (unknown source)
 6    stanzas D test (unknown source)
 7    stanzas D test (unknown source)
 8  preferences:
 9  strategy: PreferLatestForSelected
10  reorder goals: False
11  independent goals: True
12  avoid reinstalls: False
13  shadow packages: False
14  strong flags: False
15  max backjumps: infinite
16  [__0] trying: 0.A-1.0.0 (user goal)
17  [__1] trying: 0.C-1.0.0 (dependency of 0.A-1.0.0)
18  [__2] trying: 0.D-2.0.0 (dependency of 0.C-1.0.0)
19  [__3] trying: 1.B-1.0.0 (user goal)
20  [__4] trying: 1.C~>0.C-1.0.0 (dependency of 1.B-1.0.0)
21  [__5] trying: 1.D~>0.D-2.0.0 (dependency of 1.C-1.0.0)
22  [__6] rejecting: 1.B-1.0.0:!test (constraint from unknown source requires opposite flag selection)
23  [__6] rejecting: 1.B-1.0.0:*test (conflict: 1.D==2.0.0, 1.B-1.0.0:test => 1.D==1.0.0)
24  [__6] fail (backjumping, conflict set: 1.B, 1.C, 1.D, 1.B-1.0.0:test)
25  [__5] rejecting: 1.D-2.0.0 (multiple instances)
26  [__5] rejecting: 1.D-1.0.0 (dependencies not linked: cannot make 1.D canonical member of {*0.D-2.0.0,1.D-2.0.0}; conflict set: 1.C, 1.D)
27  [__5] fail (backjumping, conflict set: 1.B, 1.C, 1.D, 1.B-1.0.0:test)
28  [__4] rejecting: 1.C-1.0.0 (multiple instances)
29  [__0] fail (backjumping, conflict set: 1.B, 1.C, 1.D, 1.B-1.0.0:test)
30  FAIL
31          Unexpected error:
32          Could not resolve dependencies:
33          trying: 1.B-1.0.0 (user goal)
34          trying: 1.C~>0.C-1.0.0 (dependency of 1.B-1.0.0)
35          trying: 1.D~>0.D-2.0.0 (dependency of 1.C-1.0.0)
36          rejecting: 1.B-1.0.0:!test (constraint from unknown source requires opposite flag selection)
37          rejecting: 1.B-1.0.0:*test (conflict: 1.D==2.0.0, 1.B-1.0.0:test => 1.D==1.0.0)
38          Dependency tree exhaustively searched.

[ezyang]

Not been paying close enough attention to the solver. Do we have an explanation of what conflict sets are somewhere?

[23Skidoo]

/cc @kosmikus

[edsko]

@grayjay Ugh :/ Maybe you are right. What if the version choice for a particular package outside of the link group forces a choice of a package inside the link group? Wouldn't we also have to reconsider that outside package then as well? I should talk to @kosmikus about this. I misunderstood the exact nature of the goal reason chains in the past, and I'm still not 100% sure I understand them now. Perhaps it might be the case that when the link validation walks over the tree, the goal reason chain is all we need to construct the conflict set?

[edsko]

@ezyang it seems not. I wrote previously in a blog post:

Each Fail node in the tree is annotated with a conflict set which records why this path ended in failure. The conflict set is used to guide the backtracking; any choice that we encounter while backtracking that does not involve any variables in the conflict set does not need to be reconsidered, as it would lead to the same failure.

I guess we should add this to the code.

[edsko]

@grayjay I've pushed another commit to https://github.com/edsko/cabal/tree/pr/FixLinkDeps , which actually deletes lgBlame and lgConflictSet completely; instead of trying to compute a stricter bound on the conflict set, we now just use the goal reason chain to construct the conflict set. I get no test failures with this additional commit -- can you try it in your branch where you've merged all these changes? If both you and @kosmikus thinks that this makes sense, I suggest we do it this way instead; it makes sense to me and I feel a lot happier just an overestimate for these conflict sets than having so many subtle bugs.

[grayjay]

@edsko I cherry-picked the latest commit, and the test still fails because 0.D is missing from the conflict set. I think lgBlame and lgConflictSet are needed, because the goal reason chain only contains the variables that caused the goal to be introduced. I don't think it explains why the package was linked or why the link group exists.

When testing my branch, I couldn't find a test case that fails because the other link group members' GoalReasonChains are not added to the conflict set. It seems like other failures always end up adding the necessary variables to the conflict set higher in the tree.

[kosmikus]

Without looking very much at the code, it seems to me that the situation here is this:

[__6] rejecting: 1.B-1.0.0:*test (conflict: 1.D==2.0.0, 1.B-1.0.0:test => 1.D==1.0.0)

The choice 1.D == 2.0.0 is not really an accurate reflection of the choice contributing to the conflict. The actual choice was 1.D ~> 0.D-2.0.0, which is apparently lost here. So we do have a conflict that is caused by a choice that would not have been possible without 0.D being there.

My first attempt at a fix would be to record linking choices explicitly, which would mean changing

data CI qpn = Fixed I (Goal qpn) | ...

to

data CI qpn = Fixed I (Maybe (Goal qpn)) (Goal qpn) | ...

where we use Just goal to indicate a situation as we have here, where the 1.D goal has been linked to the 0.D goal. We'd then have the information about the link target available for inclusion in the conflict set, e.g. in the function merge.

I don't yet fully understand how lgBlame and lgConflictSet work, and how the solution proposed here is different from what I just said. I will continue to look at this.

[grayjay]

@kosmikus I think we were talking about different failures in the log. I just added line numbering. This pull request adds the linked-to variable to the conflict set on line 26, and your change would fix line 23. I think the failure on line 25 should also add 0.D to the conflict set.

[kosmikus]

Thank you. But really all of the conflict sets mentioned in the trace should contain 0.D, right (including the one mentioned on line 24 already)? Anyway, it's helpful for me to know that this PR is trying to address a different one.

[grayjay]

@kosmikus I thought about the issue some more, and I think that line 23 doesn't need 0.D because of the way that the tree is structured. Line 23 fails because 1.D-2's version doesn't match the test's constraint. But if the solver backjumps to 1.D, it can try any other version, including 1.D-1, which does fit the constraint. If 1.D-1 then fails with "dependencies not linked", as in this example, that failure should add 0.D to the conflict set.

I reran the test and made it print conflict sets for the rest of the failures:

16 [__0] trying: 0.A-1.0.0 (user goal)
17 [__1] trying: 0.C-1.0.0 (dependency of 0.A-1.0.0)
18 [__2] trying: 0.D-2.0.0 (dependency of 0.C-1.0.0)
19 [__3] trying: 1.B-1.0.0 (user goal)
20 [__4] trying: 1.C~>0.C-1.0.0 (dependency of 1.B-1.0.0)
21 [__5] trying: 1.D~>0.D-2.0.0 (dependency of 1.C-1.0.0)
22 [__6] rejecting: 1.B-1.0.0:!test (constraint from unknown source requires opposite flag selection; conflict set: 1.B, 1.B-1.0.0:test)
23 [__6] rejecting: 1.B-1.0.0:*test (conflict: 1.D==2.0.0, 1.B-1.0.0:test => 1.D==1.0.0; conflict set: 1.B, 1.C, 1.D, 1.B-1.0.0:test)
24 [__6] fail (backjumping, conflict set: 1.B, 1.C, 1.D, 1.B-1.0.0:test)
25 [__5] rejecting: 1.D-2.0.0 (multiple instances; conflict set: 1.B, 1.C, 1.D)
26 [__5] rejecting: 1.D-1.0.0 (dependencies not linked: cannot make 1.D canonical member of {*0.D-2.0.0,1.D-2.0.0}; conflict set: 1.C, 1.D)
27 [__5] fail (backjumping, conflict set: 1.B, 1.C, 1.D, 1.B-1.0.0:test)
28 [__4] rejecting: 1.C-1.0.0 (multiple instances; conflict set: 1.B, 1.C)
29 [__0] fail (backjumping, conflict set: 1.B, 1.C, 1.D, 1.B-1.0.0:test)
30 FAIL

[edsko]

@grayjay are these tests on the solver branch?

[grayjay]

Yes

[kosmikus]

@grayjay I also have thought about all this more, and I think I agree with you. I originally thought we could not split the linking from the version choice, but I now believe we can (which is good, because it is what we're doing already by separating version constraints and link groups). So if a conflict arises like here in line 23 only because of the version choice of a linked package, we don't need to include the conflict set of the link group.

[kosmikus]

FWIW, this also means I no longer believe we need to change CI as I suggested above, but that we can fix all problems just by storing the correct info in the link group.

[grayjay]

This PR is included in #3357 as 7f48059.