Switch to MathOptInterface #330

ericphanson · 2019-09-11T16:34:52Z

Still needed to match features under MPB:

Dual values for affine problems
Warmstarts

~~Some things are left to do, hence the (WIP)~~, but I thought I'd open this now to get CI on it and in case anyone wanted to look it over already.

Closes #262, and addresses the last high priority point of #320. Also fixes #288 and fixes #337.

~~The diff looks really big, but that's because this PR is built on the ProblemDepot (i.e. the first commit here), so skip that to see the MOI changes. The ProblemDepot is in #328.~~ The ProblemDepot was merged and this is rebased on that, so the diff isn't quite so gigantic.

The commit "Add initial MOI support …" reuses the MPB standard form to send the problem to MOI; this is a bit wasteful but it gets things working. The next commit, "Rewrite load_problem to not construct MBP form" instead directly sends the problem to MOI, which lets us avoid constructing more than we need to. I include both because it's harder to understand the final code without seeing the transition.

Some decisions and the reasoning behind them:

Can we support both MPB and MOI at the same time? I thought yes at first, but then I remembered they have different representations of SDP variables, which means we'd need to support two separate conic_form!s, which would mean we need a mechanism to choose the conic_form! based on the solver. That seemed too complicated.
Why base this PR on the Problem Depot PR? It made writing the tests easier, because with MOI we need to use a new solver object for each solve, so it doesn't work to have a global solvers = [SCS.Optimizer(), ECOS.Optimizer()]; we could pass around a vector of functions () -> SCS.Optimizer() and then call it with solver() instead of solver, but that seems less elegant than the Problem Depot callback. Moreover, this let me get rid of the can_solve_* functions which otherwise would need to be updated for MOI. I think they are only used for the Convex.jl tests (although getting rid of them is definitely breaking), and Problem Depot solves the same problem in a more nuanced way.

chriscoey · 2019-09-11T17:43:56Z

Wow, nice. IMO not worth supporting MPB.

ericphanson · 2019-09-11T18:01:16Z

I came to the same conclusion— actually, the commit called “Try allowing both MBP and MOI” was supposed to be called “Remove MathProgBase”, I just messed up the rebase.

rofinn · 2019-10-10T19:48:25Z

test/runtests.jl

