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.

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

Speedup thicket composition by performing profile unions in binary tree order #170

Merged
merged 6 commits into from
Jun 26, 2024

Conversation

michaelmckinsey1
Copy link
Collaborator

@michaelmckinsey1 michaelmckinsey1 commented Jun 11, 2024

This PR extends off #169. Limiting the total size of the ensemble during unification is more impactful than the performance gains from parallelism, and this solution is less complicated than using multiprocessing.

Parallel Sorting LBANN
Profiles 15734 16
Input Nodes (min, max) 5, 18 6266, 6889
Input Dataframe Size (min, max) '' ''
Output Nodes 154 7106
Output DF 2423036 113696
Dataset Thicket Version Time Unions old_to_new size (min, max)
Parallel Sorting Develop >120m 15733 157459
#170 12m '' 16, 185
#185 33m '' 157459
LBANN Develop 17m 15 106897
#170 7m '' 12824, 13821
#185 3m '' 106897

Number of unions is not affected by the different unifying strategy in this PR. This is because:

# PR170 - 3 unions
tk1 U tk2 = tk12
tk3 U tk4 = tk34
tk12 U tk34 = tk1234

is the same as

# develop - 3 unions
tk1 U tk2 = tk12
tk12 U tk3 = tk123
tk123 U tk4 = tk1234

This PR increases performance by keeping the old_to_new mapping dictionary small as we call concat_thickets n-1 times instead of one time (as in the develop branch). The old_to_new dictionary requires an operation to update the dictionary mappings per each union, which can become expensive if the dictionary is too large. The old_to_new dictionary is necessary to replace the node objects in the performance data with the new node objects in the graph that results from union.

@michaelmckinsey1 michaelmckinsey1 force-pushed the feature-speedup_reader branch from 00e02ba to e4d2ebc Compare June 12, 2024 23:08
@michaelmckinsey1 michaelmckinsey1 changed the title Speedup Reader by Splitting Data into Chunks Speedup Reader by Splitting Ensemble into Sub-lists Jun 13, 2024
@michaelmckinsey1 michaelmckinsey1 force-pushed the feature-speedup_reader branch from e4d2ebc to f57450e Compare June 13, 2024 17:31
@michaelmckinsey1 michaelmckinsey1 marked this pull request as ready for review June 13, 2024 22:28
@michaelmckinsey1 michaelmckinsey1 self-assigned this Jun 13, 2024
@michaelmckinsey1 michaelmckinsey1 added area-thicket Issues and PRs involving Thicket's core Thicket datastructure and associated classes priority-normal Normal priority issues and PRs status-ready-for-review This PR is ready to be reviewed by assigned reviewers type-feature Requests for new features or PRs which implement new features labels Jun 13, 2024
@michaelmckinsey1 michaelmckinsey1 force-pushed the feature-speedup_reader branch from f57450e to fe69690 Compare June 14, 2024 22:54
@pearce8 pearce8 changed the title Speedup Reader by Splitting Ensemble into Sub-lists Speedup thicket composition by performing profile unions in binary tree order Jun 14, 2024
Copy link
Collaborator

@ilumsden ilumsden left a comment

Choose a reason for hiding this comment

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

The code is good, but something has to be done to make the tree-based concatenation clearer. At bare minimum, this needs to be well commented, otherwise no one is going to have a clue what this is doing.


return thicket_object
# n - 1 edges in a binary tree
pbar = tqdm.tqdm(range(len(ens_list) - 1), disable=disable_tqdm)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using tqdm here makes this process very hard to understand. I'd suggest not using tqdm and/or extensively documenting this with comments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tqdm is really useful to see what is happening for longer reads.

I changed it to use a while loop, which should be clearer with the popping and appending that is happening in the loop. And I added very detailed comments.

@michaelmckinsey1 michaelmckinsey1 added status-revisions-needed Revisions have been requested from a reviewer for this PR and removed status-ready-for-review This PR is ready to be reviewed by assigned reviewers labels Jun 20, 2024
@michaelmckinsey1 michaelmckinsey1 force-pushed the feature-speedup_reader branch from fe69690 to bce8513 Compare June 20, 2024 23:03
@michaelmckinsey1 michaelmckinsey1 force-pushed the feature-speedup_reader branch from bce8513 to bb1b19d Compare June 24, 2024 20:57
@michaelmckinsey1 michaelmckinsey1 force-pushed the feature-speedup_reader branch from 6fd50d7 to a0afa2d Compare June 24, 2024 22:11
@michaelmckinsey1 michaelmckinsey1 added status-ready-for-review This PR is ready to be reviewed by assigned reviewers and removed status-revisions-needed Revisions have been requested from a reviewer for this PR labels Jun 26, 2024
Copy link
Collaborator

@ilumsden ilumsden left a comment

Choose a reason for hiding this comment

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

LGTM. The stuff you're doing with tqdm also makes much more sense now that you've added the comments and switched to a while loop

@ilumsden
Copy link
Collaborator

@pearce8 this PR is also ready for your review

@ilumsden ilumsden added status-approved No more revisions are required on this PR and it is ready for merge and removed status-ready-for-review This PR is ready to be reviewed by assigned reviewers labels Jun 26, 2024
@pearce8 pearce8 merged commit ace73f2 into LLNL:develop Jun 26, 2024
4 checks passed
michaelmckinsey1 added a commit to michaelmckinsey1/thicket that referenced this pull request Jul 9, 2024
…ee order (LLNL#170)

* Speedup reader by splitting into chunks

* Switch to binary tree order composition

* Make code clearer with while loop and comments

* Clarity

* Update comment

* Update comment

---------

Co-authored-by: Michael Richard Mckinsey <mckinsey@quartz1154.llnl.gov>
michaelmckinsey1 added a commit to michaelmckinsey1/thicket that referenced this pull request Jul 9, 2024
…ee order (LLNL#170)

* Speedup reader by splitting into chunks

* Switch to binary tree order composition

* Make code clearer with while loop and comments

* Clarity

* Update comment

* Update comment

---------

Co-authored-by: Michael Richard Mckinsey <mckinsey@quartz1154.llnl.gov>
Yejashi pushed a commit to TauferLab/thicket that referenced this pull request Sep 4, 2024
…ee order (LLNL#170)

* Speedup reader by splitting into chunks

* Switch to binary tree order composition

* Make code clearer with while loop and comments

* Clarity

* Update comment

* Update comment

---------

Co-authored-by: Michael Richard Mckinsey <mckinsey@quartz1154.llnl.gov>
@slabasan slabasan added this to the 2024.2.0 milestone Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-thicket Issues and PRs involving Thicket's core Thicket datastructure and associated classes priority-normal Normal priority issues and PRs status-approved No more revisions are required on this PR and it is ready for merge type-feature Requests for new features or PRs which implement new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants