-
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
Add SCIP Persistent Support #3200
base: main
Are you sure you want to change the base?
Conversation
Two open questions left for this pull request:
For info of how I tested the code:
|
@Opt-Mucca - answering your questions:
|
@mrmundt Thanks for the fast reply!
|
@Opt-Mucca - No problem!
|
I have now added:
|
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.
Thanks for submitting this PR! Some comments from a quick pass.
if not flag: | ||
raise RuntimeError( | ||
"***The scip_direct solver plugin cannot extract solution suffix=" | ||
+ suffix | ||
) |
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.
Design question: is it better to raise an error or to log (or print loudly) that other requests are being ignored?
(Note: I don't have a strong preference either way.)
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 would lean slightly towards the print loudly option, but can see the potential danger for users. As a larger preference though, I would like to stick to similar behavior with the other interfaces.
try: | ||
import pyscipopt | ||
|
||
scip_available = True | ||
except ImportError: | ||
scip_available = False |
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 theorize that @jsiirola may want this logic instead put in the import attempt code: https://github.com/Pyomo/pyomo/blob/main/pyomo/common/dependencies.py#L1037
try: | ||
import pyscipopt | ||
|
||
scip_available = True | ||
except ImportError: | ||
scip_available = False |
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 theorize that @jsiirola may want this logic instead put in the import attempt code: https://github.com/Pyomo/pyomo/blob/main/pyomo/common/dependencies.py#L1037
@Opt-Mucca - Please make sure you're on the most recent version of |
Co-authored-by: Miranda Mundt <[email protected]>
@mrmundt Black should be run and all the issues responded to. |
@Opt-Mucca - Spelling is the new failure. (You are suffering the same issue as all the core devs... The auto-linter and spell checker.) |
@mrmundt TIL there are auto-linters that double as spell checkers. (I'll fix the spelling errors that come up each time the test fails. If it's just the two errors then I'm properly amazed.) |
@Opt-Mucca - If you could have heard the griping in our weekly dev call when I introduced the spell checker... |
@mrmundt I can imagine (would probably be guilty of griping even if makes sense) For the currently failing runs: I've added a fix that should work, but I will ask some colleagues tomorrow. (I'm not the greatest the warm start code). The fixes raise some warning where a loaded solution has variables of incorrect values (this may stem from non specified variables having a default value of 0) The current reason of the pipeline failure: SCIP was trying to load an invalid solution before checking for feasibility (I think this is a longstanding python error with the interface and one of the functions) The added solution: I simply added a check on the feasibility of the solution before adding it to the solution storage. I will check with colleagues, but I'm not sure on how SCIP handles partial solutions. I am not familiar or don't know if we have any functionality like Gurobi that tries to complete the solution. This also holds for trying to repair a given infeasible solution. |
Support for partial solution loading is now added. The failed tests are now passing locally too. |
@Opt-Mucca - If you need help with your tests, please let us know. We tend to ignore PRs until the tests are passing. |
@mrmundt I am completely unsure on the protocol here. I've never done a merge request where I was not a member of the repository. The failed test now passes locally, but I am unable to trigger any pipelines here. |
@Opt-Mucca - Please make sure to read our Contribution Guide as it'll answer most questions about our process. As for triggering tests here, because you are a first-time contributor, one of our core dev team members needs to approve them (which I have been doing for this PR). |
@mrmundt This will take awhile.
|
@mrmundt After talking to some colleagues we decided to not support reduced cost and dual values. They are simply too difficult to access for SCIP in a context that people would understand without disabling presolve (The loss in performance just doesn't outweigh the people that want to access this information). There are currently two issues, both of which I would appreciate some advice or help on:
|
We discussed this during the developer call and would advise that you not support the |
I have now removed support for the I am still a bit unsure on what to do with the trivial constraints. The Python interface for SCIP just simply doesn't allow dummy constraints to be added. Some variables have to feature somewhere in the constraint. |
@Opt-Mucca - We have been discussing redesigning the solver test suite for a fair bit of time and have added it as an action item to #1030 . We will address this as a corner case for that. |
@mrmundt Can you please run the pipeline now? I put |
Hmmmmmmmmmmmmmm. I didn't have scipy installed when I tested locally so it skipped that test. The error is that
|
@Opt-Mucca - Our general thought process is "protect users from themselves." I would recommend converting them but also log / alert the user so they know the conversion is happening. |
@mrmundt I've added a warning now for each time a constraint is added and the RHS / LHS needs to be changed. |
@mrmundt I'm not sure if the failing tests are because of my changes. Can I get a second opinion on that? |
Fixes # .
None.
Summary/Motivation:
Adds support for SCIP persistent solving. Persistent solving uses the python interface to SCIP directly as opposed to the current standard that involves read and writing a temporary model file.
The new dependency that it introduces is
PySCIPOpt
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: