Skip to content

Avoid replicated 3D boundary-flux recovery - #593

Merged
lmoresi merged 1 commit into
underworldcode:developmentfrom
gthyagi:bugfix/boundary-flux-memory-scaling
Aug 18, 2026
Merged

Avoid replicated 3D boundary-flux recovery#593
lmoresi merged 1 commit into
underworldcode:developmentfrom
gthyagi:bugfix/boundary-flux-memory-scaling

Conversation

@gthyagi

@gthyagi gthyagi commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix the MPI memory scaling of 3D boundary-flux recovery without changing its reaction, trace-mass, gauge-removal, or local-return mathematics.

Problem

The existing 3D path uses MPI.allgather for nodal reactions and P1/P2 trace elements. Every rank therefore retains the complete global boundary mesh, assembles the same sparse surface-mass matrix, and runs the same consistent-mass solve. Memory grows with both global boundary size and rank count.

This became the limiting post-processing stage for high-resolution spherical rotated-free-slip models: the Stokes solve completed, but consistent P2 normal-traction recovery exhausted aggregate job memory.

Change

  • gather reactions, trace elements, and each rank requested coordinate keys to rank zero;
  • assemble and solve the global 3D boundary-mass system once;
  • scatter only the recovered values requested by each rank, preserving local xs order;
  • preallocate sparse COO arrays instead of appending Python scalar entries;
  • broadcast root-side errors before scatter so all MPI ranks fail collectively;
  • retain the existing 2D path unchanged.

The behavior of mass={"auto","lumped","consistent","p1"}, partial-reaction sum/overwrite semantics, mean removal, and P2 midpoint interpolation is unchanged.

Validation

  • tests/test_1019_boundary_flux.py -k 3d: 3 passed.
  • Four-rank tests/parallel/test_1065_boundary_flux_parallel.py -k 3d: 2 passed on every rank.
  • Previous production validation of this exact commit completed the Zhong cellsize=1/64, 192-rank post-processing path in 7:29 with 301.77 GB peak memory; the replicated path had exhausted 576 GB.

Scope

This PR contains only the general boundary_flux gather/solve/scatter correction and its rotated_bc documentation update. Zhong geoid mathematics and benchmark-facing APIs remain isolated in #591.

Gather coordinate-keyed 3D reactions and trace elements on rank zero, assemble and solve the global P1/P2 boundary-mass system once, and scatter only each rank's requested recovered values.

Preserve existing recovery semantics while preallocating sparse COO arrays and propagating root-side failures collectively. This reduces the Zhong 1/64 geoid postprocessing peak from more than 576 GB to 301.77 GB on 192 ranks without changing the 1/32 coefficients.
@lmoresi

lmoresi commented Aug 18, 2026

Copy link
Copy Markdown
Member

Adversarial review — spherical-shell cluster (#591, #592, #593, #594)

Reviewed as a group because they overlap: #594 contains #591, and all four land
in the spherical/postprocessing path. Findings below are labelled by the PR they
apply to; the cluster-level ones come first because a single-PR reading misses
them.

Cluster

C1. #594 is stacked on #591 — merging #594 merges #591. pr591 is an
ancestor of pr594, and postprocessing/geoid.py and
postprocessing/__init__.py are byte-identical between them (git diff pr591 pr594 -- ... is empty). #594 is #591 plus two commits (95b3470, fb9ce95).
Neither PR body says so. Merge #591 first and #594 becomes a two-commit
follow-up; merge #594 first and #591 closes as already-merged. What must not
happen is reviewing the geoid code twice as though two implementations were on
offer.

C2. tests/test_1029_ is used twice after the merge. #550 landed
tests/test_1029_analytic_faulted_medium.py yesterday; #594 adds
tests/test_1029_analytic_zhong2008.py. Both survive the merge — we resolved
the tree and they are both there. Two different subjects under one number
defeats the numbering. Zhong should move to 1030.

C3. Not a defect, recorded because the diff view says otherwise. GitHub
shows #594 REMOVING FaultedMedium from analytic/__init__.py — the import,
the __all__ entry and the _SOLUTIONS entry. That is the diff against the
merge base, not against development. We resolved the merge (git merge-tree --write-tree origin/development pr594): it is clean, and the resulting
__init__.py carries FaultedMedium and Zhong2008 side by side, with
barr_houseman_96.py intact. No rebase is needed on that account.

#594 — Zhong 2008

1. Zhong2008 and Zhong2008Response are in __all__ but not in
_SOLUTIONS.
So uw.analytic.available() does not list them, describe()
does not know them, and neither the conformance suite nor
convention_audit.py measures them. That follows from what they are — neither
subclasses AnalyticSolution, and there is no sample_points, boundaries,
apply_boundary_conditions or set_fields — so the registry would reject them
anyway. It is the right call and it is invisible: everything else reachable as
uw.analytic.X is a solution carrying the validation contract, and these two are
not. Worth a sentence in the module docstring saying they are a response
calculator rather than a solution, so the next reader does not go looking for the
momentum-residual row.

#593 — boundary-flux recovery

2. The memory bound moves, it does not fall. Replacing comm.allgather with
gather-to-rank-zero removes the ×nranks replication, which is what #408 asks
for. Rank zero still holds the entire global boundary and solves the entire
sparse system alone, so the worst-case single-rank memory is unchanged and the
solve is now serial where it used to be redundantly parallel. On the mesh sizes
that motivated #408 that is a clear win; at the scale where rank zero itself is
the constraint it is not, and the docstring's "avoids replicating the global P2
surface mesh and sparse solve on every rank" is true without saying which of the
two limits now binds.

3. Every rank must reach the gather. The recovery is collective by
construction now. A rank owning no part of the boundary has nothing to
contribute, and it still has to enter gather and the scatter that follows it.
We have not run this at a rank count that starves a rank of boundary facets, and
the tests in the PR do not construct one. This is the same shape as the defect
fixed in #596, and it is worth a parallel test with a boundary that does not
reach every rank.

#592 — internal spherical shell

4. It edits test_0502_boundary_integrals.py, which is on the collection
ratchet.
That file does module-level work at import and is exempted by name in
tests/conftest.py (#587). Editing it is fine; adding more module-level work to
it is not, because the exemption hides it. Worth checking the added lines land
inside test functions.

5. discretisation_mesh.py is touched by #596 as well. The regions are far
apart — #592 near line 100, #596 near 8500 — so they merge textually. No action;
recorded so the collision is not discovered at merge time.

What we did not do

None of the four was run. The findings above come from reading the diffs and
resolving the merges mechanically; C1, C2 and C3 are verified against the git
objects, and 1 to 5 are read from the source. Where a claim needed a measurement
we have said so rather than asserting it.

Underworld development team with AI support from Claude Code

@lmoresi

lmoresi commented Aug 18, 2026

Copy link
Copy Markdown
Member

Merging. Two findings from the cluster review are carried forward rather than resolved here:

@lmoresi
lmoresi merged commit f432bb3 into underworldcode:development Aug 18, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants