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

mpi: Patch neighborhood construction #1768

Merged
merged 1 commit into from
Oct 5, 2021
Merged

Conversation

FabioLuporini
Copy link
Contributor

@FabioLuporini FabioLuporini commented Oct 5, 2021

From the MPI specs (for example at this link):

MPI_ERR_ARG
Invalid argument. Some argument is invalid and is not identified by a specific error class (e.g., MPI_ERR_RANK).

In master, we're catching the MPI_ERR_ARG as if that were an indication that the MPI rank is at the boundary. This is the case for many many MPI distributions and configurations, but (unsurprisingly) not for all. I have a server with several GPUs and the NVidia MPI configured as per our Dockerfile.nvidia where Get_cart_rank does not trigger MPI_ERR_RANK for MPI ranks at the boundary -- rather, it provides the MPI rank of the process at the opposite side of the virtual topology.

With this patch, we explicitly use MPI.PROC_NULL, when necessary, for the neighborhood of the MPI ranks at the grid boundary.

PS: I don't really know how to add a test here. But this edit simply makes sure we honor the MPI specification as we should have done in the first place

@codecov
Copy link

codecov bot commented Oct 5, 2021

Codecov Report

Merging #1768 (136901c) into master (2ce93a5) will decrease coverage by 1.63%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1768      +/-   ##
==========================================
- Coverage   89.43%   87.80%   -1.64%     
==========================================
  Files         208      203       -5     
  Lines       33937    33389     -548     
  Branches     4407     4375      -32     
==========================================
- Hits        30352    29316    -1036     
- Misses       3087     3568     +481     
- Partials      498      505       +7     
Impacted Files Coverage Δ
devito/mpi/distributed.py 92.11% <100.00%> (-0.03%) ⬇️
tests/test_gpu_openacc.py 5.80% <0.00%> (-92.26%) ⬇️
devito/ir/iet/efunc.py 40.62% <0.00%> (-50.00%) ⬇️
devito/passes/iet/orchestration.py 18.07% <0.00%> (-41.81%) ⬇️
examples/seismic/elastic/elastic_example.py 47.22% <0.00%> (-30.56%) ⬇️
...mples/seismic/viscoelastic/viscoelastic_example.py 47.22% <0.00%> (-30.56%) ⬇️
...les/seismic/viscoacoustic/viscoacoustic_example.py 45.00% <0.00%> (-25.00%) ⬇️
devito/passes/clusters/asynchrony.py 12.34% <0.00%> (-22.64%) ⬇️
examples/seismic/tti/tti_example.py 35.13% <0.00%> (-21.63%) ⬇️
devito/passes/iet/languages/openacc.py 72.82% <0.00%> (-20.66%) ⬇️
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ce93a5...136901c. Read the comment docs.

@FabioLuporini FabioLuporini merged commit a7d3b38 into master Oct 5, 2021
@FabioLuporini FabioLuporini deleted the patch-mpi-neighborhood branch October 5, 2021 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants