pyomo icon indicating copy to clipboard operation
pyomo copied to clipboard

Add SCIP Persistent Support

Open Opt-Mucca opened this issue 1 year ago • 30 comments

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:

  • SCIPPersistent
  • SCIPDirect
  • Some new tests for the introduced features

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:

  1. I agree my contributions are submitted under the BSD license.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

Opt-Mucca avatar Mar 18 '24 14:03 Opt-Mucca

Two open questions left for this pull request:

  • Is the current standard usage of persistent solving through the APPSI? If so then I should consider adding that.
  • I avoided tinkering with the GitHub action workflow scripts. I'm not sure if it is appropriate to add a PySCIPOpt dependency there.

For info of how I tested the code:

  • pip install pyscipopt
  • python pyomo/solvers/tests/checks/test_SCIP{Persistent/Direct}.py

Opt-Mucca avatar Mar 18 '24 14:03 Opt-Mucca

@Opt-Mucca - answering your questions:

  1. The current standard is APPSI, yes; however, the future standard is going to be using the persistent interface within pyomo.contrib.solver (the preview of the reworked solver interface system)
  2. If PySCIPOpt is needed in order to run tests, then yes, absolutely, it's appropriate. Happy to help with that if you'd like.

mrmundt avatar Mar 18 '24 14:03 mrmundt

@mrmundt Thanks for the fast reply!

  1. After having a glance at pyomo.contrib.solver, it seems to overlap pretty heavily with the direct interfaces in pyomo.solvers.plugins. So that should save on work. I will probably avoid writing the APPSI functionality if there's soon to be a newer interface, but can do if there's demand.
  2. Help would be appreciated! I assumed the only changes needed were those that are present in the other PRs #3165 and #2880 (replacing coptpy / maingopy with pyscipopt). If that's right then I can add those lines.

Opt-Mucca avatar Mar 18 '24 15:03 Opt-Mucca

@Opt-Mucca - No problem!

  1. Sounds good! There will be inevitable changes for the new interface (e.g., configuration options, results object, solution statuses, etc.), but we hope that they aren't too miserable for porting.
  2. That's about it, yup! Make sure to consider both the pip and conda situations, though, because we test environments built by both types of package managers.

mrmundt avatar Mar 18 '24 17:03 mrmundt

I have now added:

  • Some automatic documentation for SCIPPersistent similar to other solver interfaces
  • The pip instructions to the .github/workflows. (I avoided conda because I'm not familiar with it. I also only added PySCIPOpt via pip when using CPython)

Opt-Mucca avatar Mar 19 '24 10:03 Opt-Mucca

@Opt-Mucca - Please make sure you're on the most recent version of black and run (in the root): black . -S -C --exclude examples/pyomobook/python-ch/BadIndent.py

mrmundt avatar Mar 19 '24 13:03 mrmundt

@mrmundt Black should be run and all the issues responded to.

Opt-Mucca avatar Mar 19 '24 14:03 Opt-Mucca

@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 avatar Mar 19 '24 15:03 mrmundt

@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 avatar Mar 19 '24 15:03 Opt-Mucca

@Opt-Mucca - If you could have heard the griping in our weekly dev call when I introduced the spell checker...

mrmundt avatar Mar 19 '24 15:03 mrmundt

@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.

Opt-Mucca avatar Mar 19 '24 16:03 Opt-Mucca

Support for partial solution loading is now added. The failed tests are now passing locally too.

Opt-Mucca avatar Mar 20 '24 09:03 Opt-Mucca

@Opt-Mucca - If you need help with your tests, please let us know. We tend to ignore PRs until the tests are passing.

mrmundt avatar Apr 09 '24 18:04 mrmundt

@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 avatar Apr 10 '24 08:04 Opt-Mucca

@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 avatar Apr 10 '24 13:04 mrmundt

@mrmundt This will take awhile.

  • I fixed the current issue (I was accessing the transformed problem after solving and the pyomo map was failing + SOS and nonlinear constraints can't access slack information). Such a fix required some new wrapped functions in PySCIPOpt, so will have to add them and release a new version.
  • There are still errors in test_writers. I don't see this being easy. Most all (aside from an error with trivial constraints) stem from the reduced costs + slack being incorrect. This information is always weird to handle with SCIP for non-LP problems, so will need to think on it.

Opt-Mucca avatar Apr 11 '24 11:04 Opt-Mucca

@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:

  • I cannot calculate the slack correctly. After some tricks with ranged constraints I've ensured that SCIP is outputting the correct values, but the baseline comparisons seem to arbitrarily take either rhs - activity or activity - lhs. Internally we take the minimum of the two, but that doesn't seem to be the cases in the tests. Defaulting to one side in the case of ranged constraints also seems to fail.
  • I am not sure how to handle trivial constraints. I have set the flag _skip_trivial_constraints=True, but the corresponding tests are now failing. Adding a dummy constraint key to the internal dictionary mappings also doesn't seem to help.

Opt-Mucca avatar Apr 19 '24 17:04 Opt-Mucca

We discussed this during the developer call and would advise that you not support the slack suffix. This is old functionality that we don't intend to support in the new solver interfaces and the same information can be calculated using a simple utility function. We're supportive if you want to skip trivial constraints, however this will likely require some updates to baseline tests.

blnicho avatar Apr 23 '24 18:04 blnicho

I have now removed support for the slack suffix.

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. I think changing the tests to skip solvers with the _skip_trivial_constraints=True flag is a good idea. The alternative would be to pretend there is a constraint on the SCIP interface side, which would be extremely messy. Is anyone willing to change the test scope on the Pyomo dev team side?

Opt-Mucca avatar Apr 29 '24 09:04 Opt-Mucca

@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 avatar May 21 '24 18:05 mrmundt

@mrmundt Can you please run the pipeline now? I put scip_persistent as an expected fail for the LP_trivial_constraint test case. Everything is now passing locally.

Opt-Mucca avatar Jun 13 '24 12:06 Opt-Mucca

Hmmmmmmmmmmmmmm. I didn't have scipy installed when I tested locally so it skipped that test. The error is that np.float is a non-allowed type when using PySCIPOpt. The user must give Python float or int types. Should probably consider expanding on that. For now there's two options, both of which are pretty fast:

  • I just check if the user is giving non-standard Python values, e.g. numpy.float64, and convert them to Python (Will do this unless you advise against). Downside is this adds a tiny bit of overhead.
  • Skip tests where numpy.float64 values are passed as LHS and RHS values.

Opt-Mucca avatar Jun 13 '24 13:06 Opt-Mucca

@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 avatar Jun 13 '24 14:06 mrmundt

@mrmundt I've added a warning now for each time a constraint is added and the RHS / LHS needs to be changed. I had to use type(x) == y because isinstance(x, float) also passes for np.float (didn't know that). The failed tests are now passing.

Opt-Mucca avatar Jun 13 '24 14:06 Opt-Mucca

@mrmundt I'm not sure if the failing tests are because of my changes. Can I get a second opinion on that?

Opt-Mucca avatar Jun 14 '24 08:06 Opt-Mucca