#991: LoadModel: Add API to calculate how many phases of past load data need to be kept #995

PhilMiller · 2020-08-18T22:44:28Z

Fixes: #991

PhilMiller · 2020-08-18T22:47:38Z

I haven't figured out where to integrate this with the rest of the LB instrumentation and invocation infrastructure yet.

codecov · 2020-08-18T23:26:19Z

Codecov Report

Merging #995 into develop will increase coverage by 0.02%.
The diff coverage is 85.00%.

@@             Coverage Diff             @@
##           develop     #995      +/-   ##
===========================================
+ Coverage    77.62%   77.65%   +0.02%     
===========================================
  Files          666      667       +1     
  Lines        25552    25613      +61     
===========================================
+ Hits         19834    19889      +55     
- Misses        5718     5724       +6

Impacted Files	Coverage Δ
...c/vt/vrt/collection/balance/model/composed_model.h	`100.00% <ø> (ø)`
...rc/vt/vrt/collection/balance/model/linear_model.cc	`80.00% <0.00%> (-12.31%)`	⬇️
src/vt/vrt/collection/balance/model/linear_model.h	`100.00% <ø> (ø)`
src/vt/vrt/collection/balance/model/load_model.h	`100.00% <ø> (ø)`
...t/vrt/collection/balance/model/naive_persistence.h	`100.00% <ø> (ø)`
...c/vt/vrt/collection/balance/model/per_collection.h	`100.00% <ø> (ø)`
...lection/balance/model/persistence_median_last_n.cc	`84.21% <0.00%> (-9.91%)`	⬇️
...llection/balance/model/persistence_median_last_n.h	`100.00% <ø> (ø)`
src/vt/vrt/collection/balance/model/raw_data.h	`100.00% <ø> (+14.28%)`	⬆️
src/vt/vrt/collection/balance/node_stats.h	`100.00% <ø> (ø)`
... and 16 more

lifflander

I don't see the part of this that actually calls getNumPastPhasesNeeded from NodeStats. I assume that's coming.

src/vt/vrt/collection/balance/model/per_collection.cc

lifflander · 2020-08-19T21:43:26Z

@PhilMiller A couple notes:

NodeStats::clearStats does not clear subphase data
I think we need to also remove the history from ElemStats. It also contains a redundant copy that completely unnecessary.

PhilMiller · 2020-08-19T22:13:16Z

So, ElemStats is going to migrate with each object, while NodeStats records whatever was in place in a given phase. If we're concerned about the redundancy, NodeStats would seem to be the natural choice to flatten into an aggregated accessor for the individual ElemStats

PhilMiller · 2020-08-19T22:13:44Z

And yeah, the omission in clearStats was a mistake

pnstickne · 2020-08-23T19:39:46Z

src/vt/vrt/collection/balance/model/load_model.h

+   * \return How many phases of past load statistics will be needed to
+   * satisfy the requested history
+   */
+  virtual int getNumPastPhasesNeeded(int look_back = 0) = 0;


Force caller to choose value?

Has "How many phases into the past.." yet most usages are max(other, look_back). Or perhaps "..the minimum number of phases.." or so?

pnstickne

Might be useful to have an overall test capture.

PhilMiller · 2020-08-28T21:09:08Z

I'd like to see #1001 merged before this. Its testing was important in getting some of this right, and it carries some bug fixes of its own.

PhilMiller · 2020-08-28T21:09:42Z

Right now, this still only addresses NodeStats and not ElemStats. Half the leakage handled, at least.

src/vt/vrt/collection/balance/model/raw_data.cc

PhilMiller · 2020-08-31T18:32:59Z

There's a limitation this change will impose on load balancing usage. The LB interval can't be shorter than the minimum look-back range that the models define, or the NodeStats structure on a process p won't have all necessary data for any objects that have immigrated to p in the last LB run

JacobDomagala

Looks good to me

PhilMiller · 2020-08-31T22:34:27Z

There's a limitation this change will impose on load balancing usage. The LB interval can't be shorter than the minimum look-back range that the models define, or the NodeStats structure on a process p won't have all necessary data for any objects that have immigrated to p in the last LB run

This is not a problem for the default configuration, using NaivePersistence, because its look-back range of 1 will always be satisfied.

lifflander

Overall, this looks like a definite improvement to me.

The only thing lacking is incremental LB stat file output so the stats aren't lost from clearing them outside of the live window.

…ta need to be kept

…on of older entries

…hase count

…called independent of LB running

…peatedly over WHOLE_PHASE

…ff-by-one errors

lifflander · 2020-08-31T23:02:23Z

Here is an overview of what got changed by this pull request:

Clones added
============
- tests/unit/collection/test_model_raw_data.nompi.cc  3
         

Clones removed
==============
+ tests/unit/collection/test_model_persistence_median_last_n.nompi.cc  -1
+ tests/unit/collection/test_model_linear_model.nompi.cc  -1

See the complete overview on Codacy

PhilMiller requested review from pnstickne and lifflander August 18, 2020 22:44

PhilMiller requested a review from nlslatt August 18, 2020 22:47

lifflander reviewed Aug 19, 2020

View reviewed changes

src/vt/vrt/collection/balance/model/per_collection.cc Show resolved Hide resolved

