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

Make mpi_helper's method non-rooted if root=None #1010

Merged
merged 12 commits into from
Sep 6, 2024

Conversation

Baharis
Copy link
Contributor

@Baharis Baharis commented Sep 3, 2024

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 the mpi_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 argument root (default 0) is set to None 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 the mpi_helper.

@Baharis Baharis added xfel housekeeping Things which would make cctbx tidier - and easier to understand for regular Python programmers labels Sep 3, 2024
@Baharis Baharis self-assigned this Sep 3, 2024
Copy link
Contributor

@phyy-nx phyy-nx left a 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!

@Baharis Baharis merged commit d8e5e81 into master Sep 6, 2024
98 checks passed
@Baharis Baharis deleted the xfel_mpi_helper_all_methods branch September 6, 2024 05:02
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants