Skip to content

Swarm.migrate: shrink the working set instead of reclassifying every round - #609

Merged
lmoresi merged 2 commits into
developmentfrom
bugfix/shrinking-working-set
Aug 20, 2026
Merged

lmoresi merged 2 commits into
developmentfrom
bugfix/shrinking-working-set

Conversation

@lmoresi

@lmoresi lmoresi commented Aug 19, 2026

Copy link
Copy Markdown
Member

Addresses the dominant term in #551.

What was costing the time

Swarm.migrate's round loop re-offered the whole local array to
points_in_domain on every round, so the classification work per rank grew with
the round count. Measured through global_evaluate on a fixed 47k-point global
set (2-D simplex, cellSize=1/100, identical total work at every rank count):

np global_evaluate points_in_domain points offered the 50-cell walk
1 0.208 s 0.058 s (28%) 46 901 0.061 s (29%)
2 0.326 s 0.159 s (49%) 141 516 0.108 s (33%)
4 0.443 s 0.259 s (58%) 237 826 0.162 s (37%)

The walk holds a roughly constant share. What grows is the classification, from
28% to 58% of the call, because a fixed 47k-point set is offered to it 238k
times at np=4.

The change

A point this rank has found to be in its domain stays in its domain — neither
the coordinates nor the mesh change during migration — so it does not need
retesting. The loop remembers what it has claimed and classifies only the rest.

Keyed by coordinate, not by index. dm.migrate does not preserve the local
ordering: measured, retained points are not left at the front, so an index from
the previous round names a different particle after the move. Two particles at
the same coordinate share the answer, so a key collision is harmless. Exact
bit-pattern comparison through a void row view — 15 ms against
points_in_domain's 58 ms at 47k, so it pays whenever more than about a quarter
of the local array is already claimed, which it is from the second round on.

Measured after

np global_evaluate points offered to points_in_domain
1 0.208 -> 0.213 s 46 901 -> 46 901
2 0.326 -> 0.232 s 141 516 -> 47 558
4 0.443 -> 0.257 s 237 826 -> 50 025

42% off at np=4, and the growth with rank count is gone — the classification now
sees roughly the local set once whatever np is. np=1 is unchanged within noise
(one extra membership pass, no rounds to save). The walk shrinks with it, since
it is called from inside the classification.

This is the larger of the two faults named in #551's "amplifier" section. The
other — no cheap rejection inside the walk — attacks the smaller, flatter term
and is untouched here.

The collective

The points_in_domain call is unconditional, and that is load-bearing.
It is collective — it reaches get_max_radius() before any short-circuit
precisely so a rank with nothing to classify still joins the reduction (the #405
treatment, stated in its own source). Guarding it on whether this rank has
undecided points deadlocks at np=4, with every rank inside the call, as soon as
one rank runs out of them.

Verified

  • Behaviour-preserving by measurement, not by inference. The per-rank
    partition fingerprint after migration — the sorted coordinates each rank owns,
    hashed — is byte-identical to development at np=2 and np=4.
  • Full ./uw test: 1556 passed, 32 skipped, 2 xfailed, matching development.
  • Parallel swarm/migration/evaluate tests at np=2: 29 passed, 6 skipped.
  • test_0776_migrate_working_set_mpi.py covers the loop: the global particle
    count is conserved, every point lands on a rank whose mesh contains it, and the
    global multiset of coordinates is unchanged. It carries a premise test that the
    fixture actually migrates anything — populate alone leaves every particle
    already owned, and the loop never runs.
  • Its own negative control: reintroducing the conditional collective makes
    test_0776 time out at np=4 (rc=241); restoring the fix makes it pass. The
    test catches the defect it was written for.

Underworld development team with AI support from Claude Code

…round

The round loop re-offered the whole local array to points_in_domain on every
round, so the classification work per rank grew with the round count. Measured
on a fixed 47k-point global set through global_evaluate, points offered to
points_in_domain: 47k at np=1, 142k at np=2, 238k at np=4.

A point this rank has found to be in its domain stays in its domain — neither
the coordinates nor the mesh change during migration — so it does not need
retesting. The loop now remembers what it has claimed and classifies only the
rest.

The claimed set is keyed by COORDINATE rather than by index. dm.migrate does
not preserve the local ordering: measured, retained points are NOT left at the
front, so an index from the previous round names a different particle after the
move. Two particles sharing a coordinate share the answer, so a key collision
is harmless. Exact bit-pattern comparison, vectorised through a void row view
(15 ms against points_in_domain's 58 ms at 47k, so it pays whenever more than
about a quarter of the local array is already claimed).

Measured after, same probe:

    np   global_evaluate      points offered to points_in_domain
    1    0.208 -> 0.213 s     46 901 -> 46 901
    2    0.326 -> 0.232 s     141 516 -> 47 558
    4    0.443 -> 0.257 s     237 826 -> 50 025

42% off global_evaluate at np=4, and the growth with rank count is gone: the
classification now sees roughly the local set once whatever np is. The
nearest-centroid walk shrinks with it, since it is called from inside the
classification.

The call to points_in_domain is UNCONDITIONAL. It is collective — it reaches
get_max_radius() before any short-circuit precisely so a rank with nothing to
classify still joins the reduction (#405) — and an earlier draft of this
skipped it when a rank had no undecided points, which deadlocked at np=4 with
every rank inside the call.

Verified behaviour-preserving rather than merely green: the per-rank partition
fingerprint after migration is byte-identical to development at np=2 and np=4.
Full ./uw test 1556 passed, matching development.

test_0776 covers the loop, with a premise test that the fixture actually
migrates anything, and its own negative control: reintroducing the conditional
collective makes it time out at np=4 (rc=241), and the fix makes it pass.

Underworld development team with AI support from Claude Code
Copilot AI lite review requested due to automatic review settings August 19, 2026 05:23

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@lmoresi

lmoresi commented Aug 19, 2026

Copy link
Copy Markdown
Member Author

Adversarial review

Reviewed at 7811686. Four findings.

1. The claimed set grows without bound within a call, and is rebuilt on every
call.
claimed_keys is a NumPy array concatenated once per round, so a swarm
of N local points costs O(N) memory per migrate and O(rounds x N) copying to
build it. At the sizes measured here that is invisible against the classification
it removes, but migrate is called every timestep in an advecting model and the
concatenation is not amortised across calls. A pre-allocated buffer, or keeping
the set between calls keyed on a population generation, would remove it; neither
is done here because neither is needed at the sizes we measured, which is a
statement about our benchmark rather than about a production run.

2. np.isin on a void view is O(n log n) by sort, and it is now on the hot
path.
Measured 15 ms against 58 ms for the classification at 47k, so it pays —
but the margin is a factor of four, not an order of magnitude, and it narrows as
the already-claimed fraction falls. A run where almost every point moves every
round (a violently advecting swarm, or a first migrate after a large mesh
deformation) pays the membership cost for very little saving. It never costs
more than the classification it replaces, so the worst case is a wash rather
than a regression, but the win is workload-dependent in a way the numbers above
do not show.

3. The equivalence evidence covers np=2 and np=4 on one mesh. The partition
fingerprint is byte-identical, which is the right instrument, and it was taken on
a single 2-D simplex box with one particle distribution. It says the change is
behaviour-preserving there; it does not cover 3-D, deformed or adapted meshes,
split-node fault swarms with coincident coordinates, or np=8. The coincident-
coordinate case is the one worth naming: two particles at the identical position
on opposite faces of a fault produce the same key, and the argument that this is
harmless — they share the containment answer — is sound but untested.

4. The premise test constrains the fixture, not the property.
test_premise_the_fixture_actually_migrates asserts some point crosses ranks,
which is what makes the other two tests meaningful. It does not assert that the
round loop runs more than once, and the saving only exists from the second round
onward. A fixture that migrated everything in a single round would pass all three
tests while exercising none of the bookkeeping this PR adds.

Checked and clean. The points_in_domain call is unconditional, and the
negative control confirms it must be: reintroducing the conditional deadlocks at
np=4 (rc=241) and the fix clears it. Every rank reaches the reduction on every
round regardless of how much work it has.

Underworld development team with AI support from Claude Code

@lmoresi

lmoresi commented Aug 19, 2026

Copy link
Copy Markdown
Member Author

The np=4 parallel-test hang is pre-existing, not from this PR

Reporting this because an earlier run made it look otherwise.

Running tests/parallel/ -k "swarm or migrat or evaluate" at np=4 hangs on this
branch. It hangs identically on development at 8b7c8b9, so it is not caused by
the change here. Bisected to
test_0760_swarm_cache_migration.py::test_global_evaluate_after_migration, which
passes at np=2 and hangs at np=4 on the trunk; filed as #611, along with the
reason nobody has seen it — that file matches neither of the two globs
scripts/test.sh runs under mpirun.

One earlier np=4 result on this branch should be discarded rather than trusted:
it was running in the background while the negative control rebuilt the
environment underneath it (broken build, then restored), so it was executing
against site-packages being swapped mid-run. That is why it is not quoted in the
description.

What stands for this PR: full ./uw test 1556 passed matching development; the
np=2 parallel selection 29 passed, 6 skipped; byte-identical partition
fingerprints at np=2 and np=4; and test_0776 passing at np=2 and np=4 with a
working negative control.

tests/parallel/test_0776_linear_rbf_proxy_parallel.py already exists, so the
new file collided with it — the same defect as #600, where two subjects ended
up sharing 1029. Caught while auditing which parallel files scripts/test.sh
actually runs.

Underworld development team with AI support from Claude Code
@lmoresi
lmoresi merged commit 85bd9c9 into development Aug 20, 2026
2 checks passed
lmoresi added a commit that referenced this pull request Sep 4, 2026
)

* Run the whole parallel directory, and run it at four ranks as well as two

Two scripts, two different holes. scripts/test.sh (what CI runs) used
tests/parallel/test_075*py and test_10*py; scripts/test_levels.sh (what
./uw test runs) used tests/parallel/test_07*py. Of the 32 collectible files in
that directory, test.sh named 14 and missed test_0005, test_0700,
test_0760..test_0790, test_0855 and test_0873; test_levels.sh missed
test_0005, test_0855, test_0873 and the whole test_10* solver set.

Three files therefore ran at NO rank count in either script: test_0005,
test_0855, and test_0873 — the last added two days ago in #596 to guard
against a parallel hang.

Both now name the directory. A glob that names ranges grows holes as files are
added between them, which is the #570 class and is how #611 survived
unnoticed.

Both scripts also run the set at four ranks. Two ranks is a special case: the
defect this suite exists to catch is a collective entered by some ranks and
not others, and with two the mismatched pair often still meets. Every instance
found recently passed at np=2 and hung at np=4 — the conditional collective in
#609, and #611 itself. test_levels.sh already had --full-parallel for this and
was pointing it at the narrower glob.

Measured on this directory, machine otherwise idle:

    np=2   128 passed, 11 skipped     156 s
    np=4   135 passed,  3 skipped     170 s   (1 deselected)

end to end, scripts/test.sh --p 2: 135 passed, 3 skipped, 1 deselected, 198 s
for the parallel section, rc=0. The skip counts differ because some tests
require four ranks and skip at two, which is a second reason to run both.

The single deselection is #611: test_global_evaluate_after_migration passes at
np=2 and hangs at np=4 on development. Its node id carries no `tests/` prefix
because tests/pytest.ini puts rootdir at `tests/`; a deselect that does not
match is ignored in silence, which cost two wrong diagnoses while measuring
this.

Underworld development team with AI support from Claude Code

* Make the four-rank pass opt-in: it does not fit the CI budget

Run unconditionally, the second parallel pass took the test job past the
120-minute cap. Recent runs on this repo take 26 to 40 minutes, so there was
about 80 minutes of headroom; on a two-core runner np=4 is oversubscribed and
the pass costs far more than the ~3 minutes it takes on a workstation.

It is now behind --full-parallel, off by default, which is the same shape
test_levels.sh already used for its own four-rank pass. CI keeps the part that
matters and costs little: the whole tests/parallel/ directory at the requested
rank count, which is what closes the glob hole.

Verified: ./scripts/test.sh --p 2 --parallel-only exits 0 and runs no four-rank
pass.

Underworld development team with AI support from Claude Code

* Cover tests/parallel by enumeration, and keep the batching

Two requirements that pull against each other, and the previous commit met
only one.

Coverage: the globs `test_075*py` and `test_10*py` named 14 of the 32 files and
left a hole from 0760 to 0999, so test_0760, test_0765..test_0790, test_0855
and test_0873 ran in parallel at no rank count. The list is now enumerated from
the directory, so it covers by construction rather than by ranges that grow
holes as files are added between them.

Batching: this script does not run one monolithic pytest, for the reason in its
own header — PETSc objects accumulate across files and tests begin to interact.
Handing the whole directory to a single mpirun took the CI test job past its
120-minute cap twice. The enumeration is therefore chunked, PARALLEL_BATCH
files at a time, default 6.

Measured in-environment at np=2: 32 files, 128 passed, 180 s across 6 batches.
The four-rank pass stays opt-in behind --full-parallel, where "1 deselected"
confirms the #611 exclusion matches.

Underworld development team with AI support from Claude Code

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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