Skip to content
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

Rework pull request #60: Enabling the use of gurobipy directly #98

Merged
merged 16 commits into from
Oct 4, 2024

Conversation

slivingston
Copy link
Member

This pull request applies updates to changes from #60 that are necessary before this can be merged, including feedback from @johnyf

I propose to squash-merge this because the commit history is not useful except for review of this PR to show changes to #60

@slivingston
Copy link
Member Author

In 2eed8aa I add links in the module documentation to the upstream Python packages. PyPI is a canonical reference. We can also include commercial website URLs, for example

Copy link
Member

@johnyf johnyf left a comment

Choose a reason for hiding this comment

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

Thank you for reworking the gurobipy addition. Squashing the commits when merging would group the changes, which are related.

tests/pytest.ini Outdated
[pytest]
addopts = --strict-markers -m 'not nonfree'
markers =
nonfree: marks tests as requiring nonfree (i.e., having restrictive licenses) dependencies
Copy link
Member

Choose a reason for hiding this comment

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

Another possibility could be to use @pytest.mark.skipif() to test presence of the dependency gurobipy. Motivation would be to show to users which test was not run when pytest -v is used.

try:
    import gurobipy
except ImportError:
    gurobipy = None


@pytest.mark.skipif(
    gurobipy is None,
    reason='`gurobipy` is not installed')
def test_gurobipy_return_same_result_as_scipy():
    ...

This is similar to how the presence of matplotlib activates test_plot_transition_arrow() in plot_test.py.

The output of pytest -v . in case gurobipy is not installed includes:

polytope_test.py::test_gurobipy_return_same_result_as_scipy SKIPPED (`gurobipy is not installed`)

When using additional markers, pytest --markers can be used to find which markers annotate tests that were not run.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, and I changed to this instead. A summary of the reasons is in the commit message:

skipif() has the advantage of explicitly warning the user if tests
are not included due to gurobipy not being installed. This is entirely
similar to behavior in plot_test.py with matplotlib.

This changeset replaces the nonfree marker, which was not friendly
to newcomers: they would need to know to pytest -m nonfree if they
wanted to test with Gurobi. Otherwise, pytest (without the marker)
would silently not include those tests.

The advantages of using the nonfree marker are:

  1. emphasizing to the user that tests involve behavior by code without
    open source,
  2. failing if the corresponding packages (gurobipy, possibly others in
    the future) are not installed.
    However, users who know enough to use pytest -m nonfree likely are
    aware of Gurobi's license and likely would notice a warning when tests
    are running, so indeed, skipif() is the best choice.

return result
elif m.Status == gurobi.GRB.INFEASIBLE:
result['status'] = 2
elif m.Status == gurobi.GRB.INF_OR_UNBD or m.Status == gurobi.GRB.UNBOUNDED:
Copy link
Member

Choose a reason for hiding this comment

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

It seems that status 3 (dual infeasible) can be returned also for a primal infeasible problem, when m.Params.DualReductions = 1.

https://www.gurobi.com/documentation/current/refman/optimization_status_codes.html

INF_OR_UNBD 4 Model was proven to be either infeasible or unbounded. To obtain a more definitive conclusion, set the DualReductions parameter to 0 and reoptimize.

Relevant documentation:

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! There are at least 4 approaches here:

  1. change DualReductions to 0, call logger.warning, and run again;
  2. result['status'] = 4 if m.Status == gurobi.GRB.INF_OR_UNBD and add a comment about DualReductions and manually using gurobipy in result['message'];
  3. same as previous, but add **kwargs to lpsolve(), and then forward these keyword arguments to _solve_lp_using_gurobi, handling an optional dual_reductions keyword;
  4. similar to previous, but create a special solver name "gurobi_DualReductions" that can be used to run with DualReductions=0.

Copy link
Member

Choose a reason for hiding this comment

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

Rerunning

Rerunning with DualReductions = 0 (and logger.warning) seems simpler.
(The option to run once, with DualReductions = 0 is discussed below.)

Rerunning seems to require also resetting with m.reset(0).
https://www.gurobi.com/documentation/current/refman/py_model_reset.html#pythonmethod:Model.reset

The requirement to reset before rerunning after obtaining GRB_INF_OR_UNBD is discussed at:
https://support.gurobi.com/hc/en-us/community/posts/4402929210641-Help-with-Unbounded-or-Infeasible
from which the conclusion for rerunning is:

m.reset(0)
m.Params.DualReductions = 0
m.optimize()

Resetting is described as:

The Gurobi algorithms keep careful track of the state of the model, so calls to Model.optimize will only perform further optimization if relevant data has changed since the model was last optimized. If you would like to discard previously computed solution information and restart the optimization from scratch without changing the model, you can call Model.reset.

https://www.gurobi.com/documentation/current/refman/py_python_api_overview.html

About the option to set m.Params.DualReductions = 0 and run once, it seems that dual reductions can reduce runtime. So running twice might improve runtime on average.

Reductions are the names given to the different techniques used to transform a model during presolve (see How does presolve work?). Reductions which are solely based on reasoning applied to the feasible region are known as "primal reductions", while dual reductions consider the objective function. Primal reductions do not remove feasible solutions from the solution space where as dual reductions may, provided at least one optimal solution is guaranteed to remain.
...
Note that dual reductions can be manually disabled by setting DualReductions=0. In very rare instances this may improve performance, but typically it will make the solver perform worse.

https://support.gurobi.com/hc/en-us/articles/27875045953041-What-are-dual-reductions

(Apparently dual reductions can also make the optimization take very long https://support.gurobi.com/hc/en-us/community/posts/13519151774481-Using-DualReductions-completely-halts-optimization-What-can-cause-this)

About other options

Since gurobi can refine its decision between infeasible and unbounded problem, and the distinction is used in several functions of module polytope.polytope (e.g., bounding_box(), reduce()), it seems that automatically rerunning (or always) with DualReductions = 0 might increase the accuracy of those functions.

Other options that require the user to configure DualReductions = 0 result in all optimization problems being solved without dual reductions. This might increase the overall runtime (e.g. for feasible problems where an optimal solution is found when DualReductions = 1 faster than it would with DualReductions = 0).

Another option for the case m.Status == gurobi.GRB.INF_OR_UNBD is to raise ValueError, to print the status code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I found that in my local installation of gurobipy, the status code INF_OR_UNBD occurs for an optimization problem with objective min x and feasible set x =< 1, so I added it to the function test_gurobipy_return_same_result_as_scipy(), confirming that the result status is unbounded and warning if INF_OR_UNBD did not occur. This is only a warning because it is an implementation detail where, if Gurobi identifies the problem as unbounded on the first run, the optimization result is correct. Warning encourages us to investigate why reoptimize is not necessary, but doing so is not urgent.

SciPy linprog may include a field message in the optimization result. We are not currently using it explicitly in polytope.solvers, so in the case of reoptimizing with Gurobi, I added the note:

result['message'] = 'Gurobi optimization status was INF_OR_UNBD, so reoptimized with DualReductions=0'

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the discussion. I implemented automatic rerunning with DualReductions=0 in 5753984

polytope/solvers.py Outdated Show resolved Hide resolved
Base automatically changed from pkg-maintenance to main October 3, 2024 19:35
RichardOberdieck and others added 16 commits October 3, 2024 12:37
The Gurobi Optimizer (https://www.gurobi.com/) is not open source.
Automatically using a restrictive-license-encumbered dependency is
an anti-pattern of open source code. Users should have to explicitly
request it.
Use assert_allclose() instead of assert_almost_equal(), following
recommendation in documentation of assert_almost_equal [1].
Swap order of the arguments such that Gurobi output is first;
it is the "actual" value that we compare to the "desired" SciPy [2].

[1] https://numpy.org/doc/stable/reference/generated/numpy.testing.assert_almost_equal.html
[2] https://numpy.org/doc/stable/reference/generated/numpy.testing.assert_equal.html
The example code at https://pypi.org/project/gurobipy/ uses the
abbreviation `import gurobipy as gp`, but I think it is too short
and not easily recognized for use as a module-level name.
And this can be used to mark tests with MOSEK and other
extra dependencies with restrictive licenses in the future.

These tests are not included by default and must be requested:

    pytest -m nonfree tests
skipif() has the advantage of explicitly warning the user if tests
are not included due to gurobipy not being installed. This is entirely
similar to behavior in plot_test.py with matplotlib.

This changeset replaces the `nonfree` marker, which was not friendly
to newcomers: they would need to know to `pytest -m nonfree` if they
wanted to test with Gurobi. Otherwise, `pytest` (without the marker)
would silently not include those tests.

The advantages of using the `nonfree` marker are:
1. emphasizing to the user that tests involve behavior by code without
   open source,
2. failing if the corresponding packages (gurobipy, possibly others in
   the future) are not installed.
However, users who know enough to use `pytest -m nonfree` likely are
aware of Gurobi's license and likely would notice a warning when tests
are running, so indeed, skipif() is the best choice.
@slivingston slivingston force-pushed the rework-pull-request-60 branch from 7521bd9 to 5753984 Compare October 3, 2024 21:17
Copy link
Member

@johnyf johnyf left a comment

Choose a reason for hiding this comment

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

Thank you for revising the changes.

@slivingston slivingston merged commit 83026e5 into main Oct 4, 2024
10 checks passed
@slivingston slivingston deleted the rework-pull-request-60 branch October 4, 2024 17:09
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.

3 participants