-
Notifications
You must be signed in to change notification settings - Fork 95
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
Feature: Gurobi backend interface #551
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #551 +/- ##
==========================================
+ Coverage 95.30% 95.92% +0.61%
==========================================
Files 25 26 +1
Lines 3664 3899 +235
Branches 803 768 -35
==========================================
+ Hits 3492 3740 +248
+ Misses 83 69 -14
- Partials 89 90 +1
|
I have added tests, but we don't have gurobipy installed in the CI builds so those tests are being skipped. Hence the massive coverage drop. |
db4d420
to
4ff46d0
Compare
I have run the urban scale model for a full year using the pyomo (with gurobi solver) and gurobi interfaces and get the attached results. Headline: Gurobi interface peaks at ~30% less memory than pyomo. gurobi run: import calliope
calliope.set_log_verbosity("info")
m = calliope.examples.urban_scale(time_subset=None)
m.build(backend="gurobi")
m.solve(solver_options={"Threads": 1}) pyomo run: import calliope
calliope.set_log_verbosity("info")
m = calliope.examples.urban_scale(time_subset=None)
m.build(backend="pyomo")
m.solve(
solver="gurobi", solver_options={"Threads": 1}) Profiling using memray and setting [out] and [runfile] separately for pyomo and gurobi runs: memray run --trace-python-allocators -o [out].bin [runfile].py
memray flamegraph [out].bin -o [out].html |
Last week's new pyomo release has caused an issue in one of our tests (see #597). I've pinned Pyomo upper bound to avoid this issue for now. |
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.
Fairly big review. I tried to be thorough since this is an important change.
"Nice to haves" are just that, consider ignoring them if they result in too much work.
Also, I expect that the recent changes to the backend handling and the linter will trigger errors once you try to merge this into main.
tests/test_backend_gurobi.py
Outdated
from .common.util import build_test_model as build_model | ||
from .common.util import check_error_or_warning | ||
|
||
if importlib.util.find_spec("gurobipy") is not None: |
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.
Shouldn't we just add the solver to our dev requirements to avoid this one?
Avoiding it for the base requirements makes sense. For dev, not so much, imo.
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 tried, but it won't work. Our dependencies are currently pip and conda cross-compatible. On PyPI, this dependency is gurobipy, on Anaconda it is gurobi... So, this breaks our compatibility. I've still removed the check in the tests for gurobipy. I would prefer to alert the developer to the need to install gurobipy!
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.
Hmm... in the future we might want to switch to pixi
to avoid this kind of stuff. It prefers conda-forge
by default, but you can redirect dependencies in a case-by-case basis if needed...
I'm ok with leaving this as is for now though!
Thanks for the review @irm-codebase - updated! |
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.
Very happy with the changes!
I've added a couple of things to clarify. After that, I can approve.
docs/creating/config.md
Outdated
@@ -39,6 +39,16 @@ To test your model pipeline, `config.init.time_subset` is a good way to limit yo | |||
|
|||
## Deep-dive into some key configuration options | |||
|
|||
### `config.build.backend` | |||
|
|||
By default, the optimisation problem is built using the [pyomo][] library. |
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.
Broken pyomo
link?
Mkdocs doesn't like the pyomo inventory.
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.
Great, thank you for the changes @brynpickering!
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 was going to send this PR back because pytest failed for some tests (due to some duplicated warning messages)... but re-making my mamba
environment fixed this.
I think we should re-think our approach to dependencies, though, because following your instructions also leads to failures due to the lack of cbc
solver, but that's been there for a while so it does not relate to this PR.
We can't provide generalised instructions for cbc in the installation guide due to Windows users not being able to install it directly (and |
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.
Since I had looked at this previously and Ivan did a detailed review I'm only making some minor suggestions for the documentation. Good to go!
59d6e19
Co-authored-by: Stefan Pfenninger <stefan@pfenninger.org>
Fixes #349
Fixes #597 (by pinning pyomo)
Summary of changes in this pull request:
gurobipy
interfaceHow to use:
gurobipy
into your calliope env:mamba install gurobi::gurobi
NOTE: without a license, one can only work with very small models. Our example models are already too big (maybe if run over ~2 timesteps it would work?).
Reviewer checklist: