-
Notifications
You must be signed in to change notification settings - Fork 505
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
Support user-provided CyIpopt callbacks with 13 arguments #3289
Conversation
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 have one minor edit suggestion but otherwise this looks fine.
…pyomo into cyipopt-extend-callback
Looks like the Jenkins test failure is real. Here is the stack-trace:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3289 +/- ##
=======================================
Coverage 88.53% 88.54%
=======================================
Files 868 868
Lines 98495 98509 +14
=======================================
+ Hits 87206 87221 +15
+ Misses 11289 11288 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
One comment, but not something that I will hold up the PR for.
# callback to infer whether they want this argument. To preserve backwards | ||
# compatibility if the user asked for variable-length *args, we do not pass | ||
# the Problem object as an argument in this case. |
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 am a big fan of backwards compatibility, but in this case, I think I disagree: if the user defined the callback using *args
, then it is their responsibility to track any changes to our callback API.
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 counterpoint is that they may reasonably expect the callback API to be stable. Personally, I would rather we didn't support *args
at all. Maybe we should raise a deprecation warning if a 12-arg callback or *args
is provided?
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 counter-counterpoint is this is in contrib
, so this is where we make the weakest (i.e., no) guarantee on backwards compatibility.
With so many arguments, I like the model where we pass everything by name, and deprecate all use of positional arguments. We could be clever and even allow callbacks with subsets of named arguments (which would support backwards compatibility), future-proof us to passing new arguments, and remove the current reliance on a large set of ordered arguments.
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.
While CyIpoptInterface
is in contrib, a user can provide a callback via pyo.SolverFactory("cyipopt", intermediate_callback=callback)
. I've always had the impression that solvers accessible via SolverFactory
(without some prefix e.g. "contrib.cyipopt"
), should remain stable. To me, it's a gray area. That said, I do like the idea of only supporting named arguments.
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.
@Robbybp how do you want to proceed with this? Should we merge it as-is to get it into the August release and open an issue to deprecate positional arguments in favor of named arguments? Or wait and modify this PR directly?
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.
Let's merge this as-is to get it in for the release. I created issue #3354 to track this discussion.
Summary/Motivation:
Currently, users are unable to access the
get_current_iterate
andget_current_violations
methods during an intermediate callback as they don't have access to thecyipopt.Problem
object. We currently expect a user-provided callback to be a function with 12 arguments: the 11 arguments that Ipopt gives us, plus the NLP object that cyipopt is using. This PR adds support for a new callback API, where the user's callback accepts 13 arguments. The new argument is thecyipopt.Problem
instance (ourCyIpoptNLP
), from which the user can access theget_current*
methods.This is the first step towards implementing something like #2860. With this PR, users can implement their own callbacks to track primal-dual iterates and infeasibilities. Eventually, I'd like to commit a basic debugging callback that automatically tracks useful information.
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: