-
Notifications
You must be signed in to change notification settings - Fork 95
[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.
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
Conversation
# Dual infeasibility certificates do not include the primal | ||
# objective constant. | ||
obj -= MOI.constant(f, typeof(obj)) | ||
try |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Closing in favor of #2415 |
This is a possible fix for #2410 (comment)
https://github.com/jump-dev/MathOptInterface.jl/actions/runs/7635259102