Skip to content

Conversation

mlubin
Copy link
Contributor

@mlubin mlubin commented Mar 5, 2022

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:

  • We were not able to polish off a numpy interface thanks to pybind11_protobuf being incompatible with cmake. We'll have something for the next release. In the meantime, there will be extra overhead in translating the model between python and C++.
  • The apply function is duplicated between the GLOP and the PDLP interface. This code is going to change (and likely diverge) when we have numpy interfaces, so it seemed premature to refactor to share the code.
  • I'm not sure of the right way to check that OR-Tools is version 9.3 or better. (Otherwise the code will error out.)
  • There's a internal warning printed after each solve, this will go away in the version bump of OR-Tools: 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

Copy link
Collaborator

@rileyjmurray rileyjmurray left a 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.

Copy link
Contributor Author

@mlubin mlubin left a 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.

@rileyjmurray
Copy link
Collaborator

@mlubin it seems glop_conif.py is using Version but doesn't have from cvxpy.utilities.versioning import Version

@mlubin
Copy link
Contributor Author

mlubin commented Mar 6, 2022

@mlubin it seems glop_conif.py is using Version but doesn't have from cvxpy.utilities.versioning import Version

Just fixed that. I'm confused why tests were passing and this was caught only by the linter.

@mlubin
Copy link
Contributor Author

mlubin commented Mar 6, 2022

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

@rileyjmurray
Copy link
Collaborator

@mlubin it seems glop_conif.py is using Version but doesn't have from cvxpy.utilities.versioning import Version

Just fixed that. I'm confused why tests were passing and this was caught only by the linter.

Tests were passing because import_solver() raised an error, so cvxpy assumed GLOP wasn't installed. Ideally, we should be checking for what kind of error is raised from import_solver() and have logic to handle the expected case (package not installed) versus unexpected cases (a function being called within import_solver() that was never imported).

Copy link
Collaborator

@rileyjmurray rileyjmurray left a 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.

Copy link
Collaborator

@SteveDiamond SteveDiamond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@SteveDiamond SteveDiamond merged commit 19e13eb into cvxpy:master Mar 6, 2022
@Mizux
Copy link

Mizux commented Mar 6, 2022

FYI pybind11_protobuf CMake support is tracked here: pybind/pybind11_protobuf#60

@mlubin mlubin deleted the ml/pdlp branch March 6, 2022 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interface to OR-Tools/PDLP
4 participants