Skip to content

Comments

Fix pickling with MPI full mode#1291

Merged
FabioLuporini merged 12 commits intomasterfrom
fix-mpiregion-pickling
May 19, 2020
Merged

Fix pickling with MPI full mode#1291
FabioLuporini merged 12 commits intomasterfrom
fix-mpiregion-pickling

Conversation

@FabioLuporini
Copy link
Contributor

fixes #1286

@codecov
Copy link

codecov bot commented May 15, 2020

Codecov Report

Merging #1291 into master will increase coverage by 0.02%.
The diff coverage is 98.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1291      +/-   ##
==========================================
+ Coverage   86.62%   86.64%   +0.02%     
==========================================
  Files         182      182              
  Lines       26042    26106      +64     
  Branches     3587     3592       +5     
==========================================
+ Hits        22559    22620      +61     
- Misses       3058     3061       +3     
  Partials      425      425              
Impacted Files Coverage Δ
devito/core/gpu_openacc.py 44.23% <50.00%> (-0.36%) ⬇️
devito/mpi/routines.py 95.12% <100.00%> (+0.04%) ⬆️
devito/passes/iet/openmp.py 96.16% <100.00%> (ø)
devito/symbolics/extended_sympy.py 92.34% <100.00%> (+0.88%) ⬆️
devito/symbolics/printer.py 69.86% <100.00%> (-2.36%) ⬇️
tests/test_pickle.py 99.40% <100.00%> (+0.07%) ⬆️
devito/ir/support/basic.py 91.52% <0.00%> (-0.22%) ⬇️

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 88c7170...a6ae587. Read the comment docs.

Copy link
Contributor

@tjb900 tjb900 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for getting to it so quickly.

@@ -1058,6 +1058,7 @@ def __init__(self, name, key, arguments, owned):
name = "%s%d" % (name, key)
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually... isn't this going to make it different when it's unpickled? Since the name property is no longer the name passed to init?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oohhh right nice catch! I even wrote an MFE for MPIMsg but not for MPIRegion.

should be a trivial fix, will do on Monday

@FabioLuporini FabioLuporini force-pushed the fix-mpiregion-pickling branch from 59d15d3 to 88128a8 Compare May 18, 2020 07:00
@FabioLuporini
Copy link
Contributor Author

@tjb900 should be fine now?

@FabioLuporini
Copy link
Contributor Author

jeez... this was a rabbit hole: sympy/sympy#4297

causing pickling to break with MPI_fullmode + OpenMP

I just pushed a workaround

obj = Function.__new__(cls, name, *arguments)
obj._name = name
obj._arguments = arguments
return obj
Copy link
Contributor

Choose a reason for hiding this comment

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

That's...... :D But reading through the issue no really other way around it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I ditched 1 hour on this this morning than I gave up as it's just not worth it

Copy link
Contributor

@tjb900 tjb900 left a comment

Choose a reason for hiding this comment

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

Wow, rabbit hole indeed! LGTM, thanks.

@FabioLuporini FabioLuporini merged commit 126a28e into master May 19, 2020
@FabioLuporini FabioLuporini deleted the fix-mpiregion-pickling branch May 19, 2020 10:11
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.

MPIRegion pickle_args inconsistent with constructor

3 participants