Improve the local optimizer at the end of resolve (#2715)

* Improve the local optimizer at the end of resolve

For each package, instead of trying to bump just one package,
try to propagate the effect of the bump outward through the graph.

Simple example: if package Av1 depends on Bv1 and Av2 depends on Bv2,
and we are considering bumping Av1 to Av2, with the previous code we
would just give up. With this change, we try to bump Bv1 to Bv2 and see
if that works.

Still todo: polishing, tests, improve logging (?)

* Fix a bug, further improve the optimizer, add test

Bugfix: in propagate_constraints, actually propagate the constraints
throughout the reachable graph. Otherwise, inconsistent situations might
arise (under peculiar circumstances).

Optimizer improvements: allow the local optimizer to also install new
packages, if this allows other packages to be bumped.

The test is a reduced form that fails with the previous optimizer (even
with the bug fix) and succeeds with the new one.

(cherry picked from commit 0f66046)

Author: carlobaldassi

src/Resolve/Resolve.jl

@@ -341,57 +341,18 @@ function verify_solution(sol::Vector{Int}, graph::Graph)
     return true
 end
 
+
 """
-Push the given solution to a local optimium if needed: keeps increasing
-the states of the given solution as long as no constraints are violated.
-It also removes unnecessary parts of the solution which are unconnected
-to the required packages.
+Uninstall unreachable packages:
+start from the required ones and keep only the packages reachable from them along the graph.
 """
-function enforce_optimality!(sol::Vector{Int}, graph::Graph)
+function _uninstall_unreachable!(sol::Vector{Int}, why::Vector{Union{Symbol,Int}}, graph::Graph)
     np = graph.np
     spp = graph.spp
     gadj = graph.gadj
     gmsk = graph.gmsk
     gconstr = graph.gconstr
-    pkgs = graph.data.pkgs
 
-    # keep a track for the log
-    why = Union{Symbol,Int}[0 for p0 = 1:np]
-
-    restart = true
-    while restart
-        restart = false
-        for p0 = 1:np
-            s0 = sol[p0]
-            s0 == spp[p0] && (why[p0] = :uninst; continue) # the package is not installed
-
-            # check if bumping to the higher version would violate a constraint
-            gconstr[p0][s0+1] || (why[p0] = :constr; continue)
-
-            # check if bumping to the higher version would violate a constraint
-            viol = false
-            for (j1,p1) in enumerate(gadj[p0])
-                s1 = sol[p1]
-                msk = gmsk[p0][j1]
-                if !msk[s1, s0+1]
-                    viol = true
-                    why[p0] = p1
-                    break
-                end
-            end
-            viol && continue
-
-            # So the solution is non-optimal: we bump it manually
-            sol[p0] += 1
-            restart = true
-        end
-    end
-
-    # Finally uninstall unneeded packages:
-    # start from the required ones and keep only
-    # the packages reachable from them along the graph.
-    # (These should have been removed in the previous step, but in principle
-    # an unconnected yet self-sustaining cycle may have survived.)
     uninst = trues(np)
     staged = Set{Int}(p0 for p0 = 1:np if !gconstr[p0][end])
     seen = copy(staged)
@@ -400,7 +361,7 @@ function enforce_optimality!(sol::Vector{Int}, graph::Graph)
         staged_next = Set{Int}()
         for p0 in staged
             s0 = sol[p0]
-            @assert s0 < spp[p0]
+            s0 == spp[p0] && continue
             uninst[p0] = false
             for (j1,p1) in enumerate(gadj[p0])
                 gmsk[p0][j1][end,s0] && continue # the package is not required by p0 at version s0
@@ -415,6 +376,173 @@ function enforce_optimality!(sol::Vector{Int}, graph::Graph)
         sol[p0] = spp[p0]
         why[p0] = :uninst
     end
+end
+
+"""
+Push the given solution to a local optimum if needed: keeps increasing
+the states of the given solution as long as no constraints are violated.
+It might also install additional packages, if needed to bump the ones already
+installed.
+It also removes unnecessary parts of the solution which are unconnected
+to the required packages.
+"""
+function enforce_optimality!(sol::Vector{Int}, graph::Graph)
+    np = graph.np
+    spp = graph.spp
+    gadj = graph.gadj
+    gmsk = graph.gmsk
+    gconstr = graph.gconstr
+    pkgs = graph.data.pkgs
+
+    # keep a track for the log
+    why = Union{Symbol,Int}[0 for p0 = 1:np]
+
+    # Strategy:
+    # There's a cycle in which first the unnecessary (unconnected) packages are removed,
+    # then we make a pass over the whole packages trying to bump each of them.
+    # We repeat the above two steps until no further action is allowed.
+    # When attempting to bump a package, we may attempt to bump or install other packages
+    # if needed. Except if the bump would uninstall a package, in which cases we don't
+    # touch anything else: we do it only if it has no consequence at all. This strategy
+    # favors installing packages as needed.
+    # During the bumping pass, we keep an upper and lower bound for each package, which
+    # progressively shrink. These are used when adjusting for the effect of an attempted bump.
+    # The way it's written should ensure that no package is ever downgraded (unless it was
+    # originally unneeded, and then got removed, and later reinstalled to a lower version as
+    # a consequence of a bump of some other package).
+
+    # move_up is used to keep track of which packages can move up
+    # (they start installed and can be bumped) and which down (they start uninstalled and
+    # can be installed)
+    move_up = BitVector(undef, length(sol))
+    # lower and upper bounds on the valid range of each package
+    upperbound = similar(spp)
+    lowerbound = similar(spp)
+    # backup space for restoring the state if an attempted bump fails
+    bk_sol = similar(sol)
+    bk_lowerbound = similar(lowerbound)
+    bk_upperbound = similar(upperbound)
+
+    # auxiliary sets to perform breadth-first search on the graph
+    seen = Set{Int}()
+    staged = Set{Int}()
+    staged_next = Set{Int}()
+
+    old_sol = similar(sol)       # to detect if we made any changes
+    allsols = Set{Vector{Int}}() # used to make 100% sure we avoid infinite loops
+
+    while true
+        copy!(old_sol, sol)
+        push!(allsols, copy(sol))
+
+        # step 1: uninstall unneded packages
+        _uninstall_unreachable!(sol, why, graph)
+
+        # setp 2: try to bump each installed package in turn
+
+        move_up .= sol .≠ spp
+        copy!(upperbound, spp)
+        let move_up = move_up
+            lowerbound .= [move_up[p0] ? sol[p0] : 1 for p0 = 1:np]
+        end
+
+        for p0 = 1:np
+            s0 = sol[p0]
+            s0 == spp[p0] && (why[p0] = :uninst; continue) # the package is not installed
+            move_up[p0] || continue # the package is only installed as a result of a previous bump, skip it
+
+            @assert upperbound[p0] == spp[p0]
+
+            # pick the next version that doesn't violate a constraint (if any)
+            bump_range = collect(s0+1:spp[p0])
+            bump = let gconstr = gconstr
+                findfirst(v0->gconstr[p0][v0], bump_range)
+            end
+
+            # no such version was found, skip this package
+            bump ≡ nothing && (why[p0] = :constr; continue)
+
+            # assume that we will succeed in bumping the version (otherwise we
+            # roll-back at the end)
+
+            new_s0 = bump_range[bump]
+            try_uninstall = new_s0 == spp[p0] # are we trying to uninstall a package?
+
+            copy!(bk_sol, sol)
+            copy!(bk_lowerbound, lowerbound)
+            copy!(bk_upperbound, upperbound)
+            sol[p0] = new_s0
+
+            # if we're trying to uninstall, the bump is "soft": we don't update the
+            # lower bound so that the package can be reinstalled later in the pass
+            # if needed by another package
+            try_uninstall || (lowerbound[p0] = new_s0) # note that we're in the move_up case
+
+            empty!(seen)
+            empty!(staged)
+            empty!(staged_next)
+            push!(staged, p0)
+
+            while !isempty(staged)
+                for f0 in staged
+                    for (j1,f1) in enumerate(gadj[f0])
+                        f1 == p0 && continue
+                        f1 ∈ seen && continue
+                        s1 = sol[f1]
+                        msk = gmsk[f0][j1]
+                        if try_uninstall
+                            bump_range = [s1] # when uninstalling, no further changes are allowed
+                        else
+                            lb1 = lowerbound[f1]
+                            ub1 = upperbound[f1]
+                            @assert lb1 ≤ s1 ≤ ub1
+                            if move_up[f1]
+                                s1 > lb1 && @assert s1 == spp[f1]
+                                    # the arrangement of the range gives precedence to improving the
+                                    # current situation, but allows reinstalling a package if needed
+                                bump_range = vcat(s1:ub1, s1-1:-1:lb1)
+                            else
+                                bump_range = collect(ub1:-1:lb1)
+                            end
+                        end
+                        bump = let gconstr = gconstr
+                            findfirst(v1->(gconstr[f1][v1] && msk[v1, sol[f0]]), bump_range)
+                        end
+                        if bump ≡ nothing
+                            why[p0] = f1 # TODO: improve this? (ideally we might want the path from p0 to f1)
+                            @goto abort
+                        end
+                        new_s1 = bump_range[bump]
+                        sol[f1] = new_s1
+                        new_s1 == s1 && continue
+                        push!(staged_next, f1)
+                        if move_up[f1]
+                            lowerbound[f1] = new_s1
+                        else
+                            upperbound[f1] = new_s1
+                        end
+                    end
+                end
+                union!(seen, staged)
+                staged, staged_next = staged_next, staged
+                empty!(staged_next)
+            end
+
+            # if we're here the bump was successful, there's nothing more to do
+            continue
+
+            ## abort the bumping: restore the solution
+            @label abort
+
+            copy!(sol, bk_sol)
+            copy!(lowerbound, bk_lowerbound)
+            copy!(upperbound, bk_upperbound)
+        end
+        sol ≠ old_sol || break
+        # It might be possible in principle to contrive a situation in which
+        # the solutions oscillate
+        sol ∈ allsols && break
+    end
 
     @assert verify_solution(sol, graph)
 

src/Resolve/graphtype.jl

@@ -44,7 +44,7 @@ mutable struct ResolveLog
     pool::Dict{UUID,ResolveLogEntry}
 
     # journal: record all messages in order (shared between all entries)
-    journal::Vector{Tuple{UUID,String}}
+    journal::ResolveJournal
 
     # exact: keeps track of whether the resolve process is still exact, or
     #        heuristics have been employed
@@ -1019,6 +1019,8 @@ function propagate_constraints!(graph::Graph, sources::Set{Int} = Set{Int}(); lo
         Set{Int}(p0 for p0 = 1:np if !gconstr[p0][end]) :
         sources
 
+    seen = copy(staged)
+
     while !isempty(staged)
         staged_next = Set{Int}()
         for p0 in staged
@@ -1048,6 +1050,8 @@ function propagate_constraints!(graph::Graph, sources::Set{Int} = Set{Int}(); lo
                 if gconstr1 ≠ old_gconstr1
                     push!(staged_next, p1)
                     log_events && log_event_implicit_req!(graph, p1, added_constr1, p0)
+                elseif p1 ∉ seen
+                    push!(staged_next, p1)
                 end
                 if !any(gconstr1)
                     if exact
@@ -1060,6 +1064,7 @@ function propagate_constraints!(graph::Graph, sources::Set{Int} = Set{Int}(); lo
                 end
             end
         end
+        union!(seen, staged_next)
         staged = staged_next
     end
     return graph

test/resolve.jl

@@ -407,11 +407,19 @@ end
 
     @test sanity_tst(ResolveData.deps_data, ResolveData.problematic_data)
     @test resolve_tst(ResolveData.deps_data, ResolveData.reqs_data, ResolveData.want_data)
+
+    ## DEPENDENCY SCHEME 12: A LARGER, MORE DIFFICULT REALISTIC EXAMPLE
+    ## ref Pkg.jl issue #1949
+
+    include("resolvedata2.jl")
+
+    @test sanity_tst(ResolveData2.deps_data, ResolveData2.problematic_data)
+    @test resolve_tst(ResolveData2.deps_data, ResolveData2.reqs_data, ResolveData2.want_data)
 end
 
 @testset "nasty" begin
     VERBOSE && @info("SCHEME NASTY")
-    ## DEPENDENCY SCHEME 12: A NASTY CASE
+    ## DEPENDENCY SCHEME 13: A NASTY CASE
 
     include("NastyGenerator.jl")
     deps_data, reqs_data, want_data, problematic_data = NastyGenerator.generate_nasty(5, 20, q=20, d=4, sat = true)

test/resolve_utils.jl

@@ -52,12 +52,11 @@ function graph_from_data(deps_data)
         end
         isempty(r) && continue
         rp = r[1]
-        rvs = VersionSpec(r[2:end])
+        rvs = VersionSpec(r[2:end]...)
         deps[p][vn][rp] = rvs
     end
     for (p,preq) in deps
         u = uuid(p)
-
         deps_pkgs = Dict{String,Set{VersionNumber}}()
         for (vn,vreq) in deps[p], rp in keys(vreq)
             push!(get!(Set{VersionNumber}, deps_pkgs, rp), vn)
@@ -117,7 +116,26 @@ function resolve_tst(deps_data, reqs_data, want_data = nothing; clean_graph = fa
     add_reqs!(graph, reqs)
     simplify_graph!(graph, clean_graph = clean_graph)
     want = resolve(graph)
-    return want == wantuuids(want_data)
+
+    id(u) = pkgID(u, graph)
+    wd = wantuuids(want_data)
+    if want ≠ wd
+        for (u,vn) in want
+            if u ∉ keys(wd)
+                @info "resolver decided to install $(id(u)) (v$vn), package wasn't expected"
+            elseif vn ≠ wd[u]
+            @info "version mismatch for $(id(u)), resolver wants v$vn, expected v$(wd[u])"
+            end
+        end
+        for (u,vn) in wd
+            if u ∉ keys(want)
+                @info "was expecting the resolver to install $(id(u)) (v$vn)"
+            end
+        end
+        return false
+    else
+        return true
+    end
 end
 
 end