Shuffle shard dispatch order before fan-out (issue 197)

[espg]

Closes #197

What / why

ShardMap.shard_keys comes out of mortie.morton_coverage spatially sorted, and _select_cells preserved that order, so dispatch() fanned out morton-contiguous shards together. Their output S3 keys share long leading byte-prefixes, concentrating the write burst on one S3 partition (~3,500 PUT/s per-partition → 503 SlowDown at high concurrency).

Fix, per the issue's "one line plus a test": _select_cells now shuffles the selected (shard_key, granule_urls) pairs with a seeded local random.Random before returning:

if max_cells:
    pairs = pairs[:max_cells]
random.Random(",".join(str(k) for k, _ in pairs)).shuffle(pairs)
return pairs

• Single insertion point covers both backends. _run_local (src/zagg/runner.py:1104) uses the returned order directly. _run_lambda (src/zagg/runner.py:1288) re-sorts by granule count descending — that sort is stable, so its throughput heuristic is preserved while equal-count tie groups (previously morton-contiguous runs) are now randomized. The n_cells counting call in agg only takes len(), unaffected.
• --max-cells N semantics preserved. Truncation happens first, so N still selects the first N in morton order (a spatially coherent subset, per the issue note); only the selected set is shuffled.
• morton_cell path returns early (single shard) — no shuffle, unchanged.

Seed derivation

Seeded from the selected shard keys themselves: ",".join(str(k) for k, _ in pairs). There is no run id available at selection time (the async-channel run_id at src/zagg/runner.py:1332 is a per-run uuid4, deliberately not stable across reruns), so the selected-key set is the config-identity input: the same catalog + same max_cells yields the same seed string, so a rerun or resume sees the identical order. random.Random(str) seeding is deterministic across processes (unlike hash() on strings, which PYTHONHASHSEED randomizes), and a local Random instance avoids perturbing the global RNG.

Phases

• Phase 1: shuffle in _select_cells + tests

How it was tested

• uv run ruff check src/zagg/runner.py tests/test_runner.py — passes (see note below on a pre-existing repo-wide finding)
• uv run ruff format --check src/zagg/runner.py tests/test_runner.py — 2 files already formatted
• uv run pytest tests/test_runner.py -v — 155 passed
• uv run pytest -q — 1554 passed, 28 skipped (credential-gated), in 300s

New tests in TestSelectCells: same inputs → identical order (rerun determinism); result is a permutation of the selection (keys match exactly, order differs from morton for n=50); max_cells=10 of 50 still yields exactly keys 0–9 (truncate-then-shuffle). The two existing order-asserting tests now compare sorted keys.

Questions for review

• Pre-existing lint/format failures on main (not touched here): with the pre-commit-pinned ruff 0.14.10, ruff check src tests fails N818 on src/zagg/registry.py:64 (UnknownCapability), and ruff format --check src tests would reformat 7 files (src/zagg/__init__.py, src/zagg/__main__.py, src/zagg/catalog/sources.py, src/zagg/viz/crs.py, tests/test_catalog.py, tests/test_extract.py, tests/test_integration.py). All reproduce on a clean origin/main checkout; the PR lint bot's selection (--select=E,F,W,I --ignore=E501) passes repo-wide. Left as-is per convention (unrelated pre-existing failures are flagged, not fixed).
• The lambda path's granule-count sort means its shuffle benefit is within equal-count tie groups only. If full randomization on lambda is preferred over the throughput sort, that would be a follow-up decision — this PR deliberately leaves the sort untouched.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

Fresh-context adversarial review of PR #204 (claude/197-dispatch-shuffle, head 01887ae) against issue #197 and CLAUDE.md. Diff is small, terse, and convention-clean; the one substantive problem is whether it achieves the issue's goal on the backend that motivated it.

Findings

