The adapt level loop stops on a collective fact, not a rank-local one

[lmoresi]

Closes #512.

Three follow-ups from the #488 re-review. The first turned out to be a live
hang rather than a latent one, and for a different reason than the issue gives.

The stop, not the metric

Each level of the adapt marking loop ends with a DM refinement, which is
collective. Leaving that loop is therefore a decision every rank has to take
together. The sbr path broke on refine.size == 0 — this rank's own cells are
all fine enough — which says nothing about its peers'. The rank that ran out of
work left for good; the others went round again into the refinement and waited.

Measured on development at np=2, with a callable metric demanding h = 0.01
within r < 0.15 of one corner and nothing elsewhere, on 132 cells per rank:

[1] entering adapt
[0] entering adapt
<mpirun reached its timeout; neither rank returned>

and with the fix:

[1] ADAPT RETURNED cells=132
[0] ADAPT RETURNED cells=2421

No empty rank is required, and the metric is a callable — evaluated rank-locally
— so global_evaluate is not involved.

What the issue attributes it to, and what we measured

#512 describes the rank-local if cur_h.size: guards as skipping a collective,
on the grounds that eval_metric is global_evaluate for a field or expression
metric. That is not what happens. With one rank asleep for ten seconds, the
other's global_evaluate returned in 0.06 s — for in-domain points, and for
points stranded outside the mesh entirely, which is the branch that allgathers:

	rank 0 (sleeps 10 s first)	rank 1 (calls immediately)
in-domain points	0.06 s	0.06 s
stranded points	0.06 s	0.06 s

It does not wait for its peers on these shapes, so a cell-less rank skipping it
does not hang. Those guards now route through a marking_metric helper for
uniformity with the collective stop, and its docstring records which of the two
readings the measurement supports — working from the issue's premise, the next
person would "fix" sites that were never broken.

The pure-Python nvb cell-list engine keeps its rank-local marking and break, with
a comment saying why: it raises NotImplementedError at np>1, so it is serial by
construction.

reconnect.py — a collective verdict

The "a shared point was deleted" guard is assert-class, but raising it on one
rank while the others carry on into the next collective hangs the peers it is
trying to inform. It is now an allgathered verdict naming the offending ranks.

Note the shape of the fix: the emptiness test above it is read early and
acted on after the verdict. Putting the allgather where the guard was would
have left a rank that shares nothing returning before it — introducing exactly
the defect being fixed.

The vacuous test

test_pinned_set_is_partition_independent allgathered the pinned coordinates,
took the union, and asserted len(union) == allreduce(len(union), MAX). Every
rank builds that union from the same gathered list, so the two numbers are equal
by construction and the assertion could not fail whatever the pinning did.

Replaced with one that can fail: every rank must pin every band vertex present in
its own coordinate array, so a rank-local pin that misses a vertex a peer pinned
is caught — which is the defect class, since the owner is then free to move a
vertex the seam expects to stay.

Verified

• test_0873_adapt_collective_stop_mpi.py: 4 passed at np=2, and with
  ptest_0845, 7 passed at np=4. It carries a premise test asserting the metric
  really does leave a rank with no work — without that, a passing run says only
  that adapt returned.
• Full ./uw test: 1509 passed, 32 skipped, 2 xfailed.

Underworld development team with AI support from Claude Code

[Copilot]

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

[lmoresi]

Adversarial review

Reviewed at 94d35c7. Three findings.

1. marking_metric adds a collective per level to two engines that did not
need one. The measurement in the description says the if cur_h.size: guards
were not hanging, so routing nvb-native and edge_split through the helper buys
uniformity and costs an allreduce per level per engine — on a 4-level adapt at
np=8 that is a handful of reductions against a refinement pass, so we judged it
free, but it is a real addition made for tidiness rather than for a defect. The
sbr path genuinely needs it, since that is where the collective stop reads
"nobody has cells".

2. The 0.06 s measurement bounds the call shapes we tried, not
global_evaluate. We drove it with a rank asleep and the other calling, for
in-domain and stranded points, at np=2. global_evaluate_nd does contain
uw.mpi.barrier() and an allgather/Allreduce ladder on the best-claim path;
what we can say is that neither of the shapes we could construct entered them in
a way that blocked a peer. A different metric, a different partition, or np>2
could still reach them. We would rather the code did not depend on which — hence
finding 1 being a cost we accepted rather than an error.

3. The premise test constrains the fixture, not the defect.
test_premise_the_metric_splits_the_ranks asserts some rank has no work at the
FIRST level. The hang needs a rank to run out of work at some level while
another continues, which the first level happens to give on this fixture at
np=2 and np=4. On a partition where every rank holds corner cells the parametrised
tests would pass without exercising anything — the premise test then fails
loudly, which is the right outcome, but it means the coverage is
partition-dependent and we have checked it only at np=2 and np=4.

Checked and clean: the reconnect.py verdict is reached by every rank. The
emptiness test above it is read before the allgather and acted on after, so a
rank that shares nothing does not return early past the collective — which is
the mistake the first draft of that fix made.

Underworld development team with AI support from Claude Code

[lmoresi]

Responses to the review above

Positions on the three findings, so none is left open.

1. marking_metric adds a collective to two engines that did not need one.
Keeping it, and narrowing the claim rather than the code. The measurement says
the nvb-native and edge_split guards were not hanging, so routing them through
the helper is uniformity, not a fix — the docstring already says which of the
two the measurement supports. The cost is one allreduce of a single int per
level per engine, against a DM refinement in the same iteration. Reverting those
two would leave three marking loops with two different emptiness disciplines,
and the sbr one genuinely needs the collective form, so the reader would have to
know which is which. That is a worse failure mode than a reduction nobody
notices.

2. The 0.06 s measurement bounds the shapes we tried. Agreed, and it is
stated that way in the note and the docstring. global_evaluate_nd does contain
uw.mpi.barrier() and an Allreduce ladder on the best-claim path; what we
established is that neither an in-domain call nor a stranded-point call at np=2
blocked a peer. We are not claiming the function is never collective, only that
the mechanism #512 names is not the one that hangs the adapt loop. Finding 1's
answer is the hedge against being wrong about this.

3. The premise test constrains the fixture, not the defect. Standing, and it
is the reason the premise test exists. It asserts some rank has no work at the
first level; if a partition gave every rank corner cells the parametrised tests
would pass without exercising anything, and the premise test then fails loudly
rather than the suite going quietly green. Verified at np=2 and np=4 only — a
run at np=8 or on a different partitioner could need the metric's radius
adjusted, and the failure would name itself.

Underworld development team with AI support from Claude Code