Skip to content
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

Completely re-write the Gurobi MOI wrapper. #216

Merged
merged 44 commits into from
Aug 13, 2019
Merged

Completely re-write the Gurobi MOI wrapper. #216

merged 44 commits into from
Aug 13, 2019

Conversation

odow
Copy link
Member

@odow odow commented Jun 10, 2019

All tests pass locally

image

Most recent benchmarks compared to current Gurobi master:

========== Results ==========

delete_constraint
BenchmarkTools.TrialJudgement: 
  time:   -96.03% => improvement (5.00% tolerance)
  memory: -98.63% => improvement (1.00% tolerance)

add_variable_constraints
BenchmarkTools.TrialJudgement: 
  time:   -32.44% => improvement (5.00% tolerance)
  memory: -36.64% => improvement (1.00% tolerance)

add_variables
BenchmarkTools.TrialJudgement: 
  time:   -20.63% => improvement (5.00% tolerance)
  memory: -31.15% => improvement (1.00% tolerance)

add_constraint
BenchmarkTools.TrialJudgement: 
  time:   -48.83% => improvement (5.00% tolerance)
  memory: -46.73% => improvement (1.00% tolerance)

add_variable
BenchmarkTools.TrialJudgement: 
  time:   -29.04% => improvement (5.00% tolerance)
  memory: -82.91% => improvement (1.00% tolerance)

delete_variable_constraint
BenchmarkTools.TrialJudgement: 
  time:   -52.91% => improvement (5.00% tolerance)
  memory: -76.74% => improvement (1.00% tolerance)

add_variable_constraint
BenchmarkTools.TrialJudgement: 
  time:   -52.85% => improvement (5.00% tolerance)
  memory: -54.30% => improvement (1.00% tolerance)

add_constraints
BenchmarkTools.TrialJudgement: 
  time:   -29.20% => improvement (5.00% tolerance)
  memory: -16.59% => improvement (1.00% tolerance)

delete_variable
BenchmarkTools.TrialJudgement: 
  time:   -35.54% => improvement (5.00% tolerance)
  memory: +6315.52% => regression (1.00% tolerance)

!!! The massive increase in memory in delete_variable is due to the rebuild of the OrderedDict !!!

Notable changes include:

  • removing the dependence on LQOI
  • more comprehensive coverage of MOI tests
  • removing VectorAffine support

I found a couple of bugs in MOIT, will push a PR soon.

Todos

  • RawParameter
  • Indicator constraints
  • BasisStatus
  • benchmark compared to LQOI
  • add_constraints?

Closes #217
Closes #215
Closes #213
Closes #203
Closes #201
Closes #144

@odow odow changed the title WIP: Completely re-write the Gurobi MOI wrapper. Completely re-write the Gurobi MOI wrapper. Jun 10, 2019
test/MOI_wrapper.jl Outdated Show resolved Hide resolved
test/MOI_wrapper.jl Outdated Show resolved Hide resolved

@testset "start_values_test" begin
# We don't support ConstraintDualStart or ConstraintPrimalStart yet.
# @test_broken MOIT.start_values_test(Gurobi.Optimizer(GUROBI_ENV), OPTIMIZER)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: consider implementing ConstraintDualStart and ConstraintPrimalStart

@chriscoey
Copy link
Contributor

Are we saying bye bye to LQOI?

src/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI_wrapper.jl Outdated Show resolved Hide resolved
@odow
Copy link
Member Author

odow commented Jun 10, 2019

Are we saying bye bye to LQOI?

Not definitely, but I would like to. I think this wrapper is considerably more readable than previously (it has a lot less re-direction) at the cost of some duplication (2000 lines compared to 500 + LQOI).

Because of all the tests, it was pretty easy to implement, if a little tedious. It also enables Gurobi-specific work-arounds like the needs_update thing.

@chriscoey
Copy link
Contributor

Sounds reasonable. Hopefully it makes it easier to maintain and extend.

Copy link
Member

@mlubin mlubin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Closes 5 issues :)
This really lowers the barrier to understanding the wrapper.

src/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI_wrapper.jl Show resolved Hide resolved
src/MOI_wrapper.jl Show resolved Hide resolved
src/MOI_wrapper.jl Show resolved Hide resolved
src/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI_wrapper.jl Show resolved Hide resolved
@odow
Copy link
Member Author

odow commented Jun 11, 2019

Okay, now with an implementation of CleverDict™. Vector-backed storage so long as no elements are deleted. Keeps track of an offset so MOI.empty! works. Switches to Dict on delete!, and then back to vector on empty!.

@odow
Copy link
Member Author

odow commented Jun 12, 2019

EDIT: latest benchmarks are at the top of the PR.

Now with a benchmarking script, that yields the following compared to Gurobi#master + LQOI#master.

add_constraints is slower because we haven't implemented that yet.

========== Results ==========

delete_constraint
BenchmarkTools.TrialJudgement: 
  time:   -53.10% => improvement (5.00% tolerance)
  memory: -76.74% => improvement (1.00% tolerance)

add_variable_constraint
BenchmarkTools.TrialJudgement: 
  time:   -47.47% => improvement (5.00% tolerance)
  memory: -85.41% => improvement (1.00% tolerance)

add_constraints
BenchmarkTools.TrialJudgement: 
  time:   +18.24% => regression (5.00% tolerance)
  memory: -55.91% => improvement (1.00% tolerance)

Project.toml Show resolved Hide resolved
perf/perf.jl Outdated Show resolved Hide resolved
src/CleverDict.jl Outdated Show resolved Hide resolved
src/CleverDict.jl Outdated Show resolved Hide resolved
src/CleverDict.jl Outdated Show resolved Hide resolved
@odow
Copy link
Member Author

odow commented Jun 13, 2019

Okay, things could do with a review again. I can't believe how much speed we left on the table with LQOI (see first post). The benchmarks definitely call for re-writing individual wrappers. It's also now much easier to test improvements via benchmarking. I wonder if we should roll these out via MOI.

src/CleverDict.jl Outdated Show resolved Hide resolved
@mlubin
Copy link
Member

mlubin commented Jun 14, 2019

It's also now much easier to test improvements via benchmarking. I wonder if we should roll these out via MOI.

Makes sense. The CleverDict could also go to MOIU if it's going to be used in multiple wrappers.

src/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI_wrapper.jl Outdated Show resolved Hide resolved
# constraints cannot be distinguished from previously created ones.
last_constraint_index::Int
# ScalarAffineFunction{Float64}-in-Set storage.
affine_constraint_info::Dict{Int, ConstraintInfo}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why these are not Clever ones?

Copy link

@coroa coroa Jul 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the last_constraint_index counter is shared over the types of constraints, so that there would be large holes in the CleverDict Array automatically.

Note, that this is not necessary, since the i in ConstraintIndex{F,S}(i) must only be unique per F, S type (see docstring). It would probably be better to switch to last_{affine,sos,quadratic}_constraint_index and CleverDict for each of them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly because it's not performance critical. The only time we really need it is querying duals/ConstraintFunctions/etc. The extra hash doesn't matter. In contrast, we get column indices all the time, so it makes sense to make these fast.

@odow
Copy link
Member Author

odow commented Jul 11, 2019

image

@blegat
Copy link
Member

blegat commented Jul 11, 2019

What is the 0printed at the end ?

@remi-garcia
Copy link

I'm reading this to learn and I found a typo, not exactly in the PR though
https://github.com/JuliaOpt/Gurobi.jl/blob/bf90918ab72943ffb06713d72db39a63ef65a362/src/MOI_wrapper.jl#L197-L208
_require_update -> _update_if_necessary

@odow
Copy link
Member Author

odow commented Aug 8, 2019

Thanks @remi-garcia. I've corrected.

@joaquimg
Copy link
Member

MOI 0.9 almost ready.
@odow do you have plans to take a last look in this PR before we can do the same for xpress and cplex?

@odow
Copy link
Member Author

odow commented Aug 13, 2019

I'll take a look today. You should be able take a start with Xpress. It should be sufficient to copy paste the whole thing, and
then change out the Xpress specific calls.

@odow
Copy link
Member Author

odow commented Aug 13, 2019

Currently getting these errors:

Name test with MathOptInterface.Bridges.LazyBridgeOptimizer{Gurobi.Optimizer}: Test Failed at /Users/oscar/.julia/packages/MathOptInterface/mvlGI/src/Test/modellike.jl:68
  Expression: MOI.get(model, MOI.VariableIndex, "Var1")
    Expected: Exception
  No exception thrown
Stacktrace:
 [1] macro expansion at /Users/oscar/.julia/packages/MathOptInterface/mvlGI/src/Test/modellike.jl:68 [inlined]
 [2] macro expansion at /Users/osx/buildbot/slave/package_osx64/build/usr/share/julia/stdlib/v1.1/Test/src/Test.jl:1083 [inlined]
 [3] nametest(::MathOptInterface.Bridges.LazyBridgeOptimizer{Gurobi.Optimizer}) at /Users/oscar/.julia/packages/MathOptInterface/mvlGI/src/Test/modellike.jl:22
Name test with MathOptInterface.Bridges.LazyBridgeOptimizer{Gurobi.Optimizer}: Test Failed at /Users/oscar/.julia/packages/MathOptInterface/mvlGI/src/Test/modellike.jl:120
  Expression: MOI.get(model, MOI.ConstraintIndex, "Con2")
    Expected: Exception
  No exception thrown
Stacktrace:
 [1] nametest(::MathOptInterface.Bridges.LazyBridgeOptimizer{Gurobi.Optimizer}) at /Users/oscar/.julia/packages/MathOptInterface/mvlGI/src/Test/modellike.jl:120
 [2] top-level scope at /Users/oscar/.julia/dev/Gurobi/test/MOI_wrapper.jl:59
 [3] top-level scope at /Users/osx/buildbot/slave/package_osx64/build/usr/share/julia/stdlib/v1.1/Test/src/Test.jl:1083
 [4] top-level scope at /Users/oscar/.julia/dev/Gurobi/test/MOI_wrapper.jl:59
 [5] top-level scope at /Users/osx/buildbot/slave/package_osx64/build/usr/share/julia/stdlib/v1.1/Test/src/Test.jl:1083
 [6] top-level scope at /Users/oscar/.julia/dev/Gurobi/test/MOI_wrapper.jl:48
Name test with MathOptInterface.Bridges.LazyBridgeOptimizer{Gurobi.Optimizer}: Test Failed at /Users/oscar/.julia/packages/MathOptInterface/mvlGI/src/Test/modellike.jl:121
  Expression: MOI.get(model, typeof(c2), "Con2")
    Expected: Exception
  No exception thrown
Stacktrace:
 [1] nametest(::MathOptInterface.Bridges.LazyBridgeOptimizer{Gurobi.Optimizer}) at /Users/oscar/.julia/packages/MathOptInterface/mvlGI/src/Test/modellike.jl:121
 [2] top-level scope at /Users/oscar/.julia/dev/Gurobi/test/MOI_wrapper.jl:59
 [3] top-level scope at /Users/osx/buildbot/slave/package_osx64/build/usr/share/julia/stdlib/v1.1/Test/src/Test.jl:1083
 [4] top-level scope at /Users/oscar/.julia/dev/Gurobi/test/MOI_wrapper.jl:59
 [5] top-level scope at /Users/osx/buildbot/slave/package_osx64/build/usr/share/julia/stdlib/v1.1/Test/src/Test.jl:1083
 [6] top-level scope at /Users/oscar/.julia/dev/Gurobi/test/MOI_wrapper.jl:48
Name test with MathOptInterface.Bridges.LazyBridgeOptimizer{Gurobi.Optimizer}: Test Failed at /Users/oscar/.julia/packages/MathOptInterface/mvlGI/src/Test/modellike.jl:122
  Expression: MOI.get(model, typeof(cx), "Con2")
    Expected: Exception
  No exception thrown
Stacktrace:
 [1] nametest(::MathOptInterface.Bridges.LazyBridgeOptimizer{Gurobi.Optimizer}) at /Users/oscar/.julia/packages/MathOptInterface/mvlGI/src/Test/modellike.jl:122
 [2] top-level scope at /Users/oscar/.julia/dev/Gurobi/test/MOI_wrapper.jl:59
 [3] top-level scope at /Users/osx/buildbot/slave/package_osx64/build/usr/share/julia/stdlib/v1.1/Test/src/Test.jl:1083
 [4] top-level scope at /Users/oscar/.julia/dev/Gurobi/test/MOI_wrapper.jl:59
 [5] top-level scope at /Users/osx/buildbot/slave/package_osx64/build/usr/share/julia/stdlib/v1.1/Test/src/Test.jl:1083
 [6] top-level scope at /Users/oscar/.julia/dev/Gurobi/test/MOI_wrapper.jl:48
Name test with MathOptInterface.Bridges.LazyBridgeOptimizer{Gurobi.Optimizer}: Test Failed at /Users/oscar/.julia/packages/MathOptInterface/mvlGI/src/Test/modellike.jl:154
  Expression: MOI.get(model, MOI.ConstraintIndex, "Con2")
    Expected: Exception
  No exception thrown
Stacktrace:
 [1] nametest(::MathOptInterface.Bridges.LazyBridgeOptimizer{Gurobi.Optimizer}) at /Users/oscar/.julia/packages/MathOptInterface/mvlGI/src/Test/modellike.jl:154
 [2] top-level scope at /Users/oscar/.julia/dev/Gurobi/test/MOI_wrapper.jl:59
 [3] top-level scope at /Users/osx/buildbot/slave/package_osx64/build/usr/share/julia/stdlib/v1.1/Test/src/Test.jl:1083
 [4] top-level scope at /Users/oscar/.julia/dev/Gurobi/test/MOI_wrapper.jl:59
 [5] top-level scope at /Users/osx/buildbot/slave/package_osx64/build/usr/share/julia/stdlib/v1.1/Test/src/Test.jl:1083
 [6] top-level scope at /Users/oscar/.julia/dev/Gurobi/test/MOI_wrapper.jl:48
Name test with MathOptInterface.Bridges.LazyBridgeOptimizer{Gurobi.Optimizer}: Test Failed at /Users/oscar/.julia/packages/MathOptInterface/mvlGI/src/Test/modellike.jl:155
  Expression: MOI.get(model, typeof(c2), "Con2")
    Expected: Exception
  No exception thrown
Stacktrace:
 [1] nametest(::MathOptInterface.Bridges.LazyBridgeOptimizer{Gurobi.Optimizer}) at /Users/oscar/.julia/packages/MathOptInterface/mvlGI/src/Test/modellike.jl:155
 [2] top-level scope at /Users/oscar/.julia/dev/Gurobi/test/MOI_wrapper.jl:59
 [3] top-level scope at /Users/osx/buildbot/slave/package_osx64/build/usr/share/julia/stdlib/v1.1/Test/src/Test.jl:1083
 [4] top-level scope at /Users/oscar/.julia/dev/Gurobi/test/MOI_wrapper.jl:59
 [5] top-level scope at /Users/osx/buildbot/slave/package_osx64/build/usr/share/julia/stdlib/v1.1/Test/src/Test.jl:1083
 [6] top-level scope at /Users/oscar/.julia/dev/Gurobi/test/MOI_wrapper.jl:48
Name test with MathOptInterface.Bridges.LazyBridgeOptimizer{Gurobi.Optimizer}: Test Failed at /Users/oscar/.julia/packages/MathOptInterface/mvlGI/src/Test/modellike.jl:156
  Expression: MOI.get(model, typeof(cx), "Con2")
    Expected: Exception
  No exception thrown
Stacktrace:
 [1] nametest(::MathOptInterface.Bridges.LazyBridgeOptimizer{Gurobi.Optimizer}) at /Users/oscar/.julia/packages/MathOptInterface/mvlGI/src/Test/modellike.jl:156
 [2] top-level scope at /Users/oscar/.julia/dev/Gurobi/test/MOI_wrapper.jl:59
 [3] top-level scope at /Users/osx/buildbot/slave/package_osx64/build/usr/share/julia/stdlib/v1.1/Test/src/Test.jl:1083
 [4] top-level scope at /Users/oscar/.julia/dev/Gurobi/test/MOI_wrapper.jl:59
 [5] top-level scope at /Users/osx/buildbot/slave/package_osx64/build/usr/share/julia/stdlib/v1.1/Test/src/Test.jl:1083
 [6] top-level scope at /Users/oscar/.julia/dev/Gurobi/test/MOI_wrapper.jl:48
