-
Notifications
You must be signed in to change notification settings - Fork 51
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 QP solver through OSQP #225
Conversation
@Midnighter @KristianJensen this is done for now. There may be small fixes we can make in the future but it's pretty complete. |
@cdiener I will try to have a look at it as soon as possible |
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.
A couple of lines about OSQP should be added to installation.rst
. Also, I left some comments regarding cuosqp
. Other than that, it looks good to me.
# OSQP can be provided by the base or CUDA package | ||
solvers['OSQP'] = False | ||
try: | ||
import cuosqp |
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.
cuosqp
does not provide a linear system solver, so it does not cover all the functionality of osqp
(see osqp/osqp#30 and osqp/cuosqp#2). I guess the user will know that, but it would be nice to document that (and also document osqp
at installation.rst
).
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 think that just means that the direct linear solver does not run on the GPU for now, but it still uses a linear solver.
import unittest | ||
|
||
try: # noqa: C901 | ||
import osqp |
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.
Tested this with cuosqp
and it does not work for some of the classes of tests (as we would expect). Not that it matters for CI, but would it be fine to have some tests that should pass for cuosqp
in case it is installed?
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.
Yeah, it fixes some of the parameters like lambda. What tests fail? I would suspect the LP problems since those have a lambda adjustment in the code that is probably ignored. I'll add a warning when using cuosqp.
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 tests that fail with cuosqp
are those under ConstraintTestCase
and ModelTestCase.test_binary_variables
. There could be more. The thing is that the tests don't fail, they error out:
test_binary_variables (optlang.tests.test_osqp_interface.ModelTestCase) ... /home/georg/.virtualenvs/opt/lib/python3.9/site-packages/cuosqp/utils.py:20: UserWarning: Linear system solver not recognized. Using default solver CUDA PCG.
warn("Linear system solver not recognized. " +
terminate called after throwing an instance of 'thrust::system::system_error'
what(): parallel_for failed: cudaErrorNoKernelImageForDevice: no kernel image is available for execution on the device
[1] 19842 abort (core dumped) python -m nose src/optlang/tests -v
That's why I thought that they don't have a linear solver. Does it work for you? It might be a problem with my cuda version (using 11.2), but they say v11 is supported (other libraries which use cuda work for me and some of the OSQP tests pass).
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 looks more like an issue with the compute capability of the used card. I think I tried it on Google Colab before and it kind of worked.
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 could reproduce the error on colab https://gist.github.com/carrascomj/a4d91a926814646e36592a853516c7ac
Is there something that I am not installing maybe?
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 think you just did not change the Runtime to one with a GPU. If you do that it does run but it is slow due to the setting issue and some tests fail because it can't hit the needed accuracy for LPs.
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.
RIght! I had changed it but when I restarted the runtime it went back to CPU for some reason. It works (with some caveats as you said), so this PR looks done to me.
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 will add a warning ng message for cuosqp on load to make clear that is an experimental feature. Since you have already worked through it, would be great if you could add a review to the PR, thanks!
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 don't have write access either (although you may have it now?). First thing on Monday, I will ask Niko to approve it so it can be finally merged. Also, please remember the docs! Something along the lines of
OSQP:
osqp
|cuosqp
OSQP is an efficient open source solver for Quadratic Programs. It is self-contained and can be installed via pip. Alternatively, cuOSQP provides a cuda-enabled implementation.
under installation.rst
would suffice, I guess.
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.
OK, I have write access now so feel free to merge it.
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.
LGTM.
Failing now due to missing packages for swiglpk 5.0.0 for Mac and some security issue with pyyaml which is not a dependency so no idea where that comes from. |
Okay nice, at that point the only thing missing is to remove Python 3.5 from the CI. I'll send a separate PR for that. |
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.
Looks good to me. I agree that it is a useful addition to the solver set.
Ups, that was a bit too early. This needed additional fixes to make the CI work. I'll send another PR. |
This is a first working version of a QP solver via OSQP. OSQP expects the problem to be sparse and static so I added a thin compatibility layer that manages the OSQP problem via dictionaries. All operations on that layer are at worst O(nv + nc) but it makes problem setup about 10-20% of the entire solve time for most models. Could maybe be optimized more in the future but should be sufficient for now. Still need to add tests but I added some more complicated models to the test data for now.
It now supports all features such as reduced costs etc. This version is also faster than the previous PR since it gets primals and duals via the C interface instead of evaluating expressions. We used it in the recent MICOM course and it worked well.
Demo:
One should note that this is a direct solver so it performs pretty bad for LPs or at very low tolerances. I added some optimizations for LP so it works ok-ish but worse than GLPK. However it is great for QPs so I think having a specialized solver for that is still helpful.
TODO