Skip to content

Conversation

blegat
Copy link
Member

@blegat blegat commented Aug 13, 2019

No description provided.

@blegat
Copy link
Member Author

blegat commented Aug 17, 2019

The failure appears to be due to JuliaLang/julia#32167

@codecov-io
Copy link

codecov-io commented Aug 21, 2019

Codecov Report

Merging #834 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #834      +/-   ##
==========================================
- Coverage   94.95%   94.93%   -0.03%     
==========================================
  Files          72       72              
  Lines        7910     7912       +2     
==========================================
  Hits         7511     7511              
- Misses        399      401       +2
Impacted Files Coverage Δ
src/Bridges/Variable/Variable.jl 87.5% <0%> (-12.5%) ⬇️
src/Bridges/Constraint/Constraint.jl 96% <0%> (-4%) ⬇️

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 2558aad...7668eab. Read the comment docs.

@odow
Copy link
Member

odow commented Aug 21, 2019

Hmm. @blegat still no luck. Are there other occurrences where this would be needed?

@odow
Copy link
Member

odow commented Aug 21, 2019

Progress. We're now failing the tests rather than timing out: https://travis-ci.org/JuliaOpt/MathOptInterface.jl/jobs/575006947

@odow
Copy link
Member

odow commented Aug 21, 2019

An alternative fix is to replace
https://github.com/JuliaOpt/MathOptInterface.jl/blob/2558aad1321026828e973ab9d7f097100b48f89a/src/Bridges/bridge_optimizer.jl#L866-L882
with

@noinline function _no_inlined_bridge_constrained_variable(b, set)
    BridgeType = Variable.concrete_bridge_type(b, typeof(set))
    bridge = Variable.bridge_constrained_variable(BridgeType, b, set)
    return Variable.add_key_for_bridge(Variable.bridges(b), bridge, set)
end

function MOI.add_constrained_variable(
    b::AbstractBridgeOptimizer, set::MOI.AbstractScalarSet
)
    if is_bridged(b, typeof(set)) ||
        is_bridged(b, MOI.SingleVariable, typeof(set))
        if B.supports_bridging_constrained_variable(b, typeof(set))
            return _no_inlined_bridge_constrained_variable(b, set)
        else
            variable = MOI.add_variable(b)
            constraint = MOI.add_constraint(b, MOI.SingleVariable(variable), set)
            return variable, constraint
        end
    else
        return MOI.add_constrained_variable(b.model, set)
    end
end

The problem is that the compiler can't infer BridgeType or bridge (see below), so without a function barrier (that is prevented from inlining!), the code generated by add_constrained_variable is terrible.

The current fix works around this by defining a fallback to Variable.bridge_constrained_variable, but I'm not as sure why it works.

julia> @code_warntype _no_inlined_bridge_constrained_variable(b, MOI.GreaterThan(0.0))
Variables
  #self#::Core.Compiler.Const(_no_inlined_bridge_constrained_variable, false)
  b::MathOptInterface.Bridges.LazyBridgeOptimizer{Model{Float64}}
  set::MathOptInterface.GreaterThan{Float64}
  BridgeType::DataType
  bridge::MathOptInterface.Bridges.Variable.VectorizeBridge{Float64,_A} where _A

Body::Tuple{MathOptInterface.VariableIndex,MathOptInterface.ConstraintIndex{MathOptInterface.SingleVariable,MathOptInterface.GreaterThan{Float64}}}
1 ─       $(Expr(:meta, :noinline))
│   %2  = MathOptInterface.Bridges.Variable::Core.Compiler.Const(MathOptInterface.Bridges.Variable, false)
│   %3  = Base.getproperty(%2, :concrete_bridge_type)::Core.Compiler.Const(MathOptInterface.Bridges.Variable.concrete_bridge_type, false)
│   %4  = Main.typeof(set)::Core.Compiler.Const(MathOptInterface.GreaterThan{Float64}, false)
│         (BridgeType = (%3)(b, %4))
│   %6  = MathOptInterface.Bridges.Variable::Core.Compiler.Const(MathOptInterface.Bridges.Variable, false)
│   %7  = Base.getproperty(%6, :bridge_constrained_variable)::Core.Compiler.Const(MathOptInterface.Bridges.Variable.bridge_constrained_variable, false)
│   %8  = BridgeType::DataType
│         (bridge = (%7)(%8, b, set))
│   %10 = MathOptInterface.Bridges.Variable::Core.Compiler.Const(MathOptInterface.Bridges.Variable, false)
│   %11 = Base.getproperty(%10, :add_key_for_bridge)::Core.Compiler.Const(MathOptInterface.Bridges.Variable.add_key_for_bridge, false)
│   %12 = MathOptInterface.Bridges.Variable::Core.Compiler.Const(MathOptInterface.Bridges.Variable, false)
│   %13 = Base.getproperty(%12, :bridges)::Core.Compiler.Const(MathOptInterface.Bridges.Variable.bridges, false)
│   %14 = (%13)(b)::MathOptInterface.Bridges.Variable.Map
│   %15 = bridge::MathOptInterface.Bridges.Variable.VectorizeBridge{Float64,_A} where _A
│   %16 = (%11)(%14, %15, set)::Tuple{MathOptInterface.VariableIndex,MathOptInterface.ConstraintIndex{MathOptInterface.SingleVariable,MathOptInterface.GreaterThan{Float64}}}
└──       return %16

