Skip to content

The parallel add-on partition keeps a base's moved finest level static (#919) - #926

Merged
lmoresi merged 5 commits into
developmentfrom
bugfix/addon-partition-static-mark-919
Nov 3, 2026
Merged

lmoresi merged 5 commits into
developmentfrom
bugfix/addon-partition-static-mark-919

Conversation

@lmoresi

@lmoresi lmoresi commented Nov 2, 2026

Copy link
Copy Markdown
Member

In parallel, partition_addon moved a base's own finest level and marked the moved copy _mg_static = False. Mesh.add_fault then took that background level for an add-on level and replayed the fault onto it (split_tail_levels). On the UWTN 2026-022 fixture the replay failed with a warning and left the level welded. Serial never attempts the replay, so how the hierarchy was built depended on the rank count.

Change

  • partition_addon marks the moved finest static when the mesh it partitions owns no tail, which is what makes that mesh a base. An adapt child owns a tail and stays add-on.
  • The base never moves inside partition_addon, so the first level's recorded parents are already current addresses, and they are now taken as they stand. Without this, a moved base finest that becomes the base of a later partition (the one add_fault(cut=True) makes of the placed mesh) resolved its parents through the record of its own earlier move: 3076 cells outside their parent at np 2.
  • _moved_wrap keeps a moved level's own mark (18e5714). On its own that commit did not fix In parallel, the add-on partition marks the base's finest level as an add-on level, so the fault is replayed onto a background level #919: the base mesh carries no mark (only its wraps do), so the moved copy stayed unmarked, and test_1132 still failed at np 2 and 4. The line stays because it gives the right mark to a moved level that does carry one.
  • fault-split-multigrid.md and the parallel add-on design note say what moves when the base has no add-on levels.

Tests

tests/parallel/test_1132_addon_partition_static_mark_parallel.py (level 2, tier B; np 1 in CI's serial phase, np 2 in the MPI phase) places the 80-rung fixture without a solve. It asserts:

  • no add-on level in the mesh-owned tail;
  • no split tail level;
  • no carry warning;
  • every cell's centroid lies in the parent its record names.

parents_contain_centroids moves from test_1096 into fault_fixtures.py, so there is one copy.

Fail first (kaiju):

  • development and 18e5714: AssertionError [2] at np 2;
  • 7c0c62f: 3076 cells outside their parent at np 2, RuntimeError / SIGSEGV at np 4.

c7e6fa3 on kaiju (Linux, PETSc 3.25.0):

  • test_1132 passes at np 1, 2 and 4, and test_1096 at np 2 and 4.
  • tests/parallel at np 2 and 4, and tier A+B, show no failure beyond the development baseline.
  • The HDF5 error-76 casualties pass on reruns at low load.

The validation details are in #919.

scripts/test.sh gains a line in serial batch 4 at the same place as the patch summary PR and the serial_reference PR, so the second and third to merge need a development merge first.

Closes #919. Left open in this territory: #873, #874, #886, #888 and #898 (verdicts in the review comment).

Underworld development team with AI support from Claude Code

… rank count

In parallel, partition_addon moved the base's own finest level (a base built with
refinement=2 has no add-on levels) and marked the copy _mg_static = False. So
Mesh.add_fault took it for an add-on level, and split_tail_levels tried to cut the
S-sampled trace into it. On the 022 fixture the snap inverted a cell and the level
stayed welded, with a warning in every np > 1 run. Serial never tries, so whether
the replay was attempted depended on the rank count.

The test places the fixture (80 rungs, no solve) and asserts no add-on level in the
mesh-owned tail, no split tail level and no carry warning. At np 1 it passes on
development; at np 2 it fails there.

Underworld development team with AI support from Claude Code
addon_partition._moved_wrap marked every moved level _mg_static = False. A base
with no add-on levels moves its own finest, so at np > 1 that background level was
taken for an add-on level, and Mesh.add_fault replayed the fault onto it
(split_tail_levels). On the 022 fixture the replay failed with a warning and left
the level welded, which serial never attempts. The moved copy now carries the
level's own mark, as custom_mg's moved coarse meshes and the extruded levels
already do. Real add-on levels are non-static and are unchanged.

The split-level multigrid doc says what moves when the base has no add-on levels.

Underworld development team with AI support from Claude Code
18e5714 had the moved copy take the level's own mark. That is a no-op for the
case #919 is about: a base with no add-on levels moves the base mesh itself, and
the base mesh carries no _mg_static (only the wraps of its levels do). The
moved copy stayed unmarked, so at np 2 and 4 on kaiju test_1132 still found
the solver mesh's tail level 2 classified add-on (AssertionError: [2]).

partition_addon now marks the moved finest static when the mesh it partitions
owns no tail, which is what makes it a base. An adapt child owns a tail and
stays add-on; add-on levels keep their own (absent) mark in _moved_wrap.

Underworld development team with AI support from Claude Code
…s not move

The adversarial review of 7c0c62f found that marking the moved base finest
static makes it the BASE of the next partition. On the ribbon that next
partition is the one add_fault(cut=True) makes of the placed mesh.
partition_addon then resolved the first level's parents through
_resolve_parents(base, ...). That function reads base._addon_self_old,
which holds the record of the base's own EARLIER move, so the current
parent addresses went through the wrong directory. At 25d6281 the host was
add-on and moved again with the placed mesh, which kept the relation
consistent.

The base never moves inside partition_addon. The first level's recorded
parents are its current addresses, and they are now taken as they stand.

- test_1132 checks the relation at np > 1: every cell's centroid lies in
  the parent its record names on the tail's last level.
  parents_contain_centroids moves from test_1096 into fault_fixtures.py
  (one copy).
- test_1132 joins the serial phase of scripts/test.sh, as test_1106 does,
  so its np 1 baseline runs in CI.
- The partition_addon comment states the constraint. The split-level and
  partition design docs no longer say that no base level moves.

Serial: test_1132 passes, and test_1096 collects. The np 2 and 4 checks go
to kaiju.

Underworld development team with AI support from Claude Code
Copilot AI balanced review requested due to automatic review settings November 2, 2026 23:14
@lmoresi

lmoresi commented Nov 2, 2026

Copy link
Copy Markdown
Member Author

Pre-PR adversarial review

One round on 7c0c62f (over 25d6281). Each finding was fixed in c7e6fa3, the fix the review proposed, and kaiju confirmed it.

HIGH 1: a moved base finest became the base of the next partition, and _resolve_parents read that level's stale move record.

  • Marking the moved host static made _levels_of return it as base when add_fault(cut=True) partitions the placed mesh.
  • _resolve_parents(base, lev) then mapped the first level's current parent addresses through the host's own earlier move directory (_addon_self_old). On a rank holding more host cells than before, the indices run past the directory.
  • The result was wrong rows in the (host, finest) transfer on the mesh-owned tail, at np > 1 only. At 25d6281 the host was add-on and moved with the placed mesh, so the relation was consistent there.
  • Fixed: for l == 0 the recorded parents are taken as they stand, because the base never moves inside partition_addon. test_1132 asserts parent containment. On kaiju it fails at 7c0c62f (3076 cells outside their parent at np 2; RuntimeError / SIGSEGV at np 4) and passes at c7e6fa3 at np 1, 2 and 4.

LOW 2: the partition_addon comment overclaimed and told history. Fixed: it states the constraint.

LOW 3: docs. fault-split-multigrid.md and parallel-addon-distribution-2026-10.md said no base level moves. Fixed, and the history and the overlong line are gone.

LOW 4: test_1132's np 1 claim had no CI run. Fixed: a serial batch 4 line.

LOW 5: 18e5714's _moved_wrap line is untested and alone fixes nothing. Kept, and the PR body says so. It gives the same mark to every level that can reach it.

Attacks that failed:

  • rank asymmetry in the new predicates;
  • a single-level base;
  • adapt children with and without add-on levels;
  • re-partitioning a moved adapt child;
  • extruded meshes;
  • CI reach at np 2.

Issue verdicts

Underworld development team with AI support from Claude Code

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new test’s tier_b marker is applied at module scope, contrary to the style charter’s requirement for tier markers on individual tests.

1 open issue
What changed in this PR

This PR addresses #919 by keeping a base mesh’s moved finest level classified as static background, preventing rank-dependent fault replay in the multigrid hierarchy.

Changes:

  • Preserve static marks and use current parent addresses when partitioning above an already-moved base.
  • Add serial/MPI regression coverage and share the parent-containment helper.
  • Document which background levels move during partitioning.
File Description
tests/​parallel/​test_1132_addon_partition_static_mark_parallel.py Adds checks for static classification, absent fault replay, and parent containment.
tests/​parallel/​test_1096_addon_partition_parallel.py Reuses the shared parent-containment helper.
tests/​parallel/​fault_fixtures.py Houses the extracted parent-containment helper.
src/​underworld3/​utilities/​addon_partition.py Corrects static marking and first-level parent-address handling.
scripts/​test.sh Includes the new regression in serial batch 4.
docs/​developer/​subsystems/​fault-split-multigrid.md Explains moved background levels and subsequent partition behavior.
docs/​developer/​design/​parallel-addon-distribution-2026-10.md Clarifies the base-finest movement exception.

Review effort: Balanced

Comment on lines +49 to +55
pytestmark = [pytest.mark.level_2, pytest.mark.tier_b]

A, B = np.array([0.30, 0.45]), np.array([0.70, 0.55])
N_RUNGS = 80


def test_a_placed_fault_leaves_the_base_levels_static():
…k-919

One conflict in scripts/test.sh, adjacent batch-4 lines (test_1132 from this
branch, test_1133 from #928); both kept.

Underworld development team with AI support from Claude Code
@lmoresi
lmoresi merged commit e7dfbf9 into development Nov 3, 2026
8 checks passed
lmoresi pushed a commit that referenced this pull request Nov 3, 2026
One conflict in scripts/test.sh, adjacent batch-4 lines (test_1138 from #930,
test_1134 from this branch); both kept.

Underworld development team with AI support from Claude Code
lmoresi added a commit that referenced this pull request Nov 3, 2026
#930)

custom_mg: the split function's docstring keeps #921's text (no cover_max=None)
with #918's 2-D scope.

Underworld development team with AI support from Claude Code
lmoresi added a commit that referenced this pull request Nov 3, 2026
#930)

Underworld development team with AI support from Claude Code
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.

3 participants