-
Notifications
You must be signed in to change notification settings - Fork 22
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
refactor: remove Gurobi requirement #106
Conversation
:param None/int threads: number of threads to use. | ||
:param None/dict solver_kwargs: keyword arguments to pass to solver (if any). | ||
:return: (*int*) runtime of scenario in seconds | ||
""" |
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 try to understand how you can avoid to instantiate the parent class using super()
. The parameters used in REISE.run_scenario_gurobi
) are all attributes of the Launcher
class (to the exception of execute_dir
and threads
) are set in the constructor of Launcher
. I guess I am missing something.
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.
GurobiLauncher
inherits the __init__
from Launcher
since we don't redefine our own.
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 see. I guess I have never been in a situation where the child class does not have a constructor
Think we'll need to add a |
Good catch, if Gurobi is not a requirement of REISE.jl then yes we need to install it manually. It looks like we would modify l.24 of the current Dockerfile, but I don't entirely understand what's going on there currently:
|
Good question, I think @ahurli would know. |
What's the final word on dockerfile changes? Are they needed, or can we avoid the add and let users of the dockerfile add whichever solver they want? |
I'd say we should add it by default, since there isn't currently a great mechanism to have users specify that. They would have to attach a shell to the container and run the command in a julia prompt anytime the image is updated, which could be as often as code is merged. |
In any case, I think the flexibility will come from something like a parameter to call.py where we choose the |
I guess I'm still a bit fuzzy on how this dockerfile is intended to be used. Is it supposed to be the minimum requirements to get this package up and running, or something that will cover all use cases for all users? Does it make sense to have multiple images to cover multiple use cases? |
That's a good question.. usually the answer depends on how much variation there is between use cases. Like in Todd's case for CERF, there is a large base image with several different images built on top of that for different cases which are totally different (this is my interpretation). Here, I think the trade off of having potentially several extra packages is worth it since the other option adds complexity of setting up multiple builds and having users make a choice. Also, it feels like still one use case here - the different solvers don't really change the "mode of operation". If anything, the rest api and command line use cases are more different. To answer the question - right now the goal is more to cover all use cases, but in the future it would be reasonable to consider splitting into multiple images depending on the situation. |
Alright, I added what is needed (I think) to be able to launch Gurobi scenarios from within this Dockerfile without any extra steps. We add Gurobi to the environment, and then import it to enable the |
Huh, I thought I had it work without the import statement, but gonna trust you on that one. |
Working as in compiling without an error, or working as in able to launch a scenario successfully? |
I thought I had launched a scenario, but can't remember since I was switching branches a lot. Let's go ahead with this since I trust you on getting the julia environment right. |
Purpose
Removes the requirement that Gurobi.jl be installed as a requirement of REISE.jl, since it can now be run with any optimizer (Closes #101). Allows the user to pass solver settings from python (Closes #102).
What is the code doing
run_scenario_gurobi
wraps aroundrun_scenario
as defined in src/REISE.jl.new_model(env::Gurobi.Env)
creates another method for thenew_model
function defined in loop.jl; the one in loop.jl will take any type, but if we pass aGurobi.Env
object then Julia will use the most specific method available, the one defined in gurobi.jl (see https://docs.julialang.org/en/v1/manual/methods/ for more details).Gurobi
import, instead importingRequires
, and using this package to only include src/solver_specific/gurobi.jl if Gurobi is also imported.solver_kwargs
to be passed torun_scenario
.JuMP.Model
instead that we built ininterval_loop
so that when we are running with the shared gurobi environment in we can free the Gurobi environment in the most proper way according to their documentation.interval_loop
:JuMP.Model
instance, to get returned viarun_scenario
and cleaned up as necessary inrun_scenario_gurobi
launch_scenario
function into a class with methods, and we define a gurobi-specific subclass that does gurobi-specific things.main
to use this new object-based launch method.Testing
Julia
From Julia, we can verify that this works by creating fresh environments, installing REISE.jl along with with different solvers (note that Gurobi.jl is not installed by default), and running as desired:
With Gurobi.jl
With GLPK
Python
Time to review
30-60 minutes.