1835: fix remaining issues in `RawData::getModeledComm` #1842

cz4rs · 2022-06-06T11:30:50Z

fixes #1835
fixes #1836
fixes #1837

fix subphases handling
add per message and per byte weights
move the implementation into a separate load model

Note: these were tiny issues (created per comment), so it's convenient to fix them with a single PR.

TODO:

add unit test for the new model (see test_model_comm_overhead.nompi.cc)
discussion: should we avoid adding a new model and simply replace the logic in CommOverhead? (no)

github-actions · 2022-06-06T11:35:04Z

Pipelines results

PR tests (gcc-6, ubuntu, mpich)

Build for 05cc300

Compilation - successful

Testing - passed

Build log

PR tests (gcc-5, ubuntu, mpich)

Build for 05cc300

Compilation - successful

Testing - passed

Build log

PR tests (clang-3.9, ubuntu, mpich)

Build for 05cc300

Compilation - successful

Testing - passed

Build log

PR tests (gcc-9, ubuntu, mpich, zoltan)

Build for 05cc300

Compilation - successful

Testing - passed

Build log

PR tests (gcc-7, ubuntu, mpich, trace runtime, LB)

Build for 05cc300

Compilation - successful

Testing - passed

Build log

PR tests (gcc-8, ubuntu, mpich, address sanitizer)

Build for 05cc300

Compilation - successful

Testing - passed

Build log

PR tests (gcc-10, ubuntu, openmpi, no LB)

Build for 05cc300

Compilation - successful

Testing - passed

Build log

PR tests (clang-5.0, ubuntu, mpich)

Build for 05cc300

Compilation - successful

Testing - passed

Build log

PR tests (clang-9, ubuntu, mpich)

Build for 05cc300

Compilation - successful

Testing - passed

Build log

PR tests (nvidia cuda 10.1, ubuntu, mpich)

Build for 05cc300

Compilation - successful

Testing - passed

Build log

PR tests (clang-13, alpine, mpich)

Build for 05cc300

Compilation - successful

Testing - passed

Build log

PR tests (nvidia cuda 11.0, ubuntu, mpich)

Build for 05cc300

Compilation - successful

Testing - passed

Build log

PR tests (clang-12, ubuntu, mpich)

Build for 05cc300

Compilation - successful

Testing - passed

Build log

PR tests (clang-11, ubuntu, mpich)

Build for 05cc300

Compilation - successful

Testing - passed

Build log

PR tests (clang-13, ubuntu, mpich)

Build for 05cc300

Compilation - successful

Testing - passed

Build log

PR tests (clang-14, ubuntu, mpich)

Build for 05cc300

Compilation - successful

Testing - passed

Build log

PR tests (intel icpx, ubuntu, mpich)

Build for 05cc300

Compilation - successful

Testing - passed

Build log

PR tests (gcc-11, ubuntu, mpich)

Build for 05cc300

Compilation - successful

Testing - passed

Build log

PR tests (gcc-12, ubuntu, mpich)

Build for 05cc300

Compilation - successful

Testing - passed

Build log

PR tests (intel icpc, ubuntu, mpich)

Build for 05cc300

Compilation - successful

Testing - passed

Build log

PR tests (clang-10, ubuntu, mpich)

Build for 05cc300

Compilation - successful

Testing - passed

Build log

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

codecov · 2022-06-06T11:38:18Z

Codecov Report

Merging #1842 (05cc300) into develop (a525788) will increase coverage by 0.05%.
The diff coverage is 90.54%.

@@             Coverage Diff             @@
##           develop    #1842      +/-   ##
===========================================
+ Coverage    84.33%   84.39%   +0.05%     
===========================================
  Files          758      760       +2     
  Lines        26715    26776      +61     
===========================================
+ Hits         22531    22598      +67     
+ Misses        4184     4178       -6

Impacted Files	Coverage Δ
src/vt/vrt/collection/balance/model/raw_data.cc	`100.00% <ø> (+28.88%)`	⬆️
src/vt/vrt/collection/balance/model/raw_data.h	`100.00% <ø> (ø)`
.../vrt/collection/balance/model/weighted_messages.cc	`84.21% <84.21%> (ø)`
...t/collection/test_model_weighted_messages.nompi.cc	`91.66% <91.66%> (ø)`
...t/vrt/collection/balance/model/weighted_messages.h	`100.00% <100.00%> (ø)`
src/vt/scheduler/base_unit.cc	`87.50% <0.00%> (-1.39%)`	⬇️
src/vt/utils/mpi_limits/mpi_max_tag.cc	`92.85% <0.00%> (-0.48%)`	⬇️
src/vt/runnable/runnable.cc	`70.73% <0.00%> (-0.36%)`	⬇️
src/vt/pool/pool.cc	`87.50% <0.00%> (-0.13%)`	⬇️
...it/collection/test_model_select_subphases.nompi.cc	`93.44% <0.00%> (-0.11%)`	⬇️
... and 11 more

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

cz4rs · 2022-06-06T11:47:42Z

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

