-
Notifications
You must be signed in to change notification settings - Fork 119
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
Make mpi_helper
's method non-rooted if root=None
#1010
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Baharis
added
xfel
housekeeping
Things which would make cctbx tidier - and easier to understand for regular Python programmers
labels
Sep 3, 2024
phyy-nx
approved these changes
Sep 3, 2024
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, the mm24 change would have used up too much memory I think, but the other changes look great!
This reverts commit b4edf43.
Baharis
force-pushed
the
xfel_mpi_helper_all_methods
branch
from
September 5, 2024 22:42
de9efd6
to
3fd0d62
Compare
cschlick
pushed a commit
that referenced
this pull request
Sep 12, 2024
* Auto-select `mpi_helper`'s collectives using new context manager * Remove unused `from dials.array_family import flex` statement * Change misleading (?) name in `gather_variable_length_numpy_arrays` * Simplify code following changes in `beam_statistics.py` * Simplify code following changes in `error_modifier_mm24.py` * Simplify code following changes in `unit_cell_statistics.py` * Bugfix multiprocessing reporting in `global_filter.py` * Extend `adaptive_collective` `contextmanager`'s docstring * Revert "Simplify code following changes in `error_modifier_mm24.py`" This reverts commit b4edf43. * Bugfix: non-rooted collective should be never passed `root` kwarg * define `mpiCommEmulator.Allgatherv` * Remove walrus operator for python2.7 compliance
cschlick
pushed a commit
that referenced
this pull request
Nov 15, 2024
* Auto-select `mpi_helper`'s collectives using new context manager * Remove unused `from dials.array_family import flex` statement * Change misleading (?) name in `gather_variable_length_numpy_arrays` * Simplify code following changes in `beam_statistics.py` * Simplify code following changes in `error_modifier_mm24.py` * Simplify code following changes in `unit_cell_statistics.py` * Bugfix multiprocessing reporting in `global_filter.py` * Extend `adaptive_collective` `contextmanager`'s docstring * Revert "Simplify code following changes in `error_modifier_mm24.py`" This reverts commit b4edf43. * Bugfix: non-rooted collective should be never passed `root` kwarg * define `mpiCommEmulator.Allgatherv` * Remove walrus operator for python2.7 compliance
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
housekeeping
Things which would make cctbx tidier - and easier to understand for regular Python programmers
xfel
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A common pattern in multiprocessing is to collect data on the root (typically rank 0), process it there, and broadcast it back to other ranks. However, the better practice is typically to use a non-rooted operation: pass the inputs between all ranks and perform calculations independently on each. This removes the need for a second communication (broadcast) and improves code readability.
Non-rooted approach is inferior only in these rare cases where the cost of the switching to non-rooted operation is much larger than the cost of the secondary broadcast. Non-rooted approach might potentially also cause issues with memory consumption if the input or temporary objects (created when analyzing input) are very volumous.
In xfel, communicating between ranks can be done using the
mpi_helper
class, whose methods alias more complex operations to improve readability and remove repetition. Currently all of thempi_helper
methods are rooted, forcing one to copy or rewrite them whenever an non-rooted collective is more desired.This patch slightly modifies all
mpi_helper
methods, allowing them to use non-rooted mpi4py collectives. Whenever their keyword argumentroot
(default0
) is set toNone
instead of an integer, the methods will use a non-rooted collective instead of the collective variant. This will produce identical output on all ranks, removing need for some follow-up broadcasts and allowing for cleaner and potentially faster code.Following the update, I have also modified three (cctbx_project) instances where
mpi_helper
methods were used and could be changed to utilize non-rooted collective variants to potentially speed up the execution. Each was done in a separate commit to revert if change is not desired. The change will also positively affect code in other repositories utilizing thempi_helper
.