-
Notifications
You must be signed in to change notification settings - Fork 526
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
Merged
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
d11cfb1
unit tests for intermediate callback
Robbybp fd039f5
add integration tests for cyipopt intermediate callback with 12 or 13…
Robbybp b1cc0a4
apply black
Robbybp 170df3e
guard against cyipopt not avilable
Robbybp 9c91aa2
Merge branch 'main' into cyipopt-extend-callback
mrmundt 8892c85
replace vTBD with current dev version
Robbybp 2b8f4e0
spaces in error message
Robbybp 142341f
add comment attempting to explain myself
Robbybp ad73e71
Merge branch 'main' into cyipopt-extend-callback
blnicho c9956ac
grammar
Robbybp 7238bbe
Merge branch 'cyipopt-extend-callback' of https://github.com/robbybp/…
Robbybp 6820db3
Merge branch 'main' of https://github.com/pyomo/pyomo into cyipopt-ex…
Robbybp 58b5df8
test ipopt version before trying get_current_iterate
Robbybp 671d8c6
Merge branch 'main' of https://github.com/pyomo/pyomo into cyipopt-ex…
Robbybp File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 viapyo.SolverFactory("cyipopt", intermediate_callback=callback)
. I've always had the impression that solvers accessible viaSolverFactory
(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.