Name test with MathOptInterface.Bridges.LazyBridgeOptimizer{Gurobi.Optimizer}: Error During Test at /Users/oscar/.julia/packages/MathOptInterface/mvlGI/src/Test/modellike.jl:21
  Got exception outside of a @test
  MethodError: Cannot `convert` an object of type MathOptInterface.ConstraintIndex{MathOptInterface.ScalarAffineFunction{Float64},MathOptInterface.EqualTo{Float64}} to an object of type String
  Closest candidates are:
    convert(::Type{T<:AbstractString}, !Matched::T<:AbstractString) where T<:AbstractString at strings/basic.jl:207
    convert(::Type{T<:AbstractString}, !Matched::AbstractString) where T<:AbstractString at strings/basic.jl:208
    convert(::Type{T}, !Matched::T) where T at essentials.jl:154
  Stacktrace:
   [1] setindex!(::Dict{String,MathOptInterface.ConstraintIndex}, ::String, ::MathOptInterface.ConstraintIndex{MathOptInterface.ScalarAffineFunction{Float64},MathOptInterface.EqualTo{Float64}}) at ./dict.jl:373
   [2] set(::Gurobi.Optimizer, ::MathOptInterface.ConstraintName, ::MathOptInterface.ConstraintIndex{MathOptInterface.ScalarAffineFunction{Float64},MathOptInterface.EqualTo{Float64}}, ::String) at /Users/oscar/.julia/dev/Gurobi/src/MOI_wrapper.jl:1348
   [3] set at /Users/oscar/.julia/packages/MathOptInterface/mvlGI/src/Bridges/bridge_optimizer.jl:619 [inlined]
   [4] macro expansion at /Users/oscar/.julia/packages/MathOptInterface/mvlGI/src/Test/modellike.jl:171 [inlined]
   [5] macro expansion at /Users/osx/buildbot/slave/package_osx64/build/usr/share/julia/stdlib/v1.1/Test/src/Test.jl:1083 [inlined]
   [6] nametest(::MathOptInterface.Bridges.LazyBridgeOptimizer{Gurobi.Optimizer}) at /Users/oscar/.julia/packages/MathOptInterface/mvlGI/src/Test/modellike.jl:22
   [7] top-level scope at /Users/oscar/.julia/dev/Gurobi/test/MOI_wrapper.jl:59
   [8] top-level scope at /Users/osx/buildbot/slave/package_osx64/build/usr/share/julia/stdlib/v1.1/Test/src/Test.jl:1083
   [9] top-level scope at /Users/oscar/.julia/dev/Gurobi/test/MOI_wrapper.jl:59
   [10] top-level scope at /Users/osx/buildbot/slave/package_osx64/build/usr/share/julia/stdlib/v1.1/Test/src/Test.jl:1083
   [11] top-level scope at /Users/oscar/.julia/dev/Gurobi/test/MOI_wrapper.jl:48
   [12] include at ./boot.jl:326 [inlined]
   [13] include_relative(::Module, ::String) at ./loading.jl:1038
   [14] include(::Module, ::String) at ./sysimg.jl:29
   [15] include(::String) at ./client.jl:403
   [16] top-level scope at /Users/oscar/.julia/dev/Gurobi/test/runtests.jl:17
   [17] top-level scope at /Users/osx/buildbot/slave/package_osx64/build/usr/share/julia/stdlib/v1.1/Test/src/Test.jl:1083
   [18] top-level scope at /Users/oscar/.julia/dev/Gurobi/test/runtests.jl:17
   [19] include at ./boot.jl:326 [inlined]
   [20] include_relative(::Module, ::String) at ./loading.jl:1038
   [21] include(::Module, ::String) at ./sysimg.jl:29
   [22] include(::String) at ./client.jl:403
   [23] top-level scope at none:0
   [24] eval(::Module, ::Any) at ./boot.jl:328
   [25] exec_options(::Base.JLOptions) at ./client.jl:243
   [26] _start() at ./client.jl:436
Academic license - for non-commercial use only
Academic license - for non-commercial use only
Academic license - for non-commercial use only
Academic license - for non-commercial use only
Academic license - for non-commercial use only
Academic license - for non-commercial use only
Test Summary:                                                                       | Pass  Fail  Error  Broken  Total
MathOptInterface Tests                                                              | 2085     7      1       1   2094
  Unit Tests                                                                        |  807                         807
  Linear tests                                                                      |  517                         517
  Quadratic tests                                                                   |  190                         190
  Linear Conic tests                                                                |  135                         135
  Integer Linear tests                                                              |  139                         139
  ModelLike tests                                                                   |  213     7      1       1    222
    default_objective_test                                                          |                            No tests
    default_status_test                                                             |    3                           3
    nametest                                                                        |  103     7      1            111
      Name test with MathOptInterface.Bridges.LazyBridgeOptimizer{Gurobi.Optimizer} |  103     7      1            111
    validtest                                                                       |   11                          11
    emptytest                                                                       |   12                          12
    orderedindicestest                                                              |   13                          13
    copytest                                                                        |   37                          37
    scalar_function_constant_not_zero                                               |    2                           2
    start_values_test                                                               |    3                           3
    supports_constrainttest                                                         |    8                    1      9
    set_lower_bound_twice                                                           |   10                          10
    set_upper_bound_twice                                                           |   10                          10
  Gurobi Callback                                                                   |   10                          10
  LQOI Issue #38                                                                    |                            No tests
  User limit handling (issue #140)                                                  |    3                           3
  Constant objective (issue #111)                                                   |    4                           4
  Env                                                                               |   11                          11
  Conflict refiner                                                                  |   41                          41
  RawParameter                                                                      |    2                           2
  QCPDuals without needing to pass QCPDual=1                                        |   12                          12
  Add constraints                                                                   |    1                           1
ERROR: LoadError: Some tests did not pass: 2085 passed, 7 failed, 1 errored, 1 broken.
in expression starting at /Users/oscar/.julia/dev/Gurobi/test/runtests.jl:16
ERROR: Package Gurobi errored during testing

cc @blegat Fix incoming.

@odow
Copy link
Member Author

odow commented Aug 13, 2019

image

@odow
Copy link
Member Author

odow commented Aug 13, 2019

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
8 participants