-    include("test_mip.jl")
+
+    @testset "SCS" begin
+        run_tests(; exclude=[   r"mip",


Might be better as:

# bug: https://github.com/JuliaOpt/SCS.jl/issues/153 run_tests(; exclude=[r"mip", r"sdp_matrix_frac_atom"]) do p solve!(p, SCS.Optimizer(verbose=0, eps=1e-6)) end

?

Thanks very much for the review! I kind of prefer the current way, since it makes it clear that the bug is only with sdp_matrix_frac_atom; as you might know, the fact that SCS cannot handle mixed-integer problems is just a limitation of SCS (which we used to gate around by can_solve_mip conditionals in the tests-- now with the new system, we have to be explicit, which I think is an advantage). What is the advantage of putting the comment above?

What is the advantage of putting the comment above?

Just that it makes the actual code easier to read and doesn't require lining up the elements of the array in a certain way.

Ok, fair enough. I moved the comment up but added a few words to say that it's the sdp_matrix_frac_atom test which hits the SCS bug. Hopefully then we have the best of both.

rofinn · 2019-10-10T20:04:33Z

src/problems.jl

@@ -107,121 +87,34 @@ function conic_form!(p::Problem, unique_conic_forms::UniqueConicForms=UniqueConi
    return objective, objective_var.id_hash
 end

-function conic_problem(p::Problem)


I may have missed it, but does this function not exist anymore? It's still referenced in the updated docs, but that appears to be it?

Yes, I did remove this function, and I forgot to update the ProblemDepot docs that mention it. I just pushed a commit to fix that-- Thanks for pointing that out! I probably should not have added it to the docs in the first place (~~since they are live now, mentioning that function~~ edit: nope, I forgot we hadn't merged #328 yet. It's mentioned in Fixing the Guts but that's documenting internals), because that makes this removal slightly breaking. But I was planning on making a breaking release for MOI anyway, since one needs different syntax for the solvers.

The reason I removed the function is that it was quite specific to MPB: it constructs the MPB standard form, along with some extra information. The closest analog now with MOI is probably load_MOI_model!, although that both constructs the various MOI structures that define the problem and loads them into a given MOI model. The reason for this coupling is that with MOI, one can't construct the objects separately the same way one could with MPB, because you have to ask the model to give you some variables to use in defining the constraints etc, whereas in MPB it just assumed the variables were 1:n where n is the length of the objective vector.

Cool, that all seems reasonable. I just wanted to clarify that load_MOI_model! was the closest thing to what this previously did and that we want to remove that reference from the docs. Thanks for fixing that up already 👍

ericphanson · 2019-10-28T17:37:07Z

I added variable warmstarts just now, as well as some comments and cleanup.

Actually, I had variable warmstarts working about a month ago but I hoped to add constraint dual warmstarts as well, but I had a bit of trouble getting it working (minimal example: https://gist.github.com/ericphanson/9ab9f8460bbe5200be82d0b0cb55ecd5), either due to a bug in MOI or a bug in my understanding :). However, as @mlubin pointed out on the JuMP-dev gitter, we don't support constraint dual warmstarts under MPB (we can see this directly in the code, shown below), so we probably shouldn't consider this as blocking for merging this PR.

Hence, to my knowledge this PR now matches the current functionality we have, except for the obvious changes of not supporting MPB-only solvers such as Pajarito (currently), while adding support for MOI-only solvers, such as SDPAFamily.jl.

The only user-facing change should be accessing solvers via e.g. SCS.Optimizer() instead of SCSSolver().

No constraint warmstarts: See

https://github.com/JuliaOpt/Convex.jl/blob/e0513acc7a2fa3c22325c7ea9e528b0a340f068b/src/solution.jl#L41-L43

and

https://github.com/JuliaOpt/Convex.jl/blob/e0513acc7a2fa3c22325c7ea9e528b0a340f068b/src/solution.jl#L55-L88

ericphanson · 2019-10-28T19:52:49Z

src/Convex.jl

@@ -20,7 +25,6 @@ include("constraints/signs_and_sets.jl")
 include("constraints/soc_constraints.jl")
 include("constraints/exp_constraints.jl")
 include("constraints/sdp_constraints.jl")
-include("solver_info.jl")


This file had the functions such as can_solve_sdp(solver) and can_solve_mip(solver) which wrapped MPB functionality. I removed it since we only use these functions for tests, and the ProblemDepot lets us choose which tests to run for which solvers in a more nuanced way.

ericphanson · 2019-10-28T19:54:53Z

src/atoms/affine/conv.jl

@@ -11,7 +11,7 @@ function conv(x::Value, y::AbstractExpr)
    end
    m = length(x)
    n = y.size[1]
-    X = spzeros(m+n - 1, n)
+    X = spzeros(eltype(x), m + n - 1, n)


This fixes #288, but isn't really related to the MOI changes. I just had it on this branch due to using MOI's generic number type support.

ericphanson · 2019-10-28T19:56:44Z

src/constraints/sdp_constraints.jl

@@ -46,24 +46,22 @@ function conic_form!(c::SDPConstraint, unique_conic_forms::UniqueConicForms=Uniq
        # the upper triangular part (not including diagonal)
        # and the corresponding entries in the lower triangular part, so
        # symmetry => c.child[upperpart]
-        # scale off-diagonal elements by sqrt(2)
-        rescale = sqrt(2)*tril(ones(n,n))
+        rescale = triu(ones(n,n))


These changes (all of the ones in this file) are due to how MOI handles the SDP cone vs how MPB did. See https://www.juliaopt.org/MathOptInterface.jl/v0.9/apireference/#MathOptInterface.AbstractSymmetricMatrixSetTriangle

ericphanson · 2019-10-28T19:57:57Z

src/problems.jl

-const Float64OrNothing = Union{Float64, Nothing}
-
-# TODO: Cleanup
-mutable struct Solution{T<:Number}


We only used Solution for variable warmstarts but don't actually benefit from it

ericphanson · 2019-10-28T19:58:43Z

src/problems.jl

-# constr_size: m
-# var_to_ranges a dictionary mapping from variable id to (start_index, end_index)
-# where start_index and end_index are the start and end indexes of the variable in A
-function find_variable_ranges(constraints, id_to_variables)


This was moved to solution.jl under the new name get_variable_indices!

ericphanson · 2019-10-28T20:01:09Z

src/problems.jl

-                end
-            end
-            constr_size += constraint.sizes[i]
+function Base.getproperty(p::Problem, s::Symbol)


I'm especially open to feedback on this one. The reason I made optval lazy in this way is so that we don't need to query MOI.ObjectiveValue() during the solve! command (which would be eagerly populating optval). That way if the solver did not find a solution and hence may throw if you ask for its objective value, we don't error during solve! and you still get the status and termination info, along with whatever else the solver returned. This also matches how JuMP does it.

We could query to ask MOI if there is a primal feasible solution etc and only populate under those circumstances as well. It wasn't entirely clear to me exactly what conditions will guarantee get(model, MOI.ObjectiveValue()) will not throw, however.

Is this the only instance where you override getproperty? Why not deprecate p.optval in favor of objective_value that's implemented below?

ericphanson · 2019-10-28T20:03:43Z

src/utilities/show.jl

+    else
+        print(io, "\ntermination status: $(p.status)")
+        print(io, "\nprimal status: $(primal_status(p))")
+        print(io, "\ndual status: $(dual_status(p))")


MOI returns us lots of status information (termination status, primal status, dual status), and we might as well give it all to the user, otherwise they probably won't know it's there.

ericphanson · 2019-10-28T20:05:58Z

test/test_utilities.jl

@@ -1,5 +1,12 @@
 @testset "Utilities" begin

+    @testset "`solve!` does not return anything" begin


This isn't a new test; it was just moved around during the problem depot changeover and ended up here

ericphanson · 2019-10-28T20:07:11Z

test/runtests.jl


 # Seed random number stream to improve test reliability
 using Random
 Random.seed!(2)

 @testset "ProblemDepot" begin
-    @testset "Problems can run without `solve!`ing if `test==false`" begin
+    @testset "Problems can run without `solve!`ing if `test==false`; T=$T" for T in (Float64, BigFloat)


We check that our tests run even when the problem is specified with high precision numeric types

ericphanson · 2019-10-28T20:10:07Z

Ok, I tried to help make this more reviewable by merging #328 and adding some review comments. This is good to go from my side. Let me know if there are other things I can do to help make it easier to review, or if you have any questions/comments/etc.

nickrobinson251 · 2019-10-29T10:41:08Z

Thanks so much for all your work on this, @ericphanson!

We use Convex in some private code, and I will try switching it to use this branch to double check everything still runs as expected :)

nickrobinson251 · 2019-10-29T10:42:19Z

Will this is released as v0.13?

ericphanson · 2019-10-29T11:19:22Z

@nickrobinson251 awesome! Please let me know if there are any issues.

Yes, my plan was to release this as v0.13.0, maybe also with #336 fixed (lambdamin and lambdamax deprecated in favor of eigmin and eigmax) to try to group the breaking changes together. And maybe I can add a deprecation for solve!(problem, solver) for MathProgBase solver arguments to guide users to the MOI syntax. It might be good to then release all that without deprecations as 1.0.

nickrobinson251 · 2019-10-29T17:45:53Z

We use Convex in some private code, and I will try switching it to use this branch to double check everything still runs as expected :)

hmm, I switched to this branch and updated the private code from ECOSSolver <: MathProgBase.SolverInterface.AbstractMathProgSolver to instead use ECOS.Optimizer <: MathOptInterface.AbstractOptimizer (on a quadratic optimization problem, and using ECOS v0.10.1, MathOptInterface v0.9.6).

I get an error from moi_populate_solution!:

  DimensionMismatch("new dimensions (0, 1) must be consistent with array size 659")
  Stacktrace:
   [1] (::Base.var"#throw_dmrsa#197")(::Tuple{Int64,Int64}, ::Int64) at ./reshapedarray.jl:41
   [2] reshape at ./reshapedarray.jl:45 [inlined]
   [3] unpackvec(::Array{Float64,1}, ::Tuple{Int64,Int64}, ::Bool) at /Users/nick/repos/Convex.jl/src/solution.jl:311
   [4] moi_populate_solution!(::MathOptInterface.Bridges.LazyBridgeOptimizer{MathOptInterface.Utilities.CachingOptimizer{ECOS.Optimizer,MathOptInterface.Utilities.UniversalFallback{MathOptInterface.Utilities.Model{Float64}}}}, ::Convex.Problem{Float64}, ::OrderedCollections.OrderedDict{UInt64,Convex.Variable}, ::Dict{Convex.ConicConstr,Convex.Constraint}, ::Array{Convex.ConicConstr,1}, ::Dict{UInt64,Array{MathOptInterface.VariableIndex,1}}, ::Array{MathOptInterface.ConstraintIndex{MathOptInterface.VectorAffineFunction{Float64},S} where S,1}) at /Users/nick/repos/Convex.jl/src/solution.jl:287
   [5] #solve!#14(::Bool, ::Bool, ::Bool, ::typeof(Convex.solve!), ::Convex.Problem{Float64}, ::ECOS.Optimizer) at /Users/nick/repos/Convex.jl/src/solution.jl:231
   [6] solve!(::Convex.Problem{Float64}, ::ECOS.Optimizer) at /Users/nick/repos/Convex.jl/src/solution.jl:209

The problem seems to be that constr.size == (0, 1).

Adding an @show to Convex.jl/src/solution.jl:283

constr = conic_constr_to_constr[conic_constr] = <= constraint (affine)
├─ index (affine; real)
│  └─ 659-element real variable (id: 657…592)
└─ 0

ericphanson · 2019-10-29T18:08:31Z

Thanks very much for the report. Strange-- I think constr.size == (0, 1) should never happen. The size should be set here:

https://github.com/JuliaOpt/Convex.jl/blob/cba179feeaa7934e0e70011ef74c181a7e88ccbd/src/constraints/constraints.jl#L75-L78

so in this case it should be (659, 1). Now, LtConstraint is a mutable struct so it could get changed somewhere, somehow. Just to check, constr.size == (0, 1) and constr isa LtConstraint both hold? And constr.lhs.size == (659, 1) and constr.rhs.size == (1,1)?

ericphanson · 2019-10-31T18:04:12Z

Before merging this, it might be good to think a bit about the user-facing API. Right now, if a user does eg

optimizer = SCS.Optimizer(verbose=0)
p = ...
solve!(p, optimizer)
q = ...
solve!(q, optimizer)

This won't work, since solve! expects a fresh optimizer instance for each solve. JuMP currently handles this by the with_optimizer construction which holds the details needed to construct an optimizer (i.e. the constructor and its arguments). This is being discussed to be changed in jump-dev/JuMP.jl#2090 to instead just pass the constructor and then instead of using arguments to the constructor, set them via calls like set_parameter(model, verbose, 0).

We could just ask the user to always give us a fresh instance. In other words, the example above should be written eg

optimizer() = SCS.Optimizer(verbose=0)
p = ...
solve!(p, optimizer())
q = ...
solve!(q, optimizer())

and we just document / redirect users to that way of interacting with Convex. Or we can think about other ways, such something like what JuMP is doing.

ericphanson · 2019-10-31T20:00:56Z

Also, @nickrobinson251 if you have any more details or followup, I'd be quite happy to help get to the bottom of it, particularly if there's a bug in the MOI code above. You can also message me on the Julia slack (I'm @ericphanson there too) if that's easier.

nickrobinson251 · 2019-10-31T21:11:31Z

Thanks! I'll try to look at this again tomorrow :)

mlubin

I only eyeballed the code, but looks great. Glad to see Convex enter the MOI world!

mlubin · 2019-11-01T00:00:36Z

src/problems.jl

-                end
-            end
-            constr_size += constraint.sizes[i]
+function Base.getproperty(p::Problem, s::Symbol)


Is this the only instance where you override getproperty? Why not deprecate p.optval in favor of objective_value that's implemented below?

src/solution.jl

ericphanson · 2019-11-27T20:39:19Z

Thanks! I'll try to look at this again tomorrow :)

Friendly ping @nickrobinson251 :)

nickrobinson251 · 2019-11-27T20:49:36Z

Yeah, sorry about that! It's possible I'll be able to investigate this before the weekend but unlikely, so if not I think just go ahead - don't let me hold up the pr.

ericphanson · 2019-11-27T21:53:09Z

Yeah, sorry about that! It's possible I'll be able to investigate this before the weekend but unlikely, so if not I think just go ahead - don't let me hold up the pr.

Ok, sounds good. I think this might sit on master for a bit anyway even after it's merged, because we'll want to finish all the v0.13-tagged stuff first anyway before tagging a release.

ericphanson · 2019-11-28T13:02:43Z

Ah good, CI is failing because the examples need to be updated for MOI. (Wasn't sure if that would fail the docs build or not, but glad to see it does).

Add parametric type to `Problem` Fix `conv` for complex inputs Change type parametrization from Number to Real Add another test to check BigFloat data really is passed through `conic_problem` Add type parameter to testsets Test with MOI Try using a bridge for SDPA Relax `solve!` signature Refactor, run formatter to clean up Add bridge to optimizer for SDPA Fix `test_const.jl` with solver change remove optimizer return Loosen types for sets and constrs Change `can_solve_*` functions to test more atoms

refactor (doesn't work yet) test with Mosek Fixed! Clean up, test ECOS Update for MOI 0.9 Fix embarrassing typo, save model for easy access Switch to MOIU.Model (now exponential cones work) Update Project.toml Update Project.toml fixed add_terms! Notation Fix tests

Ignore test/Project.toml for local testing Merge `remove-id-to-variables` to remove globals Update MOI solves for no-globals Fix type mistake for MPB Test with MOI and MPB (sdp failures) Remove MathProgBase Order Project.toml (from `resolve` after a rebase) Fix tests Update benchmarks Remove `is_optimal`

Update tests to allow choice of numeric type

Add another SOCP label Move `solve!` empty return test Remove code duplication; test duals explicitly

Fix typo

Move comment for code clarity

Add test for jump-dev#337

Simplify objective, add comments

Fix `show` and its tests

ericphanson · 2019-12-02T21:43:45Z

Ok, I think all review comments have been addressed (modulo #346 which I'll leave for a separate PR), and with the docs and examples updated and codecov passing, I think we're good to go here.

iamed2 · 2019-12-02T22:49:25Z

Fantastic!

ericphanson mentioned this pull request Sep 12, 2019

Update MOI.Test to support any coefficient that is a subtype of Real jump-dev/MathOptInterface.jl#841

Closed

This was referenced Sep 24, 2019

Introducing the Convex.jl Problem Depot #328

Merged

error S not defined ds4dm/Tulip.jl#27

Closed

(WIP) Allow variables to carry constraints, custom variable types #313

Closed

ranocha mentioned this pull request Sep 27, 2019

Call for test problems with dual values #290

Closed

ericphanson mentioned this pull request Oct 8, 2019

add more tests, in particular for dual variables, cf. #290 #332

Merged

rofinn reviewed Oct 10, 2019

View reviewed changes

ericphanson mentioned this pull request Oct 13, 2019

Dual values of complex constraints wrong, poisons later dual values #337

Closed

ericphanson changed the title ~~(WIP) Switch to MathOptInterface~~ Switch to MathOptInterface Oct 28, 2019

ericphanson force-pushed the MathOptInterface branch from 1a88fd6 to d6c9b26 Compare October 28, 2019 19:49

ericphanson commented Oct 28, 2019

View reviewed changes

This was referenced Oct 28, 2019

Parametrize Problem with a numeric type T #289

Closed

Dev plan #320

Closed

mlubin approved these changes Nov 1, 2019

View reviewed changes

ericphanson mentioned this pull request Nov 1, 2019

quadform returns a 1x1 matrix #341

Closed

ericphanson added this to the v0.13.0 milestone Nov 1, 2019

ericphanson mentioned this pull request Nov 5, 2019

Update docs and cleanup #343

Merged

ericphanson mentioned this pull request Nov 27, 2019

Better public API (deprecate field access) #346

Closed

ericphanson force-pushed the MathOptInterface branch from 06013c6 to 6b6c106 Compare November 27, 2019 21:39

ericphanson added 16 commits December 1, 2019 20:21

Allow passing numeric type by keyword

adc6473

Update tests to allow choice of numeric type

Add socp labels to problems; test duals explicitly

ad0f541

Add another SOCP label Move `solve!` empty return test Remove code duplication; test duals explicitly

Add dual variables; remove legacy Solution

dcf400e

Fix typo

Fixes from @rofinn's review

799078a

Move comment for code clarity

Add tests from jump-dev#322

95eb330

Fix missing reshape, jump-dev#337

c25639d

Add test for jump-dev#337

Add variable warmstarts

55fbbf0

Cleanup; slightly refactor; add comments

698bb1c

Simplify objective, add comments

Rename file MOI_solve to solution

4e188b4

Add primal and dual status to show; lazy optval

af6fd2d

Fix `show` and its tests

Update README for MOI syntax

de51328

Remove test_moi.jl; these tests are in the problem depot

9182c88

Update docs and examples for MOI

65e47e6

ericphanson force-pushed the MathOptInterface branch from 6b6c106 to 65e47e6 Compare December 1, 2019 20:49

ericphanson added 2 commits December 1, 2019 20:58

Update from code review: remove MPB comments

86214c6

Restore MAXWIDTH test; add solved problem show test; reformat

e6587e3

ericphanson merged commit 60d8797 into jump-dev:master Dec 2, 2019

ericphanson mentioned this pull request Jan 14, 2020

AbstractVariable interface; allow variables to carry constraints #358

Merged

4 tasks

		@@ -1,5 +1,12 @@
		@testset "Utilities" begin

		@testset "`solve!` does not return anything" begin

Switch to MathOptInterface #330

Switch to MathOptInterface #330

Conversation

ericphanson commented Sep 11, 2019 • edited Loading

chriscoey commented Sep 11, 2019

ericphanson commented Sep 11, 2019

rofinn Oct 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericphanson Oct 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericphanson Oct 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericphanson commented Oct 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericphanson Oct 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericphanson commented Oct 28, 2019 • edited Loading

nickrobinson251 commented Oct 29, 2019

nickrobinson251 commented Oct 29, 2019

ericphanson commented Oct 29, 2019

nickrobinson251 commented Oct 29, 2019 • edited Loading

ericphanson commented Oct 29, 2019

ericphanson commented Oct 31, 2019

ericphanson commented Oct 31, 2019

nickrobinson251 commented Oct 31, 2019

mlubin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericphanson commented Nov 27, 2019

nickrobinson251 commented Nov 27, 2019

ericphanson commented Nov 27, 2019

ericphanson commented Nov 28, 2019

ericphanson commented Dec 2, 2019

iamed2 commented Dec 2, 2019

ericphanson commented Sep 11, 2019 •

edited

Loading

rofinn Oct 10, 2019 •

edited

Loading

ericphanson Oct 11, 2019 •

edited

Loading

ericphanson Oct 11, 2019 •

edited

Loading

ericphanson commented Oct 28, 2019 •

edited

Loading

ericphanson Oct 28, 2019 •

edited

Loading

ericphanson commented Oct 28, 2019 •

edited

Loading

nickrobinson251 commented Oct 29, 2019 •

edited

Loading