Add MOI.attribute_value_type by odow · Pull Request #1283 · jump-dev/MathOptInterface.jl

odow · 2021-03-11T02:00:42Z

Originally proposed all the way back in #31.

Problem

Lots of MOI.get fail to infer. For example, getting the termination status
of a caching optimizer is Any instead of TerminationStatusCode.

julia> b = backend(model)
MOIU.CachingOptimizer{MOI.AbstractOptimizer, MOIU.UniversalFallback{MOIU.Model{Float64}}}
in state ATTACHED_OPTIMIZER
in mode AUTOMATIC
with model cache MOIU.UniversalFallback{MOIU.Model{Float64}}
  with 1 optimizer attribute
  fallback for MOIU.Model{Float64}
with optimizer MOIB.LazyBridgeOptimizer{GLPK.Optimizer}
  with 0 variable bridges
  with 0 constraint bridges
  with 0 objective bridges
  with inner model A GLPK model

julia> @code_warntype MOI.get(b, MOI.TerminationStatus())
Variables
  #self#::Core.Const(MathOptInterface.get)
  model::MathOptInterface.Utilities.CachingOptimizer{MathOptInterface.AbstractOptimizer, MathOptInterface.Utilities.UniversalFallback{MathOptInterface.Utilities.Model{Float64}}}
  attr::Core.Const(MathOptInterface.TerminationStatus())

Body::Any
1 ─ %1  = MathOptInterface.is_set_by_optimize::Core.Const(MathOptInterface.is_set_by_optimize)
│   %2  = (%1)(attr)::Core.Const(true)
│         %2
│   %4  = MathOptInterface.Utilities.state(model)::MathOptInterface.Utilities.CachingOptimizerState
│   %5  = (%4 == MathOptInterface.Utilities.NO_OPTIMIZER)::Bool
└──       goto #4 if not %5
2 ─ %7  = MathOptInterface.TerminationStatus::Core.Const(MathOptInterface.TerminationStatus)
│   %8  = (%7)()::Core.Const(MathOptInterface.TerminationStatus())
│   %9  = (attr == %8)::Core.Const(true)
│         %9
│   %11 = MathOptInterface.OPTIMIZE_NOT_CALLED::Core.Const(MathOptInterface.OPTIMIZE_NOT_CALLED)
└──       return %11
3 ─       Core.Const(:(MathOptInterface.PrimalStatus))
│         Core.Const(:((%13)()))
│         Core.Const(:(attr == %14))
│         Core.Const(:(%15))
│         Core.Const(:(MathOptInterface.NO_SOLUTION))
│         Core.Const(:(return %17))
│         Core.Const(:(MathOptInterface.DualStatus))
│         Core.Const(:((%19)()))
│         Core.Const(:(attr == %20))
│         Core.Const(:(%21))
│         Core.Const(:(MathOptInterface.NO_SOLUTION))
│         Core.Const(:(return %23))
│         Core.Const(:(Base.string("Cannot query ", attr, " from caching optimizer because no")))
│         Core.Const(:(%25 * " optimizer is attached."))
└──       Core.Const(:(MathOptInterface.Utilities.error(%26)))
4 ┄ %28 = Base.getproperty(model, :optimizer_to_model_map)::MathOptInterface.Utilities.IndexMap
│   %29 = MathOptInterface.get::Core.Const(MathOptInterface.get)
│   %30 = Base.getproperty(model, :optimizer)::Union{Nothing, MathOptInterface.AbstractOptimizer}
│   %31 = (%29)(%30, attr)::Any
│   %32 = MathOptInterface.Utilities.map_indices(%28, %31)::Any
└──       return %32
5 ─       Core.Const(:(MathOptInterface.get))
│         Core.Const(:(Base.getproperty(model, :model_cache)))
│         Core.Const(:((%34)(%35, attr)))
└──       Core.Const(:(return %36))

Whereas with this PR we have

julia> @code_warntype MOI.get(b, MOI.TerminationStatus())
Variables
  #self#::Core.Const(MathOptInterface.get)
  model::MathOptInterface.Utilities.CachingOptimizer{MathOptInterface.AbstractOptimizer, MathOptInterface.Utilities.UniversalFallback{MathOptInterface.Utilities.Model{Float64}}}
  attr::Core.Const(MathOptInterface.TerminationStatus())

