feature: Private Dependencies #9743
Conversation
alt-romes
force-pushed
the
wip/private-dependencies
branch
5 times, most recently
from
493cdd4 to
ed3579a
Compare
alt-romes
force-pushed
the
wip/private-dependencies
branch
2 times, most recently
from
61ee29b to
6a9c45b
Compare
alt-romes
force-pushed
the
wip/private-dependencies
branch
8 times, most recently
from
4b7608e to
08689b9
Compare
alt-romes
force-pushed
the
wip/private-dependencies
branch
12 times, most recently
from
9c97b3e to
ae07142
Compare
This reverts commit c3b5013. We don't need to fail in cabal check for private dependencies in particular because hackage will already reject packages whose Cabal version is "too new" to prevent experimental features to be used in hackage. This way, it is hackage which controls when private dependencies make it there, rather than controlling this via the Cabal version.
Improvement/refactor in the implementation of private dependencies to use the more descriptive `IsPrivate` type rather than the isomorphic `Maybe PrivateAlias`
Documents private dependencies in the user facing user guide
Redesigns the algorithm for checking the private scope closure property in the solver. The algorithm is described in detail in the comment of `findBadPrivClosures`. A key change is that we construct and propagate together with the `RevDepMap` a cache of the private scopes at every node, which allow us to efficiently look up the "root" packages from which we can easily validate the property holds. This change was prompted by stress testing private dependencies: The stress test uncovered that way too much time was being spent on validating the closure property, despite the property already being checked at every node. Specifically, the baseline for solving without the check was 11s, however, with the check in place, solving instead took over 50s when NO private dependencies were in the plan, and over 2 minutes when there were a lot of private scopes. After this change, the check of the private scope closure property is negligible on performance, being slightly faster (40ms) when there are a lot of private scopes (because the algorithm short circuits faster) and slightly slower (90ms) when there are no private scopes at all. This is more in line with the `findCycles` check takes some 110ms on my machine. This commit also adds the stress test which uncovered the problem, however, this stress test is disabled by default and should only be run manually since testing time-based performance in CI can be problematic.
Originally, we were qualifying a private dependency with the component
with that private dep. Now, we qualify a private dependency without the
component, i.e. only with the package name and the scope name.
However, we want the same private scope name to be shared across all
components of a package (and, if different dependencies are desired
across components, different scope names should be used). For example,
the following package should not be possible to solve (and does not with
this commit):
cabal-version: 3.0
name: pkgC
version: 0.1.0.0
build-type: Simple
library
private-build-depends: SameName with (libA == 0.1.0.0)
build-depends: base
executable myexe
private-build-depends: SameName with (libA == 0.2.0.0)
build-depends: base
See the second case of the `same-scope-name` test.
Since we dropped the `Component` field of `ScopePrivate` in the previous commit, we can now use `ScopeQualified` instead of having a specific constructor `ScopePrivate`. This is mostly a clean up.
This test makes a long chain of packages violate the private closure property. In particular, it ensures all of the packages that are missing from the private scope are listed in the error message.
We would previously include terminal nodes if these were not private.
alt-romes
force-pushed
the
wip/private-dependencies
branch
from
72a8fe2 to
b7ff4a7
Compare
|
@alt-romes I had a question about the design before I start reviewing the code. How similar is this PR to the design described in this document? #4035 (comment) If I understood the first commit message correctly, this PR already differs by not automatically pulling intermediate dependencies into private scopes. |
|
@grayjay In general, the implementation/final version should be very similar to the original draft design. You point out that we do not implicitly qualify packages into the private scope: that's right. I suppose in the original draft we would. |
|
@grayjay I have attempted to do the backtracking test by changing Cabal to "randomly" qualify certain packages privately instead of at the top-level. In principle, this should always be possible because having scopes with singleton private dependencies should only make more plans valid, not the other way around. Unfortunately, I wasn't able to get a fully working example of the hackage-benchmarks test using this strategy. I got the modified version of Cabal to finish in roughly the same time as the non-modified version, but the modified Cabal produced "Unknown" final results rather than "OK". Given time constraints, I haven't pursued this second benchmark any further. I still believe it's important, but given said time constraints and the robust results of the first stress test I'm of the opinion we can proceed with merging this PR before such test is concluded. I recall that the first stress test, after iteration, showed negligible performance impact on solving for a plan with thousands of dependencies roughly half of which were private. Adding to this the fact that private dependencies are introduced as "shallow" goals (ie not transitive), I'm reasonably confident that the performance impact even with backtracking should be negligible, and that we could merge this as is before having the results of the second test which we can address with time, without worrying about this PR bit rotting in the meanwhile. I can open a follow-up ticket to track stress-testing private dependencies with backtracking further. If you don't oppose to this omission, I would greatly appreciate your review to move this PR forward diligently. |
|
@alt-romes Sorry for the delay. Thank you for working on the backtracking stress test. It sounds promising that the runtimes were comparable to master. I'm not sure I understand what you meant by "unknown" test results. Is that error specific to the benchmark, or was it an error given by the cabal executable under test? Did it occur in all runs, or only when cabal failed to find a solution? Even if the benchmark doesn't complete, are you able to capture any -v3 logs that show how cabal handles the new types of conflicts added by the PR? I still think that it is important to analyze the performance of private dependencies when there are conflicts, in some way, before merging. The first stress test mainly evaluated the overhead added to each step of dependency solving, but this test mainly evaluates the total number of steps, which is almost orthogonal. It is easy for a change to add almost no overhead to the individual steps but add an exponential number of steps to resolve conflicts, for example, by adding too many variables to the conflict set. It is very difficult to address some types of performance issues once a feature is released and being used by packages on Hackage.
Is it possible to combine some of the dependencies into the same scope so that they aren't all singletons? I'm concerned that singleton scopes won't exercise all of the conflict types, such as enforcement of the scope closure property. The best test cases would involve many realistic conflicts, of various types, and significant amounts of backtracking, with some cases having solutions and others having no solution due to private dependencies. It's fine for packages to go from succeeding to failing and vice versa, compared to master, since these changes already can't be directly compared to master. |
|
@grayjay the details of my attempt are slightly fuzzy, but they were, in the end, inconclusive about the performance of the solver with private dependencies when backtracking is involved. I was wondering, given your expertise with the solver and recent suggestions, if you could independently attempt to benchmark this PR on your machine with the backtracking relevant machinery. I wasn't able to get the benchmark working properly in my backtracking attempt, and currently cannot afford to spend a lot of time figuring out what was wrong, but perhaps you may be able to get a working solution without trouble since you've worked with these benchmarks in the past. |
|
@alt-romes I don't have very much time to spend on Cabal currently, but I could try running your benchmark to see if I have any ideas about how to proceed, especially debugging the "unknown" result. Do you have a link to the branch? |
That would be great! Here's a commit with the change: https://github.com/alt-romes/cabal/tree/wip/romes/private-deps-bench |
|
@alt-romes I tried running the Hackage benchmark manually on your branch, and I was able to reproduce the "Unknown" error. It indicates that the cabal under test failed but the benchmark didn't recognize the error message. Then I manually ran the modified cabal on one of the packages producing the error. Here is an example error from a lower level package: It looks like the latest commit modifies qualifiers in the middle of solving, which causes an inconsistency. I think it could be fixed by only modifying packages at the start of solving, such as during index conversion, in |
|
@grayjay thanks for investigating. Unfortunately, |
Hackage is not yet ready for private dependencies, so we mustn't allow packages with private deps to be uploaded just yet. Hackage already rejects packages using too-recent cabal versions, however, private dependencies is a special case which must be guarded against until more extensive benchmarks regarding the performance of solving private dependencies mixed in with a lot of backtracking. See haskell#9743
Hackage is not yet ready for private dependencies, so we mustn't allow packages with private deps to be uploaded just yet. Hackage already rejects packages using too-recent cabal versions, however, private dependencies is a special case which must be guarded against until more extensive benchmarks regarding the performance of solving private dependencies mixed in with a lot of backtracking. See haskell#9743
|
We discussed code review at the last meeting Cabal meeting. I'd like to focus on the solver part of this pull request. If I understand correctly, @fgaz is interested in reviewing the changes to the Cabal library. I think that we should also have another reviewer for the high level design and user visible behavior, and others at the meeting suggested @gbaz. @gbaz are you interested in being a reviewer? We also discussed making this feature experimental, so that Hackage rejects packages with private dependencies, until we've finished testing the performance, especially performance in the presence of conflicts. |
I meant that you could directly insert private dependencies during index conversion, as if they were read from the cabal/cabal-install-solver/src/Distribution/Solver/Modular/IndexConversion.hs Lines 344 to 346 in b7ff4a7 I would expect the solver to add the correct qualifier later. |
Hackage is not yet ready for private dependencies, so we mustn't allow packages with private deps to be uploaded just yet. Hackage already rejects packages using too-recent cabal versions, however, private dependencies is a special case which must be guarded against until more extensive benchmarks regarding the performance of solving private dependencies mixed in with a lot of backtracking. See haskell#9743
|
@alt-romes I finally finished adding private dependencies to the QuickCheck tests as I mentioned in the meeting, and I opened a PR for this branch: mpickering#2 |
Tracking ticket for private dependencies: #4035
See the commit message of the commit with the same name as the pull request for the long explanation.
TODO checklist:
Fail cabal-check with private dependencies (we need to ensure it all works with hackage before allowing packages with private dependencies to be uploadedTemplate Α: This PR modifies
cabalbehaviourInclude the following checklist in your PR: