-
Notifications
You must be signed in to change notification settings - Fork 8
Global ops reduction #988
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
Global ops reduction #988
Conversation
…al, remove requirements, dynamically query test requirements, clean up test venv creation
bump to 0.4.1
Enable deploy workflow
fix wheel download for publishing on pypi
…ong", but we want "longlong", i.e. all spaces removed.
Add support for bool, int32, int64 in python bindings
Bump submodules
…osition.py Co-authored-by: Mikael Simberg <[email protected]>
…osition.py Co-authored-by: Mikael Simberg <[email protected]>
c1cab19 to
73f02e0
Compare
msimberg
left a comment
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.
Two minor requests, sorry for not spotting them earlier. Let's get this in after that.
model/common/src/icon4py/model/common/decomposition/mpi_decomposition.py
Outdated
Show resolved
Hide resolved
| if self._calc_buffer_size(buffer, array_ns) == 0: | ||
| raise ValueError("global_min requires a non-empty buffer") |
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.
Commenting for future reference: if we wanted to avoid this we could supply the neutral element as an initial value.
No change needed now, this is fine.
model/common/tests/common/decomposition/mpi_tests/test_mpi_decomposition.py
Outdated
Show resolved
Hide resolved
e374bcf to
73f02e0
Compare
|
cscs-ci run default |
msimberg
left a comment
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, looks good to me. Let's hope the CI comes back clean afterwards (alps is down now, that's why the latest run failed).
I'll try to remove the torus xfails in #692.
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.
Pull request overview
This PR implements global reduction operations (min, max, sum, and mean) for MPI environments to enable proper distributed computation across multiple processes. The main focus is adding MPI-aware global reductions that perform both local and global (cross-process) reduction operations using mpi4py.
Changes:
- Added
Reductionsprotocol and implementations (SingleNodeReductionsandGlobalReductions) for min/max/sum/mean operations - Updated
compute_nflat_gradpto use global min reduction for proper MPI support - Added
exchangeparameter tocompute_coeff_gradekinfor halo exchange - Updated serialized test data URLs for multiple experiments to support 2 and 4 process configurations
- Changed
do_exchangetoTruefor inverse field computations in grid geometry
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| model/common/src/icon4py/model/common/decomposition/definitions.py | Adds Reductions protocol and SingleNodeReductions implementation, plus factory function |
| model/common/src/icon4py/model/common/decomposition/mpi_decomposition.py | Implements GlobalReductions class with MPI-aware reduction operations |
| model/common/src/icon4py/model/common/metrics/metric_fields.py | Updates compute_nflat_gradp to use configurable min reduction |
| model/common/src/icon4py/model/common/metrics/metrics_factory.py | Passes global reductions to metric field computations |
| model/common/src/icon4py/model/common/metrics/compute_coeff_gradekin.py | Adds exchange parameter for halo synchronization |
| model/common/src/icon4py/model/common/grid/icon.py | Adds mean_reduction parameter to GlobalGridParams.from_fields |
| model/common/src/icon4py/model/common/grid/grid_manager.py | Integrates global reductions into grid manager |
| model/common/src/icon4py/model/common/grid/geometry.py | Enables halo exchange for inverse field computations |
| model/standalone_driver/src/icon4py/model/standalone_driver/standalone_driver.py | Creates and passes global reductions to grid manager |
| model/standalone_driver/src/icon4py/model/standalone_driver/driver_utils.py | Updates signature to accept global_reductions parameter |
| model/testing/src/icon4py/model/testing/definitions.py | Updates serialized data URLs for multi-process test configurations |
| model/testing/src/icon4py/model/testing/fixtures/datatest.py | Adds skip for GAUSS3D experiment with MPI |
| model/common/tests/common/decomposition/mpi_tests/test_mpi_decomposition.py | Adds comprehensive tests for all global reduction operations |
| model/common/tests/common/metrics/mpi_tests/test_parallel_metrics.py | Adds test for distributed nflat_gradp computation |
| model/common/tests/common/fixtures.py | Integrates global reductions into test fixtures |
| model/common/tests/common/metrics/unit_tests/test_compute_coeff_gradekin.py | Updates test to pass dummy exchange function |
| model/common/tests/common/grid/mpi_tests/test_parallel_geometry.py | Updates imports for test definitions |
Comments suppressed due to low confidence (2)
model/common/src/icon4py/model/common/metrics/compute_coeff_gradekin.py:32
- The docstring is outdated. It mentions
horizontal_endparameter which doesn't exist in the function signature. The new parametersexchangeandarray_nsare not documented.
"""
Compute coefficients for improved calculation of kinetic energy gradient
Args:
edge_cell_length: edge_cell_length
inv_dual_edge_length: inverse of dual_edge_length
horizontal_start: horizontal start index
horizontal_end: horizontal end index
"""
model/common/src/icon4py/model/common/metrics/metric_fields.py:607
- The docstring is incomplete. It should document the new parameters
min_reductionandarray_ns, as well as all existing parameters (flat_idx_max,e_owner_mask,lateral_boundary_level,nlev) and the return value.
"""
compute the nflat_gradp value as the minimum value of the flat_idx_max array.
"""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
model/common/src/icon4py/model/common/decomposition/mpi_decomposition.py
Outdated
Show resolved
Hide resolved
model/common/src/icon4py/model/common/decomposition/mpi_decomposition.py
Outdated
Show resolved
Hide resolved
model/common/src/icon4py/model/common/decomposition/mpi_decomposition.py
Show resolved
Hide resolved
model/common/tests/common/grid/mpi_tests/test_parallel_geometry.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
…osition.py Co-authored-by: Copilot <[email protected]>
…osition.py Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
…osition.py Co-authored-by: Copilot <[email protected]>
…y.py Co-authored-by: Copilot <[email protected]>
jcanton
left a comment
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.
LGTM
|
Mandatory Tests Please make sure you run these tests via comment before you merge!
Optional Tests To run benchmarks you can use:
To run tests and benchmarks with the DaCe backend you can use:
To run test levels ignored by the default test suite (mostly simple datatest for static fields computations) you can use:
For more detailed information please look at CI in the EXCLAIM universe. |
|
cscs-ci run default |
Implementation of global
maxandsumfor MPI operations in the same fashion asmin:Addition of
global_minreduction fornflat_gradp:Note: mpi4py.MPI does not have a
meanoperator