RFC: Remove top-down solver (v1)

[BardurArantsson]

DO NOT MERGE! This is just an RFC

Ok, so in a moment of madness I just went ahead and tried to see what would happen if we were to remove the TopDown solver.

I think the diffstat mostly speaks for itself.

Two caveats:

• I haven't slavishly gone through everything in the diff to see if there's further opportunities for deleting things that are no longer needed. (I'll do that if we get consensus.)
• I haven't tried to handle (i.e. ignore) the "solver:" option in config files. Not sure if that's necessary or how to do it. If it's deemed necessary, then any hints would be appreciated.

EDIT: Obviously this should probably be post-1.24.0, but it shouldn't be a problem to bring the patch up-to-date once that's released.

[BardurArantsson]

/cc @kosmikus @23Skidoo @grayjay @bgamari @hvr @ezyang @ttuegel @dcoutts

(Please add any cc's you might think are relevant.)

What do you think? Do you have any arguments against removal?

[BardurArantsson]

Oh, I should add. My main arguments for removal:

• 2000 fewer lines of code to worry about
• Smaller maintenance burden and less cognitive overhead for maintainers; see e.g. the TODO I've removed from resolveDependencies.
• Less to test using QuickCheck; which expands the "search space" of the tests for things that are actually relevant.
• Dead (or at the very least obsolete) code is a bad thing in general.

[dcoutts]

Ultimately we ought to remove it. Perhaps this is a good time to re-run the check on which packages can be solved with the top down solver that cannot be solved with the modular solver.

This is related to the idea of splitting the solver out, in as much as having an interface with two solver impls has also been a mechanism to "keep us honest" with not intermingling too much.

[BardurArantsson]

Perhaps this is a good time to re-run the check on which packages can be solved with the top down solver that cannot be solved with the modular solver.

Is there some "golden set of packages" that can be used for this purpose? I'd be quite happy to go through the 'mechanical' issue of scripting whatever's necessary for such a one-off test; I just don't have the necessary data to actually do it. Any help would be appreciated.

(Though I'd be very surprised if anything actually turned up. I haven't been following along fanatically, but I don't recall a single bug reports in the last year or so having anything to do with the choice of solver.)

This is related to the idea of splitting the solver out, in as much as having an interface with two solver impls has also been a mechanism to "keep us honest" with not intermingling too much.

That's a good point against that I hadn't thought of... but given that we're not currently doing bisimulation style testing in any meaningful sense I don't think it's a big issue. :) The newly added QuickCheck tests (thanks @grayjay!) will hopefully also be better (in every sense) than a bisimulation-style test where one of the simulees is... dodgy.

EDIT: Oh, and I should say: We can "keep more honest" by simply splitting out the solver! Then we can guarantee that we're not even relying on implementation details of cabal-install :). We could initially go for the "private library" approach rather than the "separate directory/.cabal file approach" if that allays the fears of increased developer burden.

[dcoutts]

Is there some "golden set of packages" that can be used for this purpose? I'd be quite happy to go through the 'mechanical' issue of scripting whatever's necessary for such a one-off test; I just don't have the necessary data to actually do it. Any help would be appreciated.

See #2531 and the wiki where @edsko did this experiment previously.

[BardurArantsson]

@dcoutts Thanks, I'll investigate tomorrow!

[grayjay]

The quickcheck tests currently only test the solver against itself, but it would be nice to add some tests that construct solvable dependency problems and test that the solver finds a solution.

[BardurArantsson]

@grayjay Oh, I see, but there's no actual bisimulation going on, right?

[grayjay]

@BardurArantsson There is no bisimulation. It's just testing that different parameters and target orders don't affect whether the solver finds a solution.

[kosmikus]

I don't have any really good reasons against removal: I find the topdown solver occasionally useful for quick comparisons. I don't think its presence does much harm or causes much work (we haven't added most new / advanced features to it anyway). And in general, I think it's good if there are two solvers, because it ensures that we keep the solver interface relatively explicit and do not blur the lines too much.

But I can see that it's of significant size, and the longer it is just there without really being used or worked on, the more mysterious it becomes. And I don't think anyone really needs it as this point. So after considering it for a while, I'm not going to block the request for removal if Duncan is also ok with it.

I'd be really surprised if the topdown solver could solve anything the modular solver could not. After all, the modular solver is supposed to be complete with infinite backjumps. The more interesting question would be whether there are situations where the topdown solver is much faster than the modular solver, or finds a significantly better solution, which is certainly both possible. So running a few final comparisons as long as we still easily can is probably a good idea.

[edsko]

Actually, there are a number of packages that the top down solver can solve but the modular solver cannot. See the issue and the wiki cited by @dcoutts above, where I have also done detailed performance comparisons.

[kosmikus]

@edsko Ok, fair enough. I guess I was too caught up in the "theoretical view" that tells me that the modular solver should find a solution eventually, but I of course agree that missing a 5 minute timeout is clearly the same as not finding a solution in practice. I will also take a closer look.

[kosmikus]

@edsko Hmm, it actually looks like the topdown solver is completely broken right now? Any plan it generates seems to be rejected by the sanity check. Was this known?

[edsko]

Nope, that is news to me.

On 23 Apr 2016, at 18:02, Andres Löh notifications@github.com wrote:

@edsko Hmm, it actually looks like the topdown solver is completely broken right now? Any plan it generates seems to be rejected by the sanity check. Was this known?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

[BardurArantsson]

This seems to imply that there cannot really be any actual current users. I wonder how long it's been broken...

[23Skidoo]

FTR, I'm +1 on removing the old solver from master.

[kosmikus]

@BardurArantsson I don't really think there are users. But also, it's only broken on master, not in any released version, AFAICT.

[kosmikus]

It seems to be the combination of cfb124f and 09528c2 that causes the topdown solver to break. I'll see if it is easy to temporarily restore it once more. /cc @ezyang

[23Skidoo]

@kosmikus So it's only broken on master, not on 1.24?

[kosmikus]

@23Skidoo Indeed, it looks fine on 1.24. The two patches I mentioned don't seem to be in 1.24. So that's good, actually. Perhaps we can do the "final evaluation and comparison" using 1.24, and if that indeed leads us to the conclusion that topdown can be removed, then we don't need to fix it on master anymore.

[kosmikus]

Ok, so I can already say that the two solvers have become much more difficult to compare, because of pkgconfig and setup dependencies which lead to some package being rejected that are not rejected by the topdown solver.

A naive comparison run on a plain ghc-7.10.3 database on every Hackage package (9621 packages) with standard flags (so no increased backjump limit, no reorder-goals) yields 239 packages where the topdown solver finds a plan, but the modular solver does not.

Re-running the 239 packages with a few more, but by far not all, pkgconfig-relevant libraries installed and unlimited backjumps and reorder goals and a timeout of 90 seconds, will find install plans for 116 of these, leaving 123 unsolved. Of these 123, only 6 seem to actually fail due to timeout. The others fail due to the dependency tree having been exhaustively searched (failure due to pkgconfig error or setup dependency error or possibly something else).

The 6 packages failing due to timeout are:

classy-prelude-yesod
GuiTV
llvm-tools
persistent-protobuf
phooey
yesod-pure

I'm somewhat surprised that @edsko seems to have had a much larger number of "true failures" of the modular solver. I'll see if I can still reproduce this with older versions of cabal-install or older versions of ghc.

[BardurArantsson]

@kosmikus Did you reach any conclusions?

[BardurArantsson]

@kosmikus Ping :)

[kosmikus]

@BardurArantsson Pong. Planning to work on my cabal-install backlog during ZuriHac. But I guess the conclusion here is that I'm not really willing to spend time figuring out why the topdown solver is broken in HEAD, and it still working in 1.24 seems good enough for the remaining comparisons. So I think that yes, we can remove it.

[BardurArantsson]

Thanks. I apologize for pestering you, btw :).

My general thinking here is that given:

1. The amount of time passed with a broken TopDown solver (in master).
2. The lack of bug reports (AFAICT; GH isn't the ideal interface to keep up with these things.)
3. The recent improvements to the modular solver (kudos, btw!) should mean that ModSolver is "good enough". If there are still failing cases with the modular solver, well, then we need to fix the modular solver, regardless of what TopDown does.

... we should just remove, so I'm glad you agree.

So: LAST CALL: I'll rebase this changeset and merge tomorrow unless someone else objects.

[kosmikus]

@BardurArantsson Fine with me. Thanks for being so patient.

[kosmikus]

@BardurArantsson And if I've understood @grayjay correctly, she's also in agreement.

[grayjay]

Yes, I think it makes sense to remove the Topdown solver.

[BardurArantsson]

Closing this; see #3598