Actually, it isn't.

@odow
Copy link
Member

odow commented Aug 21, 2019

Here's a puzzle on the latest MOI master, not this branch, but on Julia 1.2.

using MathOptInterface
const MOI = MathOptInterface

@generated function MOI.Bridges.Constraint.bridge_constraint(BridgeType, b, f, s)
    Core.println("Constraint: ") #, BridgeType, b, f, s)
    return :(error("Generated Constraint"))
end
@generated function MOI.Bridges.Variable.bridge_constrained_variable(BridgeType, b, s)
    Core.println("Variable: ") #, BridgeType, b, s)
    return :(error("Generated Variable"))
end

MOI.Utilities.@model(Model, (), (), (MOI.Nonnegatives,), (), (), (), (MOI.VectorOfVariables,), ())
MOI.supports_constraint(::Model{T}, ::Type{MOI.SingleVariable}, ::Type{MOI.GreaterThan{T}}) where {T} = false
const b = MOI.Bridges.full_bridge_optimizer(Model{Float64}(), Float64);

@time MOI.add_constrained_variable(b, MOI.GreaterThan(0.0))

yields

Variable: 
Variable: 
Variable: 
Variable: 
Constraint: 
Constraint: 
Constraint: 
Constraint: 
Constraint: 
Constraint: 
Variable: 
Variable: 
Constraint: 
Constraint: 
Constraint: 
Constraint: 
Constraint: 
Constraint: 
Constraint: 
Constraint: 
Constraint: 
Variable: 
Constraint: 
Variable: 
Constraint: 
Constraint: 
Variable: 
  3.272182 seconds (15.30 M allocations: 822.902 MiB, 8.54% gc time)
(MathOptInterface.VariableIndex(-1), MathOptInterface.ConstraintIndex{MathOptInterface.SingleVariable,MathOptInterface.GreaterThan{Float64}}(-1))

But, if I try

using MathOptInterface
const MOI = MathOptInterface

@generated function MOI.Bridges.Constraint.bridge_constraint(BridgeType, b, f, s)
    Core.println("Constraint: ", BridgeType, b, f, s)
    return :(error("Generated Constraint"))
end
@generated function MOI.Bridges.Variable.bridge_constrained_variable(BridgeType, b, s)
    Core.println("Variable: ", BridgeType, b, s)
    return :(error("Generated Variable"))
end

MOI.Utilities.@model(Model, (), (), (MOI.Nonnegatives,), (), (), (), (MOI.VectorOfVariables,), ())
MOI.supports_constraint(::Model{T}, ::Type{MOI.SingleVariable}, ::Type{MOI.GreaterThan{T}}) where {T} = false
const b = MOI.Bridges.full_bridge_optimizer(Model{Float64}(), Float64);

@time MOI.add_constrained_variable(b, MOI.GreaterThan(0.0))

I get (note: no prints)

  3.194577 seconds (15.28 M allocations: 821.955 MiB, 8.54% gc time)
(MathOptInterface.VariableIndex(-1), MathOptInterface.ConstraintIndex{MathOptInterface.SingleVariable,MathOptInterface.GreaterThan{Float64}}(-1))

Why are the generated methods not being called if they print the argument types? Edit: they can print f or s, but not BridgeType or b.

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

Successfully merging this pull request may close these issues.

3 participants