-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PDLP interface #1682
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
PDLP interface #1682
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.
This mostly looks good. I left comments on how to check the ortools version and about updating the web documentation. You also need to update this line of our CI setup script so that the appropriate version of ortools gets installed. Right now the unit tests are failing on all macOS builds.
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.
You also need to update this line of our CI setup script so that the appropriate version of ortools gets installed. Right now the unit tests are failing on all macOS builds.
The 9.3 release hasn't yet been posted for macOS. If you prefer I can update install_dependencies.sh to skip installing ortools on macOS, but if I understand correctly the new version checking code should get the tests passing now (because GLOP and PDLP won't show up as installed solvers), and then when 9.3 is available for macOS, the tests will start running it, which is what we want.
@mlubin it seems |
Just fixed that. I'm confused why tests were passing and this was caught only by the linter. |
Oh, the exception in import_solver would just cause the solver to not be "installed". |
Tests were passing because |
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! Normally we'd need two project maintainers to approve expanding the API with another solver, so I won't merge just yet. I'll check with Steven and Akshay on Discord to see if they want to review or just approve it as a natural extension of your GLOP PR.
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
FYI pybind11_protobuf CMake support is tracked here: pybind/pybind11_protobuf#60 |
Adds an interface to PDLP, using a similar structure to the GLOP interface.
Also switch the Glop interface to use a proto struct rather than a text string for parameters. My impression is that most users won't be familiar with the text format for protos and that it's easier to work with the normal proto object directly.
Notes:
String field 'operations_research.MPSolutionResponse.solver_specific_info' contains invalid UTF-8 data when parsing a protocol buffer. Use the 'bytes' type if you intend to send raw bytes.
Closes #1649
CC @dapplegate @lperron @Mizux