[DEV] Refactoring parallelization computation and remove duplication from solver #1017

kathvargasr · 2022-12-14T14:02:10Z

src/solver/simulation/solver.hxx's method buildSetsOfParallelYears and Study's getNumberOfCores had code duplication.
Parallelization computation is now managed by a separate class: SetsOfParallelYearCalculator

Although, due to dependencies on loading and calculations performed later at the study class, SetsOfParallelYearCalculator's results are still managed by the study and passed to the solver through study's member.

src/libs/antares/study/parallel-years.cpp

src/libs/antares/study/parallel-years.h

src/libs/antares/study/parallel-years.cpp

src/tests/src/libs/antares/study/parallel-years/test-parallel-years-calculator.cpp

src/libs/antares/study/parallel-years.cpp

src/libs/antares/study/parallel-years.h

src/libs/antares/study/parallel-years.cpp

sonarqubecloud · 2022-12-19T17:30:09Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
18 Code Smells

43.2% Coverage
0.0% Duplication

src/libs/antares/CMakeLists.txt

src/libs/antares/study/parallel-years.cpp

src/tests/src/libs/antares/study/parallel-years/test-parallel-years-calculator.cpp

src/libs/antares/study/parallel-years.h

guilpier-code · 2022-12-21T16:21:29Z

src/ui/simulator/windows/options/advanced/advanced.cpp

@@ -930,7 +930,7 @@ void AdvancedParameters::onSelectNumberOfCoresLevel(Data::NumberOfCoresMode ncMo
    {
        study->parameters.nbCores.ncMode = ncMode;
        // Force refresh for study->nbYearsParallelRaw
-        study->getNumberOfCores(false, 1 /* ignored */);
+        study->getNumberOfCores(false, false, 1 /* ignored */);


Question : why do we suppose that the getNumberOfCores's second argument (which is new here) is necessarily false ?

I don't know. Honestly it feels like the lesser issue of "getNumberOfCores"

It is in the study

It's called "get" but doesn't return anything

Last parameter is ignored if the first one is false

I do agree.
See review of src/libs/antares/study/study.cpp (#1017 (comment)) for my opinion on this

guilpier-code · 2022-12-21T16:22:31Z

src/ui/simulator/application/study.cpp

@@ -320,7 +320,7 @@ class JobSaveStudy final : public Toolbox::Jobs::Job

        // Updating the number of logical cores to use when saving the study
        // so that the run window is up to date.
-        study->getNumberOfCores(false, 0);
+        study->getNumberOfCores(false, false, 0);


Same question as in advanced.cpp's review.

src/libs/antares/study/parallel-years.h

sonarqubecloud · 2023-01-25T10:38:53Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
18 Code Smells

42.9% Coverage
0.0% Duplication

guilpier-code · 2023-01-30T13:02:00Z

src/libs/antares/study/study.cpp

+
+    SetsOfParallelYearCalculator setsBuilder(forceParallel,
+                                             enableParallel,
+                                             nbYearsParallelForced, Yuni::System::CPU::Count(),


Move Yuni::System::CPU::Count() to next line, for more readability

guilpier-code · 2023-01-30T13:54:15Z

src/libs/antares/study/parallel-years.h

+    uint forcedNbOfParallelYears_;
+    uint number_of_cores_;
+    bool thermalTSRefresh_;
+    Parameters& p;


p should be renamed parameters_

guilpier-code · 2023-01-30T15:13:08Z

src/libs/antares/study/parallel-years.h

+    // Un lot d'année à exécuter en parallèle.
+    // En fonction d'une éventuelle play-list, certaines seront jouées et d'autres non.
+
+    unsigned int setsSizes = 0;


We don't need setsSizes, we already have nbPerformedYears as a data member in this struct.
It contains exactly the same information and has a better name.
Besides, the use of this data member in class SetsOfParallelYearCalculator is a regression compared to the previous version of the parallel years orchectrator : the previous counterpart of setsSizes was the size of a set, containing also years not performed (disabled by the playlist).

A question is : what should we use instead of setsSizes ?
Should we use the set's size or the set->nbPerformedYears ? (Especially if we deal with the hydro hot start, see function allSetsParallelYearsHaveSameSize)

guilpier-code · 2023-01-30T15:30:49Z

src/libs/antares/study/parallel-years.h

+    uint number_of_cores_;
+    bool thermalTSRefresh_;
+    Parameters& p;
+    uint nbYearsReallyPerformed{0};


nbYearsReallyPerformed :
To be strict, this data should not belong to class SetsOfParallelYearCalculator.
The job of this class is to define the years that will be run at the same time (each in a different thread).
This class does not mean counting the performed years over the whole simulation.
But this class currently takes advantages of its own treatments to compute this number.

By the way, this number is already computed in other contexts :

src/libs/antares/benchmarking/info_collectors.cpp : see function performedYearsCountToFileContent(...)

parameters.cpp : see function prepareForSimulation(...), especially look for local variable effectiveNbYears

We should remove nbYearsReallyPerformed from the class and compute it somewhere else.
For now, I can't see another location to hold and compute it than the parameters.cpp, because it already contains the data member yearsFilter (see this data).

guilpier-code · 2023-01-30T16:32:11Z

src/libs/antares/study/parallel-years.cpp

+        else
+            buildNewSet = false;
+
+     // End of loop over years


This comment :
// End of loop over years
should be moved to the same line we close the bracket of the loop

guilpier-code · 2023-02-01T09:56:24Z

src/libs/antares/study/parallel-years.cpp

+
+}
+
+void SetsOfParallelYearCalculator::buildSetsOfParallelYears()


I think we could make this algorithm drier, and reject some data member assignement in a further method.
For instance, set->nbYears can be computed later, as well as set->yearFailed or
set->isFirstPerformedYearOfASet (not exhaustive list).
It would clarify this algorithm.

guilpier-code · 2023-02-01T13:36:08Z

src/tests/src/libs/antares/study/parallel-years/test-parallel-years-calculator.cpp

+
+    SetsOfParallelYearCalculator builder(forceParallel,
+                                         enableParallel,
+                                         maxNbYearsInParallel, 1,


The number of available cores on the machine is quite hidden : here it's 1.
I would recommend to declare a new variable uint machine_nb_cores = 1; and pass it to the constructor below.

guilpier-code · 2023-02-01T13:44:32Z

src/tests/src/libs/antares/study/parallel-years/test-parallel-years-calculator.cpp

+    params.resetYearsWeigth();
+    params.setYearWeight(0,4);
+    params.setYearWeight(1,10);
+    params.setYearWeight(2,3);


Why do we need to set some years' weight to test the parallel years orchestrator (or builder) ?
It seems to me that year weights and parallel years orchestrator are not related.

guilpier-code · 2023-02-01T15:08:42Z

src/libs/antares/study/parallel-years.cpp

+** \warning TODO: REMOVE THIS METHOD ONCE THE TSGEN HAS BEEN DECOUPLED FROM SOLVER
+*/
+
+bool TempAreaListHolder::checkThermalTSGeneration(YString folder_)


Thermal clusters are special regarding TS refresh.
As for any other production mean, clusters have their own global parameter to enable time series refreshment. But user can ask the refresh to any particular cluster, that is not to obey to the global parameter.
Here we check if any cluster has to refresh its TS, and we return true in that case, false otherwise.

Question : why get this information form input files, and not from memory ?
Probably because when we build the parallel years orchestrator, this information on clusters is not loaded yet.
In that case, why not build the parallel years orchestrator after the information on clusters is loaded ?

guilpier-code · 2023-02-01T16:38:38Z

About the file run.cpp, which has not been touched in the PR, but should be commented as well.
This file is about the run simulation window in the GUI.
Consider the snippet :

    // Needed For RAM estimation
    uint& maxNbCores = Data::Study::Current::Get()->maxNbYearsInParallel;
    uint maxNbCoresParallelMode = Data::Study::Current::Get()->maxNbYearsInParallel_save;

    // Needed for run window's simulation cores field
    uint& minNbCores = Data::Study::Current::Get()->minNbYearsInParallel;
    uint minNbCoresParallelMode = Data::Study::Current::Get()->minNbYearsInParallel_save;

    if (featuresAlias[pFeatureIndex] == Solver::parallel)
    {
        maxNbCores = maxNbCoresParallelMode; // For RAM estimation
        minNbCores = minNbCoresParallelMode; // For run window's simulation cores field
        ...
    }
    else
    {
        maxNbCores = 1; // For RAM estimation
        minNbCores = 1; // Not needed (because simulation cores field is hidden here)
        ....
    }

This snippet sets the variables minNbCores and maxNbCores, which are later used in the window to print a line of information like :

Simulation cores : 4 (smallest batch size : 1)

So back to the snippet : minNbCores is set to 1 and maxNbCores is set to 4.
Note that these 2 variables are references to actual variables inside the study.
So changing them here has an effect on the study state.
But what's the point of changing the study regarding these variables in the study ?
Indeed, if we run the study, these 2 values will be re-computed by the solver in a separate process.
So there is no need to change the study internally.
For me we should have :

    uint minNbCores = 0;
    uint maxNbCores = 0;
    if (featuresAlias[pFeatureIndex] == Solver::parallel)
    {
        maxNbCores = Data::Study::Current::Get()->maxNbYearsInParallel; // For RAM estimation
        minNbCores = Data::Study::Current::Get()->minNbYearsInParallel; // For run window's simulation cores field
        ...
    }
        else
    {
        // No need to set minNbCores or maxNbCores here
        ....
    }

guilpier-code · 2023-02-01T16:05:06Z

src/libs/antares/study/study.cpp

+    maxNbYearsInParallel = setsBuilder.getForcedNbOfParallelYears();
+    parameters.allSetsHaveSameSize = setsBuilder.allSetsParallelYearsHaveSameSize();
+    setsOfParallelYears = setsBuilder.getSetsOfParallelYears();
+    pNbYearsReallyPerformed = setsBuilder.getNbYearsReallyPerformed();


About :

pNbYearsReallyPerformed = setsBuilder.getNbYearsReallyPerformed();

If we agree that computing the number of performed years in the study is not part of the responsibility of the parallel years orchestrator, this line should be removed.

guilpier-code · 2023-02-01T16:47:03Z

src/libs/antares/study/study.cpp

+                                             parameters);
+    // For GUI
+    minNbYearsInParallel_save = setsBuilder.getMinNbParallelYearsForGUI();
+    maxNbYearsInParallel_save = setsBuilder.getForcedNbOfParallelYears();


About :

maxNbYearsInParallel_save = setsBuilder.getForcedNbOfParallelYears();

The data member maxNbYearsInParallel_save is useless, as it is the same as maxNbYearsInParallel, except that it is used by the GUI to print some information in the run simulation window.
We should remove this line.

guilpier-code · 2023-02-01T16:51:53Z

src/libs/antares/study/study.cpp

+                                             thermalTSRefresh,
+                                             parameters);
+    // For GUI
+    minNbYearsInParallel_save = setsBuilder.getMinNbParallelYearsForGUI();


We don't need to name minNbYearsInParallel_save as it is.
minNbYearsInParallel is enough.
Note this value is only read by GUI to print some information about the minimum size there is over all set (of parallel years)

guilpier-code · 2023-02-01T17:23:54Z

src/libs/antares/study/study.cpp

-            }
-        }
-    } // End if hot start
+void Study::getNumberOfCores(const bool forceParallel, const bool enableParallel, const uint nbYearsParallelForced) {


The function getNumberOfCores(...) updates some Study data members that are used elsewhere either in the GUI or in the solver to run the simulation properly.
Ideally, we should have an instance of SetsOfParallelYearCalculator that would contain and would be able to give all information needed about parallel sets to any part of the code that requests them.
I can't see anywhere else in the code to put this instance than as a Study's data member.
Indeed, as a singleton, the study can be reached from anywhere in the code, and deliver information.
But if some part of code needs information about parallel years, it should retrieve them through the instance study->parallelYearCalculator, and not directly from the study itself.

Furthermore, instead of having getNumberOfCores(...), study->parallelYearCalculator should have a different interface :

We could have at least 2 updates(...) methods :

one for a default update() : calculating the sets of parallel years with the characteristics of the study

one update(....) in case we have external information to compute the sets of parallel years (I mean information from the solver command line option to force the number of cores used)

some get functions to give information to any part of code requiring it.

Any other methods should be private, especially those that are used to compute the sets of parallel years

With this, we would encapsulate the information about parallel years, and furthermore we would avoid some unclean lines that can be found in the code like :
study->getNumberOfCores(false, false, 0);
Instead, we would have :
study->parallelYearCalculator->update()
which is cleaner I guess.

guilpier-code · 2023-02-02T09:31:06Z

src/libs/antares/study/parallel-years.cpp

+}
+
+
+bool SetsOfParallelYearCalculator::allSetsParallelYearsHaveSameSize()


About allSetsParallelYearsHaveSameSize() :
This function answer the question : does all sets contains the same number of performed years ?
For now It is only called in case the study enables the hydro hot start.
And that is reflected in the body of this function.
Well it should not : the function should stick to its name and answer the simple question without knowing if it's called in the context of hydro hot start or not.
So the body of this function should be :

bool SetsOfParallelYearCalculator::allSetsParallelYearsHaveSameSize() { uint currentSetSize = (uint)setsOfParallelYears[0].setsSizes; return all_of(setsOfParallelYears.begin(), setsOfParallelYears.end(), [currentSetSize](const setOfParallelYears& v){ return v.setsSizes == currentSetSize; } ); }

and that's it.
Another benefit of this simplification is that p.initialReservoirLevels.iniLevels is not required anymore in class SetsOfParallelYearCalculator, so we loose a dependency on a study parameter.

guilpier-code · 2023-02-02T09:38:59Z

src/libs/antares/study/parallel-years.h

+
+    bool allSetsParallelYearsHaveSameSize();
+
+    [[nodiscard]] uint getForcedNbOfParallelYears() const


As already explained in review of parallel-years.cpp, we return actualMaxNbOfPerformedYearsInASet_, and rename this getter accordingly

guilpier-code · 2023-02-02T10:28:40Z