Body::MathOptInterface.TerminationStatusCode
1 ─ %1  = MathOptInterface.is_set_by_optimize::Core.Const(MathOptInterface.is_set_by_optimize)
│   %2  = (%1)(attr)::Core.Const(true)
│         %2
│   %4  = MathOptInterface.Utilities.state(model)::MathOptInterface.Utilities.CachingOptimizerState
│   %5  = (%4 == MathOptInterface.Utilities.NO_OPTIMIZER)::Bool
└──       goto #4 if not %5
2 ─ %7  = MathOptInterface.TerminationStatus::Core.Const(MathOptInterface.TerminationStatus)
│   %8  = (%7)()::Core.Const(MathOptInterface.TerminationStatus())
│   %9  = (attr == %8)::Core.Const(true)
│         %9
│   %11 = MathOptInterface.OPTIMIZE_NOT_CALLED::Core.Const(MathOptInterface.OPTIMIZE_NOT_CALLED)
└──       return %11
3 ─       Core.Const(:(MathOptInterface.PrimalStatus))
│         Core.Const(:((%13)()))
│         Core.Const(:(attr == %14))
│         Core.Const(:(%15))
│         Core.Const(:(MathOptInterface.NO_SOLUTION))
│         Core.Const(:(return %17))
│         Core.Const(:(MathOptInterface.DualStatus))
│         Core.Const(:((%19)()))
│         Core.Const(:(attr == %20))
│         Core.Const(:(%21))
│         Core.Const(:(MathOptInterface.NO_SOLUTION))
│         Core.Const(:(return %23))
│         Core.Const(:(Base.string("Cannot query ", attr, " from caching optimizer because no")))
│         Core.Const(:(%25 * " optimizer is attached."))
└──       Core.Const(:(MathOptInterface.Utilities.error(%26)))
4 ┄ %28 = Base.getproperty(model, :optimizer_to_model_map)::MathOptInterface.Utilities.IndexMap
│   %29 = MathOptInterface.get::Core.Const(MathOptInterface.get)
│   %30 = Base.getproperty(model, :optimizer)::Union{Nothing, MathOptInterface.AbstractOptimizer}
│   %31 = (%29)(%30, attr)::Any
│   %32 = MathOptInterface.return_type::Core.Const(MathOptInterface.return_type)
│   %33 = (%32)(attr)::Core.Const(MathOptInterface.TerminationStatusCode)
│   %34 = Core.typeassert(%31, %33)::MathOptInterface.TerminationStatusCode
│   %35 = MathOptInterface.Utilities.map_indices(%28, %34)::MathOptInterface.TerminationStatusCode
└──       return %35
5 ─       Core.Const(:(MathOptInterface.get))
│         Core.Const(:(Base.getproperty(model, :model_cache)))
│         Core.Const(:((%37)(%38, attr)))
└──       Core.Const(:(return %39))

Unfortunately, attributes like MOI.VariablePrimal fail to infer, because we don't know if they are Float64 or not.

Alternatives

If we won't want to add MOI.return_type, I can move it into CachingOptimizer.

joehuchette

Is this a general problem that a number of optimizers run into? Or is it mostly just a problem with the CachingOptimizer, because of it's extra layer or redirection?

src/attributes.jl

odow · 2021-03-11T03:28:02Z

This is mainly CachingOptimizer with this problem, because it has a backend of Union{Nothing,AbstractOptimizer} which needs dynamic dispatch.

But more widely, standardizing the return types of MOI.xxx functions would be a good thing. There are lots of things like
#1282 which cause issues if the return types are not explicitly stated.

blegat · 2021-03-11T12:29:52Z

Alternatively, we could avoid having a non-concrete type for the optimizer as suggested in jump-dev/JuMP.jl#2520.
We have a non-concrete type for the JuMP backend so we need to annotate all JuMP function to avoid these issues.
Now we need to workaround the same issues at the level of the CachingOptimizer.
Only one non-concrete type is needed to be able to implement set_optimizer so why have two ?

mlubin · 2021-03-11T14:30:43Z

we need to annotate all JuMP function to avoid these issues

If these types are properly defined in MOI, then we can add unit tests that solvers return the correct types. This prevents surprises when using the solver from JuMP.

In terms of bikeshedding, I'd suggest attribute_value_type instead of return_type. "Return type" applies to functions, and attributes aren't functions.

blegat · 2021-03-11T16:09:13Z

Independently of the alternative, this could indeed be useful, attribute_value_type seems more appropriate indeed.
We could have

attribute_value_type(::ModelLike, attr::AnyAttribute) = attribute_value_type(attr)
attribute_value_type(::ModelLike, attr::Union{MOI.VariablePrimal, MOI.VariablePrimalStart, ...}) = MOI.get(model, CoefficientType())

If there is only one method of attribute_value_type(::ModelLike, attr::AnyAttribute) for a specific attribute, Julia might be able to get around not knowing the model type but I'm not sure, we need to check.

joehuchette · 2021-03-11T19:54:27Z

I'm a +1 to standardizing the "attribute value types" of MOI attributes, and having MOIT unit tests for this expected behavior.

blegat · 2021-03-17T18:45:14Z

src/Test/UnitTests/attributes.jl

+        function $(f)(model::MOI.ModelLike, config::TestConfig)
+            MOI.empty!(model)
+            MOI.optimize!(model)
+            attribute = MOI.$(attr)()


Maybe skip the test is supports is false

For now I've made these tests opt-in individually (They aren't in the unittest testset). We probably want to wait a few releases for solvers to adapt before enforcing it and breaking every solver.

What are the changes needed in the solver ? I assume they already have objects of the correct type returned, don't they ?

They will return the correct object, but they may not infer correctly.

Now that we're breaking things, I added the tests by default. It would be good for solvers to fix them.

src/attributes.jl

odow · 2021-04-19T04:27:16Z

@blegat how is this. I changed to Int64.

odow · 2021-04-30T00:10:48Z

@blegat bump.

src/Utilities/model.jl

blegat · 2021-05-31T12:25:43Z

src/Test/UnitTests/attributes.jl

+                MOI.get(model, attribute)
+            catch err
+                if err isa ArgumentError
+                    return  # Solver does not support accessing the attribute.


@test !MOI.supports(...) ?

odow added Submodule: Utilities About the Utilities submodule Type: Performance labels Mar 11, 2021

joehuchette reviewed Mar 11, 2021

View reviewed changes

src/attributes.jl Outdated Show resolved Hide resolved

odow force-pushed the od/rettype branch 2 times, most recently from d639eea to dc25770 Compare March 17, 2021 18:27

blegat reviewed Mar 17, 2021

View reviewed changes

odow added the Status: Needs review label Mar 17, 2021

odow requested a review from blegat March 17, 2021 21:51

blegat reviewed Mar 19, 2021

View reviewed changes

src/attributes.jl Outdated Show resolved Hide resolved

odow force-pushed the od/rettype branch from bf84f66 to 6098f25 Compare March 26, 2021 02:03

blegat reviewed May 3, 2021

View reviewed changes

src/Utilities/model.jl Outdated Show resolved Hide resolved

odow force-pushed the od/rettype branch from f82ceb7 to 94050cb Compare May 11, 2021 01:36

odow added this to the v0.10 milestone May 11, 2021

odow added breaking and removed Status: Needs review labels May 11, 2021

odow changed the title ~~Add MOI.return_type~~ Add MOI.attribute_value_type May 11, 2021

odow force-pushed the od/rettype branch from c255dae to 0374237 Compare May 17, 2021 03:14

odow force-pushed the od/rettype branch from 0374237 to a895ef6 Compare May 27, 2021 22:03

Add MOI.attribute_value_type for attributes.

952b7a4

odow force-pushed the od/rettype branch from a895ef6 to cdcfe9c Compare May 31, 2021 01:33

Add docs and breaking tests

f021eab

odow force-pushed the od/rettype branch from adbf35e to f021eab Compare May 31, 2021 03:47

blegat reviewed May 31, 2021

View reviewed changes

Merge branch 'master' into od/rettype

8aede83

odow added the Status: Needs review label Jun 3, 2021

odow and others added 3 commits June 22, 2021 10:27

Merge branch 'master' into od/rettype

8ec7bdf

Merge latest changes

8a63413

Fix formatting

f748730

odow removed the Status: Needs review label Aug 4, 2021

odow mentioned this pull request Aug 4, 2021

Test getting CanonicalConstraintFunction #1507

Merged

odow added 6 commits August 4, 2021 16:32

Test getting CanonicalConstraintFunction

a6dbc9f

Fix

e75844b

Improve coverage

a4f0fed

Fix ResultCount in mockoptimizer

a1c138b

Fix on 32-bit

5d7316b

Fix ResultCount

fcb3bb1

odow requested a review from blegat August 5, 2021 23:20

odow added 4 commits August 7, 2021 10:25

Merge branch 'master' into od/rettype

02b8ea9

remove old method

3198b07

Use return type annotations instead

3cca754

Fix stragglers

f60fc41

blegat approved these changes Aug 8, 2021

View reviewed changes

odow merged commit 3093a7c into master Aug 8, 2021

odow deleted the od/rettype branch August 8, 2021 23:36

odow mentioned this pull request Aug 12, 2021

Return type of getattribute functions #31

Closed

Conversation

odow commented Mar 11, 2021

Problem

Alternatives

Uh oh!

joehuchette left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

odow commented Mar 11, 2021

Uh oh!

blegat commented Mar 11, 2021

Uh oh!

mlubin commented Mar 11, 2021

Uh oh!

blegat commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joehuchette commented Mar 11, 2021

Uh oh!

blegat Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

odow Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

blegat Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

odow Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

odow May 11, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

odow commented Apr 19, 2021

Uh oh!

odow commented Apr 30, 2021

Uh oh!

Uh oh!

blegat May 31, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

blegat commented Mar 11, 2021 •

edited

Loading