mpi: Prevent parallel halo exchanges - #2972
Conversation
Sorry, something went wrong.
| # the first nest triggering all necessary halo exchanges along `f` | ||
| mapper = as_mapper(found, lambda c1: c1.ispace) | ||
| for k, v in mapper.items(): | ||
| mapper = as_mapper(found, lambda c1: (c1.ispace, c1.properties)) |
There was a problem hiding this comment.
How does c1.properties end up being used here?
Sorry, something went wrong.
There was a problem hiding this comment.
that's the actual bug fix
if you don't use the HaloTouch's properties, where the d loop in correctly tagged as sequential, but rather reuse c's properties via rebuild (main's behaviour) then d may erroneously be tagged as parallell depending on its status in c
Sorry, something went wrong.
There was a problem hiding this comment.
Oh, so the rebuild was actually changing the properties of the innermost dimension of the cluster?
Sorry, something went wrong.
There was a problem hiding this comment.
not quite. We were using the rebuild to create a new Cluster -- a Cluster whose sole purpose was carrying a halo_scheme. But using c's rebuild cause us inheriting the wrong properties
Sorry, something went wrong.
| for v in mapper.values(): | ||
| hs = HaloScheme.union([c1.halo_scheme for c1 in v]) | ||
| processed.append(c.rebuild(exprs=[], ispace=k, halo_scheme=hs)) | ||
| processed.append(v[0].rebuild(exprs=[], halo_scheme=hs)) |
There was a problem hiding this comment.
Isn't this changing the type being appended to processed from Cluster to IterationSpace?
Sorry, something went wrong.
There was a problem hiding this comment.
no, v[0] is any c1 within v, that is, a Cluster
if what you said were true, many many tests would fail badly
Sorry, something went wrong.
There was a problem hiding this comment.
Ah ok, this is from me misunderstanding as_mapper
Sorry, something went wrong.
| halo_update = tloop.nodes[0].body[0].body[0].body[0] | ||
| assert isinstance(halo_update, HaloUpdateList) | ||
|
|
||
| @pytest.mark.parallel(mode=4) |
There was a problem hiding this comment.
The incorrect code still gets generated with 1 rank of MPI fwiw. It also won't hang CI in this configuration if it does go wrong, and will instead fail on the structure tests
Sorry, something went wrong.
| g = Function(name="g", grid=grid, space_order=8, dimensions=(x, y, d), shape=(n, n, 2)) | ||
| h = g.func(name='h') | ||
|
|
||
| eqns = [Eq(a, 0.0), |
There was a problem hiding this comment.
Some of these equations can be dropped or simplified iirc. I had something along the lines of:
eqns = [Eq(h, 1), Inc(a, h.dx + h.dy)]which also allows for the Functions to be tidied up
Sorry, something went wrong.
There was a problem hiding this comment.
yes, I know, but I don't think it hurts to leave it a little bit more convoluted, in this particular case and based on what I see while I was debugging it
Sorry, something went wrong.
| assert iters[2].dim is d and iters[2].is_Parallel | ||
| assert iters[3].dim is d and iters[3].is_Sequential | ||
|
|
||
| # Probably unnecessary, but ensures there's no hanging |
There was a problem hiding this comment.
Hanging CI is annoying at best and I agree the .apply() is overkill
Sorry, something went wrong.
There was a problem hiding this comment.
it won't hang anyway since we run it under a timeout of 300 secs -- not sign of life in 5 mins => death
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 9.52381% with 19 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## main #2972 +/- ##
==========================================
- Coverage 83.55% 83.53% -0.03%
==========================================
Files 257 257
Lines 53558 53576 +18
Branches 4585 4585
==========================================
+ Hits 44751 44755 +4
- Misses 8013 8028 +15
+ Partials 794 793 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
No description provided.