-
Notifications
You must be signed in to change notification settings - Fork 191
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
Fixes for nodegroups running on multiple nodes #6285
Fixes for nodegroups running on multiple nodes #6285
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Squash directly if you decide to change anything
auto my_proxy = Parallel::get_parallel_component<ParallelComponent>(cache); | ||
auto my_proxy = Parallel::get_parallel_component<ParallelComponent>( | ||
cache)[Parallel::my_node<size_t>(cache)]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for my understanding, this used to do a broadcast to the whole collection for every element on this node? But that didn't matter because you only ran on a single node? And now you only send the messages to the local node?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, and the issue was that it would try to run an element that wasn't on its node and would then fail with "I can't find this element!"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation!
d1f1f13
to
d756f61
Compare
I squashed the move of |
auto my_proxy = Parallel::get_parallel_component<ParallelComponent>(cache); | ||
auto my_proxy = Parallel::get_parallel_component<ParallelComponent>( | ||
cache)[Parallel::my_node<size_t>(cache)]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation!
Proposed changes
There's still some issue I need to resolve, but these were the easy ones to find.
Upgrade instructions
Code review checklist
make doc
to generate the documentation locally intoBUILD_DIR/docs/html
.Then open
index.html
.code review guide.
bugfix
ornew feature
if appropriate.Further comments