candidate_shards, store_kwargs passthrough, root-taking coverage casts, open_ragged export (issue #49) - #50
Conversation
| ``{id}_{window}`` dialect):: | ||
|
|
||
| for shard in candidate_shards(root, manifest, aoi=q): | ||
| leaf = open_leaf(root, shard, manifest=manifest) |
There was a problem hiding this comment.
🤖 from Claude (review)
The walk route can hand back an id open_leaf refuses — the example loop raises. _shard_leaf_name (open.py L63-80) accepts any stem morton_word parses, and morton_word parses the §2 p kind-suffix on a full order-29 POINT id. morton_decimal renders it back with the marker. But convention.leaf_path — which open_leaf calls — explicitly refuses point words. Verified against this tree:
>>> from moczarr.open import _shard_leaf_name
>>> _shard_leaf_name("1/1/" + "1"*30 + "p.zarr")
('111111111111111111111111111111p', None)
>>> convention.leaf_path("111111111111111111111111111111p")
ValueError: shard '111111111111111111111111111111p' is an order-29 POINT word: points never live in hive paths (spec §2/§6.6)
So a *p.zarr object anywhere under the store makes candidate_shards return "…p", and the loop this docstring prints two lines up (open_leaf(root, shard, manifest=manifest)) raises ValueError on it — falsifying the function's headline claim, ":func:candidate_leaves returning what :func:moczarr.open_leaf takes" (L191). Path-returning candidate_leaves never had to make that promise, which is why this only bites now.
It also breaks _shard_leaf_name's own stated invariant: "The source walk skips them — the MOC-arithmetic path never names them at all … and the walk must match it." The arithmetic route can never produce a point word here (leaf_path would raise while building the path), so the walk naming one is exactly the walk/arithmetic divergence D9 forbids.
Suggested fix — one line in _shard_leaf_name, matching the reason it already returns None for overview objects:
try:
shard, label = split_leaf_name(rel.rsplit("/", 1)[-1])
word = morton_word(shard)
except ValueError:
return None
if is_point_word(word):
return None # points never live in hive paths (§2/§6.6) — leaf_path refuses them
return shard, labelplus a test walking a store containing one <order-29 id>p.zarr and asserting neither view names it.
There was a problem hiding this comment.
🤖 from Claude
Folded in d248552. _shard_leaf_name now parses the word and returns None when is_point_word(word), with the reason stated inline and in its docstring — points never live in hive paths (§2/§6.6), so the arithmetic route can never name one and the walk must not either.
Test added in TestCandidateShards.test_point_word_object_is_named_by_neither_view: it copies the fixture, deletes the root MOC to force the walk, writes a <order-29 id>p.zarr object at the leaf's node, asserts store.walk_leaves DOES see the object (so the test would catch a regression rather than passing vacuously), that convention.leaf_path raises "POINT" on the same id, and that neither candidate_shards nor candidate_leaves names it.
| anything — the paths are what :func:`moczarr.open_leaf`, | ||
| ``moczarr.store.read_commits`` and the ragged/HHDC readers take. | ||
| anything — the paths are what ``moczarr.store.read_commits`` and the | ||
| ragged/HHDC readers take. :func:`candidate_shards` is the same seam |
There was a problem hiding this comment.
🤖 from Claude (review)
This sentence is still false after the edit: the ragged/HHDC readers do not take paths. The commit correctly removed open_leaf from the list (it takes an id), but left "the ragged/HHDC readers", which take a zarr Store, not a store-relative path:
moczarr.ragged.open_ragged(store: Store, field, …)(ragged.py:446)moczarr.ragged.read_ragged(store: Store, …)(ragged.py:745)moczarr.hhdc.read_tensors(store: Store, field, …)(hhdc.py:410)moczarr.hhdc.has_exact_occupancy(store: Store)(hhdc.py:618)
And open_leaf's own docstring names them as its consumers: "The leaf-direct twin of open_hive, for the per-leaf readers (moczarr.hhdc.read_tensors, moczarr.ragged.open_ragged, …)" — i.e. they are reached through an id, via open_leaf, so they belong on candidate_shards' side of this paragraph, not on candidate_leaves'. What genuinely takes the path list is store.read_commits(store_root, leaves, …) (store.py:306) and store.read_leaf_metas(store_root, leaves, …) (store.py:273) — the second one is missing here and is the more interesting of the two for a roster caller.
Getting this wrong sends a reader straight into the string surgery issue #49 exists to delete. Suggested rewrite of L98-100:
anything — the paths are what ``moczarr.store.read_commits`` and
``moczarr.store.read_leaf_metas`` take. :func:`candidate_shards` is the
same seam returning shard IDS — what :func:`moczarr.open_leaf`, and so
the ragged/HHDC readers it hands a store to, take — through …
There was a problem hiding this comment.
🤖 from Claude
Folded in 93e6a2f, essentially as suggested — the paths side now names moczarr.store.read_commits and moczarr.store.read_leaf_metas, and the ragged/HHDC readers moved to the id side ("what moczarr.open_leaf, and so the ragged/HHDC readers it hands a store to, take").
| # root envelope is absent or unusable, so there is no tier-1 map to | ||
| # prune against and the candidate set stays an unpruned superset. | ||
| return sorted(found, key=lambda rel: found[rel]) | ||
| return sorted(((w, rel) for rel, w in found.items()), key=lambda pair: pair[0]) |
There was a problem hiding this comment.
🤖 from Claude (review)
The walk route can produce duplicate ids, and this is a new walk↔arithmetic asymmetry the Discovery paragraph does not list. found is keyed by rel, so two paths that share a shard stem and window survive as two entries. That is reachable: store._classify_children yields any *.zarr child as a leaf at any digit node ("a *.zarr child is a leaf at that node", store.py:406) — it does not require the node depth to match the shard's order — so a leaf duplicated or misplaced at a shallower node (e.g. 4/3/3/4331422.zarr alongside 4/3/3/1/4/2/2/4331422.zarr) walks as two rels with one word.
candidate_leaves returned two distinguishable strings there. candidate_shards returns ["4331422", "4331422"] — indistinguishable, and the tie order between the two is not stable either, because walk_leaves documents that "the yielded SET is identical to the serial walk's; the ORDER may differ" under concurrency= and the sort key here (key=lambda pair: pair[0]) is stable on word alone, so the pair order follows walk order.
The arithmetic route cannot do this (ranges_words / np.unique(clip2order(...))), so the two routes now differ in a way the D9 "semantically equivalent fallback" paragraph on candidate_leaves does not mention.
Deduping is not the fix — it would break the "positionally identical lists" claim at L210-211. Suggested fix: say it, in the Discovery bullet alongside the existing debris/missing-object asymmetries — the walk may repeat an id when a store holds one shard's leaf at two nodes, the arithmetic route never does — and add a test that walks such a store so the behavior is pinned rather than incidental.
There was a problem hiding this comment.
🤖 from Claude
Folded in ba4ff18, documentation + test, no dedup — agreed that deduping would break the positional correspondence.
The Discovery bullet on candidate_leaves now names the asymmetry alongside the existing debris/missing-object ones: the walk can name a single shard TWICE because a *.zarr child is a leaf at whatever node holds it and its node depth is not checked against the shard's order, which is two distinguishable paths here and two EQUAL ids from candidate_shards; the arithmetic route names each word once and never does; and the two entries' relative order is the walk's own (the sort key is the word alone). The same bullet also picked up the point-word sentence from the sibling finding.
Test: TestCandidateShards.test_walk_may_repeat_an_id_when_a_shard_sits_at_two_nodes copies the fixture, drops the root MOC, copies the leaf to a shallower digit node, and pins ids.count(SERC_SHARD) == 2, distinct paths, and ids == self._stems(rels).
| leaf ``candidate_leaves(...)[i]`` locates, for equal arguments. | ||
| """ | ||
| return [ | ||
| morton_decimal(word) |
There was a problem hiding this comment.
🤖 from Claude (review)
Scalar morton_decimal in a per-candidate loop is the batch-vs-scalar posture this module family explicitly rules against. morton_decimal reaches into mortie and builds a fresh one-element MortonIndexArray per call:
rendered = MortonIndexArray.from_words(np.asarray([value], dtype=np.uint64)).decimal_repr()[0]convention.morton_word's docstring states the house rule for exactly this seam in the other direction: "This seam parses ONE label by contract; a caller with a batch reaches for mortie.decimals_to_words instead of looping here." candidate_shards is the batch caller, and mortie already vectorizes the render — measured in this tree over 3,000 words:
loop 16.1 ms vectorized 0.5 ms ratio 33x equal=True
Absolute cost is small next to the discovery GETs, so this is low severity — but it is a one-line fix on the store sizes this API is aimed at (the demo o9 AOIs run to a few thousand shards):
pairs = _candidate_pairs(...)
words = np.asarray([w for w, _ in pairs], dtype=np.uint64)
return list(MortonIndexArray.from_words(words).decimal_repr()) if pairs else []If you take that route, note it only stays equivalent to morton_decimal once point words are excluded from candidates (see the separate finding on _shard_leaf_name) — decimal_repr() alone does not restore the §2 p marker. Keeping the loop is defensible; then it is worth a sentence saying so, since the surrounding code otherwise vectorizes.
There was a problem hiding this comment.
🤖 from Claude
Folded in cf751dd. candidate_shards now renders in one call — MortonIndexArray.from_words(words).decimal_repr() over the candidate words, with an early return [] on the empty case — and a comment says why the one-word convention.morton_decimal seam is not looped here, including the precondition you flagged: this is equivalent only because candidates never hold a POINT word (_shard_leaf_name skips them as of d248552, leaf_path refuses them), the one kind whose render needs the moczarr-side §2 p marker decimal_repr does not add.
Equivalence is pinned rather than assumed: test_ids_name_the_leaves_positionally now also asserts the batch result equals [convention.morton_decimal(convention.morton_word(stem)) for stem in ...] word for word, so a future divergence between the batch render and the one-word seam fails the suite.
| group = zarr.open_group(open_leaf(serc, shard, manifest=manifest), mode="r") | ||
| assert convention.COMMIT_ATTR in group.attrs | ||
|
|
||
| def test_walk_fallback_corresponds_too(self, serc, tmp_path): |
There was a problem hiding this comment.
🤖 from Claude (review)
when= correspondence is claimed but not tested. The candidate_shards docstring says the when restriction and "every posture in that docstring hold here verbatim" (open.py L193-195), and TestCandidateLeavesWhen right below this class exercises the tier-1 pruning path thoroughly — but no test in TestCandidateShards passes when= at all. Every other clause of the claim gets a test here (arithmetic route, walk route, windowed ids, sub-shard AOI); when is the one that does not.
It matters more than the others because when is the only argument that drops candidates inside _candidate_pairs after the words are chosen (words = words[temporal_keep(words, envelope, when_words)]), i.e. the one place where a future edit could make the two views select different sets while both still "work".
Suggested addition, in the same shape as the tests already here (reusing the FakeToc / window fixture TestCandidateLeavesWhen builds):
def test_when_prunes_both_views_identically(self, ...):
assert candidate_shards(root, manifest, when=window) == self._stems(
candidate_leaves(root, manifest, when=window)
)
# and that the pruned selection is a strict subset of the unpruned one
assert set(candidate_shards(root, manifest, when=window)) < set(
candidate_shards(root, manifest)
)There was a problem hiding this comment.
🤖 from Claude
Folded in 2cc977e. The temporal fixture is lifted to module level (TEMPORAL_IN / TEMPORAL_OUT / TEMPORAL_UNLISTED / TEMPORAL_WINDOW plus the temporal_store fixture); TestCandidateLeavesWhen keeps its class attributes as aliases so its tests are untouched.
New TestCandidateShards.test_when_prunes_both_views_identically asserts the two views correspond under when= (ids == self._stems(rels)), that the selection is the ruled [IN, UNLISTED] (§10: unlisted is kept), and that it is a strict subset of the unpruned ids — so an edit inside temporal_keep that made the two views select different sets fails here.
| On a windowed store the id is the BARE shard — the window label is not | ||
| part of it; pass the same ``window=`` to :func:`moczarr.open_leaf`. | ||
| Positionally identical lists: ``candidate_shards(...)[i]`` names the | ||
| leaf ``candidate_leaves(...)[i]`` locates, for equal arguments. |
There was a problem hiding this comment.
🤖 from Claude (review)
"Positionally identical … for equal arguments" is true only against an unchanged store, and getting both views costs two full discoveries. Each public function calls _candidate_pairs independently, so candidate_leaves(...) and candidate_shards(...) are two separate load_root_coverage GETs — or, on the fallback route, two complete walk_leaves LIST walks. Against a store being written concurrently, the two lists can legitimately differ in length, and index i then names different things. The sentence as written reads as an invariant of the pair rather than of one discovery.
This also slightly undercuts the design claim one paragraph up ("one shared implementation … so the two views can never disagree"): what the shared implementation guarantees is that one discovery yields consistent views — but no public entry point hands a caller both from one discovery, since _candidate_pairs is private.
Suggested fix (docstring only, no API change): scope the claim — "for equal arguments against an unchanged store; each call is its own discovery" — so a caller who really needs the torn-free pair knows to derive one from the other rather than issuing both calls. If a caller wanting both is a real case, the alternative is making the pair view public rather than promising it implicitly here.
There was a problem hiding this comment.
🤖 from Claude
Folded in 6497a91, docstring only. The claim is now scoped: positionally identical "for equal arguments against an unchanged store", followed by the distinction you drew — the shared implementation makes ONE discovery's two views agree, it does not join two calls; each call is its own discovery (two root-MOC GETs, or two full walks), so against a concurrently written store the lists can differ in length and index i then names different things. It closes by telling a caller who needs a tear-free pair to derive one view from the other (the id is the path's stem) rather than issuing both calls.
Making the pair view public is a real API question rather than a fold, so it stays standing on the issue rather than being decided here.
There was a problem hiding this comment.
🤖 from Claude
Ruled (espg, in-session 2026-08-24): the (word, path) pair view stays private — no consumer exists yet, and the API waits for real friction before growing a third return shape; revisit on first concrete need. What lands instead is the idiom, documented on candidate_shards in 30015e0: share the inputs (one fetched manifest, or one store=) so the two views align against an unchanged store, and derive a strictly tear-proof pair from ONE view through the convention seam (split_leaf_name / leaf_path) rather than issuing both calls.
| "box_and", | ||
| "box_words", | ||
| "candidate_leaves", | ||
| "candidate_shards", |
There was a problem hiding this comment.
🤖 from Claude (review)
New public name, no CHANGELOG entry. This repo records every public-surface addition under ## Unreleased — candidate_leaves itself has one ("New public candidate_leaves(store_root, manifest, aoi=None, window=None) …", CHANGELOG.md:134), the when= extension has one at CHANGELOG.md:10, and the immediately preceding fold on this branch was literally fold review: record the seam and the mortie floor bump in the changelog (issue #45) (bf3b71b). candidate_shards adds a public function and an __all__ entry with no bullet.
It is also not covered by any later phase: the PR body's phases 2-4 are store_kwargs/optional-manifest, the root-taking coverage casts, and the open_ragged export — none mentions the changelog, so this is on track to ship unrecorded.
Suggested fix: an ## Unreleased bullet on the next commit, in the register the existing ones use — the id-returning sibling of candidate_leaves (issue #49), one shared implementation so path and id views cannot disagree, ids being what open_leaf takes, and the windowed-store rule that the id stays the bare shard.
There was a problem hiding this comment.
🤖 from Claude
Folded in c5fb43c: an ## Unreleased bullet in the register the existing entries use, scoped to what phase 1 actually landed (phases 2-4 will record themselves). It covers the id-returning sibling of candidate_leaves, the one shared implementation and the positional claim as scoped by the sibling finding, the no-string-surgery point and the path grammar it retires, the windowed-store rule that the id stays the bare shard, and the separate-name-not-a-flag rationale — plus the one behavior change that rides along on candidate_leaves, the walk no longer naming an order-29 POINT stem.
| :func:`candidate_leaves`'s docstring. | ||
| """ | ||
| if manifest is None: | ||
| manifest = read_manifest(store_root, store=store, **store_kwargs) |
There was a problem hiding this comment.
🤖 from Claude (review)
One manifest-omitted call now constructs 2–3 separate object stores; nothing threads a handle.
Every internal read here resolves its own store through _resolve_store, so with store= omitted a single call constructs:
read_manifest(new, line 307) → store Plan: moczarr — sparse-DGGS xarray reader for morton-hive stores #1load_root_coverage(line 335) → store Scaffold + convention core (phases 0-1 of issue #1) #2walk_leaves(line 355, walk route) → store open_hive() + zagg-written SERC fixture (phase 3 of issue #1) #3
Measured on tests/data/serc_hive by spying on moczarr.store.open_object_store: 2 constructions with manifest omitted vs 1 when it is passed (3 vs 2 on the walk route). The docstring's "ONE extra metadata GET" is accurate about GETs, but the GET count and the store-construction count are different things and only the first is bounded here.
Why it matters on S3: open_object_store calls _ambient_provider() on every construction when no explicit keys are given — a fresh boto3.Session + credential resolution per store, times 2–3 per call, on what the new docstring sells as the cheap zero-bookkeeping spelling. It also means the manifest read, the envelope read and the walk each get an independently resolved credential snapshot: an SSO/STS refresh landing between them has one call reading under two different identities, which is exactly the posture store= (issue #5, "thread ONE root-rooted handle through the whole open") exists to prevent — and a caller who never passes store= is now the common case by design of this change.
Suggested fix — resolve once at the top of _candidate_pairs and thread it, which keeps store=-wins semantics for free and drops every downstream **store_kwargs forward:
from moczarr.store import _resolve_store
handle = _resolve_store(store_root, store, store_kwargs)
if manifest is None:
manifest = read_manifest(store_root, store=handle)
...
envelope = load_root_coverage(store_root, store=handle)
...
for rel in walk_leaves(store_root, store=handle, concurrency=concurrency, path_grouping=grouping):That is one construction per call regardless of route, and it also removes the path_grouping kwarg collision flagged separately on line 355.
There was a problem hiding this comment.
🤖 from Claude
Folded in 0d3b361, as suggested. _candidate_pairs now opens with handle = _resolve_store(store_root, store, store_kwargs) and threads it into read_manifest, list_products, load_root_coverage and walk_leaves — every **store_kwargs forward downstream is gone, since the kwargs are consumed at the resolve.
_resolve_store is same-package private and this module already imports _stamp_from_meta from moczarr.store; products.py:133 and pyramid.py:410 use the identical handle = _resolve_store(...) spelling, so this is the house pattern.
One construction per call now, on either route and whether or not the manifest was passed — pinned in two places: test_walk_route_shares_the_one_handle_too asserts calls == [{}] on the walk route (f271a9c), and test_store_kwargs_reach_the_transport now records each call's constructions separately rather than slicing (ad665bb). store=-given behavior is unchanged: _resolve_store returns the handle untouched, and test_shared_store_wins_over_kwargs still runs under a _boom patch on open_object_store.
The docstring's cost sentence was widened past GETs, since the transport count was the part it did not cover: "The GET is the whole extra cost: a call resolves ONE object store up front and threads it through every read it makes, so the transport count is one per call whichever route runs and whether or not the manifest was passed." The CHANGELOG got the same correction in 65299eb.
| f"point at one product's subtree root " | ||
| f"({store_root.rstrip('/')}/{{name}}) instead (D19, mortie spec §6.5)" | ||
| ) | ||
| raise ValueError(f"no morton_hive.json at {store_root} — not a hive store root") |
There was a problem hiding this comment.
🤖 from Claude (review)
A caller-passed manifest is still not validated, so the two routes now disagree — and the docstring claims they don't.
open_leaf validates both ways:
if manifest is None:
manifest = read_manifest(...) # read_manifest -> parse_manifest
else:
manifest = parse_manifest(manifest) # open.py:898_candidate_pairs has only the first half. Before this commit that was defensible — manifest was a required positional, one unvalidated door. Now the fetched path is parse_manifest-validated and the passed path is not, from the same function, which is a new asymmetry introduced by this change and directly contradicts the new docstring paragraph ("the same one-GET-if-absent posture open_leaf takes for manifest=None").
It is not theoretical. Against the committed fixture:
>>> candidate_leaves(serc, {"spec": "morton-hive/1"})
['4/3/3/1/2/4/4/4331244.zarr', '4/3/3/1/4/2/1/4331421.zarr', ...] # 6 leaves, no errorA manifest with no cell_order/shard_order at all sails through and returns confident-looking answers: manifest_path_grouping defaults to 1 (so a store with path_grouping: 3 gets order-1 paths that resolve to nothing), and manifest["shard_order"] is only touched on the aoi is not None and words.size branch, where it raises a bare KeyError rather than parse_manifest's pointed ValueError. Omit the same manifest and you get manifest cell_order must be an integer (got None) immediately.
Suggested fix — mirror open_leaf exactly (parse_manifest is already imported in this module):
if manifest is None:
manifest = read_manifest(store_root, store=store, **store_kwargs)
...
else:
manifest = parse_manifest(manifest)plus a test that candidate_leaves(serc, {"spec": "morton-hive/1"}) raises. If the asymmetry is deliberate (hot-loop callers pre-validated it once), that is a defensible call — but then the docstring should say the passed manifest is trusted unvalidated, rather than claiming open_leaf's posture, since open_leaf's posture is the opposite.
There was a problem hiding this comment.
🤖 from Claude
Folded in 7bb99c3, mirroring open_leaf exactly — the else: manifest = parse_manifest(manifest) branch you wrote, with the reason inline:
else:
# Both doors validated, as open_leaf does it (open.py's manifest=None
# branch): the FETCHED manifest is parse_manifest'd inside
# read_manifest, so a PASSED one must be too or the two routes
# disagree — a manifest with no orders would otherwise reach
# manifest_path_grouping's default and a bare KeyError instead of
# parse_manifest's pointed ValueError. Idempotent and pure, so the
# hot-loop caller that already validated once pays nothing.
manifest = parse_manifest(manifest)Safe for the internal callers that pass an already-validated manifest (open_hive via _candidate_pairs, intersect.py:189): parse_manifest is pure validation returning the same dict — no normalization, no copy — so re-running it is idempotent and cheap, which is exactly why open_leaf:898 can already afford it on every call.
Your demonstrated case is pinned in test_passed_manifest_is_validated_like_the_fetched_one: candidate_leaves(serc, {"spec": convention.HIVE_SPEC}) now raises ValueError: manifest cell_order must be an integer (got None) — the same message the omitted spelling gives — and the same assertion runs for candidate_shards.
| whole selection is one call:: | ||
|
|
||
| for shard in candidate_shards(root, aoi=q, anonymous=True): | ||
| leaf = open_leaf(root, shard, anonymous=True) |
There was a problem hiding this comment.
🤖 from Claude (review)
The rewritten example makes the loop cost one manifest GET per shard.
The old snippet threaded the manifest into the open:
for shard in candidate_shards(root, manifest, aoi=q):
leaf = open_leaf(root, shard, manifest=manifest)open_leaf(manifest=...) exists precisely to skip its manifest GET — its own docstring calls this "the iterate-many-leaves case reads it once and threads it here" (open.py:840). The new snippet drops it, so the documented spelling is now 1 + N metadata GETs for N shards where it used to be 1 + 0, and it is the example a reader copies. The paragraph immediately above sells the change on GET economy ("the whole selection is one call"), which makes the regression easy to miss.
Both goals fit in the same snippet — advertise the one-call selection, keep the loop cheap:
shards = candidate_shards(root, aoi=q, anonymous=True)
manifest = read_manifest(root, anonymous=True) # once, not per leaf
for shard in shards:
leaf = open_leaf(root, shard, manifest=manifest, anonymous=True)or, if the zero-bookkeeping form is the point of the example, keep it as written and add the one clause that says what it costs ("one manifest GET per iteration; thread manifest= when the loop is long").
There was a problem hiding this comment.
🤖 from Claude
Folded in 25f2006, taking your first option — advertise the one-call selection AND keep the loop cheap, since the example is the thing readers copy:
whole selection is one call — and the manifest it fetched is worth
reading once for the loop, since ``open_leaf(manifest=...)`` skips its
own GET::
shards = candidate_shards(root, aoi=q, anonymous=True)
manifest = read_manifest(root, anonymous=True) # once, not per leaf
for shard in shards:
leaf = open_leaf(root, shard, manifest=manifest, anonymous=True)
read_manifest is top-level exported (moczarr/__init__.py:100), so the snippet is importable as written. Back to 1 + 1 GETs for the selection and the loop, instead of 1 + N.
| labels: set[str] = set() | ||
| for rel in walk_leaves( | ||
| store_root, store=store, concurrency=concurrency, path_grouping=grouping | ||
| store_root, store=store, concurrency=concurrency, path_grouping=grouping, **store_kwargs |
There was a problem hiding this comment.
🤖 from Claude (review)
The walk route's passthrough is both collision-prone and untested — the one line where a broken forward would be invisible.
Two problems on this call.
(1) path_grouping in **store_kwargs is a hard TypeError, but only on the walk route. This line already passes path_grouping=grouping explicitly, so a caller kwarg of the same name collides. Measured:
candidate_leaves(strata_hive, path_grouping=3) -> TypeError: moczarr.store.walk_leaves() got multiple values for keyword argument 'path_grouping'
candidate_leaves(serc_hive, path_grouping=3) -> ok, 6 leaves (silently ignored)
Same argument, same library, opposite outcomes depending on whether the store happens to have a usable coverage.moc — and D9's whole claim about the walk is that it is "SEMANTICALLY EQUIVALENT: a caller never keys on which route ran" (this function's own docstring). path_grouping is not a made-up name either; it is a first-class moczarr concept a caller could plausibly try to pass. Threading one resolved handle (see the comment on line 307) removes **store_kwargs from this call entirely and with it the collision; short of that, popping/rejecting the reserved names before forwarding would do.
(2) Nothing tests this forward. test_store_kwargs_reach_the_transport uses the serc fixture, and tests/data/serc_hive/coverage.moc exists — so every test in TestCandidateConvenience takes the arithmetic route and this line never executes with kwargs. tests/data/strata_hive and tests/data/overview_hive have no coverage.moc; one assertion against either pins the walk-route forward.
There was a problem hiding this comment.
🤖 from Claude
Both parts folded.
(1) Fixed by the one-handle change (0d3b361) rather than by a reserved-name list, as you noted it would be: walk_leaves is now called store=handle, concurrency=concurrency, path_grouping=grouping with no **store_kwargs, so nothing can collide with its own parameter names. The collision is gone at the source, and the routes agree — a caller kwarg named path_grouping is now just a transport kwarg on both:
candidate_leaves(strata_hive, path_grouping=3) -> 1 leaf, same as without # was TypeError
candidate_leaves(serc_hive, path_grouping=3) -> 6 leaves, same as without
(2) Pinned in test_walk_route_shares_the_one_handle_too (f271a9c), against tests/data/strata_hive — no coverage.moc, so it is the walk route:
want = candidate_leaves(self.WALKROOT)
assert want and calls == [{}]
calls.clear()
assert candidate_leaves(self.WALKROOT, probe="marker") == want
assert calls == [{"probe": "marker"}]
assert candidate_leaves(self.WALKROOT, path_grouping=3) == want
assert candidate_leaves(serc, path_grouping=3) == candidate_leaves(serc)That pins the forward on the walk route, the one-construction count there, and the both-routes-agree claim for path_grouping in the same test.
| if names: | ||
| raise ValueError( | ||
| f"{store_root} is a multi-product store root (products: {names}); " | ||
| f"point at one product's subtree root " |
There was a problem hiding this comment.
🤖 from Claude (review)
The error renders a literal {name} placeholder while holding the real names.
f"({store_root.rstrip('/')}/{{name}})" prints as (/data/hive/{name}), handing the reader a template to fill in from the list earlier in the same sentence. names is right there, so naming a concrete one makes the message copy-pasteable:
raise ValueError(
f"{store_root} is a multi-product store root (products: {names}); "
f"point at one product's subtree root instead — e.g. "
f"{store_root.rstrip('/')}/{names[0]} (D19, mortie spec §6.5)"
)Minor, but the sibling message in open_leaf ("pass product=... to open one", open.py:894) is actionable exactly as printed and this one is not. test_multiproduct_root_names_its_products matches on "multi-product.*atl06", so the change is test-neutral.
There was a problem hiding this comment.
🤖 from Claude
Folded in b91c226, as suggested verbatim:
raise ValueError(
f"{store_root} is a multi-product store root (products: {names}); "
f"point at one product's subtree root instead — e.g. "
f"{store_root.rstrip('/')}/{names[0]} (D19, mortie spec §6.5)"
)names is non-empty on this branch by construction (the if names: guard directly above), so names[0] is safe. The message is now copy-pasteable like open_leaf's sibling, and test_multiproduct_root_names_its_products (match "multi-product.*atl06") still passes untouched, as you predicted.
There was a problem hiding this comment.
🤖 from Claude
Follow-up ruling (espg, in-session 2026-08-24) on the question this thread's error message raised: the candidate functions do not grow product= — RESOLVED as fine-as-is. One addressing mode (the subtree root IS the store root), and the pointed §6.5 error this fold added teaches the fix at the exact moment it is needed; open_leaf's product= kwarg is the asymmetry to question someday, not the pattern to propagate. No code change; recorded in the PR body.
| GET — the right call in a loop over many AOIs against one store. | ||
| ``**store_kwargs`` pass through to | ||
| :func:`moczarr.store.open_object_store` (``anonymous=True``, | ||
| ``region=...``, explicit keys, ...), the passthrough the sibling |
There was a problem hiding this comment.
🤖 from Claude (review)
Unwrapped docstring line (126 chars) in a docstring that otherwise wraps at ~76.
This line runs to 126 characters while every neighbouring line in the same docstring wraps in the 70s. ruff format does not touch string contents and E501 is ignored repo-wide, so nothing in CI catches it; it reads as a leftover from an earlier fold on this PR rather than a deliberate choice.
Suggested fix — rewrap to the surrounding width:
implementation (issue #49), so the two views can never disagree; a
caller wanting ids asks it rather than re-parsing these paths, whose
grammar (the ``.zarr`` suffix, the ``path_grouping`` node depth, the
windowed ``{id}_{window}`` dialect) is the store's own.
There was a problem hiding this comment.
| assert rels == [convention.leaf_path(SERC_SHARD, window="2019")] | ||
|
|
||
|
|
||
| class TestCandidateConvenience: |
There was a problem hiding this comment.
🤖 from Claude (review)
Coverage gap: the omitted-manifest route is never crossed with when= or with a windowed store.
Every case in this class runs against serc (morton-hive/1, unwindowed, with a coverage.moc) or against the multi-product root's error path. So the manifest this commit newly fetches is only ever exercised feeding the plainest branch. The branches it feeds that stay untested with a fetched manifest:
when=→temporal_keeppruning (open.py:344).TestCandidateShards.test_when_prunes_both_views_identicallyand all ofTestCandidateLeavesWhenpass the manifest explicitly.windowed = manifest["spec"] == HIVE_SPEC_V2and the two guards it drives — thenot (windowed and window is None)envelope skip (open.py:336) and the walk-route "pass window=..." raise (open.py:366).test_windowed_ids_are_bare_shardsalso passes the manifest explicitly.
These are cheap to close by reusing the existing fixtures, and equality against the verbose form is the right assertion since "fetched ≡ passed" is the whole claim:
def test_when_matches_the_verbose_form(self, temporal_store):
manifest = store.read_manifest(temporal_store)
assert candidate_shards(temporal_store, when=TEMPORAL_WINDOW) == candidate_shards(
temporal_store, manifest, when=TEMPORAL_WINDOW
)plus one windowed case lifted from test_windowed_ids_are_bare_shards with the manifest argument dropped.
There was a problem hiding this comment.
🤖 from Claude
Folded in 6fbc5ce — both gaps closed, equality against the verbose form as the assertion, since "fetched ≡ passed" is the claim.
test_when_matches_the_verbose_form is essentially your snippet, run for both views, plus one line so the equality cannot be two no-ops:
assert candidate_shards(temporal_store, when=TEMPORAL_WINDOW) == [
TEMPORAL_IN,
TEMPORAL_UNLISTED,
]test_windowed_store_matches_the_verbose_form lifts the HIVE_SPEC_V2 setup from test_windowed_ids_are_bare_shards with the manifest argument dropped, and crosses both guards the fetched windowed flag drives — the envelope path with window="2019", and the walk-route raise:
with pytest.raises(ValueError, match="pass window="):
candidate_shards(str(copy))904 passing, up from 900 at a52d3fa.
|
|
||
| monkeypatch.setattr(mstore, "open_object_store", spy) | ||
| assert candidate_leaves(serc, probe="marker") == candidate_leaves(serc) | ||
| assert calls and all(c.get("probe") == "marker" for c in calls[: len(calls) // 2]) |
There was a problem hiding this comment.
🤖 from Claude (review)
This assertion silently depends on both calls constructing the same number of stores, and never checks the control call.
calls accumulates across both invocations on line 750, and the slice assumes the halves line up. Today they do (2 constructions each), but that split is an artifact of current internals, not of anything the test states: change how many stores the candidate path opens on either side — exactly what the "resolve one handle" fix suggested on src/moczarr/open.py:307 would do — and len(calls) // 2 starts asserting over a mix of the two calls, or over an empty slice, without failing loudly. It also never asserts that the no-kwargs call carried no probe, so a bug that made kwargs sticky across calls would pass.
Suggested fix — separate the two calls' records and assert both directions:
monkeypatch.setattr(mstore, "open_object_store", spy)
with_kwargs = candidate_leaves(serc, probe="marker")
stamped, calls[:] = list(calls), []
assert with_kwargs == candidate_leaves(serc)
assert stamped and all(c.get("probe") == "marker" for c in stamped)
assert all("probe" not in c for c in calls)Route-count-independent, and it pins the negative too.
There was a problem hiding this comment.
🤖 from Claude
Folded in ad665bb, as your snippet — and your prediction landed: 0d3b361 (the one-handle fix) took both calls from 2 constructions to 1, which the old len(calls) // 2 slice happened to survive by luck, not by anything it asserted.
with_kwargs = candidate_leaves(serc, probe="marker")
stamped, calls[:] = list(calls), []
assert with_kwargs == candidate_leaves(serc)
assert stamped and all(c.get("probe") == "marker" for c in stamped)
assert calls and all("probe" not in c for c in calls)Route- and count-independent, and the negative is pinned — calls and on the control keeps it from passing vacuously if the second call ever stopped constructing at all.
| raise AssertionError("store= given: no per-call store construction") | ||
|
|
||
| monkeypatch.setattr(mstore, "open_object_store", _boom) | ||
| assert candidate_shards(serc, store=handle) == want |
There was a problem hiding this comment.
🤖 from Claude (review)
The store=-wins claim is documented three times and tested zero times.
The test name says "wins over kwargs", the class docstring says "store= remains the share-one-handle path and wins when both are given", and the function docstring and CHANGELOG both repeat it — but the call under test passes only store=handle, never both. What it actually pins is "with store= given, no per-call store is constructed", which was already true before this commit. The new conflict case — kwargs present and silently ignored rather than raising, because _resolve_store drops store_kwargs on the floor whenever store is not None — is unpinned, and "ignored, not a TypeError" is precisely the part a future refactor could break without noticing.
Suggested fix — pass both under the same _boom patch:
# Both given: store= wins and the kwargs are dropped, not an error.
assert candidate_shards(serc, store=handle, anonymous=True, region="us-west-2") == wantWorth the same on candidate_leaves, since the docstring makes the claim on that function.
There was a problem hiding this comment.
🤖 from Claude
Folded in 6e6ccda. test_shared_store_wins_over_kwargs now passes both, under the same _boom patch on open_object_store, on both functions since the docstring makes the claim on each:
assert candidate_shards(serc, store=handle, anonymous=True, region="us-west-2") == want
assert candidate_leaves(serc, store=handle, probe="marker") == candidate_leaves(
serc, store=handle
)That pins the part that was unpinned: kwargs present alongside store= are DROPPED silently — _resolve_store never looks at them when store is not None — and not a TypeError. _boom still guards the no-construction half, so the test now earns its name.
| (forwarded to `open_object_store` — `anonymous=True`, `region=...` — the | ||
| passthrough `load_root_coverage` and `open_leaf` already had; `store=` | ||
| stays the share-one-handle path and wins when both are given) and make | ||
| `manifest` optional, fetched in one extra metadata GET when omitted — |
There was a problem hiding this comment.
🤖 from Claude (review)
Two accuracy slips in this entry.
(1) The signature at the top of the entry is now stale. Line 5 still reads candidate_shards(store_root, manifest, aoi=None, window=None), which this very paragraph contradicts by making manifest optional and adding **store_kwargs. A reader takes that line as the API summary; it should read candidate_shards(store_root, manifest=None, aoi=None, window=None, **store_kwargs).
(2) "one extra metadata GET" is true of GETs and not of transports. With store= omitted the call now constructs 2 object stores on the arithmetic route and 3 on the walk route (1 and 2 before this change) — measured against tests/data/serc_hive by spying on open_object_store; on S3 each construction re-runs the ambient boto3 credential resolution. The GET claim as written is correct, but since the entry pitches the change on cost, either qualify it ("one extra GET — and one extra store construction; pass store= to share one handle") or land the one-handle fix noted on src/moczarr/open.py:307, after which the claim needs no qualifier.
There was a problem hiding this comment.
🤖 from Claude
Both folded in 65299eb.
(1) The signature line now matches the API this entry describes:
- New public `candidate_shards(store_root, manifest=None, aoi=None,
window=None, **store_kwargs)`
(2) Took the second of your two options — landed the one-handle fix (0d3b361), so the GET claim needs no qualifier and the transport count became a positive statement instead:
The GET is the whole extra cost: a call now resolves ONE object store up front and threads it through the manifest read, the envelope read and the discovery walk alike, so the transport count is one per call whichever route runs (it was 2-3, each re-running the ambient credential resolution).
The entry also picked up the manifest-validation change from the thread on src/moczarr/open.py:319: "A passed manifest is parse_manifest-validated like a fetched one, so the two spellings cannot disagree."
| "open_ragged", | ||
| "read_cell", | ||
| "read_ragged", | ||
| "read_tensors", |
There was a problem hiding this comment.
🤖 from Claude (review)
Item 4's asymmetry is not fully discharged: read_commits / read_leaf_metas are the batched leaf readers this PR's own docstring points callers at, and neither is on the root.
Phase 2 rewrote candidate_leaves' docstring in src/moczarr/open.py to read:
(issue #39) for readers that need the leaf roster without opening
anything — the paths are what ``moczarr.store.read_commits`` and
``moczarr.store.read_leaf_metas`` take.
Both are public-named, fully docstring'd issue #5 API in src/moczarr/store.py (read_leaf_metas at L273, read_commits at L306), and neither is in moczarr.__all__ — while the singular read_commit is. So the flow this PR documents reads exactly like the one item 4 calls out:
leaves = mz.candidate_leaves(root, aoi=q, anonymous=True)
stamps = moczarr.store.read_commits(root, leaves, anonymous=True) # module pathIt is a sharper case than open_ragged was, because the root exports the one-at-a-time function and withholds the batched sibling the docstring recommends over it — a reader who reaches for mz.read_commits finds mz.read_commit and writes the N-GET loop.
Concrete fix: export read_commits and read_leaf_metas from src/moczarr/__init__.py's moczarr.store block, add both to __all__, and list read_commits in WORKFLOW next to the other per-leaf readers. If they are meant to stay module-path-only, record that where __init__.py already records its one deliberate withholding (the moc_index comment at L90–92) and reword the candidate_leaves docstring so it does not read as a recommendation.
There was a problem hiding this comment.
🤖 from Claude
Folded in 47653e2. read_commits and read_leaf_metas are both exported from src/moczarr/__init__.py's moczarr.store block and added to __all__ (alphabetical, next to read_commit / read_leaf_coverage), and both are on the WORKFLOW roster under a new # batched leaf readers the candidate roster feeds (issue #5) group. The import graph is unchanged: moczarr.store was already imported at the root, so this adds two names, not a module.
The CHANGELOG bullet now names all three root-joining functions and states the failure the singular/batched split caused: "a reader reaching for the batched stamp read found only the singular one and wrote the N-GET loop". candidate_leaves' docstring keeps pointing at them and now reads as a reachable recommendation rather than a module-path detour.
| sat on the package root, so a notebook mixing the two spellings was forced | ||
| by the library, not chosen. The roster test pins the invariant so the | ||
| asymmetry cannot recur: a function that appears in a public workflow (the | ||
| quickstart, the demo-notebook selection loop, the per-leaf readers) must be |
There was a problem hiding this comment.
🤖 from Claude (review)
The roster's own stated sources already contain three names it omits, so the docstring rule is contradicted by the list that describes it.
This docstring derives WORKFLOW from "the quickstart, the demo-notebook selection loop, the per-leaf readers". Both notebooks, enumerated:
docs/examples/quickstart.ipynbcalls exactly three package functions —moczarr.open_hive,moczarr.parent_cells,moczarr.join_coarse.parent_cellsandjoin_coarseare not inWORKFLOW.- zagg
demo/07_minimal.ipynb(the notebook issue candidate_shards + store_kwargs passthrough + root-taking coverage_moc: remove the bookkeeping the demo notebook exposes #49 is written against, and the "demo-notebook selection loop" named here) callsmz.candidate_shards,mz.coverage_moc,mz.morton_decimal,mz.open_leaf,mz.open_ragged,mz.read_ragged,mz.read_tensors.morton_decimalis not inWORKFLOW.
All three are already on the root, so nothing fails today — which is the point: the roster is hand-maintained and its maintenance rule ("Additions to those flows belong here in the same PR that documents them", L16) is enforced by nothing at all. A list that is out of sync with its own stated derivation on the day it lands will not be in sync a year from now, and the test cannot fail for the bug it exists to catch — a new asymmetric function is invisible until someone remembers to type its name here.
Concrete fix, either:
(a) add join_coarse, parent_cells and morton_decimal so the list matches what it claims to be; or
(b) better — make the in-repo half mechanical, and keep WORKFLOW as the hand-curated supplement for the out-of-repo demo flow:
def test_quickstart_calls_only_package_level_names(self):
nb = json.loads((Path(__file__).parents[1] / "docs/examples/quickstart.ipynb").read_text())
src = "".join("".join(c["source"]) for c in nb["cells"])
for name in sorted(set(re.findall(r"\b(?:mz|moczarr)\.([A-Za-z_]\w*)", src))):
assert name in mz.__all__, f"quickstart calls mz.{name}, not on the package root"That enforces the stated rule for the one workflow source that lives in this repo, instead of only asserting it.
There was a problem hiding this comment.
🤖 from Claude
Option (a) folded in 5188049; option (b) declined, rationale below.
join_coarse, morton_decimal and parent_cells are now on the roster (all three were already exported, so the test stays green), and the docstring no longer claims a derivation it does not perform. It now says what is true: the roster is a hand-curated FLOOR, nothing enumerates the flows mechanically, the demo notebook driving issue #49 lives in another repo, and a name absent from the roster is unpinned rather than ruled out — adding a workflow means adding its names in the same PR. The module docstring got the same correction ("a function on the hand-kept WORKFLOW floor").
Declining the notebook-parsing test: \b(?:mz|moczarr)\.([A-Za-z_]\w*) over cell text does not match attribute access. It reads moczarr.store.read_commits as a call to moczarr.store and fails on a submodule that is correctly not in __all__; it matches names inside markdown cells, comments and strings; and it cannot see a call through an alias or a rebinding. Making it robust means parsing the code cells with ast and walking Attribute nodes off the two known aliases — more machinery than the invariant is worth, given docs/examples/quickstart.ipynb is re-executed by CI's notebook job, which fails on a genuinely missing name at the point of the call. I found no cheap variant that is not fragile, so the honest docstring is the fold rather than a test that would go red for the wrong reasons.
There was a problem hiding this comment.
🤖 from Claude
Correction to my reply above, checked rather than assumed: the quickstart re-execution job exists (.github/workflows/test.yml L40-63, jupyter nbconvert --execute) but is continue-on-error: true — non-blocking by design (phase 7b, issue #1). So it surfaces a missing name as a red step in the job list, it does not fail the PR. That weakens the "CI already covers it" half of the rationale; the parsing objection stands on its own, and the honest-docstring fold is what 5188049 lands. Whether that notebook job should become blocking is a CI-config call for you, not something this PR touches.
There was a problem hiding this comment.
🤖 from Claude
Follow-up ruling (espg, in-session 2026-08-24): the quickstart leg should gate (become blocking), conditional on hermeticity — and hermeticity is confirmed: the notebook runs offline against the in-tree tests/data/serc_hive fixture (its only "source.coop" match is a comment; no s3:// or http reads in any code cell). The change itself is one line — delete continue-on-error: true from the notebook job in test.yml — but this session cannot edit workflow files (permission deny-list, which an in-session ruling does not override), so that one-liner is left for espg or a permitted session. Recorded in the PR body under the resolved questions; the roster/docstring folds from the earlier replies on this thread (5188049) are unaffected.
| # sorted so additions land in one obvious place. | ||
| for name in mz.__all__: | ||
| assert getattr(mz, name, None) is not None, f"__all__ names unresolvable {name}" | ||
| assert list(mz.__all__) == sorted(mz.__all__) |
There was a problem hiding this comment.
🤖 from Claude (review)
This pins plain-str order, which is not the order ruff's own __all__ rule enforces — the constraint is one autofix away from a red test, and it duplicates a linter instead of enabling it.
__all__ is plain-sorted today (checked: list(moczarr.__all__) == sorted(moczarr.__all__) is True), so the assertion passes. But ruff ships RUF022 for exactly this invariant, and it uses isort-style natural order (SCREAMING_CASE, then CamelCase, then snake_case), which disagrees with sorted() on this file:
$ ruff check --isolated --select RUF022 src/moczarr/__init__.py
123 | "MORTON_CONVENTION_UUID",
124 + "RAGGED_SPEC",
125 + "ROOT_COVERAGE_NAME",
126 + "TEMPORAL_SPEC",
127 + "CompanionDeclaration",
- "RAGGED_SPEC",
...
Found 1 error. [*] 1 fixable with the `--fix` option.
lint.select = ["E", "F", "W", "I", "N"] in pyproject.toml, so RUF022 is off today and the two never meet. But anyone who enables RUF, or runs ruff check --fix --select RUF022 once, turns this green test red — and a contributor adding a name gets a bare assert list(...) == sorted(...) failure with no fix suggestion, where the linter would have autofixed the placement.
Concrete fix: add "RUF022" to lint.select and drop this line. The linter then is the single source, and it does what the comment on L50–51 actually asks for ("kept sorted so additions land in one obvious place") without anyone having to hand-place a name. If plain-str order is the deliberate choice, say so in that comment and name the divergence from RUF022, so the next person does not "fix" the file out from under the test.
There was a problem hiding this comment.
🤖 from Claude
Folded in a3a54b3, as the half of your fix that belongs to this PR: the assert list(mz.__all__) == sorted(mz.__all__) line is gone and the test is now test_all_names_resolve, keeping the no-dangling-names sweep intact. The comment records the divergence rather than leaving the next person to rediscover it — RUF022 owns ordering and sorts naturally (SCREAMING_CASE, CamelCase, snake_case), which disagrees with plain str order on this file, so a test pinning sorted() goes red the first time anyone runs the autofix.
Not folding the other half: adding "RUF022" to lint.select is a lint-config change for the repo, and per the house rule a side-effecting config decision is not mine to make on a review finding. Left standing for you — it is a one-line pyproject.toml edit plus one autofix pass whenever you want it, and nothing in this PR now conflicts with it.
There was a problem hiding this comment.
🤖 from Claude
Ruled (espg, in-session 2026-08-24): RUF022 enabled — lint.select now carries "RUF022" and ruff check --fix reordered __all__ to the natural sort in the same commit (6b1fb08), so the linter mechanically enforces what the dropped test asserted, exactly as this finding proposed. One enforcement caveat recorded for completeness: the PR lint bot (lint.yml) runs ruff with hardcoded --select=E,F,W,I, so RUF022 rides pre-commit / local ruff check via pyproject rather than the inline-comment bot.
| # and package-root callers share one seam. | ||
| from moczarr import ragged | ||
|
|
||
| assert mz.open_ragged is ragged.open_ragged |
There was a problem hiding this comment.
🤖 from Claude (review)
The one surface invariant stated in prose and pinned by nothing is absent from the file that exists to pin the surface.
src/moczarr/__init__.py L90–92 withholds a name from the root for a stated reason:
# moczarr.moc_index (MortonMocIndex) is imported by module path, not here:
# the package root stays xarray-import-free (the repo's lazy-import posture),
# and the index reaches most users through open_hive(index_kind="moc").src/moczarr/join.py L52 restates it (import xarray as xr # lazy: the package root stays xarray-import-free). grep -rn "sys.modules" tests/ src/ finds only test_open_leaf.py's boto3 monkeypatching — no test asserts it. It does hold today, and this phase does not disturb it (ragged.py's module-level imports are json/warnings/numpy/zarr only, and __init__ already imported moczarr.ragged, so open_ragged costs nothing):
$ python -c "import sys, moczarr; print('xarray' in sys.modules)"
False
But a roster test whose whole thesis is "a name a workflow uses must be reachable as mz.<name>" is exactly what invites the next contributor to add MortonMocIndex for symmetry, and the regression is silent — nothing goes red, the root just grows an xarray import. tests/test_coverage.py::TestPublicSurface::test_boundary_normalizers_stay_internal already sets the house precedent that the negative surface is pinned right next to the positive one.
Concrete fix — add to TestPackageSurface:
def test_the_root_stays_xarray_free(self):
# __init__.py L90-92's stated reason for withholding MortonMocIndex.
# Subprocess because a sibling test in this session has already
# imported xarray.
out = subprocess.run(
[sys.executable, "-c", "import sys, moczarr; print('xarray' in sys.modules)"],
capture_output=True, text=True, check=True,
)
assert out.stdout.strip() == "False"
assert "MortonMocIndex" not in mz.__all__There was a problem hiding this comment.
🤖 from Claude
Folded in 0a1fd9f — TestPackageSurface::test_the_root_stays_xarray_free spawns a clean interpreter (subprocess.run([sys.executable, "-c", "import sys, moczarr; assert 'xarray' not in sys.modules"], capture_output=True)), asserts returncode == 0 with the child's stderr in the failure message, and keeps your "MortonMocIndex" not in mz.__all__ assertion alongside it.
Checked where xarray actually enters so the comment is honest rather than restating the prose: the only module-level import xarray in the package is src/moczarr/moc_index.py L29-30 and src/moczarr/dggs.py L31, neither of which the root imports; every other site (join.py L52/95/172, open.py L601/1066, pyramid.py L339, dggs.py L360) is function-local. Negative control run: pointing the same probe at moczarr.moc_index exits 1, so the test fails for the regression it is written for. Full suite green, and the subprocess adds ~0.4s.
| from moczarr.store import load_root_coverage, read_manifest | ||
|
|
||
| fetched = load_root_coverage(envelope, store=store, **store_kwargs) | ||
| if fetched is None and read_manifest(envelope, store=store, **store_kwargs) is None: |
There was a problem hiding this comment.
🤖 from Claude (review)
Cross-phase: _root_form resolves two independent object stores on the absence path — the exact posture phase 2 was corrected out of, one phase earlier in this same PR.
Phase 2's fold (0d3b361) installed a one-handle rule at the top of _candidate_pairs, with the reasoning written out in src/moczarr/open.py:
# ONE handle for every read this call makes, resolved here rather than
# per callee (issue #5's "thread one root-rooted handle"; fetching the
# manifest when omitted is what made it bite). Otherwise read_manifest,
# the envelope and the walk each resolve their own — 2-3 constructions
# per call, each re-running the ambient boto3 credential resolution, so
# one call could read its three objects under two identities across an
# SSO refresh.
handle = _resolve_store(store_root, store, store_kwargs)The phase-3 manifest probe (folded in 0700bf8) reintroduces precisely that here. With store=None and **store_kwargs — which is the ruled spelling, coverage_moc(root, anonymous=True) — load_root_coverage resolves one object store and read_manifest resolves a second, so the sidecar and the manifest can be read under two identities across the same SSO refresh that comment describes. Not a rare path, either: coverage_toc(root, **S3) against any store publishing no temporal coverage takes it on every call.
Concrete fix, mirroring phase 2 (the lazy import is already local, so _resolve_store rides along):
from moczarr.store import _resolve_store, load_root_coverage, read_manifest
handle = _resolve_store(envelope, store, store_kwargs)
fetched = load_root_coverage(envelope, store=handle)
if fetched is None and read_manifest(envelope, store=handle) is None:
raise ValueError(f"no morton_hive.json at {envelope} — not a hive store root")
return fetchedThat makes the root form one construction per call, and lets both docstrings' "one metadata GET / one extra GET on the absence path" accounting describe transports as well as objects.
There was a problem hiding this comment.
🤖 from Claude
Folded in 8de4d4a, mirroring phase 2. _root_form's str branch now lazily imports _resolve_store alongside the two readers, resolves handle = _resolve_store(envelope, store, store_kwargs) at the top, and passes store=handle to both load_root_coverage and the manifest probe — one construction per call instead of one or two, with the reasoning written out in the same terms as open._candidate_pairs. Behavior is identical when the caller passed store=: _resolve_store returns the given handle untouched, and store_kwargs were already ignored in that shape. The FileNotFoundError for an unreachable local root now raises one frame earlier (from _resolve_store rather than from inside load_root_coverage) — same type, same message, and test_unreachable_store_raises_for_both still passes. The _root_form docstring picks up "on ONE object store resolved up front and threaded through every read this call makes".
Pinned with a new TestRootFormCasts::test_the_absence_path_constructs_one_store (tests/test_coverage.py): the two-object path — a hive root with no usable sidecar, so the manifest probe runs — counts exactly one open_object_store call. Negative control: stashing the coverage.py change makes it fail with 2 != 1. ruff check / ruff format --check clean, 922 passed / 55 skipped.
espg
added
the
waiting
| store is unchanged between the calls; a pair that must be tear-proof | ||
| against a concurrent writer takes one view and derives the other | ||
| through the convention seam (:func:`moczarr.convention.split_leaf_name` | ||
| on a path's stem, :func:`moczarr.convention.leaf_path` on an id) — |
There was a problem hiding this comment.
🤖 from Claude (review)
split_leaf_name on a path's stem raises — the seam needs the basename. convention.split_leaf_name hard-requires the suffix:
if not name.endswith(".zarr"):
raise ValueError(f"{name!r} is not a leaf zarr name")and a "stem" is exactly the part with the suffix removed. Following this docstring literally is an immediate ValueError:
>>> split_leaf_name(Path("1/1/2/1/3/11213.zarr").stem) # "11213"
ValueError: '11213' is not a leaf zarr name
>>> split_leaf_name(Path("1/1/2/1/3/11213.zarr").name)
('11213', None)
>>> split_leaf_name("11213_2020.zarr")
('11213', '2020')
(verified in this worktree at 30015e0 by loading src/moczarr/convention.py directly). The word "stem" is also load-bearing elsewhere in this same docstring — "the id is the path's stem the library knew before it built the path" — where it is correct and means the id, so the two uses now contradict each other: one says the stem is the id, the other says to hand the stem to a parser that rejects ids.
Concrete fix: name the basename, not the stem —
(:func:`moczarr.convention.split_leaf_name` on a path's basename,
i.e. split_leaf_name(path.rpartition("/")[2])[0], whose first element is the id with the window label dropped.
There was a problem hiding this comment.
🤖 from Claude
Accepted, fixed in 6cff86a. The three docstring findings (this one, the bare-leaf_path one, and the "views align" one) all land in the same closing paragraph of candidate_shards, so they are folded as ONE rewritten paragraph in that single commit.
On this finding specifically: "on a path's stem" is gone. The text now says the parser takes the path's basename and spells the call, so the derivation runs as written:
split_leaf_name(rel.rsplit("/", 1)[-1]) -> (id, window_label)
with an inline note that the .zarr suffix is included because that is what the parser requires, not the bare id. Confirmed in this worktree: split_leaf_name("11213_2020.zarr") -> ('11213', '2020'), while split_leaf_name("11213") raises '11213' is not a leaf zarr name. That also removes the vocabulary collision — "stem" now appears only in its earlier, correct stem-as-id sense.
uv run ruff check src tests, ruff format --check, and pytest -q (922 passed, 55 skipped) all green at 6cff86a.
| against a concurrent writer takes one view and derives the other | ||
| through the convention seam (:func:`moczarr.convention.split_leaf_name` | ||
| on a path's stem, :func:`moczarr.convention.leaf_path` on an id) — | ||
| library parsers, not caller-side string surgery — instead of issuing |
There was a problem hiding this comment.
🤖 from Claude (review)
The leaf_path-on-an-id direction is under-specified and silently wrong on exactly the two store shapes this docstring has just finished warning about. candidate_leaves builds its paths as
(int(w), leaf_path(int(w), window=window, path_grouping=grouping))(open.py _candidate_pairs, with grouping = manifest_path_grouping(manifest)), while leaf_path's signature defaults both away:
def leaf_path(shard: str | int, window: str | None = None, *, path_grouping: int = 1) -> str:So a caller who derives the path view with a bare leaf_path(shard):
- gets the unwindowed node name on a windowed (
HIVE_SPEC_V2) store — the paragraph two sentences up says "the id is the BARE shard — the window label is not part of it", which is precisely why the window has to be re-supplied here; - gets
path_grouping=1on a D21+ grouped store, i.e.1/1/2/1/3/11213.zarrwherecandidate_leavesreturned1/11/21/3/11213.zarr.
Neither failure raises; the derived list just stops naming the same objects as candidate_leaves, which defeats the whole point of the paragraph (a pair that agrees positionally).
Concrete fix: spell the two arguments in the sentence, e.g. :func:moczarr.convention.leaf_path on an id, threading the same ``window=`` and the manifest's ``path_grouping`` (:func:moczarr.convention.manifest_path_grouping).
There was a problem hiding this comment.
🤖 from Claude
Accepted, fixed in 6cff86a — same rewritten paragraph as the basename finding above (all three docstring findings live in one paragraph, so they share one commit).
The id-to-path direction now names both arguments explicitly, matching what _candidate_pairs actually builds:
leaf_path(shard, window=window, path_grouping=manifest_path_grouping(manifest))
and the text spells out the failure mode you identified — a bare leaf_path(shard) silently drops the window label and assumes path_grouping=1, so it stops naming the same objects candidate_leaves returned. Verified locally: leaf_path("11213", window="2020", path_grouping=manifest_path_grouping({"path_grouping": 2})) -> 1/12/13/11213_2020.zarr.
Green at 6cff86a (ruff check, ruff format --check, 922 passed / 55 skipped).
| the internal pair stays private until a consumer shows real friction): | ||
| make both calls share their inputs — fetch the manifest ONCE (or thread | ||
| one ``store=``) and pass it to both — and the views align whenever the | ||
| store is unchanged between the calls; a pair that must be tear-proof |
There was a problem hiding this comment.
🤖 from Claude (review)
"make both calls share their inputs … and the views align whenever the store is unchanged" reads as cause and effect, but sharing inputs buys nothing toward alignment. Each _candidate_pairs call still runs its own discovery even when manifest= is passed — the manifest fetch is the only thing the argument skips:
handle = _resolve_store(store_root, store, store_kwargs)
if manifest is None:
manifest = read_manifest(store_root, store=handle)
...
envelope = load_root_coverage(store_root, store=handle) # unconditional, per calland on the walk route it is a second full walk_leaves. So the tear window the paragraph opens with is untouched by the idiom: shared inputs save one GET, they do not snapshot anything. Threading one store= is weaker still — it shares a connection/credential identity (see _candidate_pairs' own comment), never a view of the store's contents.
The alignment clause is also already stated verbatim four sentences earlier ("for equal arguments against an unchanged store"), so as written the new sentence restates a pre-existing guarantee while implying the idiom produced it — the risk being a reader who passes one manifest and believes the pair is now tear-safe.
Concrete fix: split the two claims and say what sharing actually buys, e.g. "make both calls share their inputs — fetch the manifest ONCE (or thread one store=) and pass it to both: that saves the duplicate manifest GET and pins one path_grouping across the pair, but each call still re-reads the root envelope (or re-walks), so the alignment above is still only 'store unchanged between the calls', not a snapshot."
There was a problem hiding this comment.
🤖 from Claude
Accepted, fixed in 6cff86a — folded into the same rewritten paragraph as the two seam findings above (one paragraph, one commit).
The cause-and-effect reading is gone. The idiom sentence now states exactly what sharing buys and explicitly denies the snapshot:
Sharing buys exactly two things — the duplicate manifest GET is skipped, and ONE
path_groupingis pinned across the pair so both render the same path grammar. It snapshots nothing: each call still re-GETs the root envelope (or re-walks), so the "unchanged store" caveat above is untouched by the idiom.
That drops the redundant "views align whenever the store is unchanged" clause (the guarantee is already stated four sentences earlier and is left as the single statement of it), and it closes the misread you flagged — a reader threading one manifest can no longer conclude the pair became tear-safe.
Green at 6cff86a (ruff check, ruff format --check, 922 passed / 55 skipped).
| line-length = 100 | ||
| target-version = "py312" | ||
| lint.select = ["E", "F", "W", "I", "N"] | ||
| lint.select = ["E", "F", "W", "I", "N", "RUF022"] |
There was a problem hiding this comment.
🤖 from Claude (review)
This select does not reach CI, so the pair of commits is a net loss of enforcement for the __all__-ordering invariant rather than a move of it. The only ruff job in the repo overrides the config's select on the command line and cannot fail:
# .github/workflows/lint.yml
- uses: benny123tw/action-ruff@v1
with:
fail_level: none
ruff_flags: "--select=E,F,W,I --ignore=E501"A CLI --select replaces lint.select wholesale, so RUF022 (like the pre-existing N) is never evaluated there, and fail_level: none means the job is a review bot, not a gate. test.yml runs pytest only — and a3a54b3 removed the one assertion that was CI-gating:
- assert list(mz.__all__) == sorted(mz.__all__)Net: before this PR the ordering was enforced on every PR by pytest; after it, it is enforced only by pre-commit on a developer's machine (the hook does pick the config up — ruff-check passes no --select, and RUF022 is stable in the pinned v0.14.10, confirmed locally). Nothing in CI notices a hand-appended out-of-order name.
The rule itself is correct and clean — ruff check src tests and ruff format --check src tests both pass at 30015e0 on ruff 0.16.0, the reorder is exactly RUF022's isort-style order (SCREAMING_SNAKE, CamelCase, then the rest), and the roster survived it intact: 98 names before and after, none lost, none duplicated (AST-diffed 6b1fb08~1 against head).
Concrete fix, cheapest first: (1) add RUF (or RUF022) to ruff_flags in lint.yml — one word, though it still cannot fail with fail_level: none; (2) drop the --select/--ignore flags entirely so the bot honours this pyproject block, which is the single-source posture the config already implies; or (3) if the workflow is deliberately out of scope for this PR, say so in the PR body under "Questions for review" so the gap is a recorded decision rather than a silent regression.
There was a problem hiding this comment.
🤖 from Claude
Finding accepted as accurate; the code change is declined, and the gap is recorded instead.
Confirmed independently in this tree: lint.yml is the only ruff leg, it passes ruff_flags: "--select=E,F,W,I --ignore=E501" (a CLI --select replaces lint.select wholesale, so RUF022 — like the pre-existing N — is never evaluated) with fail_level: none, and no other workflow installs ruff. So the ordering invariant is indeed no longer PR-gated, where the removed pytest assertion was.
Why nothing changed in the diff:
.github/workflows/is out of scope and out of permission for this session — editing CI config is explicitly denied here, so the one-line fix cannot be made from this PR. Your option (1)/(2) is the right fix and is recorded verbatim for the maintainer: in.github/workflows/lint.yml, drop the hardcoded--select/--ignorefromruff_flagsso the bot honours the pyproject block (or minimally addRUF022to the select), and raisefail_levelabovenoneif it should actually gate rather than annotate.- Not reimplemented as a test. Re-asserting ruff's natural sort in pytest means hardcoding RUF022's ordering semantics (SCREAMING_SNAKE, then CamelCase, then the rest) in this repo, which is brittle across ruff versions — exactly the coupling
a3a54b3removed. A subprocessruff check --select RUF022guarded by skip-if-missing would not gate either, since the test legs install no ruff; it would be a no-op in CI and only duplicate pre-commit locally.
Taken your option (3): the PR body's RUF022 resolved-questions bullet now carries the enforcement gap, the evidence (hardcoded select + fail_level: none, no ruff in test.yml, the removed assertion), and the one-line workflow change — so this is a recorded decision for the maintainer rather than a silent regression.
|
🤖 from Claude The permission-blocked CI ruling is now landed: |
1 participant
Closes #49 — the four additive items from the issue body, per the implementation plan on the thread (plan comment). The acceptance bar is zagg's
demo/07_minimal.ipynbcoverage cell running verbatim:Phases
candidate_shards(). Sibling ofcandidate_leavesreturning shard ids ("11213"), sharing one implementation (_candidate_pairs, word+path per candidate) so the two views cannot disagree. Separate name rather than a flag because the return type changes (Moc.to_orderprecedent, Moc object: geometry-first coverage API (issue #196) mortie#197).candidate_leaveskeeps returning store-relative paths.**store_kwargspassthrough + optionalmanifeston both candidate functions; manifest fetched when omitted (documented one-GET posture mirroringopen_leaf(manifest=None));store=stays the share-a-handle path.coverage_moc/coverage_toc.strroot fetches and casts;dictenvelope keeps today's behavior byte-for-byte;coverage_toc's two temporal absences collapse toNone(docstring says so); an unreachable store still raises.open_ragged+ surface-consistency test pinning that public-workflow functions are reachable asmz.<name>.Approach notes
candidate_leavesbody moved into_candidate_pairs(...) -> list[(word, rel)]; both public functions are thin views over it. Both discovery routes (root-MOC arithmetic and the walk fallback) return pairs, so id↔path correspondence holds on either route by construction.open_leaftakes the samewindow=the selection did; the docstring pins this.Testing
TestCandidateShardsintests/test_open.py: positional id↔path correspondence (arithmetic and walk routes), ids opening throughopen_leafwith no string surgery, sub-shard AOI keeping whole shards, windowed ids staying bare.pytest891 passed / 54 skipped;ruff check+ruff format --checkclean.TestLiveNotebookAcceptance(env-gated,MOCZARR_LIVE_TESTS=1; the committed suite stays offline-by-posture) runs the notebook spelling verbatim againsts3://us-west-2.opendata.source.coop/englacial/zagg/demo/atl03_tdigest_o9.zarr(anonymous, metadata-only — no shard payloads):mz.coverage_moc(root, region="us-west-2", anonymous=True).contains(moc(yosemite_box))is True, andset(mz.candidate_shards(root, aoi=q, region=..., anonymous=True))returns the same 27-shard set as the pre-candidate_shards + store_kwargs passthrough + root-taking coverage_moc: remove the bookkeeping the demo notebook exposes #49 verbose form (open_object_store+read_manifest+candidate_leaves+ path parsing) — PASSED. The same behavior is pinned offline byTestRootFormCasts(tests/test_coverage.py) andTestCandidateConvenience(tests/test_open.py) on in-tree fixtures.Review + fold record
Each phase push was followed by a fresh-context adversarial self-review (inline comments prefixed
🤖 *from Claude (review)*) and a separate fold pass — 31 inline threads total (7 + 10 + 9 + 5), every finding folded as its ownfold review:commit and every thread replied. Nothing was declined outright; three sub-parts were left standing as decisions for review (below). Final head: 922 passed / 55 skipped locally, all CI legs green, live acceptance re-run PASSED at the final head.Questions for review — all four RESOLVED (espg rulings, 2026-08-24)
product=on the candidate functions — RESOLVED: no. One addressing mode (the product subtree root IS the store root); the pointed §6.5 error teaches the fix.open_leaf's kwarg is the asymmetry to question, not to propagate. No code change.lint.selectcarries"RUF022"and__all__is reordered to its natural sort (6b1fb08); the linter now mechanically enforces what the dropped test asserted. Enforcement gap, recorded for review (not fixed here —.github/workflows/is out of this PR's scope): no CI leg gates__all__ordering any more.lint.ymlrunsbenny123tw/action-ruffwith hardcodedruff_flags: "--select=E,F,W,I --ignore=E501"(a CLI--selectreplaceslint.selectwholesale, soRUF022— like the pre-existingN— is never evaluated) andfail_level: none, so it is a review bot, not a gate;test.ymlinstalls no ruff. Before this PR the ordering was PR-gated by the pytest assertiona3a54b3removed; after it, it is enforced by pre-commit / local ruff only. The one-liner that closes it: in.github/workflows/lint.yml, drop the hardcoded--select/--ignorefromruff_flagsso the bot honours this pyproject block (or minimally addRUF022to the select) — and raisefail_levelabovenoneif it should actually gate. Not reimplemented as a test: asserting ruff's natural-sort order in pytest is brittle across ruff versions.tests/data/serc_hivefixture (nos3:///http reads; the only "source.coop" match is a comment), so the ruling's blocking case applies. The change is deletingcontinue-on-error: truefrom thenotebookjob intest.yml— this session's permission settings deny workflow-file edits (managed deny-list; an in-session ruling doesn't override it), so that single line awaits espg or a permitted session.(word, path)pair view — RESOLVED: stays private. No consumer yet; revisit on first real friction. The tear-free idiom is documented oncandidate_shardsinstead (30015e0): share the inputs (one fetched manifest or onestore=) for aligned views against an unchanged store; derive a strictly tear-proof pair from one view viasplit_leaf_name/leaf_path.