Skip to content

[Utilities] throw correct error if get_fallback fails #2413

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

[Utilities] throw correct error if get_fallback fails #2413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e728a6c
[Utilities] throw correct error if get_fallback fails
odow Jan 24, 2024
67c15a2
Update
odow Jan 25, 2024
0bf811f
Fix
odow Jan 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/Utilities/mockoptimizer.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,7 +620,8 @@ function MOI.get(
end
primal = get(mock.callback_variable_primal, xor_index(idx), nothing)
if primal === nothing
error("No mock callback primal is set for variable `", idx, "`.")
msg = "No mock callback primal is set for variable `$idx`."
throw(MOI.GetAttributeNotAllowed(attr, msg))
end
return primal
end
Expand Down Expand Up @@ -749,14 +750,13 @@ function _safe_get_result(
index_name = index isa MOI.VariableIndex ? "variable" : "constraint"
result_to_value = get(dict, xor_index(index), nothing)
if result_to_value === nothing
error("No mock $name is set for ", index_name, " `", index, "`.")
msg = "No mock $name is set for $index_name `$index`."
throw(MOI.GetAttributeNotAllowed(attr, msg))
end
value = get(result_to_value, attr.result_index, nothing)
if value === nothing
error(
"No mock $name is set for $(index_name) `$(index)` at result " *
"index `$(attr.result_index)`.",
)
msg = "No mock $name is set for $index_name `$index` at result index `$(attr.result_index)`."
throw(MOI.GetAttributeNotAllowed(attr, msg))
end
return value
end
Expand Down
82 changes: 48 additions & 34 deletions src/Utilities/results.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,17 +37,21 @@ the `ObjectiveFunction` value.
"""
function get_fallback(model::MOI.ModelLike, attr::MOI.ObjectiveValue)
MOI.check_result_index_bounds(model, attr)
F = MOI.get(model, MOI.ObjectiveFunctionType())
f = MOI.get(model, MOI.ObjectiveFunction{F}())
obj = eval_variables(model, f) do vi
return MOI.get(model, MOI.VariablePrimal(attr.result_index), vi)
end
if is_ray(MOI.get(model, MOI.PrimalStatus()))
# Dual infeasibility certificates do not include the primal
# objective constant.
obj -= MOI.constant(f, typeof(obj))
try
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed ?

Copy link
Member Author

@odow odow Jan 26, 2024 •
edited by blegat
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the try defensively to all methods. But the issue is that get_fallback might be used by something that doesn't support getting the ConstraintFunction. That might be because it isn't implemented, or it might be because the model uses a ZerosBridge (or similar). We should throw the proper error, instead of throwing whatever error actually threw, like "can't unbridge the Zeros bridge."

In the Cbc case, it's something like CachingOptimizer(LazyBridgeOptimizer(CachingOptimizer(Cbc))

The outer cache says "can you get ConstraintPrimal." The bridge says yes. Ends up erroring, falling back to get_fallback, and then errors when it tries to unbridge.

If it thew GetAttributeNotAllowed, then the outer cache would fallback to get_fallback and this would succeed.

Even if it didn't have an outer cache to fall back do, we should throw the correct GetAttributeNotAllowed for consistency.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this also fixed by throwing a GetAttributeNotAllowed for the ZerosBridge ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't see the error of the zerosbridge then the user won't know he just has to remove this bridge, that's an issue

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What we could do is throw a custom error here:

MathOptInterface.jl/src/Bridges/Variable/map.jl

Lines 582 to 586 in 0249cfa

error(
"Cannot unbridge function because some variables are bridged by",
" variable bridges that do not support reverse mapping, for example,",
" `ZerosBridge`.",
)

Here we try catch:

MathOptInterface.jl/src/Bridges/bridge_optimizer.jl

Line 2384 in 0249cfa

f = unbridged_function(b, func)::typeof(func)

If we see exactly this error, we thro a GetAttributeNotAllowed for ConstraintFunction with the error message of:

MathOptInterface.jl/src/Bridges/Variable/map.jl

Lines 582 to 586 in 0249cfa

error(
"Cannot unbridge function because some variables are bridged by",
" variable bridges that do not support reverse mapping, for example,",
" `ZerosBridge`.",
)

otherwise, we rethrow

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would address this particular issue with ZerosBridge, but it ignores with wider issue. get_fallback may fail, and instead of throwing a nice error, we may throw some arbitrary error. But perhaps you're right, we can just disable ZerosBridge for now in Cbc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with this error is that it's difficult to debug since it provides no information on the initial error. Also, I'm not sure all the issues with ZerosBridge will go through get_fallback so I feel it's easier to fix at the source as I suggested in my previous comment

F = MOI.get(model, MOI.ObjectiveFunctionType())
f = MOI.get(model, MOI.ObjectiveFunction{F}())
obj = eval_variables(model, f) do vi
return MOI.get(model, MOI.VariablePrimal(attr.result_index), vi)
end
if is_ray(MOI.get(model, MOI.PrimalStatus()))
# Dual infeasibility certificates do not include the primal
# objective constant.
obj -= MOI.constant(f, typeof(obj))
end
return obj
catch
throw(MOI.GetAttributeNotAllowed(attr))
end
return obj
end

# MOI.DualObjectiveValue
Expand Down Expand Up @@ -152,20 +156,24 @@ function get_fallback(
::Type{T},
)::T where {T}
MOI.check_result_index_bounds(model, attr)
value = zero(T) # sum will not work if there are zero constraints
for (F, S) in MOI.get(model, MOI.ListOfConstraintTypesPresent())
value += _dual_objective_value(model, F, S, T, attr.result_index)::T
end
if MOI.get(model, MOI.ObjectiveSense()) != MOI.MAX_SENSE
value = -value
end
if !is_ray(MOI.get(model, MOI.DualStatus()))
# The objective constant should not be present in rays
F = MOI.get(model, MOI.ObjectiveFunctionType())
f = MOI.get(model, MOI.ObjectiveFunction{F}())
value += MOI.constant(f, T)
try
value = zero(T) # sum will not work if there are zero constraints
for (F, S) in MOI.get(model, MOI.ListOfConstraintTypesPresent())
value += _dual_objective_value(model, F, S, T, attr.result_index)::T
end
if MOI.get(model, MOI.ObjectiveSense()) != MOI.MAX_SENSE
value = -value
end
if !is_ray(MOI.get(model, MOI.DualStatus()))
# The objective constant should not be present in rays
F = MOI.get(model, MOI.ObjectiveFunctionType())
f = MOI.get(model, MOI.ObjectiveFunction{F}())
value += MOI.constant(f, T)
end
return value::T
catch
throw(MOI.GetAttributeNotAllowed(attr))
end
return value::T
end

# MOI.ConstraintPrimal
Expand All @@ -186,16 +194,18 @@ function get_fallback(
idx::MOI.ConstraintIndex,
)
MOI.check_result_index_bounds(model, attr)
# If there is an error getting ConstraintFunction, we instead want to
# re-throw the attribute for ConstraintPrimal, not ConstraintFunction.
f = MOI.get(model, MOI.ConstraintFunction(), idx)
c = eval_variables(model, f) do vi
return MOI.get(model, MOI.VariablePrimal(attr.result_index), vi)
end
if is_ray(MOI.get(model, MOI.PrimalStatus()))
c -= MOI.constant(f, typeof(c))
try
f = MOI.get(model, MOI.ConstraintFunction(), idx)
c = eval_variables(model, f) do vi
return MOI.get(model, MOI.VariablePrimal(attr.result_index), vi)
end
if is_ray(MOI.get(model, MOI.PrimalStatus()))
c -= MOI.constant(f, typeof(c))
end
return c
catch
throw(MOI.GetAttributeNotAllowed(attr))
end
return c
end

# MOI.ConstraintDual
Expand Down Expand Up @@ -485,6 +495,10 @@ function get_fallback(
ci::MOI.ConstraintIndex{<:Union{MOI.VariableIndex,MOI.VectorOfVariables}},
)
MOI.check_result_index_bounds(model, attr)
f = MOI.get(model, MOI.ConstraintFunction(), ci)
return _variable_dual(model, attr, ci, f)
try
f = MOI.get(model, MOI.ConstraintFunction(), ci)
return _variable_dual(model, attr, ci, f)
catch
throw(MOI.GetAttributeNotAllowed(attr))
end
end
7 changes: 3 additions & 4 deletions test/Bridges/bridge_optimizer.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,10 +124,9 @@ function test_CallbackVariablePrimal()
y = MOI.get(mock, MOI.ListOfVariableIndices())[1]
z = MOI.add_variable(bridged)
attr = MOI.CallbackVariablePrimal(nothing)
@test_throws(
ErrorException("No mock callback primal is set for variable `$z`."),
MOI.get(bridged, attr, z),
)
msg = "No mock callback primal is set for variable `$z`."
err = MOI.GetAttributeNotAllowed(attr, msg)
@test_throws err MOI.get(bridged, attr, z)
MOI.set(mock, attr, y, 1.0)
MOI.set(mock, attr, z, 2.0)
@test MOI.get(bridged, attr, z) == 2.0
Expand Down
4 changes: 2 additions & 2 deletions test/Utilities/cachingoptimizer.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,8 +357,8 @@ function test_mapping_of_variables()

@testset "CallbackVariablePrimal" begin
attr = MOI.CallbackVariablePrimal(nothing)
err =
ErrorException("No mock callback primal is set for variable `$y`.")
msg = "No mock callback primal is set for variable `$y`."
err = MOI.GetAttributeNotAllowed(attr, msg)
@test_throws err MOI.get(model, attr, x)
MOI.set(mock, attr, y, 1.0)
@test_throws MOI.InvalidIndex(x) MOI.get(mock, attr, x)
Expand Down
26 changes: 7 additions & 19 deletions test/Utilities/mockoptimizer.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ function test_optimizer_solve_with_result()
MOI.set(optimizer, MOI.ObjectiveFunction{MOI.VariableIndex}(), v[1])
MOI.set(optimizer, MOI.ResultCount(), 1)
@test_throws(
ErrorException("No mock primal is set for variable `$(v[1])`."),
MOI.GetAttributeNotAllowed{MOI.VariablePrimal},
MOI.get(optimizer, MOI.VariablePrimal(), v[1])
)
@test_throws(
Expand Down Expand Up @@ -87,15 +87,11 @@ function test_optimizer_solve_with_result()
optimizer.eval_objective_value = true
@test MOI.get(optimizer, MOI.ObjectiveValue()) == 3.0
@test_throws(
ErrorException(
"No mock primal is set for variable `$(v[1])` at result index `2`.",
),
MOI.GetAttributeNotAllowed(MOI.ObjectiveValue(2)),
MOI.get(optimizer, MOI.ObjectiveValue(2))
)
@test_throws(
ErrorException(
"No mock dual is set for constraint `$c1` at result index `1`.",
),
MOI.GetAttributeNotAllowed(MOI.DualObjectiveValue()),
MOI.get(optimizer, MOI.DualObjectiveValue())
)
@test MOI.get(optimizer, MOI.DualObjectiveValue(2)) == 5.9
Expand All @@ -104,31 +100,23 @@ function test_optimizer_solve_with_result()
@test MOI.get(optimizer, MOI.VariablePrimal(), v) == [3.0, 2.0]
@test MOI.get(optimizer, MOI.VariablePrimal(), v[1]) == 3.0
@test_throws(
ErrorException(
"No mock primal is set for variable `$(v[1])` at result index `2`.",
),
MOI.GetAttributeNotAllowed{MOI.VariablePrimal},
MOI.get(optimizer, MOI.VariablePrimal(2), v[1])
)
@test MOI.get(optimizer, MOI.ConstraintPrimal(), c1) == 3.0
@test MOI.get(optimizer, MOI.ConstraintPrimal(), soc) == [3.0, 2.0]
@test_throws(
ErrorException(
"No mock primal is set for variable `$(v[1])` at result index `2`.",
),
MOI.GetAttributeNotAllowed(MOI.ConstraintPrimal(2)),
@show MOI.get(optimizer, MOI.ConstraintPrimal(2), c1)
)
@test_throws(
ErrorException(
"No mock primal is set for variable `$(v[1])` at result index `2`.",
),
MOI.GetAttributeNotAllowed(MOI.ConstraintPrimal(2)),
MOI.get(optimizer, MOI.ConstraintPrimal(2), soc)
)
@test MOI.get(optimizer, MOI.ConstraintDual(2), c1) == 5.9
@test MOI.get(optimizer, MOI.ConstraintDual(2), soc) == [1.0, 2.0]
@test_throws(
ErrorException(
"No mock dual is set for constraint `$c1` at result index `1`.",
),
MOI.GetAttributeNotAllowed{MOI.ConstraintDual},
MOI.get(optimizer, MOI.ConstraintDual(1), c1)
)
end
Expand Down