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: Packed gathers and scatters #2109

Merged
merged 9 commits into from
May 2, 2023
Merged

mpi: Packed gathers and scatters #2109

merged 9 commits into from
May 2, 2023

Conversation

FabioLuporini
Copy link
Contributor

No description provided.

@FabioLuporini FabioLuporini added the MPI mpi-related label Apr 15, 2023
@@ -534,6 +534,67 @@ def distance_mapper(self):
retval[d] = j
return retval

@cached_property
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all these properties here are simply lifted from what is now a subclass of Relation, namely Dependence , so this is not new code

@@ -24,7 +24,20 @@ RUN apt-get install -y libgl1-mesa-glx
##############################################################
FROM base as gcc

RUN apt-get install -y mpich libmpich-dev
RUN mkdir -p /deps && mkdir -p /opt/openmpi && cd /deps && \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mloubout for homogeneity, switching to OpenMPI here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind switching but this is the expensive route, for standard CPU apt install libopenmpi-dev should be enough (gotta double check name)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but last I checked you would get openmpi 3, this way you get 4 (but I may be wrong)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, it's fine

mkdir -p lets you do any path so just mkdir -p /opt/openmpi

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is mkdir -p /opt/openmpi already... ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missread deps for openmpi my bad

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Apr 15, 2023

Codecov Report

Merging #2109 (cebf18f) into master (e6f25d5) will increase coverage by 24.70%.
The diff coverage is 94.59%.

❗ Current head cebf18f differs from pull request most recent head c9d23df. Consider uploading reports for the commit c9d23df to get more accurate results

@@             Coverage Diff             @@
##           master    #2109       +/-   ##
===========================================
+ Coverage   63.06%   87.76%   +24.70%     
===========================================
  Files         142      221       +79     
  Lines       21990    39115    +17125     
  Branches     3996     5087     +1091     
===========================================
+ Hits        13867    34331    +20464     
+ Misses       7409     4221     -3188     
+ Partials      714      563      -151     
Impacted Files Coverage Δ
tests/test_mpi.py 99.06% <91.66%> (ø)
devito/mpi/routines.py 93.69% <91.83%> (+67.20%) ⬆️
devito/ir/support/basic.py 93.15% <95.23%> (+7.61%) ⬆️
devito/ir/iet/algorithms.py 83.87% <100.00%> (+0.53%) ⬆️
devito/ir/iet/nodes.py 92.30% <100.00%> (+12.20%) ⬆️
devito/mpi/halo_scheme.py 94.11% <100.00%> (+19.64%) ⬆️
devito/passes/clusters/blocking.py 91.23% <100.00%> (+35.50%) ⬆️
devito/passes/iet/mpi.py 96.20% <100.00%> (+19.76%) ⬆️
devito/tools/dtypes_lowering.py 85.95% <100.00%> (+16.13%) ⬆️
devito/types/array.py 91.61% <100.00%> (+32.59%) ⬆️
... and 1 more

... and 180 files with indirect coverage changes

if a0 is a1:
continue

r = Relation(a0, a1)
Copy link
Contributor

@mloubout mloubout Apr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is Relation commutative? product is quite brutal (might be ok if small set) so if it's commutative you are better of with two for loop (i, a0) in enumerate(v), a1 in v[i:-1]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not commutative because the distance would flip (positive -> negative or negative -> positive)

"""
Create a new HaloScheme that contains all entries in `self` plus the one
passed in input. If `f` already exists in `self`, the old value is
overridden.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so why is it overridden?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's classic dict behaviour:

a[2] = 3
a[2] = 4

Now a[2] contains 4

IOW, it does not behave as dict.setdefault

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant what is the reason for the new value to have priority.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just the semantics I've established for this method. It seemed more convenient at the caller sites

devito/ir/support/basic.py Show resolved Hide resolved
@@ -78,21 +114,18 @@ def dtype_to_ctype(dtype):

def dtype_to_mpitype(dtype):
"""Map numpy types to MPI datatypes."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NumPy?

Copy link
Contributor

@mloubout mloubout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments but looks good

@@ -67,6 +68,11 @@ def make(self, hs):
# Sanity check
assert all(f.is_Function and f.grid is not None for f in hs.fmapper)

# Pack-up Functions into Bundles. Worst case scenario, we have Bundles
# with just one components each. This is to maximize the likelihood of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one component

if swap is False:
eq = Eq(buf[dims], f[f_indices])
swap = lambda i, j: (i, j)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For readability, ok!

Comment on lines +36 to +38
padded_count = count
if count == 3:
padded_count = 4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be better documented/explained? Not sure I get it

@mloubout mloubout merged commit bcc6b4b into master May 2, 2023
@mloubout mloubout deleted the mpi-moar branch May 2, 2023 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MPI mpi-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants