Skip to content

Retire the swarm.points writable stack: read-only snapshot, coords is the write path (#379 item 1)#382

Merged
lmoresi merged 3 commits into
developmentfrom
bugfix/swarm-points-retirement
Jul 22, 2026
Merged

Retire the swarm.points writable stack: read-only snapshot, coords is the write path (#379 item 1)#382
lmoresi merged 3 commits into
developmentfrom
bugfix/swarm-points-retirement

Conversation

@lmoresi

@lmoresi lmoresi commented Jul 22, 2026

Copy link
Copy Markdown
Member

Second of the #379 series, stacked on #381 (base branch is bugfix/canonical-callback-guard; retarget to development once #381 merges). Draft until then.

What this does

swarm.points was meant to mirror mesh.points, but the mesh side already had its dangerous half removed in the 2026-07 audit (BF-18: setter raises, pointing at mesh.deform()). The swarm side never got that treatment — it kept an entire private writable stack: a cached callback wrapper, a per-write callback that ran collective particle migration (ranks writing unevenly deadlock in parallel), a bare rank-local RuntimeError on size mismatch, and a read path that could itself trigger a collective (forced migration after mesh changes), so a read performed on some ranks only could hang. The masked-write example in migration_control()'s own docstring raised through this same wrapper.

This PR completes the symmetry:

  • Reading swarm.points (still deprecated) returns a detached, read-only snapshot in physical units — no cache, no callback, no migration side effect.
  • Assigning to it raises AttributeError with the working alternatives spelled out: swarm.coords = values for whole-array writes in physical units, or swarm._particle_coordinates.data[...] for model-unit and masked writes (which defer migration correctly under migration_control() — the new test runs that exact idiom).
  • The migration_control() docstring example now shows the interface that actually works.
  • The one live caller (model.py persistent-variable transfer) now uses swarm.coords, matching its mesh branch.

Nothing that worked is lost: partial writes through swarm.points already raised before this change; whole-array writes have swarm.coords.

Verification

  • New tests/test_0060_swarm_points_retired.py (level_1 / tier_a, written first and shown to fail): read-only snapshot semantics, loud setter refusal, coordinate state untouched by a refused write, and the supported coords / masked migration_control() paths doing what the old docstring promised.
  • Quick gate level_1 and tier_a: 410 passed, 1 pre-existing xfail.
  • Swarm write/timestep MPI smoke at np=2: passed.

Underworld development team with AI support from Claude Code

@lmoresi

lmoresi commented Jul 22, 2026

Copy link
Copy Markdown
Member Author

Adversarial review — round 1

Findings from an independent reviewer running live repros against the stack build:

  1. BLOCKING — in-repo writers of swarm.data break. swarm.data aliases the now read-only snapshot, and whole-array strided writes through it were legitimate before this change: tests/test_0006_memory_leak.py does swarm.data[...] = swarm.data[...] + ... (fails with numpy's bare ValueError: assignment destination is read-only), and three shipped Coriolis streakline examples restart particle loops via swarm.data[offset::loop, :] = .... The guidance text lives only in the points setter, which item-assignment never reaches. Fix: migrate the in-repo writers to swarm._particle_coordinates.data, and return the snapshot as a small read-only view whose write attempts raise WITH the guidance message instead of numpy's bare error.

  2. BLOCKING under units — the persistent-transfer path silently no-ops. model.py's swarm branch now passes swarm.coords, which returns a pint Quantity in metres when units are active; uw.function.evaluate cannot consume it, the blanket except swallows the TypeError, and the target variable is left untouched on every mesh rebuild. (Verified; the OLD swarm.points path was also broken here — it transferred corrupt values due to a pre-existing unit-label bug — so this converts corrupt-transfer into no-transfer.) Fix: pass swarm._particle_coordinates.data (model coordinates, matching the non-dimensional space evaluate works in).

  3. Should-fix — removing migration-on-read leaves the mesh→swarm notification channel with zero consumers. _force_migration_after_mesh_change now has no callers: after mesh.deform-style coordinate changes, stranded particles are never re-binned (verified: 432/576 particles left outside the deformed domain, and the modern collective path does not touch them). The removed read-trigger was itself a parallel hazard, so removing it was right — but the replacement is one line: the solve-entry sync compares the swarm's recorded mesh version and sets _needs_migration, slotting into the existing rank-agreed machinery.

  4. Notes. The read-only contract genuinely holds through the UnitAwareArray wrap (no hidden copy — verified); the snapshot faithfully preserves a PRE-EXISTING 1000× unit-label bug in the old points scaling path (meter magnitudes labelled kilometres — separate issue); model.py still calls swarm.data just for len() (now warns and copies — use swarm.local_size); the new test file never exercises the units regime.

Fixes follow on this branch.

Underworld development team with AI support from Claude Code

lmoresi added a commit that referenced this pull request Jul 22, 2026
…deform re-bins swarms (#382 review)

Adversarial review round 1 findings, all addressed:

1. In-repo writers of swarm.data (test_0006 particle jiggle; three
   Coriolis streakline examples) migrated to
   swarm._particle_coordinates.data — they broke against the read-only
   snapshot with numpy's bare error. The snapshot itself is now a thin
   view whose write attempts raise WITH the guidance (coords /
   _particle_coordinates.data / migration_control), since item
   assignment never reaches the property setter's message.

2. The persistent-variable transfer passes model-unit coordinates
   (_particle_coordinates.data) instead of swarm.coords: under active
   units, coords returns a pint Quantity that evaluate cannot consume,
   and the blanket except silently skipped the transfer. (The old
   swarm.points path was also broken here — it transferred corrupt
   values via a pre-existing unit-label bug.)

3. Retiring the migration-on-read trigger left the mesh->swarm change
   channel with zero consumers: a deformed mesh stranded particles
   permanently. Solve-entry sync (_sync_before_assembly) now compares
   the recorded mesh version and marks _needs_migration, slotting into
   the existing rank-agreed machinery.

4. Model summary uses swarm.local_size instead of len(swarm.data)
   (which now warned and built a full physical-coordinate copy per
   call).

New tests: snapshot writes carry the guidance message; mesh-version
change marks the swarm for deferred migration at solve entry.

Underworld development team with AI support from Claude Code
@lmoresi
lmoresi force-pushed the bugfix/swarm-points-retirement branch from 1f0fb52 to 559ecba Compare July 22, 2026 04:18
@lmoresi

lmoresi commented Jul 22, 2026

Copy link
Copy Markdown
Member Author

Round 1 — response (fixed in 559ecba; branch rebased onto the updated #381)

All four findings addressed:

  1. In-repo swarm.data writerstest_0006 and the three Coriolis streakline examples migrated to swarm._particle_coordinates.data (the memory-leak test's jiggle now runs under migration_control()). The snapshot is additionally a thin read-only view whose write attempts raise WITH the guidance message — item assignment never reaches the property setter, so the pointer to swarm.coords / _particle_coordinates.data / migration_control() now travels with the array itself. Pinned by test_snapshot_write_error_carries_guidance.
  2. Silent no-transfer under units — the persistent-variable transfer passes model-unit _particle_coordinates.data, matching the non-dimensional space evaluate() works in. This makes the swarm branch actually correct rather than corrupt-then (old points) or skipped-now (coords).
  3. Stranded swarms after mesh deform_sync_before_assembly compares the swarm's recorded mesh version and marks _needs_migration, so the deferred, rank-agreed migration machinery notices coordinate changes at solve entry. Pinned by test_mesh_deform_marks_swarm_for_migration.
  4. len(swarm.data) in the model summary — now swarm.local_size.

The pre-existing 1000× unit-label bug in the physical-coordinate scaling (meter magnitudes labelled kilometres) is real and untouched here — it predates this PR and deserves its own issue rather than a rider on a retirement PR.

Quick gate re-run in progress; test_0060 (7 tests) and test_0006 green.

Underworld development team with AI support from Claude Code

lmoresi added a commit that referenced this pull request Jul 22, 2026
…eview)

Round-1 review: np.add(x, 1, out=x) bypassed the guard that x += 1
enforces, re-arming the per-write hazard the flag exists to prevent
(its only in-tree user was the legacy swarm.points wrapper, whose
callback ran collective migrate() — removed by #382, but the flag's
contract must hold regardless). The hook now raises the same
RuntimeError for any out= target carrying the flag, before any
computation. Regression test added.

Underworld development team with AI support from Claude Code
@lmoresi
lmoresi force-pushed the bugfix/canonical-callback-guard branch from 1f42128 to d2cf355 Compare July 22, 2026 04:49
lmoresi added a commit that referenced this pull request Jul 22, 2026
…eview)

Round-1 review: np.add(x, 1, out=x) bypassed the guard that x += 1
enforces, re-arming the per-write hazard the flag exists to prevent
(its only in-tree user was the legacy swarm.points wrapper, whose
callback ran collective migrate() — removed by #382, but the flag's
contract must hold regardless). The hook now raises the same
RuntimeError for any out= target carrying the flag, before any
computation. Regression test added.

Underworld development team with AI support from Claude Code
lmoresi added a commit that referenced this pull request Jul 22, 2026
…deform re-bins swarms (#382 review)

Adversarial review round 1 findings, all addressed:

1. In-repo writers of swarm.data (test_0006 particle jiggle; three
   Coriolis streakline examples) migrated to
   swarm._particle_coordinates.data — they broke against the read-only
   snapshot with numpy's bare error. The snapshot itself is now a thin
   view whose write attempts raise WITH the guidance (coords /
   _particle_coordinates.data / migration_control), since item
   assignment never reaches the property setter's message.

2. The persistent-variable transfer passes model-unit coordinates
   (_particle_coordinates.data) instead of swarm.coords: under active
   units, coords returns a pint Quantity that evaluate cannot consume,
   and the blanket except silently skipped the transfer. (The old
   swarm.points path was also broken here — it transferred corrupt
   values via a pre-existing unit-label bug.)

3. Retiring the migration-on-read trigger left the mesh->swarm change
   channel with zero consumers: a deformed mesh stranded particles
   permanently. Solve-entry sync (_sync_before_assembly) now compares
   the recorded mesh version and marks _needs_migration, slotting into
   the existing rank-agreed machinery.

4. Model summary uses swarm.local_size instead of len(swarm.data)
   (which now warned and built a full physical-coordinate copy per
   call).

New tests: snapshot writes carry the guidance message; mesh-version
change marks the swarm for deferred migration at solve entry.

Underworld development team with AI support from Claude Code
@lmoresi
lmoresi force-pushed the bugfix/swarm-points-retirement branch from 559ecba to 73523d4 Compare July 22, 2026 04:53
@lmoresi
lmoresi marked this pull request as ready for review July 22, 2026 04:53
lmoresi added a commit that referenced this pull request Jul 22, 2026
…deform re-bins swarms (#382 review)

Adversarial review round 1 findings, all addressed:

1. In-repo writers of swarm.data (test_0006 particle jiggle; three
   Coriolis streakline examples) migrated to
   swarm._particle_coordinates.data — they broke against the read-only
   snapshot with numpy's bare error. The snapshot itself is now a thin
   view whose write attempts raise WITH the guidance (coords /
   _particle_coordinates.data / migration_control), since item
   assignment never reaches the property setter's message.

2. The persistent-variable transfer passes model-unit coordinates
   (_particle_coordinates.data) instead of swarm.coords: under active
   units, coords returns a pint Quantity that evaluate cannot consume,
   and the blanket except silently skipped the transfer. (The old
   swarm.points path was also broken here — it transferred corrupt
   values via a pre-existing unit-label bug.)

3. Retiring the migration-on-read trigger left the mesh->swarm change
   channel with zero consumers: a deformed mesh stranded particles
   permanently. Solve-entry sync (_sync_before_assembly) now compares
   the recorded mesh version and marks _needs_migration, slotting into
   the existing rank-agreed machinery.

4. Model summary uses swarm.local_size instead of len(swarm.data)
   (which now warned and built a full physical-coordinate copy per
   call).

New tests: snapshot writes carry the guidance message; mesh-version
change marks the swarm for deferred migration at solve entry.

Underworld development team with AI support from Claude Code
lmoresi added a commit that referenced this pull request Jul 22, 2026
…ds residual mutation routes (#382 review round 2)

Round 2 verified all round-1 fixes closed and found one gap: the
solve-entry swarm re-bin fires off _mesh_version, but _deform_mesh
never bumped it — only the mesh.X.coords callback path did. So
mesh.deform(), the sanctioned deform for variable-carrying meshes,
still left registered swarms stranded. The version now bumps inside
_deform_mesh (the callback path bumps again afterwards — harmless,
the consumer checks inequality). Regression test added.

Also recorded on the snapshot docstring: non-setitem mutation routes
surface numpy's read-only error rather than the guidance (still
refused), and re-enabling the base's writeable flag reaches only the
detached copy, never the swarm.

Underworld development team with AI support from Claude Code
@lmoresi
lmoresi force-pushed the bugfix/swarm-points-retirement branch from 73523d4 to 05ab799 Compare July 22, 2026 05:02
lmoresi added a commit that referenced this pull request Jul 22, 2026
…eview)

Round-1 review: np.add(x, 1, out=x) bypassed the guard that x += 1
enforces, re-arming the per-write hazard the flag exists to prevent
(its only in-tree user was the legacy swarm.points wrapper, whose
callback ran collective migrate() — removed by #382, but the flag's
contract must hold regardless). The hook now raises the same
RuntimeError for any out= target carrying the flag, before any
computation. Regression test added.

Underworld development team with AI support from Claude Code
@lmoresi

lmoresi commented Jul 22, 2026

Copy link
Copy Markdown
Member Author

Round 2 — verification and one gap (fixed in 05ab799)

Round 2 confirmed all round-1 fixes closed (snapshot mutation routes probed exhaustively — fill/sort/copyto/in-place/out= all refused; _mesh_version always initialised, so no first-solve false migrations; deform inside migration_control caught at the next solve entry). One gap: the solve-entry re-bin fires off _mesh_version, but _deform_mesh never bumped it — only the mesh.X.coords callback path did, so mesh.deform() (the sanctioned path for variable-carrying meshes) still left swarms stranded. The version now bumps inside _deform_mesh; the callback path's second bump is harmless (the consumer checks inequality). Regression test added.

Two notes recorded on the snapshot docstring: under active units the UnitAwareArray wrap stays non-writeable but surfaces numpy's error rather than the guidance, and deliberately re-enabling the base's writeable flag reaches only the detached copy — never the swarm.

Underworld development team with AI support from Claude Code

@lmoresi
lmoresi changed the base branch from bugfix/canonical-callback-guard to development July 22, 2026 08:11
lmoresi added 3 commits July 22, 2026 18:11
…ot, coords is the write path (#379 item 1)

swarm.points kept a private writable wrapper whose per-write callback
ran collective particle migration — ranks writing unevenly deadlock in
parallel — and raised a bare rank-local RuntimeError on size mismatch.
Reading it could also trigger a collective (forced migration after mesh
changes), so a read on some ranks only could hang. The masked-write
idiom migration_control()'s own docstring advertised raised through
this same wrapper.

This is the mesh.points treatment (BF-18): the deprecated read returns
a detached read-only snapshot with no side effects; the setter refuses
loudly, pointing at swarm.coords (physical units) and
swarm._particle_coordinates.data (model units, masked writes supported
under migration_control()). The migration_control() docstring example
now shows the interface that actually works, and the one live caller
(model.py persistent-variable transfer) uses swarm.coords, matching its
mesh branch.

Quick gate (level_1 and tier_a): 410 passed. Swarm write/timestep MPI
smoke at np=2: passed.

Underworld development team with AI support from Claude Code
…deform re-bins swarms (#382 review)

Adversarial review round 1 findings, all addressed:

1. In-repo writers of swarm.data (test_0006 particle jiggle; three
   Coriolis streakline examples) migrated to
   swarm._particle_coordinates.data — they broke against the read-only
   snapshot with numpy's bare error. The snapshot itself is now a thin
   view whose write attempts raise WITH the guidance (coords /
   _particle_coordinates.data / migration_control), since item
   assignment never reaches the property setter's message.

2. The persistent-variable transfer passes model-unit coordinates
   (_particle_coordinates.data) instead of swarm.coords: under active
   units, coords returns a pint Quantity that evaluate cannot consume,
   and the blanket except silently skipped the transfer. (The old
   swarm.points path was also broken here — it transferred corrupt
   values via a pre-existing unit-label bug.)

3. Retiring the migration-on-read trigger left the mesh->swarm change
   channel with zero consumers: a deformed mesh stranded particles
   permanently. Solve-entry sync (_sync_before_assembly) now compares
   the recorded mesh version and marks _needs_migration, slotting into
   the existing rank-agreed machinery.

4. Model summary uses swarm.local_size instead of len(swarm.data)
   (which now warned and built a full physical-coordinate copy per
   call).

New tests: snapshot writes carry the guidance message; mesh-version
change marks the swarm for deferred migration at solve entry.

Underworld development team with AI support from Claude Code
…ds residual mutation routes (#382 review round 2)

Round 2 verified all round-1 fixes closed and found one gap: the
solve-entry swarm re-bin fires off _mesh_version, but _deform_mesh
never bumped it — only the mesh.X.coords callback path did. So
mesh.deform(), the sanctioned deform for variable-carrying meshes,
still left registered swarms stranded. The version now bumps inside
_deform_mesh (the callback path bumps again afterwards — harmless,
the consumer checks inequality). Regression test added.

Also recorded on the snapshot docstring: non-setitem mutation routes
surface numpy's read-only error rather than the guidance (still
refused), and re-enabling the base's writeable flag reaches only the
detached copy, never the swarm.

Underworld development team with AI support from Claude Code
@lmoresi
lmoresi force-pushed the bugfix/swarm-points-retirement branch from 05ab799 to e02a828 Compare July 22, 2026 08:12
@lmoresi
lmoresi merged commit 2437a21 into development Jul 22, 2026
1 of 2 checks passed
lmoresi added a commit that referenced this pull request Jul 22, 2026
…mposed #382 bumps _mesh_version in _deform_mesh too

The #382 round-2 fix adds a version bump inside _deform_mesh (so
mesh.deform notifies swarms); the coords-callback path bumps again.
The consumer contract is inequality — the regression tests now assert
the same, instead of pinning per-write step counts that broke when the
sibling branches composed on development.

Underworld development team with AI support from Claude Code
lmoresi added a commit that referenced this pull request Jul 22, 2026
…eview)

Round-1 review: np.add(x, 1, out=x) bypassed the guard that x += 1
enforces, re-arming the per-write hazard the flag exists to prevent
(its only in-tree user was the legacy swarm.points wrapper, whose
callback ran collective migrate() — removed by #382, but the flag's
contract must hold regardless). The hook now raises the same
RuntimeError for any out= target carrying the flag, before any
computation. Regression test added.

Underworld development team with AI support from Claude Code
@lmoresi
lmoresi deleted the bugfix/swarm-points-retirement branch July 22, 2026 08:26
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.

1 participant