LP solver support #536
LP solver support #536
Conversation
and gurobi version 11.0.2 support M
…bi tests in case storm was compiled with gurobi but there is no valid license available
…cal images on a mac and options to build with soplex/gurobi
| inline void initialize() { | ||
| extern bool noGurobi; | ||
|
|
||
| inline void initialize(int* argc, char** argv) { |
There was a problem hiding this comment.
I had to lookup how gtest arguments are handled. Maybe add a comment:
// GoogleTest-specific commandline arguments were already processed before and removed from argc/argv
Dockerfile
Outdated
| @@ -19,6 +20,9 @@ ARG build_type=Release | |||
| ARG no_threads=1 | |||
| # Specify CMake arguments for Storm | |||
| ARG cmake_args="-DSTORM_PORTABLE=ON" | |||
| # Can be ON/OFF | |||
| ARG gurobi_support="ON" | |||
There was a problem hiding this comment.
I see the point of providing some convenience flags, but could we not use cmake_args instead? Then we have more flexibility to provide any CMake flags. We are also already using cmake_args to define the configurations in the CI.
The other option would be to add more on/off switches for all relevant configurations (spot, MathSAT, Cln, ...)
There was a problem hiding this comment.
I dislike the default then, because you need to carefully mimic what is already in the default. But maybe we can add the arguments hardcoded and one may overwrite them (as far as i understand, cmake takes the rightmost definition as the actual one). What do you think?
There was a problem hiding this comment.
We only need the PORTABLE flag as default argument to ensure platform independence. Should we simply hard-code this cmake flag and leave cmake_args empty? Then users can always provide their own flags.
There was a problem hiding this comment.
but how do we then set that the default is to include spot, soplex, etc in this build?
There was a problem hiding this comment.
Ah okay, you want to have the dependencies enabled by default. Then I agree that it is better to have separate flags for it.
There was a problem hiding this comment.
After this merge, I will create a new PR for some additional configurations: Spot, MathSat etc. I think it is better to have the changes to the Dockerfile in a separate PR because I should also adapt the deployed CI image then.
|
@volkm As this now seems to compile with Gurobi, we need to add the --noGurobi flag to the tests. How do we do that? I will make sure that the MILP test also listens to the noGurobi flag. |
|
One issue: ctest does not forward the arguments to googletest. We are currently using ctest to run all the tests and have a make target "test". There seems to be some workaround for supporting arguments. Another possibility would be to define the extra arguments in cmake when defining the googletest. One additional small note: warnings on unknown arguments are currently not printed because the log level is set to ERROR. |
|
Hmm. I am not so sure how to properly handle this then... |
|
Maybe not so nice, but we could add a CMake flag Another idea could be to use labels and then exclude tests based on the label. Even another idea is to use environment variables. |
Gurobi should be enabled through PR moves-rwth#536 again.
Gurobi should be enabled through PR moves-rwth#536 again.
* Added configuration options to Dockerfile * Disabled Gurobi in CI such that tests run. Gurobi should be enabled through PR #536 again.
|
@volkm i now check for the license at the start of every test suite. That also includes test suites which do not need that test, but hey... |
|
I think it looks good for now. It is a bit overkill to check the license for every test suite, but I agree that it is currently the easiest way to handle the Gurobi license. |
This PR adds four features towards better support for LP based MDP model checking: