#2074: support sparse OfflineLB maps #2145

thearusable · 2023-05-09T16:12:08Z

Closes #2074

~~Depends on #2137~~
~~Depends on #2250~~

github-actions · 2023-05-09T17:03:59Z

Pipelines results

PR tests (gcc-12, ubuntu, mpich)

Build for 30cc7f4 (2024-04-22 15:38:34 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (clang-9, ubuntu, mpich)

Build for f076ec9 (2024-05-24 12:04:00 UTC)

Compilation - successful

Testing - passed

Build log

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

Build for f076ec9 (2024-05-24 12:04:00 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (gcc-9, ubuntu, mpich, zoltan, json schema test)

Build for f076ec9 (2024-05-24 12:04:00 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (clang-12, ubuntu, mpich)

Build for f076ec9 (2024-05-24 12:04:00 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (clang-11, ubuntu, mpich)

Build for f076ec9 (2024-05-24 12:04:00 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (clang-13, ubuntu, mpich)

Build for f076ec9 (2024-05-24 12:04:00 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (clang-14, ubuntu, mpich)

Build for 30cc7f4 (2024-04-22 15:38:34 UTC)

Compilation - successful

Testing - passed

Build log

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

Build for f076ec9 (2024-05-24 12:04:00 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (gcc-11, ubuntu, mpich, trace runtime, coverage)

Build for f076ec9 (2024-05-24 12:04:00 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (clang-10, ubuntu, mpich)

Build for f076ec9 (2024-05-24 12:04:00 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (intel icpc, ubuntu, mpich)

Build for f076ec9 (2024-05-24 12:04:00 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (nvidia cuda 11.2, gcc-9, ubuntu, mpich)

Build for f076ec9 (2024-05-24 12:04:00 UTC)

/vt/src/vt/pipe/pipe_manager.impl.h(133): warning: missing return statement at end of non-void function "vt::pipe::PipeManager::makeSend<f,Target>(Target) [with f=&vt::vrt::collection::lb::GreedyLB::collectHandler, Target=vt::objgroup::proxy::ProxyElm<vt::vrt::collection::lb::GreedyLB>]"
          detected during:
            instantiation of "auto vt::pipe::PipeManager::makeSend<f,Target>(Target) [with f=&vt::vrt::collection::lb::GreedyLB::collectHandler, Target=vt::objgroup::proxy::ProxyElm<vt::vrt::collection::lb::GreedyLB>]" 
/vt/src/vt/objgroup/proxy/proxy_objgroup.impl.h(202): here
            instantiation of "vt::objgroup::proxy::Proxy<ObjT>::PendingSendType vt::objgroup::proxy::Proxy<ObjT>::reduce<f,Op,Target,Args...>(Target, Args &&...) const [with ObjT=vt::vrt::collection::lb::GreedyLB, f=&vt::vrt::collection::lb::GreedyLB::collectHandler, Op=vt::collective::PlusOp, Target=vt::objgroup::proxy::ProxyElm<vt::vrt::collection::lb::GreedyLB>, Args=<vt::vrt::collection::lb::GreedyPayload>]" 
/vt/src/vt/vrt/collection/balance/greedylb/greedylb.cc(222): here

/vt/src/vt/pipe/pipe_manager.impl.h(133): warning: missing return statement at end of non-void function "vt::pipe::PipeManager::makeSend<f,Target>(Target) [with f=&MyObj::handler, Target=vt::objgroup::proxy::ProxyElm<MyObj>]"
          detected during instantiation of "auto vt::pipe::PipeManager::makeSend<f,Target>(Target) [with f=&MyObj::handler, Target=vt::objgroup::proxy::ProxyElm<MyObj>]" 
/vt/examples/callback/callback.cc(147): here

/vt/src/vt/pipe/pipe_manager.impl.h(133): warning: missing return statement at end of non-void function "vt::pipe::PipeManager::makeSend<f,Target>(Target) [with f=&colHan, Target=vt::vrt::collection::VrtElmProxy<MyCol, vt::Index1D>]"
          detected during instantiation of "auto vt::pipe::PipeManager::makeSend<f,Target>(Target) [with f=&colHan, Target=vt::vrt::collection::VrtElmProxy<MyCol, vt::Index1D>]" 
/vt/examples/callback/callback.cc(153): here

/vt/src/vt/pipe/pipe_manager.impl.h(133): warning: missing return statement at end of non-void function "vt::pipe::PipeManager::makeSend<f,Target>(Target) [with f=&MyObj::handler, Target=vt::objgroup::proxy::ProxyElm<MyObj>]"
          detected during instantiation of "auto vt::pipe::PipeManager::makeSend<f,Target>(Target) [with f=&MyObj::handler, Target=vt::objgroup::proxy::ProxyElm<MyObj>]" 
/vt/examples/callback/callback.cc(147): here

/vt/src/vt/pipe/pipe_manager.impl.h(133): warning: missing return statement at end of non-void function "vt::pipe::PipeManager::makeSend<f,Target>(Target) [with f=&colHan, Target=vt::vrt::collection::VrtElmProxy<MyCol, vt::Index1D>]"
          detected during instantiation of "auto vt::pipe::PipeManager::makeSend<f,Target>(Target) [with f=&colHan, Target=vt::vrt::collection::VrtElmProxy<MyCol, vt::Index1D>]" 
/vt/examples/callback/callback.cc(153%0D%0A%0D%0A%0D%0A ==> And there is more. Read log. <==

Build log

PR tests (clang-13, alpine, mpich)

Build for f076ec9 (2024-05-24 12:04:00 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (intel icpx, ubuntu, mpich)

Build for 30cc7f4 (2024-04-22 15:38:34 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (nvidia cuda 12.2.0, gcc-9, ubuntu, mpich)

Build for 30cc7f4 (2024-04-22 15:38:34 UTC)

/vt/lib/CLI/CLI/CLI11.hpp(1029): warning #2361-D: invalid narrowing conversion from "double" to "unsigned long"
          TT { std::declval<CC>() }
               ^
          detected during:
            instantiation of "vt::CLI::detail::is_direct_constructible<T, C>::test [with T=std::vector<std::string, std::allocator<std::string>>, C=double]" based on template arguments <std::vector<std::string, std::allocator<std::string>>, double> at line 1041
            instantiation of class "vt::CLI::detail::is_direct_constructible<T, C> [with T=std::vector<std::string, std::allocator<std::string>>, C=double]" at line 5005
            instantiation of "void vt::CLI::Option::results(T &) const [with T=std::vector<std::string, std::allocator<std::string>>]" at line 5034
            instantiation of "T vt::CLI::Option::as<T>() const [with T=std::vector<std::string, std::allocator<std::string>>]" at line 7315

Remark: The warnings can be suppressed with "-diag-suppress <warning-number>"

/vt/lib/CLI/CLI/CLI11.hpp(1029): warning #2361-D: invalid narrowing conversion from "int" to "unsigned long"
          TT { std::declval<CC>() }
               ^
          detected during:
            instantiation of "vt::CLI::detail::is_direct_constructible<T, C>::test [with T=std::vector<std::string, std::allocator<std::string>>, C=int]" based on template arguments <std::vector<std::string, std::allocator<std::string>>, int> at line 1041
            instantiation of class "vt::CLI::detail::is_direct_constructible<T, C> [with T=std::vector<std::string, std::allocator<std::string>>, C=int]" at line 5005
            instantiation of "void vt::CLI::Option::results(T &) const [with T=std::vector<std::string, std::allocator<std::string>>]" at line 5034
            instantiation of "T vt::CLI::Option::as<T>() const [with T=std::vector<std::string, std::allocator<std::string>>]" at line 7315

Testing - passed

Build log

PR tests (intel icpx, ubuntu, mpich, verbose)

Build for f076ec9 (2024-05-24 12:04:00 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (gcc-12, ubuntu, mpich, verbose)

Build for f076ec9 (2024-05-24 12:04:00 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (clang-14, ubuntu, mpich, verbose)

Build for f076ec9 (2024-05-24 12:04:00 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (nvidia cuda 12.2.0, gcc-9, ubuntu, mpich, verbose)

Build for f076ec9 (2024-05-24 12:04:00 UTC)

/vt/lib/CLI/CLI/CLI11.hpp(1029): warning #2361-D: invalid narrowing conversion from "double" to "unsigned long"
          TT { std::declval<CC>() }
               ^
          detected during:
            instantiation of "vt::CLI::detail::is_direct_constructible<T, C>::test [with T=std::vector<std::string, std::allocator<std::string>>, C=double]" based on template arguments <std::vector<std::string, std::allocator<std::string>>, double> at line 1041
            instantiation of class "vt::CLI::detail::is_direct_constructible<T, C> [with T=std::vector<std::string, std::allocator<std::string>>, C=double]" at line 5005
            instantiation of "void vt::CLI::Option::results(T &) const [with T=std::vector<std::string, std::allocator<std::string>>]" at line 5034
            instantiation of "T vt::CLI::Option::as<T>() const [with T=std::vector<std::string, std::allocator<std::string>>]" at line 7315

Remark: The warnings can be suppressed with "-diag-suppress <warning-number>"

/vt/lib/CLI/CLI/CLI11.hpp(1029): warning #2361-D: invalid narrowing conversion from "int" to "unsigned long"
          TT { std::declval<CC>() }
               ^
          detected during:
            instantiation of "vt::CLI::detail::is_direct_constructible<T, C>::test [with T=std::vector<std::string, std::allocator<std::string>>, C=int]" based on template arguments <std::vector<std::string, std::allocator<std::string>>, int> at line 1041
            instantiation of class "vt::CLI::detail::is_direct_constructible<T, C> [with T=std::vector<std::string, std::allocator<std::string>>, C=int]" at line 5005
            instantiation of "void vt::CLI::Option::results(T &) const [with T=std::vector<std::string, std::allocator<std::string>>]" at line 5034
            instantiation of "T vt::CLI::Option::as<T>() const [with T=std::vector<std::string, std::allocator<std::string>>]" at line 7315

Testing - passed

Build log

tests/unit/lb/test_lb_reader.nompi.cc

tests/unit/lb/test_offlinelb.cc

nlslatt · 2023-05-23T20:44:33Z

src/vt/vrt/collection/balance/offlinelb/offlinelb.cc

 void OfflineLB::runLB(TimeType) {
-  auto const& distro = theLBDataReader()->getDistro(phase_ + 1);
-  for (auto&& elm : distro) {
+  auto nextPhase = theLBDataReader()->findNextPhase(phase_);
+  auto const distro = theLBDataReader()->getDistro(nextPhase);
+  for (auto&& elm : *distro) {
    migrateObjectTo(elm, theContext()->getNode());
  }
-  theLBDataReader()->clearDistro(phase_ + 1);
+  theLBDataReader()->clearDistro(nextPhase);
 }


If phase_ + 1 was skipped, does this new logic grab phase_ + 2 if it exists (whether explicitly described in the file or listed as identical) as a surrogate?

@lifflander Is this the desired behavior or do we want to disallow running OfflineLB when we're missing information about the phase?

Yes exactly, it will grab phase_ + 2 or whatever is the next existing phase.

@nlslatt I changed the behavior to disallow running of OfflineLB when the phase is skipped.

nlslatt · 2023-05-23T20:46:41Z

src/vt/vrt/collection/balance/lb_data_restart_reader.cc

-      if (local_changed_distro[i]) {
+    PhaseType curr = 0, next;
+    for (;curr < num_phases_ - 1;) {
+      next = findNextPhase(curr);


Similar to my other question, do we want to allow OfflineLB to operate over data from non-consecutive phases (where one is explicitly listed as skipped as opposed to just identical)?

@nlslatt I updated that to operate only on consecutive phases.

thearusable · 2023-10-19T14:01:24Z

@nlslatt, @lifflander - I rebased this PR and fixed all conflicts. PR is ready for review.

nlslatt

The testing still needs work.

tests/unit/lb/test_lb_reader.nompi.cc

tests/unit/lb/test_offlinelb.cc

nlslatt · 2023-11-06T18:17:05Z

tests/unit/lb/test_offlinelb.cc

+    "0 OfflineLB\n"
+    "1 NoLB\n"
+    "2 NoLB\n"
+    "3 NoLB\n"
+    "4 OfflineLB\n"
+    "5 OfflineLB\n"
+    "6 NoLB\n";


I think this configuration, coupled with your choice of which phases are identical to previous, means that OfflineLB by definition has nothing to migrate (for example, phase 1 is said to be identical to phase 0, so nothing should migrate when OfflineLB runs on phase 0). As such, this does not appear to be a good test of the sparse functionality.

Also, I think the phases that you have defined as identical to previous contradict what was loaded into the LBDataHolder (e.g., phase 1 is marked as identical to phase 0, but the LBDataHolder content indicates that they are not identical).

Ok, I updated the configuration for this test so the OfflineLB will be able to do some migrations. Also I removed unnecessary phases from LBDataHolder.
If you have more suggestions, please let me know.

src/vt/vrt/collection/balance/lb_data_restart_reader.h

…ig file

…for OfflineLB

nlslatt

Most of it looks great, but I think there's an issue with some of the tests.

tests/unit/runtime/test_initialization.cc

nlslatt

Your recent changes look good. One last thing to address, I think. It segfaults when OfflineLB is requested but the user forgets to also use the command-line argument --vt_lb_data_in. We need to give a usable error message instead.

…meters

thearusable · 2024-05-24T12:22:00Z

Your recent changes look good. One last thing to address, I think. It segfaults when OfflineLB is requested but the user forgets to also use the command-line argument --vt_lb_data_in. We need to give a usable error message instead.

Thanks for catching that issue! Looks like it also happens on the develop.

Instead of showing the error message I changed the code to allow for more relaxed OfflineLB related configuration.
With latest changes we will check if we need LBDataRestartReader to be created when we supply one or more of those parameters:
--vt_lb_data_in, --vt_lb_name, --vt_lb_file_name.
Then we will check if the --vt_lb_file_name exists and if it requires the OfflineLB to be run.

bool data_in = arg_config_->config_.vt_lb_data_in;
bool requested_offline_lb = arg_config_->config_.vt_lb_name == get_lb_names()[LBType::OfflineLB];
bool has_file = arg_config_->config_.vt_lb_file_name != "";

if (data_in || requested_offline_lb || has_file) {
  auto& config_file = arg_config_->config_.vt_lb_file_name;
  if (config_file != "") {
    bool const has_spec = ReadLBConfig::openConfig(config_file);
    if (has_spec) {
      return ReadLBConfig::hasOfflineLB();
    }
  }
  return true;
}

Please let me know if that is ok to you or do you prefer to show a message that vt is not configured properly to run OfflineLB.

nlslatt · 2024-05-29T18:02:00Z

Your recent changes look good. One last thing to address, I think. It segfaults when OfflineLB is requested but the user forgets to also use the command-line argument --vt_lb_data_in. We need to give a usable error message instead.

Thanks for catching that issue! Looks like it also happens on the develop.

Instead of showing the error message I changed the code to allow for more relaxed OfflineLB related configuration. With latest changes we will check if we need LBDataRestartReader to be created when we supply one or more of those parameters: --vt_lb_data_in, --vt_lb_name, --vt_lb_file_name. Then we will check if the --vt_lb_file_name exists and if it requires the OfflineLB to be run.

[snip]

Please let me know if that is ok to you or do you prefer to show a message that vt is not configured properly to run OfflineLB.

@lifflander and I discussed it, and we prefer just showing an error message when OfflineLB is requested but --vt_lb_data_in is not specified. Automatically enabling --vt_lb_data_in has undesirable consequences.

thearusable force-pushed the 2074-support-sparse-offlinelb-maps branch from 744a987 to 09b0807 Compare May 9, 2023 16:26

nmm0 mentioned this pull request May 9, 2023

Meeting Agenda [do not close] #925

Open

thearusable force-pushed the 2074-support-sparse-offlinelb-maps branch 2 times, most recently from 3fa3511 to 5b75e31 Compare May 16, 2023 16:33

thearusable self-assigned this May 16, 2023

thearusable force-pushed the 2074-support-sparse-offlinelb-maps branch from 5b75e31 to f9c5f29 Compare May 22, 2023 10:07

thearusable requested review from PhilMiller, stmcgovern, lifflander, cz4rs, nmm0, JacobDomagala and nlslatt May 22, 2023 16:39

thearusable marked this pull request as ready for review May 22, 2023 16:39

nlslatt reviewed May 23, 2023

View reviewed changes

thearusable force-pushed the 2074-support-sparse-offlinelb-maps branch from 0d2eb73 to 044d7c4 Compare June 5, 2023 11:13

thearusable force-pushed the 2074-support-sparse-offlinelb-maps branch 2 times, most recently from ef91bad to 8b20790 Compare June 12, 2023 14:56

thearusable requested a review from nlslatt June 13, 2023 11:56

thearusable force-pushed the 2074-support-sparse-offlinelb-maps branch from 56592af to c9120ba Compare June 20, 2023 17:08

thearusable force-pushed the 2074-support-sparse-offlinelb-maps branch from c9120ba to 3f0a613 Compare October 19, 2023 14:00

thearusable force-pushed the 2074-support-sparse-offlinelb-maps branch from 3f0a613 to b3e07a4 Compare October 31, 2023 14:50

nlslatt requested changes Nov 6, 2023

View reviewed changes

thearusable marked this pull request as draft November 29, 2023 13:54

thearusable force-pushed the 2074-support-sparse-offlinelb-maps branch 2 times, most recently from 8a72a9f to 77911e8 Compare November 30, 2023 18:05

thearusable requested a review from nlslatt November 30, 2023 18:17

thearusable marked this pull request as ready for review November 30, 2023 18:17

thearusable added 22 commits May 21, 2024 15:26

#2074: Add test for sparse OfflineLB maps

0652d7b

#2074: Allow for sparse maps in LBDataRestartReader

bed80c8

#2074: Update finding next phase for OfflineLB

9ac5b79

#2074: Create LBDataRestartReader when there is OfflineLB in the conf…

5922ee6

…ig file

#2074: Avoid adding data for identical phases to history

5fa24bc

#2074: Add tests for detecting OfflineLB in LB data file

e95bf7b

#2074: Add checks in sparse handler

6296a19

#2074: Disallow runing of OfflineLB for skipped phases.

0088212

#2074: Disallow operating on non-consecutive phases

9de8be2

#2074: Update implementation after resolving conflicts

202d567

#2074: Use getUniqueFilename for file name in offlinelb tests

fb0e597

#2074: Update data selection for offlineLB sparse test

2e00e63

#2074: Calculate number of phases after removal of the count_ property

3bbe392

#2074: Fix logic for exsiting offlineLB test

eb5e4ad

#2074: Add case to the offlinelb test which will result in assert

41562aa

#2074: Update sparseHandler in offlineLB tests

f4145ff

#2074: Remove failing part of the test

1d32b63

#2074: Check the phase if needs OfflineLB even if requested in config

a70ebb2

#2074: Fix crash when using param: --vt_lb_name=OfflineLB

6b4c2ab

#2074: Add unit tests to cover different initialization combinations …

b4f86cc

…for OfflineLB

#2074: Enable new tests only when Load Balancing is enabled

c01fc90

#2074: Add flag for gcc-8 to compile std::filesystem

8eedc00

thearusable force-pushed the 2074-support-sparse-offlinelb-maps branch from dac04e0 to 8eedc00 Compare May 21, 2024 14:01

nlslatt requested changes May 21, 2024

View reviewed changes

tests/unit/runtime/test_initialization.cc Outdated Show resolved Hide resolved

#2074: Fix creation of the test files

1c1ee3f

nlslatt requested changes May 23, 2024

View reviewed changes

thearusable added 2 commits May 24, 2024 12:57

#2074: Update unit tests to cover more combinations of OfflineLB para…

b6df3d2

…meters

#2074: Allow to create LBDataRestartReader when lb_data_in is not used

f076ec9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2074: support sparse OfflineLB maps #2145

#2074: support sparse OfflineLB maps #2145

thearusable commented May 9, 2023 •

edited

github-actions bot commented May 9, 2023 •

edited

nlslatt May 23, 2023

thearusable Jun 5, 2023

thearusable Jun 12, 2023

nlslatt May 23, 2023

thearusable Jun 13, 2023

thearusable commented Oct 19, 2023

nlslatt left a comment

nlslatt Nov 6, 2023

thearusable Nov 30, 2023

nlslatt left a comment

nlslatt left a comment

thearusable commented May 24, 2024

nlslatt commented May 29, 2024 •

edited

#2074: support sparse OfflineLB maps #2145

Are you sure you want to change the base?

#2074: support sparse OfflineLB maps #2145

Conversation

thearusable commented May 9, 2023 • edited

github-actions bot commented May 9, 2023 • edited

Pipelines results

nlslatt May 23, 2023

Choose a reason for hiding this comment

thearusable Jun 5, 2023

Choose a reason for hiding this comment

thearusable Jun 12, 2023

Choose a reason for hiding this comment

nlslatt May 23, 2023

Choose a reason for hiding this comment

thearusable Jun 13, 2023

Choose a reason for hiding this comment

thearusable commented Oct 19, 2023

nlslatt left a comment

Choose a reason for hiding this comment

nlslatt Nov 6, 2023

Choose a reason for hiding this comment

thearusable Nov 30, 2023

Choose a reason for hiding this comment

nlslatt left a comment

Choose a reason for hiding this comment

nlslatt left a comment

Choose a reason for hiding this comment

thearusable commented May 24, 2024

nlslatt commented May 29, 2024 • edited

thearusable commented May 9, 2023 •

edited

github-actions bot commented May 9, 2023 •

edited

nlslatt commented May 29, 2024 •

edited