src/libs/antares/study/parallel-years.cpp

+}
+
+
+void SetsOfParallelYearCalculator::limitNbOfParallelYearsbyMinRefreshSpan()


About functions limitNbOfParallelYearsbyMinRefreshSpan and isRefreshNeededForCurrentYear :

there are some duplication code in both functions

they imply a dependency of class SetsOfParallelYearCalculator to study->parameter, making this class more difficult to test in isolation that it should.

Just a first think (to be digged)
So we could have a parent class refreshSpanHandler :

class refreshSpanHandler { getSpan() = 0; doWeRefresh(int year) = 0; }

Then derive it for all kinds of production or load.
Maybe build them (create instance of them) outside the class SetsOfParallelYearCalculator.
Add them to the parallel years calculator, that is put them in a vector of parent class refreshSpanHandler, let's say refreshSpanHandlerList)
Then we could traverse this vector with an std::algorithm.
example :

refreshing = std::any_of(refreshSpanHandlerList.begin(), refreshSpanHandlerList.end(), [](const refreshSpanHandler h) { return h->doWeRefresh(y)});

Note we would avoid class SetsOfParallelYearCalculator a dependency to study.parameter in that way.

flomnes · 2023-02-07T16:45:08Z

Also, I spotted a bug (probably a bug in the GUI) : A study is attached, with 5 MC years. Trying to run it in parallel mode. Just before running, look at the simulation cores : it is 0. When the study is run, a set of 4 years is run, then a set of 1 year, as expected.

Please find the study here : 021 Four areas - DC law.zip

smallest batch size is incorrect as well (always 1)

guilpier-code · 2023-09-22T08:27:04Z

This PR still has some interest, its comments / remarks should be read, and the subject should be taken care of.

pull-request-size bot added the size/XXL label Dec 14, 2022

kathvargasr requested review from guilpier-code and flomnes December 16, 2022 08:33

flomnes requested changes Dec 16, 2022

View reviewed changes

flomnes reviewed Dec 16, 2022

View reviewed changes

src/libs/antares/study/parallel-years.cpp Outdated Show resolved Hide resolved

kathvargasr force-pushed the feature/refactoring-parallel-years branch from a9422c6 to 8b7b5a5 Compare December 19, 2022 16:25

kathvargasr requested a review from flomnes December 20, 2022 08:29

guilpier-code requested changes Dec 21, 2022

View reviewed changes

flomnes modified the milestones: v8.5, v8.4.3 Jan 4, 2023

kathvargasr mentioned this pull request Jan 5, 2023

[TEST] Refactoring study's loader #893

Closed

JasonMarechal25 force-pushed the feature/refactoring-parallel-years branch from 8b7b5a5 to 6e9ae1c Compare January 23, 2023 10:12

kathvargasr and others added 8 commits January 25, 2023 10:28

[DEV] Rework parallelization computation and remove duplication

9e1f5a7

Add more UTs and resolve code review comments

b2e33ee

Explain why call to loadAreaList is necessary

d8cba1d

Inject the number of core to compute parallel years

3d36a4c

Remove dependency to Data::StudyLoadOptions for test

dc8aba1

Remove unused fixture member

2da0d63

Changes setSizes to a simple counter

813cc77

Rename some member to follow conventions

813e2c0

JasonMarechal25 force-pushed the feature/refactoring-parallel-years branch from 2bf23a7 to 813e2c0 Compare January 25, 2023 09:28

JasonMarechal25 marked this pull request as ready for review January 25, 2023 10:44

