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

Legacy ortools behind API - user story 3.1 #2455

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

guilpier-code
Copy link
Contributor

@guilpier-code guilpier-code commented Oct 8, 2024

Do not merge with changes on CI yaml files

What we plan to do :

  • Replace solver ortools + Sirius with API + (ortools + Sirius).
    More specifically, we replace the current ortools LP building with a use :
    • of existing classes OrtoolsLinearProblem and LinearProblemBuilder
    • the coding of a new filler.
  • Current ortools LP run with the run of OrtoolsLinearProblem
  • Run CI end-to-end tests with options --use-ortools --ortools-solver sirius
  • Check that regressions are expected (only on Windows and for "parallel" tests)
  • No unit tests are planned

Notes :

  • The first commit of this PR only intends to run all end-to-end tests with options --use-ortools --ortools-solver sirius in the CI. We can spot the expected regressions.
  • Some comments from the author of devs were added to this comment to spot difficulties or possible enhancements.

@guilpier-code guilpier-code changed the title Legacy ortools behind API : in CI, run tests with ortools + sirius Legacy ortools behind API - user story 3.1 Oct 8, 2024
@pull-request-size pull-request-size bot added size/L and removed size/M labels Oct 9, 2024
Copy link

sonarcloud bot commented Oct 9, 2024

@guilpier-code guilpier-code marked this pull request as ready for review October 10, 2024 08:56

private:
OrtoolsMipVariable* addVariable(double lb, double ub, bool integer, const std::string& name);

std::shared_ptr<operations_research::MPSolver> mpSolver_;
operations_research::MPSolver* mpSolver_;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you can see, mpSolver_ is no longer a shared pointer, at least for now.
The reason is that current Antares code (where the ortools solver are called) is made in such a way that the solver must be shared between optimization and weeks.
So a solver cannot be destroyed too soon, it has to be persistent.
It's planned to handle the life time in the future.

@guilpier-code guilpier-code requested review from flomnes, payetvin, JasonMarechal25 and pet-mit and removed request for payetvin October 10, 2024 14:35
Comment on lines +385 to +392
auto ortoolsProblem = std::make_unique<OrtoolsLinearProblem>(Probleme.isMIP(), options.ortoolsSolver);
auto legacyOrtoolsFiller = std::make_unique<LegacyOrtoolsFiller>(ortoolsProblem->MPSolver(), &Probleme);
std::vector<LinearProblemFiller*> fillersCollection = {legacyOrtoolsFiller.get()};
LinearProblemData LP_Data;
LinearProblemBuilder linearProblemBuilder(fillersCollection);

linearProblemBuilder.build(*ortoolsProblem, LP_Data);
auto MPproblem = std::shared_ptr<MPSolver>(ortoolsProblem->MPSolver());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This block appears twice in this file.
It is meant to create a ortools MPSolver.
It's a kind of code duplication.
This duplication will be removed in a further PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant