solve problem by multiple solvers #2450

oriyalperin · 2024-05-20T12:07:59Z

Description

As @erelsgl suggested in discussion #1522,
in some problems, some solvers may be better than others, but they might also fail with a SolverError.
Therefore, we've added a new method that allows sending a list of solvers rather than just one.
This function tries them in the order they are provided, and return the first solution for which no SolverError is raised.
Issue link: #1529

Type of change

New feature (backwards compatible)
New feature (breaking API changes)
Bug fix
Other (Documentation, CI, ...)

Contribution checklist

Add our license to new files.
Check that your code adheres to our coding style.
Write unittests.
Run the unittests and check that they’re passing.
Run the benchmarks to make sure your change doesn’t introduce a regression.

CLAassistant · 2024-05-20T12:08:04Z

All committers have signed the CLA.

SteveDiamond · 2024-05-20T17:09:03Z

cvxpy/problems/problem.py

@@ -416,6 +416,50 @@ def compilation_time(self) -> float | None:
        """
        return self._compilation_time

+    def solve_multiple_solvers(self, solvers:list[tuple[str, dict] | tuple[str] | str] = None):


This is nice functionality! I don't think we need to support an argument of None, since anyone calling this function should be giving some list of solvers.

Thanks for the review!
I'll remove the support for None.
What about an empty list? Currently, the implementation uses the solve method without arguments in this case.
Do you think I should keep it like that?

I would say that an empty list should raise a ValueError.

Thanks! Should the other errors related to input validation (such as inner input types) also raise ValueError?
Currently, I've used ParameterError.

Yes, these are all ValueError. Let me sync a bit with the other devs though.

Thanks, please let me know if any further changes are needed :)

PTNobel

Overall I'm positive on this but not a huge fan of the current public API. I left my suggestions. Aditionaly, I don't like solve_multiple_solvers, could we call the method solve_via_chain, solve_with_fallback, try_solve, or try_solve_till_success? I think Stephen Boyd often uses the term solver chain for this feature; I can ask him next week.

cvxpy/problems/problem.py

cvxpy/tests/test_problem.py

cvxpy/problems/problem.py

Apply suggestions from code review Co-authored-by: Parth Nobel <parthnobel@berkeley.edu>

Co-authored-by: Parth Nobel <parthnobel@berkeley.edu>

oriyalperin · 2024-05-26T06:36:28Z

@PTNobel thank you for all your suggestions!

Overall I'm positive on this but not a huge fan of the current public API. I left my suggestions. Aditionaly, I don't like solve_multiple_solvers, could we call the method solve_via_chain, solve_with_fallback, try_solve, or try_solve_till_success? I think Stephen Boyd often uses the term solver chain for this feature; I can ask him next week.

That would be great, thanks!

PTNobel · 2024-06-03T23:20:05Z

Stephen said we should call it a "solver path" in the documentation; he also suggested making it a keyword argument to .solve instead of a new public function (maybe .solve(path=[...]). I'm wondering if maybe we could just specifialize on if the input is a string or a list to make .solve(solver=[cp.SCS, cp.CLARABEL]) the API. Thoughts on that @SteveDiamond @phschiele ?

rileyjmurray · 2024-06-03T23:44:32Z

I’m not sure about “path” as a keyword argument. I’d be in favor of “solver_path” or “solver_priority” for keyword arguments to prob.solve.

…

On Mon, Jun 3, 2024 at 7:20 PM Parth Nobel ***@***.***> wrote: Stephen said we should call it a "solver path" in the documentation; he also suggested making it a keyword argument to .solve instead of a new public function (maybe .solve(path=[...]). I'm wondering if maybe we could just specifialize on if the input is a string or a list to make .solve(solver=[cp.SCS, cp.CLARABEL]) the API. Thoughts on that @SteveDiamond <https://github.com/SteveDiamond> @phschiele <https://github.com/phschiele> ? — Reply to this email directly, view it on GitHub <#2450 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACRLIFCZAUD7QOITPGPDR7TZFT24BAVCNFSM6AAAAABH7TE5E2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBWGI4DIMJZGQ> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

PTNobel · 2024-06-05T03:07:28Z

I’m not sure about “path” as a keyword argument. I’d be in favor of “solver_path” or “solver_priority” for keyword arguments to prob.solve.

Fair! I don't like solver_priority since we fallback to the other solvers instead of giving up, so I think solver_path would be a good choice.

erelsgl · 2024-06-05T05:16:42Z

What if both "solver" and "solver_path" are given?

PTNobel · 2024-06-05T06:39:09Z

I think that should raise a runtime error.

rileyjmurray · 2024-06-05T10:03:22Z

That should raise a ValueError

…

On Wed, Jun 5, 2024 at 2:39 AM Parth Nobel ***@***.***> wrote: I think that should raise a runtime error. — Reply to this email directly, view it on GitHub <#2450 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACRLIFC22NDBTSI3NOFMKCDZF2XCHAVCNFSM6AAAAABH7TE5E2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBYHE4TMNBUGU> . You are receiving this because you commented.Message ID: ***@***.***>

SteveDiamond · 2024-06-05T17:43:48Z

Stephen said we should call it a "solver path" in the documentation; he also suggested making it a keyword argument to .solve instead of a new public function (maybe .solve(path=[...]). I'm wondering if maybe we could just specifialize on if the input is a string or a list to make .solve(solver=[cp.SCS, cp.CLARABEL]) the API. Thoughts on that @SteveDiamond @phschiele ?

How would this work when you have solver specific arguments to the solve method?

phschiele · 2024-06-05T18:09:23Z

Stephen said we should call it a "solver path" in the documentation; he also suggested making it a keyword argument to .solve instead of a new public function (maybe .solve(path=[...]). I'm wondering if maybe we could just specifialize on if the input is a string or a list to make .solve(solver=[cp.SCS, cp.CLARABEL]) the API. Thoughts on that @SteveDiamond @phschiele ?

How would this work when you have solver specific arguments to the solve method?

Could be .solve(solver=[(cp.SCS, scs_args), (cp.CLARABEL, clarabel_args)]) but it might be getting complicated at this point.

oriyalperin · 2024-06-10T10:54:00Z

I've made some changes according to your suggestions. Does this meet your expectations?

PTNobel · 2024-06-10T18:34:41Z

I like this PR with Oriya's changes. Since this is a change to the public API, let's get maintainer consensus on this proposal before writing the new docs and merging.

PTNobel · 2024-06-10T18:35:43Z

cvxpy/problems/problem.py

+        """
+
+        if not solvers:
+            raise ValueError("Solvers list must contain at least one solver.")


Suggested change

raise ValueError("Solvers list must contain at least one solver.")

raise ValueError("Solver path must contain at least one solver.")

PTNobel · 2024-06-10T18:36:21Z

cvxpy/problems/problem.py

@@ -415,6 +415,58 @@ def compilation_time(self) -> float | None:
                   last time it was compiled.
        """
        return self._compilation_time
+
+    def _solve_solver_path(self, solve_func, solvers:list[tuple[str, dict] | tuple[str] | str],
+                                *args, **kwargs):


Suggested change

*args, **kwargs):

args, kwargs):

PTNobel · 2024-06-10T18:36:38Z

cvxpy/problems/problem.py

+            if solver is not None:
+                raise ValueError(
+                    "Cannot specify both 'solver' and 'solver_path'. Please choose one.")
+            return self._solve_solver_path(solve_func,solver_path,*args,**kwargs)


Suggested change

return self._solve_solver_path(solve_func,solver_path,*args,**kwargs)

return self._solve_solver_path(solve_func,solver_path, args, kwargs)

PTNobel · 2024-06-10T18:38:35Z

cvxpy/problems/problem.py

+                        solution = solve_func(
+                            self, *args, solver=solver_name, **solver_kwargs, **kwargs)
+                    else:
+                        raise ValueError("Solver tuple input must be (str, dict) or (str)")


Suggested change

raise ValueError("Solver tuple input must be (str, dict) or (str)")

raise ValueError("Solver path entry must be str, tuple[str] or tuple[str, dict[str, Any]]")

PTNobel · 2024-06-10T18:40:16Z

cvxpy/problems/problem.py

+                        raise ValueError("Solver tuple input must be (str, dict) or (str)")
+                else:
+                    raise ValueError("Solver input must be one of the following:\
+                                                (str, dict) tuple, (str) tuple, or str.")


Can you define a constant string somewhere and use the same message on line 461 and 463?

PTNobel · 2024-06-10T18:41:39Z

cvxpy/tests/test_problem.py

+                    cp.sum_squares(cp.matmul(A, cp.Variable(40)) - b))).solve(
+                    solver_path=solvers)
+
+        ## solver_path ans solver arguments


Suggested change

## solver_path ans solver arguments

## Specify both solver_path and solver as arguments

oriyalperin added 5 commits May 16, 2024 04:43

Add solve with multiple solvers

90dcb15

Update documentation and messages

8ea4c2f

code style

067de10

Use ValueError

fa675bd

Fix documentation

c336dbd

SteveDiamond reviewed May 20, 2024

View reviewed changes

PTNobel requested changes May 23, 2024

View reviewed changes

cvxpy/problems/problem.py Outdated Show resolved Hide resolved

cvxpy/problems/problem.py Outdated Show resolved Hide resolved

cvxpy/tests/test_problem.py Outdated Show resolved Hide resolved

PTNobel mentioned this pull request May 23, 2024

Customize solver preference order #2383

Open

PTNobel requested changes May 23, 2024

View reviewed changes

cvxpy/problems/problem.py Outdated Show resolved Hide resolved

cvxpy/problems/problem.py Outdated Show resolved Hide resolved

cvxpy/problems/problem.py Outdated Show resolved Hide resolved

oriyalperin and others added 3 commits May 26, 2024 08:42

Add args and kwargs

55dab7c

Apply suggestions from code review Co-authored-by: Parth Nobel <parthnobel@berkeley.edu>

Merge branch 'master' of https://github.com/ariel-research/cvxpy

d5f09a9

Update cvxpy/tests/test_problem.py

07d8682

Co-authored-by: Parth Nobel <parthnobel@berkeley.edu>

oriyalperin added 2 commits May 26, 2024 11:07

Adjust code review changes

f0541d1

Merge branch 'master' of https://github.com/ariel-research/cvxpy

b1c925e

Use solver_path

182607f

PTNobel requested changes Jun 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

solve problem by multiple solvers #2450

solve problem by multiple solvers #2450

oriyalperin commented May 20, 2024

CLAassistant commented May 20, 2024 •

edited

SteveDiamond May 20, 2024

oriyalperin May 21, 2024

SteveDiamond May 21, 2024

oriyalperin May 21, 2024

SteveDiamond May 21, 2024

oriyalperin May 21, 2024

PTNobel left a comment •

edited

oriyalperin commented May 26, 2024

PTNobel commented Jun 3, 2024

rileyjmurray commented Jun 3, 2024 via email

PTNobel commented Jun 5, 2024

erelsgl commented Jun 5, 2024

PTNobel commented Jun 5, 2024

rileyjmurray commented Jun 5, 2024 via email

SteveDiamond commented Jun 5, 2024

phschiele commented Jun 5, 2024

oriyalperin commented Jun 10, 2024

PTNobel commented Jun 10, 2024

PTNobel Jun 10, 2024

PTNobel Jun 10, 2024

PTNobel Jun 10, 2024

PTNobel Jun 10, 2024

PTNobel Jun 10, 2024

PTNobel Jun 10, 2024

	raise ValueError("Solvers list must contain at least one solver.")
	raise ValueError("Solver path must contain at least one solver.")

	return self._solve_solver_path(solve_func,solver_path,args,*kwargs)
	return self._solve_solver_path(solve_func,solver_path, args, kwargs)

	raise ValueError("Solver tuple input must be (str, dict) or (str)")
	raise ValueError("Solver path entry must be str, tuple[str] or tuple[str, dict[str, Any]]")

	## solver_path ans solver arguments
	## Specify both solver_path and solver as arguments

solve problem by multiple solvers #2450

Are you sure you want to change the base?

solve problem by multiple solvers #2450

Conversation

oriyalperin commented May 20, 2024

Description

Type of change

CLAassistant commented May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PTNobel left a comment • edited

Choose a reason for hiding this comment

oriyalperin commented May 26, 2024

PTNobel commented Jun 3, 2024

rileyjmurray commented Jun 3, 2024 via email

PTNobel commented Jun 5, 2024

erelsgl commented Jun 5, 2024

PTNobel commented Jun 5, 2024

rileyjmurray commented Jun 5, 2024 via email

SteveDiamond commented Jun 5, 2024

phschiele commented Jun 5, 2024

oriyalperin commented Jun 10, 2024

PTNobel commented Jun 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented May 20, 2024 •

edited

PTNobel left a comment •

edited