pnstickne reviewed Aug 23, 2020

View reviewed changes

pnstickne approved these changes Aug 25, 2020

View reviewed changes

lifflander mentioned this pull request Aug 25, 2020

Meeting Agenda [do not close] #925

Open

PhilMiller force-pushed the 991-limit-nodestats-by-model-needs branch from 096fcbd to 8fdf45b Compare August 28, 2020 20:58

PhilMiller marked this pull request as draft August 28, 2020 21:06

PhilMiller mentioned this pull request Aug 31, 2020

959 Add more tests for various load models Part 1 #1001

Merged

4 tasks

PhilMiller commented Aug 31, 2020

View reviewed changes

src/vt/vrt/collection/balance/model/raw_data.cc Outdated Show resolved Hide resolved

PhilMiller force-pushed the 991-limit-nodestats-by-model-needs branch from 1169bd7 to 0e91408 Compare August 31, 2020 17:37

PhilMiller added the 1.0.0-beta.10 label Aug 31, 2020

PhilMiller force-pushed the 991-limit-nodestats-by-model-needs branch from 20c9288 to 72e0bfb Compare August 31, 2020 19:26

PhilMiller requested a review from JacobDomagala August 31, 2020 20:36

PhilMiller marked this pull request as ready for review August 31, 2020 21:26

JacobDomagala approved these changes Aug 31, 2020

View reviewed changes

lifflander approved these changes Aug 31, 2020

View reviewed changes

PhilMiller added 3 commits August 31, 2020 18:59

#991: LoadModel: Add API to calculate how many phases of past load da…

e855625

…ta need to be kept

#991: Record number of phases worth of data modeled explicitly

2129d3f

#991: Convert NodeData containers to associative maps to allow deleti…

673d32f

…on of older entries

PhilMiller added 10 commits August 31, 2020 18:59

#991: Update model unit tests for API changes

af5012c

#991: RawData model: respect data structures no longer growing with p…

3e6d03d

…hase count

#991: NodeStats: Clear out obsolete load data

c009d4a

#991: NodeStats: add missing clear of subphase data

f62cfa5

#991: Fix per-collection test to ensure that model->updateLoads gets …

b429c25

…called independent of LB running

#991: Norm Model: Fix to actually loop over subphases, rather than re…

75af2f7

…peatedly over WHOLE_PHASE

#991: Raw Data Model: Access last completed phase correctly

7b49a34

#991: Raw Data Model: Move all implementation out of header file

723ef52

#991: Raw Data Model: Refactor to simplify usage and avoid inviting o…

625aaae

…ff-by-one errors

#991: Raw Data Model: Implement comprehensive unit test

7b2aa6b

PhilMiller force-pushed the 991-limit-nodestats-by-model-needs branch from e4c1f97 to 7b2aa6b Compare August 31, 2020 23:00

PhilMiller merged commit 2fc7655 into develop Sep 1, 2020

PhilMiller deleted the 991-limit-nodestats-by-model-needs branch September 1, 2020 15:27

lifflander mentioned this pull request Sep 2, 2020

Fix NodeStats LB stat file output #1014

Closed

lifflander removed the 1.0.0-beta.10 label Sep 8, 2020

cz4rs mentioned this pull request Jun 15, 2021

move vt version number into a separate file #1467

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#991: LoadModel: Add API to calculate how many phases of past load data need to be kept #995

#991: LoadModel: Add API to calculate how many phases of past load data need to be kept #995

PhilMiller commented Aug 18, 2020 •

edited

Loading

PhilMiller commented Aug 18, 2020

codecov bot commented Aug 18, 2020 •

edited

Loading

lifflander left a comment

lifflander commented Aug 19, 2020

PhilMiller commented Aug 19, 2020

PhilMiller commented Aug 19, 2020

pnstickne Aug 23, 2020

pnstickne left a comment

PhilMiller commented Aug 28, 2020

PhilMiller commented Aug 28, 2020

PhilMiller commented Aug 31, 2020

JacobDomagala left a comment

PhilMiller commented Aug 31, 2020

lifflander left a comment

lifflander commented Aug 31, 2020

#991: LoadModel: Add API to calculate how many phases of past load data need to be kept #995

#991: LoadModel: Add API to calculate how many phases of past load data need to be kept #995

Conversation

PhilMiller commented Aug 18, 2020 • edited Loading

PhilMiller commented Aug 18, 2020

codecov bot commented Aug 18, 2020 • edited Loading

Codecov Report

lifflander left a comment

Choose a reason for hiding this comment

lifflander commented Aug 19, 2020

PhilMiller commented Aug 19, 2020

PhilMiller commented Aug 19, 2020

pnstickne Aug 23, 2020

Choose a reason for hiding this comment

pnstickne left a comment

Choose a reason for hiding this comment

PhilMiller commented Aug 28, 2020

PhilMiller commented Aug 28, 2020

PhilMiller commented Aug 31, 2020

JacobDomagala left a comment

Choose a reason for hiding this comment

PhilMiller commented Aug 31, 2020

lifflander left a comment

Choose a reason for hiding this comment

lifflander commented Aug 31, 2020

PhilMiller commented Aug 18, 2020 •

edited

Loading

codecov bot commented Aug 18, 2020 •

edited

Loading