Fix issue #2452

[odow]

Fixes #2452

https://github.com/jump-dev/MathOptInterface.jl/actions/runs/8548954452
https://github.com/jump-dev/MathOptInterface.jl/actions/runs/8550055568
https://github.com/jump-dev/MathOptInterface.jl/actions/runs/8550389750

• https://github.com/jump-dev/MathOptInterface.jl/actions/runs/8592758175

[odow]

So this seemed to do the job, but I'm not sure I can tell you why.

[blegat]

What's happening for example in the test you added ?

x >= 1 -> y = x - 1 with y nonneg
2x = 3 -> 2y = 1
func = 2y
f = 2x - 2
g = 2y

In this example, MOI.constant(g) is zero so it should be the same before and after the change you've made. What's the catch ?

[odow]

Ah, but it is actually:

x >= 1 -> y = x - 1 with y nonneg
2x = 3 -> 2y = 1
func = 2x
f = 2x
g = 2y + 2

because when we try to get func = 2y, it calls:

MathOptInterface.jl/src/Bridges/Constraint/bridges/vectorize.jl

Lines 229 to 240 in 606ba2e

	function MOI.get(
	model::MOI.ModelLike,
	attr::MOI.ConstraintFunction,
	bridge::VectorizeBridge{T,F,S,G},
	) where {T,F,S,G}
	f = MOI.Utilities.scalarize(
	MOI.get(model, attr, bridge.vector_constraint),
	true,
	)
	@assert length(f) == 1
	return convert(G, f[1])
	end

But MOI.get(model, attr, bridge.vector_constraint) returns the "unbridged" version of the function instead of the bridged. (Because it doesn't have a way to indicate that it will handle the variable bridges afterwards.)

The problematic method which does the unbridging is:

MathOptInterface.jl/src/Bridges/bridge_optimizer.jl

Lines 1380 to 1404 in 606ba2e

	function MOI.get(
	b::AbstractBridgeOptimizer,
	attr::MOI.ConstraintFunction,
	ci::MOI.ConstraintIndex,
	)
	if is_bridged(b, ci)
	MOI.throw_if_not_valid(b, ci)
	if is_variable_bridged(b, ci)
	# If it is a variable bridge, we can get the original variable quite
	# easily.
	return Variable.function_for(Variable.bridges(b)::Variable.Map, ci)
	else
	# Otherwise, we need to query ConstraintFunction in the context of
	# the bridge...
	func = call_in_context(MOI.get, b, ci, attr)
	# and then unbridge this function (because it may contain variables
	# that are themselves bridged).
	return unbridged_constraint_function(b, func)
	end
	else
	# This constraint is not bridged, but it might contain variables that
	# are.
	return unbridged_constraint_function(b, MOI.get(b.model, attr, ci))
	end
	end

I tried adding some new RawConstraintFunction attribute, but then I got confused trying to plumb it all together.

[odow]

Nightly failure is JuliaLang/julia#53896 (comment)

[odow]

Okay. I think I understand this a lot better now. I've simplified the implementation and everything looks green.

[blegat]

I think we still need the if-else with the call_in_context. Here is why.
You have the user model and the solver model. But you might also have models in between.
If a user variable x gets variable-bridged by a bridge bxy into an intermediate variable y which is then variable-bridged by a bridge byz into a solver variable z then you have an intermediate model inter for which the variable is y.
All constraints created by bxy are in the context of inter and the variable they use is y. So if a constraint created by bxy is bridged by a constraint bridge and this constraint bridge tries to get MOI.ConstraintSet(), it should get the ConstraintFunction in terms of y.
Maybe keeping the original way of querying func but then doing g = bridged_function(b, func) works ?

[odow]

Can you construct a test that demonstrates the current behavior is wrong? Otherwise, I vote we keep this as-is. I don't really understand the ins and outs of call-in-context.

[odow]

So perhaps if we kept the call_in_context, then we need the MOI.constant(g) - MOI.constant(f), but just getting ::MOI.ConstraintFunction deals with all the different constants.

I've added a test that the user can't add a function with a constant if there are variable bridges.

I don't know well this was ever tested. It didn't, for example, have a check for supports_shift_constant

[blegat]

Thinking more about it, your implementation should also handle the tricky cases with nontrivial context, see the comments I have added. In the initial implementation, I thought getting the constraint function was going to give it in the solver variable but it gives it in the variables of the current context so your PR fixes it the best way I think

[odow]

The implementation of what is going on at each level is certainly non-trivial.

[klamike]

Thank you for your help on this issue!

Unfortunately, I think this PR does not fully fix #2452. It seems that using the latest master branch, the result of the pure MOI MWE is now correct but the original MWE still gives the same (incorrect) result. Interestingly, using remove_bridge to remove VectorizeBridge still seems to fix the original MWE.

[odow]

I've re-opened #2452. I'm sure I tested it at some point and it was fixed...

[odow]

Okay. I've dug deeper, and now I have no confidence that this was actually the right fix. It just fixed the surface level get/set, but it incorrectly modified the inner model.