Factory #1384

blegat · 2018-07-28T10:15:26Z

New constructor interface as discussed here.
Factories allow to create new optimizers on demand which would allow implementing copy (#1381, #1300) without the need for any change in MOI like jump-dev/MathOptInterface.jl#387.

mlubin · 2018-07-28T12:11:18Z

test/nlp_solver.jl

@@ -40,7 +38,7 @@ new_optimizer() = IpoptOptimizer(print_level=0)
        #     1 <= x1, x2, x3, x4 <= 5
        # Start at (1,5,5,1)
        # End at (1.000..., 4.743..., 3.821..., 1.379...)
-        m = Model(optimizer=new_optimizer())
+        m = Model(with_optimizer(IpoptOptimize, print_level=0))


Typo: IpoptOptimizer. This file isn't run under CI because it depends on Ipopt. Please test manually.

codecov · 2018-07-29T09:46:44Z

Codecov Report

Merging #1384 into master will decrease coverage by 4.14%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1384      +/-   ##
==========================================
- Coverage   89.58%   85.43%   -4.15%     
==========================================
  Files          25       24       -1     
  Lines        3427     3426       -1     
==========================================
- Hits         3070     2927     -143     
- Misses        357      499     +142

Impacted Files	Coverage Δ
src/JuMP.jl	`79.28% <100%> (+0.02%)`	⬆️
src/optimizerinterface.jl	`71.42% <100%> (+4.76%)`	⬆️
src/JuMPArray.jl	`29.03% <0%> (-47.01%)`	⬇️
src/Derivatives/linearity.jl	`75.51% <0%> (-20.41%)`	⬇️
src/Derivatives/forward.jl	`75.19% <0%> (-18.61%)`	⬇️
src/Derivatives/sparsity.jl	`79.01% <0%> (-16.05%)`	⬇️
src/Derivatives/coloring.jl	`90.75% <0%> (-6.94%)`	⬇️
src/containers.jl	`86.84% <0%> (-5.27%)`	⬇️
src/Derivatives/conversion.jl	`94.33% <0%> (-3.78%)`	⬇️
src/Derivatives/subexpressions.jl	`97.5% <0%> (-2.5%)`	⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64fc678...39be6af. Read the comment docs.

codecov · 2018-07-29T09:46:44Z

Codecov Report

Merging #1384 into master will increase coverage by 0.16%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1384      +/-   ##
==========================================
+ Coverage   89.58%   89.75%   +0.16%     
==========================================
  Files          25       25              
  Lines        3427     3698     +271     
==========================================
+ Hits         3070     3319     +249     
- Misses        357      379      +22

Impacted Files	Coverage Δ
src/JuMP.jl	`76.4% <100%> (-2.86%)`	⬇️
src/optimizerinterface.jl	`76.92% <100%> (+10.25%)`	⬆️
src/variables.jl	`87.82% <0%> (-0.8%)`	⬇️
src/operators.jl	`96.69% <0%> (-0.02%)`	⬇️
src/quadexpr.jl	`92.5% <0%> (+0.09%)`	⬆️
src/affexpr.jl	`97.58% <0%> (+0.12%)`	⬆️
src/macros.jl	`90.53% <0%> (+0.66%)`	⬆️
src/nlp.jl	`82.05% <0%> (+2.49%)`	⬆️
src/print.jl	`85.65% <0%> (+5.13%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64fc678...b4b4f17. Read the comment docs.

IainNZ · 2018-07-29T11:36:34Z

The word backend doesn't seem to appear in MathOptInterface.jl at all - does it have a formal definition?

mlubin · 2018-07-29T11:47:56Z

docs/src/solvers.md

-mode. CachingOptimizer. How to set/change solvers. How to set parameters (solver
+A JuMP model stores a
+[MathOptInterface (MOI)](https://github.com/JuliaOpt/MathOptInterface.jl)
+backend internally that contains the optimization solver. The JuMP layer on top


"contains" -> "acts as"?

mlubin · 2018-07-29T11:48:59Z

docs/src/solvers.md

-mode. CachingOptimizer. How to set/change solvers. How to set parameters (solver
+A JuMP model stores a
+[MathOptInterface (MOI)](https://github.com/JuliaOpt/MathOptInterface.jl)
+backend internally that contains the optimization solver. The JuMP layer on top


The backend might also not support optimize!.

mlubin · 2018-07-29T11:49:42Z

docs/src/solvers.md

-mode. CachingOptimizer. How to set/change solvers. How to set parameters (solver
+A JuMP model stores a
+[MathOptInterface (MOI)](https://github.com/JuliaOpt/MathOptInterface.jl)
+backend internally that contains the optimization solver. The JuMP layer on top


"JuMP can be viewed as a lightweight user-friendly layer on top of the MOI backend.

mlubin · 2018-07-29T11:50:34Z

docs/src/solvers.md

+  backend to support such modifications.
+
+While this allows JuMP API to to be a thin wrapper on top of the solver API,
+as mentionned in the last point above, this seems rather demanding on the


Spelling: "mentioned"

mlubin · 2018-07-29T11:51:38Z

docs/src/solvers.md

+optimizer:
+
+* `CachingOptimizer`: it maintain a cache of the model so that when the
+  the optimizer does not support a modification of the model, the optimizer's


"incremental change" seems more appropriate than "modification" (since it could include adding constraints)

mlubin · 2018-07-29T11:53:09Z

src/JuMP.jl

@@ -77,6 +78,43 @@ const MOIBIN = MOICON{MOI.SingleVariable,MOI.ZeroOne}

 @MOIU.model JuMPMOIModel (ZeroOne, Integer) (EqualTo, GreaterThan, LessThan, Interval) (Zeros, Nonnegatives, Nonpositives, SecondOrderCone, RotatedSecondOrderCone, GeometricMeanCone, PositiveSemidefiniteConeTriangle, PositiveSemidefiniteConeSquare, RootDetConeTriangle, RootDetConeSquare, LogDetConeTriangle, LogDetConeSquare) () (SingleVariable,) (ScalarAffineFunction,ScalarQuadraticFunction) (VectorOfVariables,) (VectorAffineFunction,)

+struct Factory


OptimizerFactory maybe?
Add a comment describing the purpose. It's a user-friendly closure.

mlubin · 2018-07-29T11:54:13Z

src/JuMP.jl

-            optimizer=nothing,
-            bridge_constraints=true)
+    # Inner constructor
+    function Model(factory::Union{Nothing, Factory}, moibackend::MOI.ModelLike)


Does this still need to be an inner constructor?

mlubin · 2018-07-29T11:55:17Z

src/optimizerinterface.jl

+setoptimizer(model, with_optimizer(IpoptOptimizer, print_level=0))
+```
+"""
+function setoptimizer(model::Model, factory::Factory)


We really need to decide on a convention for underscores. with_optimizer and setoptimizer are inconsistent.

I agree with using underscores, it's a typo :)

mlubin · 2018-07-29T11:56:57Z

src/JuMP.jl

+    # In Manual and Automatic mode: Factory used to create the optimizer or
+    #                               Nothing if it has not already been set
+    # In Direct mode: Nothing
+    factory::Union{Nothing, Factory}
    # In Manual and Automatic modes, LazyBridgeOptimizer{CachingOptimizer}.
    # In Direct mode, will hold an AbstractOptimizer.
    moibackend::MOI.AbstractOptimizer


Could this be an MOI.ModelLike to support non-optimizer backends?

Yes, I don't remember which PR removed that possibility. The model of the CachingOptimizer can be a MOI.ModelLike and CachingOptimizer will still be an AbstractOptimizer so this just removes the possibility to use a non-optimizer in direct mode. Do we want to allow non-optimizer in direct mode ?

mlubin · 2018-07-29T11:58:57Z

"backend" isn't an MOI concept. It's usage needs to be defined in the docs. It's called a backend instead of the optimizer because:

In Manual and Automatic modes, it's not the same optimizer as what the user provided.
I imagine that it could not support optimize!, and just be a model container, e.g., backed by an MPS file for example.

odow · 2018-07-29T22:14:15Z

docs/src/solvers.md

-TODO: Describe the connection between JuMP and solvers. Automatic vs. Manual
-mode. CachingOptimizer. How to set/change solvers. How to set parameters (solver
+A JuMP model keeps a
+[MathOptInterface (MOI)](https://github.com/JuliaOpt/MathOptInterface.jl)


Style: lines are allowed to go over 80 chars if they end in a long URL

odow · 2018-07-29T22:21:12Z

src/optimizerinterface.jl

+set_optimizer(model, with_optimizer(IpoptOptimizer, print_level=0))
+```
+"""
+function set_optimizer(model::Model, factory::OptimizerFactory)


Does this need to check if what mode the solver is in? If I'm in direct mode I can't switch right?

odow · 2018-07-29T22:22:40Z

docs/src/solvers.md

-mode. CachingOptimizer. How to set/change solvers. How to set parameters (solver
+A JuMP model keeps a
+[MathOptInterface (MOI)](https://github.com/JuliaOpt/MathOptInterface.jl)
+backend internally that stores the optimization problem and act as the


Is backend mentioned above somewhere? It might need to be defined. (Edit: should have read the above comments better.)

s/and act/and acts

odow · 2018-07-29T22:26:04Z

docs/src/solvers.md

+In Automatic and Manual modes, two MOI layers are automatically applied to the
+optimizer:
+
+* `CachingOptimizer`: it maintain a cache of the model so that when the


s/it maintain/it maintains or s/it maintain/maintains

mlubin · 2018-07-31T01:43:11Z

docs/src/solvers.md

-TODO: Describe the connection between JuMP and solvers. Automatic vs. Manual
-mode. CachingOptimizer. How to set/change solvers. How to set parameters (solver
+A JuMP model keeps a [MathOptInterface (MOI)](https://github.com/JuliaOpt/MathOptInterface.jl)
+*backend* internally that stores the optimization problem and acts as the


Say this is an MOI.ModelLike.

mlubin · 2018-07-31T01:43:55Z

docs/src/solvers.md

+  modifications are directly applied to the MOI backend thus expecting the
+  backend to support such modifications.
+
+While this allows JuMP API to to be a thin wrapper on top of the solver API,


Nit: "JuMP API" -> "JuMP" or "the JuMP API"

mlubin · 2018-07-31T01:45:54Z

docs/src/solvers.md

+  model can be discarded and restored from the cache just before optimization.
+  The `CachingOptimizer` has two different modes: Automatic and Manual
+  corresponding to the two JuMP modes with the same names.
+* `LazyBridgeOptimizer`: when a constraint added is not supported by the


Mention that this can be disabled using the bridge_constraints keyword argument to Model.

mlubin · 2018-07-31T01:47:45Z

docs/src/solvers.md

+
+To create a fresh new JuMP model (or a fresh new copy of a JuMP model), JuMP
+needs to create a new empty optimizer instance. New optimizer instances can
+be obtained using a factory that can be created using the


I'd change "factory" to OptimizerFactory.

mlubin · 2018-07-31T01:55:52Z

src/JuMP.jl

+
+User-friendly closure that creates new MOI models. New `OptimizerFactory`s are
+created with [`with_optimizer`](@ref) and new models are created from the
+factory with [`create_model`](@ref).


Example usage would help a lot here.

mlubin · 2018-07-31T02:01:04Z

src/JuMP.jl

+    Model(; caching_mode::MOIU.CachingOptimizerMode=MOIU.Automatic,
+            bridge_constraints::Bool=true)
+
+Return a new JuMP model without any optimizer storing the model in a cache.


"without any optimizer storing the model in a cache" or just "without any optimizer"?

mlubin · 2018-07-31T02:02:03Z

src/JuMP.jl

+Return a new JuMP model using `backend` to store the model and solve it. As
+opposed to the [`Model`](@ref) constructor, no cache of the model is stored
+outside of `backend` and no bridges are automatically applied to `backend`.
+The absence of cache reduces the memory footprint but it is importnat to bear


Typo: "important"

mlubin · 2018-07-31T02:04:04Z

src/JuMP.jl

+* When `backend` does not support an operation such as adding
+  variables/constraints after solver or modifying constraints, an error is
+  thrown. With models created using the [`Model`](@ref) constructor, such
+  situations can be dealt with modifying the cache only and copying the model


"such situations can be dealt with modifying the cache only and copying the model cache once JuMP.optimize is called." -> "such situations are dealt with by storing the modifications in a cache and loading them into the optimizer when JuMP.optimize is called".

mlubin · 2018-07-31T02:04:36Z

src/JuMP.jl

+  thrown. With models created using the [`Model`](@ref) constructor, such
+  situations can be dealt with modifying the cache only and copying the model
+  cache once `JuMP.optimize` is called.
+* When `backend` does not support a constraint type, the constraint is not


More clearly, "no constraint bridging is supported by default".

mlubin · 2018-07-31T02:08:20Z

src/JuMP.jl

+  cache once `JuMP.optimize` is called.
+* When `backend` does not support a constraint type, the constraint is not
+  automatically bridged to constraints supported by `backend`.
+* The optimizer used cannot be changed. With models created using the


The explanation past "The optimizer used cannot be changed" is too verbose. I would just say "The optimizer cannot be changed after the Model is constructed."

mlubin · 2018-07-31T22:48:51Z

src/JuMP.jl

+`IpoptOptimizer`s:
+```julia
+factory = with_optimizer(IpoptOptimizer, print_level=0)
+optimizer1 = JuMP.create_model(factory)


I'd expect JuMP.create_model to create a JuMP model. This is very confusing. What about overloading the call operator on the factory? Like factory() to create an optimizer.

mlubin · 2018-07-31T22:49:54Z

src/optimizerinterface.jl

-                ignore_optimize_hook=(model.optimizehook===nothing))
+"""
+    function optimize(model::Model,
+                      factory::Union{Nothing, OptimizerFactory} = nothing;


It's better to call it optimizer_factory (in code) and "optimizer factory" (in text). factory is too generic.

mlubin · 2018-07-31T22:51:40Z

test/generate_and_solve.jl

-        MOI.set!(mocksolver, MOI.ConstraintDual(), JuMP.optimizerindex(JuMP.LowerBoundRef(y)), 1.0)
-
-        JuMP.optimize(m)
+        JuMP.optimize(m, with_optimizer(MOIU.MockOptimizer, JuMP.JuMPMOIModel{Float64}(), evalobjective=false))


I like how this looks.

mlubin · 2018-08-01T23:45:10Z

docs/src/solvers.md

+See the [MOI documentation](http://www.juliaopt.org/MathOptInterface.jl/stable/)
+for more details on these two MOI layers.
+
+To create a fresh new JuMP model, JuMP needs to create a new empty optimizer


"JuMP needs to" isn't correct. You can create a JuMP model without an optimizer.

mlubin · 2018-08-01T23:46:41Z

docs/src/solvers.md

+with_optimizer
+```
+
+The optimizer factory can be set to the JuMP model in the


This no longer involves setting, right? Maybe say that the factory can be provided either at model construction time or at optimize time.

mlubin · 2018-08-01T23:50:25Z

src/optimizerinterface.jl

    # The NLPData is not kept in sync, so re-set it here.
    # TODO: Consider how to handle incremental solves.
    if model.nlpdata !== nothing
        MOI.set!(model, MOI.NLPBlock(), create_nlp_block_data(model))
        empty!(model.nlpdata.nlconstr_duals)
    end

+    if optimizer_factory !== nothing


We may want to consider throwing an error in this case if the caching optimizer is not in the NoOptimizer state. It seems confusing to write:

model = Model(with_optimizer(Gurobi.Optimizer)) ... JuMP.optimize(model, with_optimizer(CPLEX.Optimizer))

Also error if we're in direct mode. (MOIU.resetoptimizer! will error, but the user didn't call this function.)

mlubin · 2018-08-02T00:09:59Z

docs/src/solvers.md

+[`JuMP.optimize`](@ref) function:
+```@docs
+JuMP.optimize
+```


We're completely missing a discussion about how to control the caching optimizer states. This can be done later, but we should have a TODO.

mlubin · 2018-08-02T19:30:25Z

src/optimizerinterface.jl

@@ -49,6 +49,12 @@ function optimize(model::Model,
    end

    if optimizer_factory !== nothing
+        if mode(model) == Direct
+            error("An optimizer factory cannot be provided at the `optimize` call in Direct mode")


Nit: period at the end of the error (as below).

mlubin · 2018-08-02T19:32:50Z

test/generate_and_solve.jl

@@ -90,6 +90,7 @@
        MOI.set!(mockoptimizer, MOI.ConstraintDual(), JuMP.optimizerindex(JuMP.UpperBoundRef(x)), 0.0)
        MOI.set!(mockoptimizer, MOI.ConstraintDual(), JuMP.optimizerindex(JuMP.LowerBoundRef(y)), 1.0)

+        @test_throws ErrorException JuMP.optimize(m, with_optimizer(MOIU.MockOptimizer, JuMP.JuMPMOIModel{Float64}()))


It's better to add a separate small test for this (also below).

mlubin mentioned this pull request Jul 28, 2018

Clean up JuMP.Model. #1383

Merged

mlubin reviewed Jul 28, 2018

View reviewed changes

blegat added 5 commits July 29, 2018 11:35

First skeleton for factory

ae75f18

Update tests

7650e39

Fix

553df03

Fix tests

b46399c

Add doc

1991b42

blegat force-pushed the bl/factory branch from 4783b71 to 39be6af Compare July 29, 2018 09:46

Document factory field

39be6af

blegat changed the title ~~[WIP] Factory~~ Factory Jul 29, 2018

Add doc

7a94929

mlubin reviewed Jul 29, 2018

View reviewed changes

blegat added 2 commits July 29, 2018 14:46

Fix constructor doc inclusion

ccf68e5

Address comments

811f07f

odow reviewed Jul 29, 2018

View reviewed changes

blegat added 5 commits July 30, 2018 08:28

Merge url in previous line

93e92dc

Check not in Direct mode for set_optimizer

8b52a3c

act -> acts

1b7b861

it maintain -> maintains

8fe3c11

Define backend

3548801

mlubin reviewed Jul 31, 2018

View reviewed changes

blegat added 4 commits July 31, 2018 19:48

Mention that backend is ModelLike

2f64106

Mention that bridge can be disabled

e3ed073

JuMP API -> JuMP

f42fb08

factory -> OptimizerFactory

8be79fa

blegat added 4 commits July 31, 2018 20:08

Add example for OptimizerFactory doc

0afaf32

Address comments

f70303d

Fix

2e3c6bd

Fix test

16caf11

mlubin reviewed Jul 31, 2018

View reviewed changes

Remove create_model

e40f899

mlubin reviewed Aug 2, 2018

View reviewed changes

blegat added 4 commits August 2, 2018 15:16

Add error messages in optimize

6070bf4

Address docs comments

23b23ff

Add TODO for documenting caching optimizer states

14b4ba0

Add tests for errors in optimize

29f4d3e

mlubin reviewed Aug 2, 2018

View reviewed changes

blegat added 2 commits August 2, 2018 22:16

Move tests in separate testsets

7673125

Add dot in error message

b4b4f17

mlubin approved these changes Aug 2, 2018

View reviewed changes

mlubin merged commit 0daf405 into master Aug 2, 2018

mlubin deleted the bl/factory branch August 2, 2018 20:39

blegat mentioned this pull request Aug 5, 2018

Emptying MOI Models Between Solves #1290

Closed

		@@ -77,6 +78,43 @@ const MOIBIN = MOICON{MOI.SingleVariable,MOI.ZeroOne}

		@MOIU.model JuMPMOIModel (ZeroOne, Integer) (EqualTo, GreaterThan, LessThan, Interval) (Zeros, Nonnegatives, Nonpositives, SecondOrderCone, RotatedSecondOrderCone, GeometricMeanCone, PositiveSemidefiniteConeTriangle, PositiveSemidefiniteConeSquare, RootDetConeTriangle, RootDetConeSquare, LogDetConeTriangle, LogDetConeSquare) () (SingleVariable,) (ScalarAffineFunction,ScalarQuadraticFunction) (VectorOfVariables,) (VectorAffineFunction,)

		struct Factory

Factory #1384

Factory #1384

Conversation

blegat commented Jul 28, 2018

Choose a reason for hiding this comment

codecov bot commented Jul 29, 2018

Codecov Report

codecov bot commented Jul 29, 2018 • edited Loading

Codecov Report

IainNZ commented Jul 29, 2018

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

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

Choose a reason for hiding this comment

mlubin commented Jul 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odow Jul 29, 2018 • 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

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

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

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

Choose a reason for hiding this comment

codecov bot commented Jul 29, 2018 •

edited

Loading

odow Jul 29, 2018 •

edited

Loading