JasonMarechal25 requested a review from guilpier-code January 25, 2023 10:44

guilpier-code requested changes Jan 30, 2023

View reviewed changes

flomnes modified the milestones: v8.4.3, v8.5.1 Jan 31, 2023

flomnes assigned guilpier-code Jan 31, 2023

guilpier-code requested changes Feb 1, 2023

View reviewed changes

guilpier-code requested changes Feb 2, 2023

View reviewed changes

Fix Simulation Cores in UI

11a8564

JasonMarechal25 closed this Jul 26, 2023

JasonMarechal25 deleted the feature/refactoring-parallel-years branch July 26, 2023 08:56

guilpier-code restored the feature/refactoring-parallel-years branch September 22, 2023 08:26

guilpier-code reopened this Sep 22, 2023

JasonMarechal25 removed this from the v8.5.1 milestone Nov 28, 2023

JasonMarechal25 marked this pull request as draft November 28, 2023 09:51

This was referenced Dec 6, 2023

Simplify TS numbers drawings, fix bug related to refresh & local thermal generation #1752

Merged

Add enabled property for ST storage objects, fix bug related to saving ST objects #1807

Merged

Manage logs in multithreads run #1793

Merged

This was referenced Dec 18, 2023

[ANT-992] Add ramping for thermal clusters #1817

Draft

Fix pmax contribution #1827

Merged

Remove yuni volatile warning #1828

Merged

DO NOT MERGE - BUILDING EXECUTABLES RTE-I #1832

Closed

This was referenced Dec 26, 2023

Use SimTest v8.8.0 #1848

Merged

Add possibility to release without running any tests #1852

Merged

[ANT-1224] Preparing loop around benders solver AntaresSimulatorTeam/antares-xpansion#739

Merged

flomnes closed this Jan 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DEV] Refactoring parallelization computation and remove duplication from solver #1017

[DEV] Refactoring parallelization computation and remove duplication from solver #1017

kathvargasr commented Dec 14, 2022

sonarqubecloud bot commented Dec 19, 2022

guilpier-code Dec 21, 2022

JasonMarechal25 Jan 24, 2023

guilpier-code Jan 31, 2023 •

edited

Loading

guilpier-code Dec 21, 2022

sonarqubecloud bot commented Jan 25, 2023

guilpier-code Jan 30, 2023

guilpier-code Jan 30, 2023

guilpier-code Jan 30, 2023 •

edited

Loading

guilpier-code Jan 30, 2023 •

edited

Loading

guilpier-code Jan 30, 2023 •

edited

Loading

guilpier-code Feb 1, 2023

guilpier-code Feb 1, 2023

guilpier-code Feb 1, 2023

guilpier-code Feb 1, 2023

guilpier-code commented Feb 1, 2023 •

edited

Loading

guilpier-code Feb 1, 2023

guilpier-code Feb 1, 2023

guilpier-code Feb 1, 2023

guilpier-code Feb 1, 2023

guilpier-code Feb 2, 2023 •

edited

Loading

guilpier-code Feb 2, 2023

guilpier-code Feb 2, 2023 •

edited

Loading

flomnes commented Feb 7, 2023

guilpier-code commented Sep 22, 2023


		}

		void SetsOfParallelYearCalculator::buildSetsOfParallelYears()

		}


		bool SetsOfParallelYearCalculator::allSetsParallelYearsHaveSameSize()


		bool allSetsParallelYearsHaveSameSize();

		[[nodiscard]] uint getForcedNbOfParallelYears() const

		}


		void SetsOfParallelYearCalculator::limitNbOfParallelYearsbyMinRefreshSpan()

[DEV] Refactoring parallelization computation and remove duplication from solver #1017

[DEV] Refactoring parallelization computation and remove duplication from solver #1017

Conversation

kathvargasr commented Dec 14, 2022

sonarqubecloud bot commented Dec 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilpier-code Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilpier-code Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

guilpier-code Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

guilpier-code Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilpier-code commented Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilpier-code Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilpier-code Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

flomnes commented Feb 7, 2023

guilpier-code commented Sep 22, 2023

guilpier-code Jan 31, 2023 •

edited

Loading

guilpier-code Jan 30, 2023 •

edited

Loading

guilpier-code Jan 30, 2023 •

edited

Loading

guilpier-code Jan 30, 2023 •

edited

Loading

guilpier-code commented Feb 1, 2023 •

edited

Loading

guilpier-code Feb 2, 2023 •

edited

Loading

guilpier-code Feb 2, 2023 •

edited

Loading