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
Add version validation during resolve's graph analysis #3887
base: master
Are you sure you want to change the base?
Conversation
Just a couple of passing comments:
|
5624975
to
980096c
Compare
With this the VersionSpec shows a string that can be parsed to reconstruct the same object. Avoids issues with disjoint VersionRanges, e.g. VersionSpec("[0-1, 3-5]") # before VersionSepc(["0-1", "3-5") # after
A leftover from the change String->UUID in packages representation.
This was discarding relevant info from the log.
Don't count color directives when computing the padding (this also affects the builtin rpad function)
980096c
to
bffd063
Compare
The new tests are based on a timer. So this also adds a `yield` call in the backtracking part of the resolver's code to give the scheduler an opportunity to interrupt it.
I have now added tests in a new commit, but I had to work around the difficulties in finding a MWE that I was mentioning in the OP. So the tests are based on a reduced version, which however would work fine even before this patch, only it would take a couple of minutes or so. With the patch, it takes less than a second. So the test uses a timeout with a limit of 10 seconds, after which it interrupts the resolver. In order to do this, I had to implement a timeout macro and add an explicit
I don't think that issue can be addressed in this context. |
For the timeout stuff, what about instead of yield and throwto (which AFAIK wouldn't be robust) replace the yield with a check for the elapsed time since the resolve started, against a global timeout variable that's high for normal usage and tests could reduce for test speed? |
Is this good to go? |
It might be good to go, but then there was @IanButterworth's comment about setting a timeout in the resolver itself. I have been thinking about how to do it but didn't get around to give it a try due to lack of time, sorry. I'll give it a go as soon as I can. |
This fixes #3878 by adding an extra step during graph simplification, which I'm calling "version validation".
The validation consists in going through each version of each package (including the "uninstalled" pseudo-version), temporarily pinning it, and checking whether constraint propagation would lead to impossible scenarios. Versions which do that can be disabled to further simplify the graph. But most importantly it may turn out that all versions of a package are invalid, in which case the resolver has proved that the problem is unsatisfiable and it stops with an error.
In order to make this a little faster, I'm using a heuristic in which we don't really check all versions of a package, we stop as soon as a valid version is found (a proper one, not "uninstalled"), starting from higher versions and going down. This retains the property that impossible packages may be detected.
In my experiments, the overhead seems acceptable (nearly unnoticeable most of the times, sometimes it even makes things faster by passing a simpler graph to the solver). However, I would urge other people to give this a try, especially if someone has projects involving many dependencies.
The main commit is the last one, the others are largely independent (please don't squash them); I may create a separate PR for each if deemed necessary. It's all stuff that came out while I was working on the main issue.
Besides speed concerns, there are two more minor issues:
I haven't added new tests specific to the problem in Resolving takes hours #3878. The problem is that in order to trigger the issue we need such a big graph that just parsing it takes minutes (locally, I have been using a 15Mb text data file).(now there are tests)"Optimization [7f7a1694]: restricted by version validation to versions: 3.5.0 - 3.19.3 (7/22 versions removed)"
). If all versions of a package are removed, then we say so, and trigger a failure by providing an example log of what would happen if installing one of the versions (example below).Advice/comments about these points would also be welcome.
Below is an example of the validator catching an impossible situation (the same described here) before the actual optimizer runs (this is on julia master, resolving takes 2 or 3 seconds on my laptop). Here it was found that
RigidBodyDynamics
cannot be installed. So, for the sake of providing a log, its version was pinned at 2.4.0 and then propagating the constraints triggered an issue withModelingToolkit
. If you look closely, you'll see that in theRigidBodyDynamics
sub-tree there is the sentence"pinned to version 2.4.0 during version validation"
.