From 845b54571812cac57c49f1980838f661c47bc65a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Legat?= Date: Sat, 18 Jul 2026 23:46:06 +0200 Subject: [PATCH 1/2] Fix final_touch order --- src/Bridges/Constraint/map.jl | 81 +++-- src/Bridges/bridge_optimizer.jl | 11 +- .../Bridges/General/test_final_touch_order.jl | 324 ++++++++++++++++++ 3 files changed, 393 insertions(+), 23 deletions(-) create mode 100644 test/Bridges/General/test_final_touch_order.jl diff --git a/src/Bridges/Constraint/map.jl b/src/Bridges/Constraint/map.jl index b5926b690b..981e54776f 100644 --- a/src/Bridges/Constraint/map.jl +++ b/src/Bridges/Constraint/map.jl @@ -20,14 +20,23 @@ struct Map <: AbstractDict{MOI.ConstraintIndex,AbstractBridge} # of creation so we need `OrderedDict` and not `Dict`. # For `VariableIndex` constraints: (variable, set type) -> bridge single_variable_constraints::OrderedDict{Tuple{Int64,Type},AbstractBridge} - needs_final_touch::OrderedDict{Type,OrderedSet} + # The bridges that need a final touch, mapped to the "creation order" of the + # constraint they bridge, that is, the order in which `bridge_constraint` was + # called (see `_reserve_final_touch_order`). `final_touch` is called on them + # in increasing creation order, so that a bridge gets its final touch before + # the bridges of the constraints it added. See `final_touch(::Map, ...)`. + needs_final_touch::OrderedDict{AbstractBridge,Int} + # A counter incremented once per bridged constraint, used to assign the + # creation order stored in `needs_final_touch`. + final_touch_counter::Base.RefValue{Int} function Map() return new( Union{Nothing,AbstractBridge}[], Tuple{Type,Type}[], OrderedDict{Tuple{Int64,Type},AbstractBridge}(), - OrderedDict{Type,OrderedSet}(), + OrderedDict{AbstractBridge,Int}(), + Ref(0), ) end end @@ -44,6 +53,7 @@ function Base.empty!(map::Map) empty!(map.constraint_types) empty!(map.single_variable_constraints) empty!(map.needs_final_touch) + map.final_touch_counter[] = 0 return map end @@ -296,9 +306,10 @@ function add_key_for_bridge( bridge::AbstractBridge, ::F, ::S, - is_available::Function, + is_available::Function; + final_touch_order::Int = _reserve_final_touch_order(map), ) where {F<:MOI.AbstractFunction,S<:MOI.AbstractSet} - _register_for_final_touch(map, bridge) + _register_for_final_touch(map, bridge, final_touch_order) _ensure_available(map, F, S, is_available) push!(map.bridges, bridge) push!(map.constraint_types, (F, S)) @@ -310,41 +321,69 @@ function add_key_for_bridge( bridge::AbstractBridge, func::MOI.VariableIndex, ::S, - ::Function, + ::Function; + final_touch_order::Int = _reserve_final_touch_order(map), ) where {S<:MOI.AbstractScalarSet} - _register_for_final_touch(map, bridge) + _register_for_final_touch(map, bridge, final_touch_order) map.single_variable_constraints[(func.value, S)] = bridge return MOI.ConstraintIndex{MOI.VariableIndex,S}(func.value) end -function _register_for_final_touch(map::Map, bridge::BT) where {BT} +""" + _reserve_final_touch_order(map::Map) + +Return the next "creation order" number. It must be called at the start of +`add_bridged_constraint`, that is, *before* `bridge_constraint`, so that a bridge +is given a smaller number than the bridges of the constraints it creates. The +number is passed to `add_key_for_bridge` as the `final_touch_order` keyword so +that `final_touch` is called on the bridges in that order. +""" +function _reserve_final_touch_order(map::Map) + map.final_touch_counter[] += 1 + return map.final_touch_counter[] +end + +function _register_for_final_touch(map::Map, bridge::AbstractBridge, order::Int) if MOI.Bridges.needs_final_touch(bridge) - if !haskey(map.needs_final_touch, BT) - map.needs_final_touch[BT] = OrderedSet{BT}() - end - push!(map.needs_final_touch[BT], bridge) + map.needs_final_touch[bridge] = order end return end -function _unregister_for_final_touch(b::Map, bridge::BT) where {BT} +function _unregister_for_final_touch(map::Map, bridge::AbstractBridge) if MOI.Bridges.needs_final_touch(bridge) - delete!(b.needs_final_touch[BT], bridge) + delete!(map.needs_final_touch, bridge) end return end -# Function barrier to iterate over bridges of the same type in an efficient way. -function _final_touch(bridges, model) - for bridge in bridges - MOI.Bridges.final_touch(bridge, model) - end - return +# Return the bridges that need a final touch, sorted by increasing creation +# order (the order in which their `bridge_constraint` was called). +function _sorted_needs_final_touch(map::Map) + bridges = collect(keys(map.needs_final_touch)) + sort!(bridges; by = bridge -> map.needs_final_touch[bridge]) + return bridges end function MOI.Bridges.final_touch(map::Map, model::MOI.ModelLike) - for bridges in values(map.needs_final_touch) - _final_touch(bridges, model) + # `MOI.Bridges.final_touch(bridge, model)` may add constraints that are + # bridged by bridges that themselves need a final touch. These new bridges + # are added to `map.needs_final_touch` and must also get their `final_touch` + # called (see issue #1980), after the bridge that created them: since they + # are created later, they have a larger creation order, so they are sorted + # after the current position. We cannot iterate over `map.needs_final_touch` + # directly because it may be modified during the iteration, so we snapshot + # it into a sorted vector and refresh the snapshot whenever new bridges are + # added. The already-touched prefix keeps a smaller creation order, so it is + # left unchanged by the refresh and `i` stays valid. + bridges = _sorted_needs_final_touch(map) + i = 1 + while i <= length(bridges) + MOI.Bridges.final_touch(bridges[i], model) + if length(bridges) < length(map.needs_final_touch) + bridges = _sorted_needs_final_touch(map) + end + i += 1 end return end diff --git a/src/Bridges/bridge_optimizer.jl b/src/Bridges/bridge_optimizer.jl index a13ab5093d..5053c094f4 100644 --- a/src/Bridges/bridge_optimizer.jl +++ b/src/Bridges/bridge_optimizer.jl @@ -1961,6 +1961,12 @@ function MOI.supports_constraint( end function add_bridged_constraint(b, BridgeType, f, s) + map = Constraint.bridges(b)::Constraint.Map + # Reserve the creation order *before* `bridge_constraint` so that this bridge + # is given a smaller order than the bridges of the constraints it creates + # while being bridged. `final_touch` is then called in that order (see + # `Constraint.final_touch`). + order = Constraint._reserve_final_touch_order(map) bridge = Constraint.bridge_constraint(BridgeType, recursive_model(b), f, s) # `MOI.VectorOfVariables` constraint indices have negative indices # to distinguish between the indices of the inner model. @@ -1968,11 +1974,12 @@ function add_bridged_constraint(b, BridgeType, f, s) # so we use the last argument to inform the constraint bridge mapping about # indices already taken by variable bridges. ci = Constraint.add_key_for_bridge( - Constraint.bridges(b)::Constraint.Map, + map, bridge, f, s, - !Base.Fix1(MOI.is_valid, Variable.bridges(b)), + !Base.Fix1(MOI.is_valid, Variable.bridges(b)); + final_touch_order = order, ) Variable.register_context(Variable.bridges(b), ci) return ci diff --git a/test/Bridges/General/test_final_touch_order.jl b/test/Bridges/General/test_final_touch_order.jl new file mode 100644 index 0000000000..5bef625a1b --- /dev/null +++ b/test/Bridges/General/test_final_touch_order.jl @@ -0,0 +1,324 @@ +# Copyright (c) 2017: Miles Lubin and contributors +# Copyright (c) 2017: Google Inc. +# +# Use of this source code is governed by an MIT-style license that can be found +# in the LICENSE.md file or at https://opensource.org/licenses/MIT. + +module TestFinalTouchOrder + +using Test + +import MathOptInterface as MOI + +# The order in which `final_touch` is called on the bridges, as `(tag, id)` +# where `tag` identifies the bridge and `id` the constrained variable. +const RECORD = Tuple{Symbol,Int64}[] + +# When `true`, `BridgeA` adds a `SetB` constraint in its `final_touch`. This is +# used to test that a bridge of a *new* type added while `final_touch` is being +# called still gets its own `final_touch`. +const ADD_B = Ref(false) + +# When `true`, the next `BridgeA.final_touch` adds another `SetA` constraint (the +# flag is consumed so only one is added). This tests that a bridge of an +# *already-processed* type added while `final_touch` is being called still gets +# its own `final_touch` (MathOptInterface issue #1980). +const ADD_A = Ref(false) + +struct SetA <: MOI.AbstractScalarSet end + +struct SetB <: MOI.AbstractScalarSet end + +# BridgeB: `VariableIndex`-in-`SetB` -> `VariableIndex`-in-`GreaterThan`. +struct BridgeB{T} <: MOI.Bridges.Constraint.AbstractBridge + x::MOI.VariableIndex + ci::MOI.ConstraintIndex{MOI.VariableIndex,MOI.GreaterThan{T}} +end + +function MOI.Bridges.Constraint.bridge_constraint( + ::Type{BridgeB{T}}, + model::MOI.ModelLike, + func::MOI.VariableIndex, + ::SetB, +) where {T} + ci = MOI.add_constraint(model, func, MOI.GreaterThan(zero(T))) + return BridgeB{T}(func, ci) +end + +function MOI.supports_constraint( + ::Type{<:BridgeB}, + ::Type{MOI.VariableIndex}, + ::Type{SetB}, +) + return true +end + +function MOI.Bridges.Constraint.concrete_bridge_type( + ::Type{<:BridgeB{T}}, + ::Type{MOI.VariableIndex}, + ::Type{SetB}, +) where {T} + return BridgeB{T} +end + +MOI.Bridges.added_constrained_variable_types(::Type{<:BridgeB}) = Tuple{Type}[] + +function MOI.Bridges.added_constraint_types(::Type{BridgeB{T}}) where {T} + return Tuple{Type,Type}[(MOI.VariableIndex, MOI.GreaterThan{T})] +end + +function MOI.get( + ::BridgeB{T}, + ::MOI.NumberOfConstraints{MOI.VariableIndex,MOI.GreaterThan{T}}, +)::Int64 where {T} + return 1 +end + +function MOI.get( + bridge::BridgeB{T}, + ::MOI.ListOfConstraintIndices{MOI.VariableIndex,MOI.GreaterThan{T}}, +) where {T} + return [bridge.ci] +end + +MOI.Bridges.needs_final_touch(::BridgeB) = true + +function MOI.Bridges.final_touch(bridge::BridgeB, ::MOI.ModelLike) + push!(RECORD, (:B, bridge.x.value)) + return +end + +MOI.delete(model::MOI.ModelLike, bridge::BridgeB) = MOI.delete(model, bridge.ci) + +# BridgeA: `VariableIndex`-in-`SetA`. It optionally adds a `VariableIndex`-in- +# `SetB` constraint in its `final_touch` (which is itself bridged by `BridgeB`). +mutable struct BridgeA{T} <: MOI.Bridges.Constraint.AbstractBridge + x::MOI.VariableIndex + ci::Union{Nothing,MOI.ConstraintIndex{MOI.VariableIndex,SetB}} +end + +function MOI.Bridges.Constraint.bridge_constraint( + ::Type{BridgeA{T}}, + ::MOI.ModelLike, + func::MOI.VariableIndex, + ::SetA, +) where {T} + return BridgeA{T}(func, nothing) +end + +function MOI.supports_constraint( + ::Type{<:BridgeA}, + ::Type{MOI.VariableIndex}, + ::Type{SetA}, +) + return true +end + +function MOI.Bridges.Constraint.concrete_bridge_type( + ::Type{<:BridgeA{T}}, + ::Type{MOI.VariableIndex}, + ::Type{SetA}, +) where {T} + return BridgeA{T} +end + +MOI.Bridges.added_constrained_variable_types(::Type{<:BridgeA}) = Tuple{Type}[] + +function MOI.Bridges.added_constraint_types(::Type{<:BridgeA}) + return Tuple{Type,Type}[(MOI.VariableIndex, SetB)] +end + +function MOI.get( + bridge::BridgeA, + ::MOI.NumberOfConstraints{MOI.VariableIndex,SetB}, +)::Int64 + return bridge.ci === nothing ? 0 : 1 +end + +function MOI.get( + bridge::BridgeA, + ::MOI.ListOfConstraintIndices{MOI.VariableIndex,SetB}, +) + if bridge.ci === nothing + return MOI.ConstraintIndex{MOI.VariableIndex,SetB}[] + end + return [bridge.ci] +end + +MOI.Bridges.needs_final_touch(::BridgeA) = true + +function MOI.Bridges.final_touch(bridge::BridgeA, model::MOI.ModelLike) + push!(RECORD, (:A, bridge.x.value)) + if ADD_B[] && bridge.ci === nothing + bridge.ci = MOI.add_constraint(model, bridge.x, SetB()) + end + if ADD_A[] + ADD_A[] = false # consume the flag so only one constraint is added + y = MOI.add_variable(model) + MOI.add_constraint(model, y, SetA()) + end + return +end + +function MOI.delete(model::MOI.ModelLike, bridge::BridgeA) + if bridge.ci !== nothing + MOI.delete(model, bridge.ci) + end + return +end + +# BridgeC: `VariableIndex`-in-`SetC`. Unlike `BridgeA`, it adds its `SetB` +# constraint in `bridge_constraint` (not `final_touch`), so `BridgeB` is created +# and registered *before* `BridgeC` (registration is post-order). Its +# `final_touch` must still be called *before* `BridgeB`'s, because `BridgeC` +# created the constraint that `BridgeB` bridges. This is the case that motivated +# ordering `final_touch` by creation order. +struct SetC <: MOI.AbstractScalarSet end + +struct BridgeC{T} <: MOI.Bridges.Constraint.AbstractBridge + x::MOI.VariableIndex + ci::MOI.ConstraintIndex{MOI.VariableIndex,SetB} +end + +function MOI.Bridges.Constraint.bridge_constraint( + ::Type{BridgeC{T}}, + model::MOI.ModelLike, + func::MOI.VariableIndex, + ::SetC, +) where {T} + ci = MOI.add_constraint(model, func, SetB()) + return BridgeC{T}(func, ci) +end + +function MOI.supports_constraint( + ::Type{<:BridgeC}, + ::Type{MOI.VariableIndex}, + ::Type{SetC}, +) + return true +end + +function MOI.Bridges.Constraint.concrete_bridge_type( + ::Type{<:BridgeC{T}}, + ::Type{MOI.VariableIndex}, + ::Type{SetC}, +) where {T} + return BridgeC{T} +end + +MOI.Bridges.added_constrained_variable_types(::Type{<:BridgeC}) = Tuple{Type}[] + +function MOI.Bridges.added_constraint_types(::Type{<:BridgeC}) + return Tuple{Type,Type}[(MOI.VariableIndex, SetB)] +end + +function MOI.get(::BridgeC, ::MOI.NumberOfConstraints{MOI.VariableIndex,SetB})::Int64 + return 1 +end + +function MOI.get( + bridge::BridgeC, + ::MOI.ListOfConstraintIndices{MOI.VariableIndex,SetB}, +) + return [bridge.ci] +end + +MOI.Bridges.needs_final_touch(::BridgeC) = true + +function MOI.Bridges.final_touch(bridge::BridgeC, ::MOI.ModelLike) + push!(RECORD, (:C, bridge.x.value)) + return +end + +MOI.delete(model::MOI.ModelLike, bridge::BridgeC) = MOI.delete(model, bridge.ci) + +function _model() + inner = MOI.Utilities.Model{Float64}() + b = MOI.Bridges.LazyBridgeOptimizer(inner) + MOI.Bridges.add_bridge(b, BridgeA{Float64}) + MOI.Bridges.add_bridge(b, BridgeB{Float64}) + MOI.Bridges.add_bridge(b, BridgeC{Float64}) + return b +end + +function runtests() + for name in names(@__MODULE__; all = true) + if startswith("$(name)", "test_") + @testset "$(name)" begin + getfield(@__MODULE__, name)() + end + end + end + return +end + +# `final_touch` must be called in the order the bridges were added, not grouped +# by bridge type. Adding `A`, `B`, `A` must give the order `A, B, A` and not +# `A, A, B`. +function test_final_touch_in_addition_order() + empty!(RECORD) + ADD_B[] = false + model = _model() + x = MOI.add_variables(model, 3) + MOI.add_constraint(model, x[1], SetA()) + MOI.add_constraint(model, x[2], SetB()) + MOI.add_constraint(model, x[3], SetA()) + MOI.Utilities.final_touch(model, MOI.Utilities.IndexMap()) + @test RECORD == + [(:A, x[1].value), (:B, x[2].value), (:A, x[3].value)] + return +end + +# A bridge must get its `final_touch` before the bridges of the constraints it +# created in its `bridge_constraint`, even though those are registered first +# (registration is post-order but `final_touch` follows creation order). +function test_final_touch_parent_before_child() + empty!(RECORD) + model = _model() + x = MOI.add_variable(model) + MOI.add_constraint(model, x, SetC()) + MOI.Utilities.final_touch(model, MOI.Utilities.IndexMap()) + # `BridgeC` created the `SetB` constraint bridged by `BridgeB`, so + # `BridgeC.final_touch` must run before `BridgeB.final_touch`. + @test RECORD == [(:C, x.value), (:B, x.value)] + return +end + +# A bridge of a *new* type added while `final_touch` is being iterated must still +# get its `final_touch` called. +function test_final_touch_new_type_added_during_iteration() + empty!(RECORD) + ADD_B[] = true + model = _model() + x = MOI.add_variable(model) + MOI.add_constraint(model, x, SetA()) + MOI.Utilities.final_touch(model, MOI.Utilities.IndexMap()) + ADD_B[] = false + # `BridgeA.final_touch` added a `SetB` constraint bridged by `BridgeB`; its + # `final_touch` must have been called after `BridgeA`'s. + @test RECORD == [(:A, x.value), (:B, x.value)] + return +end + +# A bridge of an *already-processed* type added while `final_touch` is being +# iterated must still get its `final_touch` called (MathOptInterface issue +# #1980). +function test_final_touch_same_type_added_during_iteration() + empty!(RECORD) + ADD_A[] = true + model = _model() + x = MOI.add_variable(model) + MOI.add_constraint(model, x, SetA()) + MOI.Utilities.final_touch(model, MOI.Utilities.IndexMap()) + ADD_A[] = false + # The first `BridgeA.final_touch` added a second `SetA` constraint (a bridge + # of the same, already-processed type); it must also get its `final_touch`. + @test length(RECORD) == 2 + @test all(r -> r[1] == :A, RECORD) + @test RECORD[1][2] != RECORD[2][2] # two different variables + return +end + +end # module + +TestFinalTouchOrder.runtests() From 174b5f29f11daa3249447a3b1f1ed1050fa8f28e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Legat?= Date: Thu, 23 Jul 2026 20:04:24 +0200 Subject: [PATCH 2/2] Fix format --- test/Bridges/General/test_final_touch_order.jl | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/Bridges/General/test_final_touch_order.jl b/test/Bridges/General/test_final_touch_order.jl index 5bef625a1b..722118bbf6 100644 --- a/test/Bridges/General/test_final_touch_order.jl +++ b/test/Bridges/General/test_final_touch_order.jl @@ -212,7 +212,10 @@ function MOI.Bridges.added_constraint_types(::Type{<:BridgeC}) return Tuple{Type,Type}[(MOI.VariableIndex, SetB)] end -function MOI.get(::BridgeC, ::MOI.NumberOfConstraints{MOI.VariableIndex,SetB})::Int64 +function MOI.get( + ::BridgeC, + ::MOI.NumberOfConstraints{MOI.VariableIndex,SetB}, +)::Int64 return 1 end @@ -264,8 +267,7 @@ function test_final_touch_in_addition_order() MOI.add_constraint(model, x[2], SetB()) MOI.add_constraint(model, x[3], SetA()) MOI.Utilities.final_touch(model, MOI.Utilities.IndexMap()) - @test RECORD == - [(:A, x[1].value), (:B, x[2].value), (:A, x[3].value)] + @test RECORD == [(:A, x[1].value), (:B, x[2].value), (:A, x[3].value)] return end