Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

#367: make output JSON files compatible with vt offline LB #374

Merged
merged 61 commits into from
May 11, 2023
Merged

#367: make output JSON files compatible with vt offline LB #374

merged 61 commits into from
May 11, 2023

Conversation

ppebay
Copy link
Contributor

@ppebay ppebay commented Apr 15, 2023

Resolves #367

@ppebay ppebay changed the title #367: make outputJSON files compatible with vt offline LB #367: make output JSON files compatible with vt offline LB Apr 15, 2023
@ppebay
Copy link
Contributor Author

ppebay commented Apr 17, 2023

Documenting that the execution logic of the reworked VT JSON writer is now working. Left: visualization of the results of LBAF (8 iterations of InformAndTransfer algorithm); right: visualization of a single phase of the PhaseStepper applied to the JSON output of the former:
Screen Shot 2023-04-17 at 7 58 59 AM

At this point the communications are not written to the JSON but the object mappings are correctly written and can be re-read by the reader.

@ppebay ppebay requested review from nlslatt and lifflander April 17, 2023 12:11
ppebay added 18 commits April 17, 2023 16:48
@ppebay
#367: further refactoring to prepare for multiphase reading
8e7be91
@ppebay
#367: modified execution flow to allow for multiphase loading
4c73b3e
@ppebay
#367: simplified Phase API by removing unnecessary debug info
a4f8ce5
@ppebay
#367: improved internals and erroring out of reader
bada871
@ppebay
#367: further compactification of writer
c143464
@ppebay
#367: more renaming with consistent variable names
b432f3e
@ppebay
#367: ensure phase length consistency
20a1687
@ppebay
#367: removed debug statements
66dac4a
@ppebay
#367: updated reader communications parsing for multi-phase support
1b3adc5
@ppebay
#367: completed JSON reader refactoring
0c95322
@ppebay
#367: whitespace cleanup
18e0c2a
@ppebay
#367: attempting different acceptance conf parameters
0ce9ee8
@ppebay
#367: updated test data with respect to new requirement for empty ranks
240eeab
@ppebay
#367: renamed "stats" into "data" for consistency
6fbbcb3
@ppebay
#367: overhaul of test harness and data to adapt to code changes
381d16f
@ppebay
#367: whitespace cleanup
f9d8990
@ppebay
#367: protected against empty task lists in a JSON phase
25bfd96
@ppebay
#367: all tests should now pass
aa39d8f
@ppebay
Copy link
Contributor Author

ppebay commented Apr 23, 2023

  • Validating that this is now also working when there are shared blocks (following bug fix above); first the balancing process (4 iterations of InformAndTransfer algorithm with Clustering strategy:
user-defined-memory-toy-problem.mp4
  • Subsequently we can see the rendering of the 2 phases of the JSON files produced by the above, when visualized with the PhaseStepper:

test_file_00
test_file_01

@ppebay ppebay marked this pull request as ready for review April 24, 2023 20:48
@ppebay ppebay requested a review from pierrepebay April 24, 2023 20:48
@ppebay
Copy link
Contributor Author

ppebay commented Apr 24, 2023

This PR is now ready for review; it has actually turned into somewhat of a refactoring, due to the fact that the initial design only considered in-place rebalancing, for the sake of execution speed and memory efficiency.

In order to achieve the desired goals of this issue, the underlying model for the lbsPhase had to be entirely revisited, and as a result the lbsRuntime and algorithm classes as well. There was therefore much more to the story than just changing the VT JSON writer, and for this reason reviewers will have much more work than anticipated to review this PR ;) !

@ppebay ppebay self-assigned this Apr 25, 2023
@nmm0 nmm0 mentioned this pull request Apr 25, 2023
Open
nlslatt
nlslatt reviewed Apr 28, 2023
View reviewed changes
Copy link
Contributor

@nlslatt nlslatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will anything in this PR cause a failure for cases where objects can be created or deleted on any phase?

This looks like great progress, but it still does not seem to read in everything in the json that pertains to task attributes or metadata and retain it for output.

src/lbaf/IO/lbsVTDataWriter.py Show resolved Hide resolved
src/lbaf/IO/lbsVTDataWriter.py Show resolved Hide resolved
src/lbaf/IO/lbsVTDataReader.py Show resolved Hide resolved
@ppebay
Copy link
Contributor Author

ppebay commented May 4, 2023

Hello dear reviewers @lifflander @nlslatt @pierrepebay this is a reminder :)

@ppebay ppebay requested a review from pierrepebay May 10, 2023 19:55
@ppebay ppebay mentioned this pull request May 11, 2023
Closed
@pierrepebay
Copy link
Contributor

In rank_object_enumerator.py, the LoadReader is not initialized with correct arguments and calls the read function which doesn't exist anymore:
https://github.com/DARMA-tasking/LB-analysis-framework/pull/374/files#file-src-lbaf-applications-rank_object_enumerator-py-L30
https://github.com/DARMA-tasking/LB-analysis-framework/pull/374/files#file-src-lbaf-applications-rank_object_enumerator-py-L34
It appears only the compute_min_max_arrangements_work function from this file is used elsewhere, so I'm not sure if this is very relevant.

@ppebay
Copy link
Contributor Author

ppebay commented May 11, 2023

@pierrepebay creating a follow-on issue to fix or deprecate the rank_object_enumerator.py script.

We can't just remove the one call to the function right now, because it itself calls other functions of that same script. Ideally we would move all of that to a helper class somewhere.

pierrepebay
pierrepebay approved these changes May 11, 2023
View reviewed changes
Copy link
Contributor

@pierrepebay pierrepebay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok; other than that I have tested thoroughly the different configurations and gone over the changed features, everything looks good to me.

@ppebay ppebay merged commit fc286b6 into develop May 11, 2023
@ppebay ppebay deleted the 367-Make-output-JSON-files-compatible-with-vt-OfflineLB branch May 11, 2023 20:01
github-actions bot pushed a commit that referenced this pull request May 11, 2023
@ppebay @pierrepebay
#367: make output JSON files compatible with vt offline LB (#374)
d9128d4 
* #367: first bit of refactoring

* #367: internal consistency between writers

* #367: improved internal consistency between the 2 types of writers

* #367: cleaner use of data parallelism

* #367: refactored usage of JSON writer from LBAF

* #367: fixed annoying logical error that happened with empty task lists

* #367: slight reorg of initializations

* #367: whitespace cleanup

* #367: further refactoring to prepare for multiphase reading

* #367: modified execution flow to allow for multiphase loading

* #367: simplified Phase API by removing unnecessary debug info

* #367: improved internals and erroring out of reader

* #367: further compactification of writer

* #367: more renaming with consistent variable names

* #367: ensure phase length consistency

* #367: removed debug statements

* #367: updated reader communications parsing for multi-phase support

* #367: completed JSON reader refactoring

* #367: whitespace cleanup

* #367: attempting different acceptance conf parameters

* #367: updated test data with respect to new requirement for empty ranks

* #367: renamed stats into data for consistency

* #367: overhaul of test harness and data to adapt to code changes

* #367: whitespace cleanup

* #367: protected against empty task lists in a JSON phase

* #367: all tests should now pass

* #367: updated Phases test

* #367: raise proper exception for failed work model

* #367: fixed missing tasks key which for now is required even for empty lists

* #367: code style: consistency

* #367: better handling of statistics in LBAF

* #367: added JSON writer parameters discussed with JL and NS

* #367: whitespace cleanup

* #367: now passing parameters to writer and optionally compressing

* #367: modified LBAF for multi-phase writing

* #367: progress checkpoint of refactoring of execution model for multiphase processing

* #167: factored out initialization steps

* #367: multiphase JSON writer now functional (multi-threaded)

* #367: restore Visualizer functionality with new data model

* #367: updated PhaseStepper algorithm to new data model

* #367: processed --> rebalanced for clarity

* #367: whitespace cleanup

* #367: fixed typo

* #367: upated CI for data model changes

* #367: removed debug statements

* #367: completed refactoring for multiphase

* #367: fixed CI

* #367: improved test

* #367: better failing tests parameters

* #367: fixed rank deep copy bug of shared blocks and testing it now

* #367: removed debug statements

* #367: modified indexing of processed phase; also improved API

* #367: fixed typo

* #367: completed implementation with application of load changes

* #367: removed debug statement

* #367: fixed post-merge errors

* #367: fixed an other post-merge error

* #367: missing trimmed down data set

* #367: fix private variable name

* #367: addressed review comment

---------

Co-authored-by: Pierre Pébay <[email protected]>
@ppebay ppebay mentioned this pull request May 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nlslatt nlslatt nlslatt left review comments

@pierrepebay pierrepebay pierrepebay approved these changes

@lifflander lifflander Awaiting requested review from lifflander

Assignees

@ppebay ppebay

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make output JSON files compatible with vt's OfflineLB
3 participants
@ppebay @pierrepebay @nlslatt