+  // observer pointer to the underlying comm data
+  std::unordered_map<PhaseType, CommMapType> const* proc_comm_;
+
+  TimeType per_msg_weight_  = 0.001;


Up for discussion what the default values should be.

@ppebay Does LBAF use a per-message weight of zero (meaning it only considers bytes, not the number of messages)?

Going with following defaults:

per_msg_weight_ = 0.0; per_byte_weight_ = 1.0;

to make the behavior consistent with current LBAF implementation. This means TemperedWMin's beta parameter is acting as per byte coefficient.

cz4rs · 2022-06-06T12:06:58Z

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

+
+namespace vt { namespace vrt { namespace collection { namespace balance {
+
+struct CommModel : public ComposedModel {


Naming: how should this be called?

Maybe something reflecting that the model is an affine combination of the number of messages and their size?

I went with WeightedMessages.

nlslatt

This looks pretty good. It could be made a little more rigorous by making the different subphases not all have the same load but I'm okay with it as is. I think there are some long lines that should be wrapped.

cz4rs · 2022-06-20T15:33:51Z

This looks pretty good. It could be made a little more rigorous by making the different subphases not all have the same load but I'm okay with it as is. I think there are some long lines that should be wrapped.

Rebased on top of develop + minor formatting fixes (05cc300).

cz4rs commented Jun 6, 2022

View reviewed changes

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

cz4rs commented Jun 6, 2022

View reviewed changes

src/vt/vrt/collection/balance/model/comm_overhead.h Outdated Show resolved Hide resolved

cz4rs commented Jun 6, 2022

View reviewed changes

nmm0 mentioned this pull request Jun 7, 2022

Meeting Agenda [do not close] #925

Open

cz4rs force-pushed the 1835-subphases-handling-modeled-comm branch 4 times, most recently from 9283f7b to b34f45e Compare June 8, 2022 15:27

cz4rs marked this pull request as ready for review June 8, 2022 15:47

cz4rs requested review from JacobDomagala, jstrzebonski, lifflander, nlslatt, nmm0, PhilMiller and stmcgovern June 8, 2022 15:47

cz4rs force-pushed the 1835-subphases-handling-modeled-comm branch from b34f45e to 25d4c28 Compare June 15, 2022 16:13

nlslatt approved these changes Jun 17, 2022

View reviewed changes

cz4rs added 7 commits June 20, 2022 10:32

#1835: split modeled comm proportionally when using subphases

6b82e95

#1835: move modeled comm out of RawData model

8583aa6

#1835: use more sensible weight values

5dfab48

#1835: add unit test for CommModel

18d092e

#1835: tests: make sure to use unique namespace

02fb94b

#1835: lb: rename CommModel

0ce2999

#1835: lb: make default per message weights consistent with LBAF

1dfae1a

cz4rs force-pushed the 1835-subphases-handling-modeled-comm branch from 781c7dc to 6d6be22 Compare June 20, 2022 09:00

#1835: fix formatting

05cc300

cz4rs force-pushed the 1835-subphases-handling-modeled-comm branch from 6d6be22 to 05cc300 Compare June 20, 2022 09:03

nlslatt merged commit 7d7013f into develop Jun 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1835: fix remaining issues in `RawData::getModeledComm` #1842

1835: fix remaining issues in `RawData::getModeledComm` #1842

cz4rs commented Jun 6, 2022 •

edited

Loading

github-actions bot commented Jun 6, 2022 •

edited

Loading

codecov bot commented Jun 6, 2022 •

edited

Loading

cz4rs Jun 6, 2022

nlslatt Jun 13, 2022

cz4rs Jun 15, 2022 •

edited

Loading

cz4rs Jun 6, 2022

nlslatt Jun 13, 2022

cz4rs Jun 15, 2022

nlslatt left a comment

cz4rs commented Jun 20, 2022


		namespace vt { namespace vrt { namespace collection { namespace balance {

		struct CommModel : public ComposedModel {

1835: fix remaining issues in RawData::getModeledComm #1842

1835: fix remaining issues in RawData::getModeledComm #1842

Conversation

cz4rs commented Jun 6, 2022 • edited Loading

github-actions bot commented Jun 6, 2022 • edited Loading

Pipelines results

codecov bot commented Jun 6, 2022 • edited Loading

Codecov Report

cz4rs Jun 6, 2022

Choose a reason for hiding this comment

nlslatt Jun 13, 2022

Choose a reason for hiding this comment

cz4rs Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

cz4rs Jun 6, 2022

Choose a reason for hiding this comment

nlslatt Jun 13, 2022

Choose a reason for hiding this comment

cz4rs Jun 15, 2022

Choose a reason for hiding this comment

nlslatt left a comment

Choose a reason for hiding this comment

cz4rs commented Jun 20, 2022

1835: fix remaining issues in `RawData::getModeledComm` #1842

1835: fix remaining issues in `RawData::getModeledComm` #1842

cz4rs commented Jun 6, 2022 •

edited

Loading

github-actions bot commented Jun 6, 2022 •

edited

Loading

codecov bot commented Jun 6, 2022 •

edited

Loading

cz4rs Jun 15, 2022 •

edited

Loading