search

DARMA-tasking / LB-analysis-framework

Analysis framework for exploring, testing, and comparing load balancing strategies
Other
3 stars 1 forks source link

#323: Refactor distributed information stage #390

Closed ppebay closed 1 year ago

ppebay commented 1 year ago

Resolves #323

cwschilly commented 1 year ago

@ppebay I fixed the PR so it passes the CI tests; is this ready to be taken out of draft?

ppebay commented 1 year ago

Thanks @cwschilly for fixing the CI on this PR

It's not ready yet as I still have to complete the implementation of the distributed information usage for issue #323

But please continue monitoring this issue as subsequent pushes might continue to break the CI.

ppebay commented 1 year ago

@cwschilly your CI-fixing commit is good. Please consider adding those tests removed from lbsRank testing, to lbsInformAndTransferAlgorithm testing (as these methods have been moved there).

cwschilly commented 1 year ago

@ppebay There doesn't seem to be any unit testing done on lbsInformandTransferAlgorithm--does a new test file need to be written?

ppebay commented 1 year ago

@cwschilly handing this over to you, for the remaining Tox/CI testing errors (some updates are needed). Thanks!

ppebay commented 1 year ago

Example with user-defined memory toy problem, same behavior as before refactoring:

Screen Shot 2023-06-11 at 2 30 21 PM

Screen Shot 2023-06-11 at 2 30 36 PM

ppebay commented 1 year ago

Conflicts due to mergers with PR #400 & #402 now resolved.

@cwschilly can you please have a look at the code quality CI test errors in this PR? Thanks

cwschilly commented 1 year ago

Conflicts due to mergers with PR #400 & #402 now resolved.

@cwschilly can you please have a look at the code quality CI test errors in this PR? Thanks

Yes I'll figure these out ASAP

cwschilly commented 1 year ago

@ppebay The only remaining errors come from the deletion of the "__known_loads" attribute from Rank; has this functionality been moved somewhere else, or should I restructure the tests so they avoid calling it altogether?

ppebay commented 1 year ago

@cwschilly __known_loads as attribute of ranks has been entirely removed.

Instead, a close concept of __known_peers was added but specifically to the class of information-and-transfer algorithms (it does not make general sense for all algorithms).

cwschilly commented 1 year ago

@ppebay The code now passes all CI tests apart from commit formatting on "Resolved merge conflicts"

lifflander commented 1 year ago

This looks great to me. We still need to actually simulate the locking. It looks like this mostly just adjusts the peers so that they are based on the known PEs during the information stage.