1. should-fix (correctness vs. issue #197's goal) — _run_lambda's stable descending granule-count sort (runner.py:1297) largely restores morton/spatial locality: the shuffle survives only within exact-count tie groups, granule counts are spatially autocorrelated, and simulation shows a near-morton in-flight key-span (114 vs 99 morton / 1960 pure-shuffle) under smooth counts — with fully distinct counts the shuffle is a strict no-op. The high-concurrency lambda path is precisely where the S3 503s occur; the local thread-pool path that reliably benefits is not. Options for the maintainer are in the inline comment (coarsen the sort key, drop the exact sort, or accept local-only benefit). The PR body's "Questions for review" flags this but understates it as "tie groups only."
2. should-fix (test gap) — no test covers the composed lambda dispatch order (shuffle then count-sort); the _data helper gives all cells equal counts, the one regime where the shuffle fully survives. See inline comment in tests/test_runner.py.
3. question-for-author — seed-derivation caveats: resume with a different max_cells gets an unrelated order (unavoidable given truncate-first; worth a docstring clause), and Random(str)+shuffle cross-interpreter stability is de facto, not contractual. See inline comment.
4. nit — runner.py is now 2,412 lines, well past the ~1000-line guidance in CLAUDE.md §4. Pre-existing and not this PR's doing (net +9 lines); flagging per convention rather than asking this PR to split it.

Verified, no action needed: truncate-before-shuffle is genuinely pinned by test_shuffle_after_max_cells_truncation (a shuffle-first implementation fails it under this seed); morton_cell early-return stays unshuffled; empty/single selections are shuffle no-ops; the n_cells caller (runner.py:314) and _dry_run_summary are order-insensitive, so no hidden behavior change there; local random.Random instance avoids perturbing global RNG state; stdlib-only, no new dependency (§4); import placement and commit style match the repo.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

should-fix — the lambda path largely undoes this shuffle, and lambda is the path issue #197 targets.

_run_lambda re-sorts right after this call (runner.py:1296-1297):

cells = _select_cells(catalog_data, morton_cell=morton_cell, max_cells=max_cells)
if not morton_cell:
    cells.sort(key=lambda kv: len(kv[1]), reverse=True)

The PR body's claim that the stable sort preserves the shuffle "within equal-count tie groups" is technically correct, but it understates the consequence. Granule counts are spatially autocorrelated — morton-neighboring cells are crossed by nearly the same set of ATL06 granules, so len(kv[1]) varies smoothly across the keyspace. Two regimes follow:

1. Counts near-unique: the sort totally determines the order and the shuffle is a no-op. If counts also correlate with key position, the dispatch order is (reversed) spatial order — exactly the pattern the issue calls worst-case.
2. Counts smooth/near-monotone with small ties: simulation with count = i//8 + rand(0,2) over 2,000 cells, shuffle-then-stable-sort: the mean key-span of a 100-wide in-flight window is 114 vs 99 for raw morton order and 1960 for a pure shuffle. That is ~85% of the morton-order locality restored.

The benefit only materializes when tie groups are large and each count value occurs at spatially scattered locations. So the fix reliably helps _run_local (a thread pool at small max_workers, where SlowDown was never the problem) and unreliably helps _run_lambda (~2,000 concurrent workers, the stated motivation). Concrete options to put to the maintainer: (a) coarsen the lambda sort key so tie groups are large and the shuffle survives, e.g. key=lambda kv: len(kv[1]) // B or int(log2(len(kv[1])+1)), descending; (b) drop the exact count sort on lambda — longest-first mainly trims tail latency at run end, worth measuring against the 503 cost; (c) accept local-only benefit and note it on issue #197. As written, the PR's headline claim ("single insertion point covers both backends") does not hold for the backend that matters.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

question-for-author — seed stability caveats worth stating explicitly.

I checked the determinism claims; most hold, with two caveats:

• Same catalog + same max_cells → same order: holds. ShardMap.shard_keys comes out of mortie.morton_coverage deterministically sorted, so the joined seed string is stable across reruns of the same catalog, and random.Random(str) seeding is PYTHONHASHSEED-immune (correct choice over hash()).
• Resume with a different max_cells (e.g. a --max-cells 100 smoke run, then the full run into the same store): the seed string differs, so the 100 overlapping cells are visited in an unrelated order. No truncate-first scheme can avoid this (a different-length list permutes differently even under a shared seed), so it is fine — but the docstring's "reruns see the same order" silently assumes identical selection args. Worth one clause saying so.
• Cross-version stability: Random(str) version-2 seeding and Fisher-Yates shuffle have been stable across CPython releases in practice, but that is de facto, not contractual. If a resume can happen from a different Python minor (CI matrix is 3.12 + 3.13), the order could in principle diverge. Is same-interpreter resume a safe assumption here?

Also a micro-nit: for a CONUS-scale catalog the joined seed string is built once and hashed — O(total key chars), negligible; no action needed.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

should-fix (test gap) — nothing pins the order that is actually dispatched on the lambda path.

The three new tests pin _select_cells in isolation, and they do it well:

• test_shuffle_after_max_cells_truncation genuinely pins truncate-before-shuffle: I verified a broken shuffle-first implementation yields keys [2, 3, 4, 19, 21, 28, 39, 40, 41, 42] under this seed, so the assertion fails — good.
• test_shuffle_is_deterministic would also pass with a constant seed (random.Random(42)), but a constant seed still satisfies issue Shuffle shard dispatch order before fan-out to spread S3 prefix load #197, so that is acceptable coverage.

What is missing is the composition with _run_lambda's cells.sort(key=lambda kv: len(kv[1]), reverse=True) (runner.py:1297). The _data helper gives every cell exactly one granule — all counts tie — which is the one regime where the shuffle fully survives the sort. A test that builds a catalog with distinct granule counts and asserts the post-sort dispatch order is spread (or at least asserts what the composed order is) would have surfaced the locality-restoration problem flagged on runner.py:932. Suggest either testing at the _run_lambda dry-run level or extracting the lambda ordering (select + sort) into a helper that can be unit-tested directly.

---

Generated by Claude Code

[espg]

🤖 from Claude

Folded the adversarial-review findings (review) in 4bbcbc3:

1. Lambda sort undoing the shuffle (inline comment) — took option (a), coarsened: the exact-count sort in _run_lambda is now a stable descending sort on len(granule_urls).bit_length() (log2 buckets), extracted as _lambda_dispatch_order. Chosen over (b) drop-the-sort because it keeps the longest-first tail-latency heuristic at essentially no cost, and over (c) accept-local-only because lambda is the path issue Shuffle shard dispatch order before fan-out to spread S3 prefix load #197 targets. Spatially autocorrelated counts now land in wide buckets, so the seeded shuffle survives within each bucket instead of only within exact-count ties; the run-end straggler ordering is preserved to within a factor of 2 of granule count.

2. Composed-order test gap (inline comment) — added TestLambdaDispatchOrder with distinct granule counts (cell i has i+1 granules, the regime where the old exact-count sort made the shuffle a strict no-op): asserts the composed order is a permutation of the selection, bucket keys are non-increasing, the largest bucket (19 cells, counts 32–50) is internally not morton-sorted under the fixed seed, and the composed order is deterministic across calls.

3. Seed caveats (inline comment) — _select_cells's docstring now states both: a resume with a different max_cells seeds differently and gets an unrelated order (inherent to truncate-first, per the review's own analysis), and determinism relies on random.Random(str) + shuffle being stable in practice across CPython 3.12/3.13 (the CI matrix). On the question: same-interpreter resume is the working assumption; cross-minor divergence would only reshuffle dispatch order, never correctness, so no guard added.

4. Module-size nit (runner.py ~2,400 lines, pre-existing) — left standing for human review; this PR is net +60 lines and a split is out of its scope.

Local after the fold: ruff check / ruff format --check clean on changed files, pytest tests/test_runner.py -q 157 passed, full pytest -q 1556 passed / 28 skipped. No ruff-bot inline comments on the previous commit as of this writing.

---

Generated by Claude Code

[espg]

🤖 from Claude

Status: complete. Single phase done, all adversarial-review findings folded (4bbcbc3), CI fully green on the folded head (Lint, Tests, Lambda Build, Docs). Applying waiting — awaiting review.

One process note: CLAUDE.md §2 says a PR in exactly this state should be flipped from draft to ready-for-review by the routine itself, but this run's routine configuration explicitly forbids marking PRs ready ("drafts only"). Per the stop-on-conflict rule I'm leaving it draft rather than guessing — flip it ready at your convenience; everything else about the §2 end-state (phases done, findings folded, CI green, waiting applied) holds.

---

Generated by Claude Code