h5coro-hidefix floor 0.3.1 -> 0.3.2 for the io_stats kwarg (issue #374) - #436
Conversation
|
🤖 from Claude Status: one phase landed, CI red on a single self-inflicted test, and the last step needs a ruling. Marking Where CI stands
That is this PR's own doing and it is the correct alarm: raising the $PIP install "h5coro==1.0.5" "h5coro-hidefix==0.3.1" "$MORTIE_SPEC" --no-deps ...means the layer would ship 0.3.1 against a floor of 0.3.2 — the exact class of gap the parity test was added to catch. Why the fix is not in this PRThe one-line sync lives under The ruling I need — pick one
Recommendation: (1). The pin it contradicts already carries a Why this matters beyond the parity testThe merge-CI So the Also flagged, not touchedTwo lint findings exist on this branch but are pre-existing on
Neither gates CI, because the PR lint bot runs Generated by Claude Code |
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Adversarial pass on fd0e6f4, fresh context, reviewed against CLAUDE.md and the PR's own stated goal.
The version work is sound; the framing and the scope are not. Verified independently: 0.3.2 is on PyPI (2026-08-12T07:40Z, abi3 manylinux_2_28 wheels for x86_64 and aarch64, ~2.2 MB each, matching the lambda comment), the upstream tag exists, espg/h5coro-hidefix#7 is merged (2026-08-11) and confirms 0.3.1 raised TypeError on every sidecar group read, and the 0.3.2 installed in this checkout accepts io_stats. Bumping the core floor (not only the lambda pin) is the correct lever, since worker.py:436 forwards the kwarg to every backend with no gate. Rewriting the comment block was overdue — the old text claimed "Floor is 0.2.0" beside a >=0.3.1 pin.
Five findings, in priority order:
pyproject.toml:37— the diff does not fix the thing it is motivated by. The merge benchmark rebuilds the layer frombuild_layer.shon every run (lambda-benchmark.yml:116-122) andbuild_function.sh:50shipssrc/zaggbycp -rwith no resolver, so no deployed path reads this floor. The target stays empty and red at the next merge, not merely "before the next layer rebuild".pyproject.toml:98— this line breaks an explicitly-enforced invariant and cannot be split off.TestLayerExtraParity::test_every_lambda_extra_pin_is_in_build_layerfails at this commit (reproduced locally;test (3.13)red,test (3.12)cancelled). Not a pre-existing failure under §4 — this change causes it. Landing only the floor would instead makezagg[lambda]unresolvable, so the three lines are atomic and the §1/§7 conflict needed raising before the half-commit. The unlock is an espg-filed issue namingdeployment/aws/build_layer.sh.pyproject.toml:36— the rewrite orphansbuild_layer.sh:82-89, which cites this block as the worked example of "the floor is ahead of what PyPI has published"; that state is no longer documented anywhere. (TheL30-38range itself still lines up — the content vanished, not the numbers.)pyproject.toml:34— issue #374 is closed and is a telemetry issue, so it is fine as kwarg provenance but cannot serve as this PR's originating issue: the body has noCloses/Refsline (§2) and no labeled work item stands behind the change (§5) — which is also what leaves thebuild_layer.shline unauthorized under §1.pyproject.toml:33— the asserted contract is untested (§4). The zagg side is pinned (tests/test_index.py:335) but nothing asserts the external backend acceptsio_stats; theinspect.signaturecheck already run by hand is three lines inside the existingimportorskipblock attests/test_lambda_build.py:66-73. A floor-versus-pin consistency check belongs next toTestLayerExtraParityfor the same reason.
Process note tied to (2): "How tested" reports pytest -k "sidecar or index_backend". §4 asks for the full pytest -v (plus ruff format --check, or pre-commit run --all-files); the narrow selection is why a self-inflicted red test reached CI unmentioned. While the answer on (2) is outstanding the PR is red by construction — the body should say so up front, and the PR should sit waiting rather than reading as merely "one small question left".
Generated by Claude Code
| # unconditionally (issue #374); 0.3.1 raises TypeError on every sidecar | ||
| # group read, which is what emptied the tdigest_healpix_o9_hive_sidecar | ||
| # benchmark target on merge CI (espg/h5coro-hidefix#7). | ||
| "h5coro-hidefix>=0.3.2", |
There was a problem hiding this comment.
🤖 from Claude (review)
The floor bump does not fix the red benchmark job, and the PR body understates that. The version claim itself checks out — I verified 0.3.2 is on PyPI (uploaded 2026-08-12T07:40Z; abi3 manylinux_2_28 wheels for x86_64 and aarch64), tag 0.3.2 exists upstream, espg/h5coro-hidefix#7 is merged (2026-08-11) and its own description says "on zagg main every sidecar-path group read raises TypeError", and the 0.3.2 in this checkout's .venv does accept the kwarg (site-packages/h5coro_hidefix/zagg_backend.py:437-447, io_stats=None). The core floor (not just the lambda pin) is the right lever for library consumers, since src/zagg/processing/worker.py:436 forwards it to every backend with no gate:
read_kwargs = {"arrow": use_arrow, "io_stats": io_stats}What is not true is that this diff repairs the failing target. Nothing in the deployed path ever reads this floor:
.github/workflows/lambda-benchmark.yml:116-122rebuilds the layer from source on every run (chmod +x build_layer.sh && ./build_layer.sh arm64), so the benchmark's runtimeh5coro-hidefixis whateverdeployment/aws/build_layer.sh:97pins — still"h5coro-hidefix==0.3.1".deployment/aws/build_function.sh:50ships zagg bycp -r "$REPO_ROOT/src/zagg", i.e. no dist metadata and no resolver, so the floor is not enforced at build or import time either.
So the PR body's "it needs its own PR before the next layer rebuild" is optimistic in a way that matters: the next layer rebuild is the next merge-CI benchmark run, and the target stays empty and red until build_layer.sh:97 moves. Please say that plainly in the body rather than as a footnote — as landed, this PR changes nothing observable and turns test (3.13) red (see the note on the lambda pin below).
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Corrected in the PR body — no code change, since the line itself is right and the defect was the framing.
The body now carries a "What this PR does NOT fix" section that states plainly what the old footnote understated: no deployed path reads a pyproject floor, so the benchmark target stays red at the next merge, not merely "before the next layer rebuild". Both of your references are cited there:
.github/workflows/lambda-benchmark.yml:116-122rebuilds the layer from source on every merge run (./build_layer.sh arm64), so the runtimeh5coro-hidefixis whateverdeployment/aws/build_layer.sh:97pins — still"h5coro-hidefix==0.3.1".deployment/aws/build_function.sh:50ships zagg bycp -r "$REPO_ROOT/src/zagg"— no dist metadata, no resolver — so the floor is not enforced at build or import time either.
The body now says outright that as landed this PR changes nothing observable in the fleet, and that the next layer rebuild is the next merge-CI benchmark run. Kept the distinction you drew between the two levers: the core floor still matters for library consumers installing from PyPI (worker.py:436 forwards io_stats to every backend ungated), it just is not the fleet fix.
The test (3.13) half of your note is the lambda-pin thread, answered there — left standing, since fixing it requires deployment/aws/build_layer.sh.
Generated by Claude Code
| # Keep this pin in sync with deployment/aws/build_layer.sh (abi3 | ||
| # manylinux_2_28 wheels cover both layer arches, ~2.2 MB each). | ||
| "h5coro-hidefix==0.3.1", | ||
| "h5coro-hidefix==0.3.2", |
There was a problem hiding this comment.
🤖 from Claude (review)
This line is the one that breaks the tree, and it cannot be split off cleanly — which means the §1 conflict had to be raised before committing, not after. Reproduced locally at fd0e6f4:
tests/test_lambda_build.py::TestLayerExtraParity::test_every_lambda_extra_pin_is_in_build_layer
E AssertionError: lambda-extra pins absent from deployment/aws/build_layer.sh:
['h5coro-hidefix==0.3.2'] (the layer would ship without them — see issue #218's async-tiff gap)
1 failed, 31 passed
That test exists precisely to stop this state (tests/test_lambda_build.py:573-579: "The script's comments say 'keep the pin in sync with the lambda extra', but nothing enforced it"). It is not a pre-existing failure this PR may flag and move past under §4 — it is caused by this line. Note also that build / build-arm64 and build / build-x86_64 are green: those gate size, not correctness, so a layer carrying 0.3.1 under a zagg that requires 0.3.2 passes them, and the runtime symptom is silent (worker.py:443-449 catches the per-group TypeError, increments group_errors, and the shard reports "No data after filtering").
Two consequences for how the PR should be framed:
- The comment two lines up is now false in the tree. "Keep this pin in sync with
deployment/aws/build_layer.sh" reads as an invariant, and the diff deliberately violates it without saying so at the site. - Splitting is not a clean escape. Landing only the core floor and leaving
==0.3.1here would makezagg[lambda]unresolvable (core>=0.3.2versus extra==0.3.1) — nothing in the suite catches that, so it would be worse. The two lines plusbuild_layer.sh:97are one atomic change.
Given (2), the §1 read ("nothing under deployment/aws/ unless the work names it") is defensible in letter but leaves the PR red-by-construction with no reachable green state. CLAUDE.md's preamble covers exactly this: when §1 collides with §7's "keep them in sync with the layer", stop and raise the conflict rather than land half of it. The unlock is cheap and belongs to espg: an issue that names deployment/aws/build_layer.sh satisfies §1's "explicitly asks for it by name", after which all three lines land together. Concretely, please offer that as option (1) in "Questions for review", with option (2) being "hold the whole bump until that issue exists" — and until one is chosen, the PR should carry waiting and the body should state up front that CI is red by design.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Left standing by design — this one is not mine to fix, and I am not reverting the line.
Fixing the parity failure requires editing deployment/aws/build_layer.sh:97. §1 fences everything under deployment/aws/ "unless the issue/PR you are working explicitly asks for it by name", and no issue names it — so the edit stays out of this PR regardless of how cheap it is.
Confirming your resolution point: "revert the extra to ==0.3.1, keep the core floor at >=0.3.2" is not a valid option. I verified the unresolvability rather than asserting it. Directly, on the two specs:
$ uv pip compile # h5coro-hidefix>=0.3.2 + h5coro-hidefix==0.3.1
× No solution found when resolving dependencies:
╰─▶ Because you require h5coro-hidefix>=0.3.2 and h5coro-hidefix==0.3.1, we
can conclude that your requirements are unsatisfiable.
and in the actual split state, applied to the tree and then reverted:
× No solution found when resolving dependencies for split:
╰─▶ Because zagg[lambda] depends on h5coro-hidefix==0.3.1 and your project
depends on h5coro-hidefix>=0.3.2, we can conclude that your project and
zagg[lambda] are incompatible.
And because your project requires zagg[lambda], we can conclude that
your project's requirements are unsatisfiable.
An extra carries the base requirements too, so the specs intersect to nothing. Worth noting it is worse than you described: in that state uv run cannot build the environment at all, so pytest never starts and the whole suite is unrunnable — strictly worse than one honest red test. The three lines are atomic, as you said.
Two things folded in response, both inside my remit:
- The atomicity is now tested, so this trap cannot be walked into silently again —
TestLayerExtraParity::test_every_lambda_extra_pin_satisfies_its_core_floorin 23e2ec9 (see the line 33 thread). - The PR body now leads with "CI is red by design" and offers your two options concretely, as (1a) open/label an issue naming
deployment/aws/build_layer.sh(after which all three lines land here together) and (1b) hold the whole bump until that issue exists. The PR already carrieswaiting, which is the correct state while that decision is outstanding.
On the process point — that the §1/§7 conflict should have been raised before committing rather than after — noted, and not relitigated. The body now states the conflict up front instead of as a footnote. Opening or labeling that issue is a side-effecting action reserved to espg (§6), so I have not done it.
Your point (1), that the "keep this pin in sync with deployment/aws/build_layer.sh" comment two lines up is now false in the tree: I have deliberately not softened it. It is a true invariant that this PR violates, and weakening the comment would hide the violation that the failing test and the body now both announce. It becomes true again the moment build_layer.sh:97 moves under option (1a).
Generated by Claude Code
| # SidecarIndex.read_group accepts the io_stats kwarg the worker forwards | ||
| # unconditionally (issue #374); 0.3.1 raises TypeError on every sidecar | ||
| # group read, which is what emptied the tdigest_healpix_o9_hive_sidecar | ||
| # benchmark target on merge CI (espg/h5coro-hidefix#7). |
There was a problem hiding this comment.
🤖 from Claude (review)
The rewrite orphans a cross-reference in build_layer.sh that depends on the old text. Rewriting the block was right — the old version claimed "Floor is 0.2.0" while the pin next to it read >=0.3.1, so it was stale in the worst way. But deployment/aws/build_layer.sh:82-89 justifies the MORTIE_SPEC derivation by pointing at this block as the worked example:
# --no-deps means pip never resolves the runtime floor declared there ...
# Passing the declared spec (a) makes pip error and abort under `set -e`
# when the floor is ahead of what PyPI has published
# (pyproject L30-38 documents that state for h5coro-hidefix), and (b) ...
"That state" is the floor-ahead-of-PyPI situation the deleted lines described ("0.2.0 is not on PyPI yet — resolution fails until upstream publishes it"). The new block documents no such state, so that parenthetical now points at a comment that does not support it, and the one concrete illustration of why the derivation exists is gone from the tree.
One correction to the record, since it would be easy to over-claim here: the line range is still right. The block occupied L31-36 with the pin at L37 before this commit and occupies exactly the same lines after (git show HEAD~1:pyproject.toml versus the tree — the rewrite is six comment lines for six), so L30-38 remains as accurate/loose as it always was. The defect is the vanished content, not the numbers.
Suggested fix, entirely inside this file: keep one sentence of the old mechanism, e.g. a trailing line such as "a floor ahead of what PyPI has published makes resolution (and the layer build's set -e pip step) fail loudly — the state build_layer.sh's MORTIE_SPEC derivation cites." If you would rather not carry it here, then the stale cross-reference is a second item for the build_layer.sh follow-up and should be listed under "Questions for review" alongside line 97 — right now the PR mentions only the pin.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Fixed in e12d108 — took your suggested route (entirely inside pyproject.toml, no build_layer.sh edit), so this does not become a second item for the follow-up.
The block now closes with the mechanism the rewrite dropped:
# group read, which is what emptied the tdigest_healpix_o9_hive_sidecar
# benchmark target on merge CI (espg/h5coro-hidefix#7). A floor ahead of
# what PyPI has published (as 0.2.0 once was) fails resolution loudly — the
# state build_layer.sh's MORTIE_SPEC derivation cites this block for.Two things I checked so the restore actually lands where the cross-reference points:
- The restored sentence sits at L36-38, inside the
L30-38rangebuild_layer.sh:82-89cites. The pin moves to L39, but the citation is "pyproject L30-38 documents that state" — it needs to cover where the state is documented, not the pin, so the parenthetical is now true again rather than approximately true. - Kept it to three lines and one sentence. The 0.2.0 history is carried as the worked example ("as 0.2.0 once was") rather than re-narrated, since the floor-ahead-of-PyPI mechanism is what the derivation cites, not that particular release.
Thanks for the correction on the line numbers — I did not touch them on the basis that they had drifted, only to keep the restored sentence inside the cited range.
Generated by Claude Code
| "h5coro-hidefix>=0.3.1", | ||
| # espg sign-off on the #149 thread). Floor is 0.3.2: the first release whose | ||
| # SidecarIndex.read_group accepts the io_stats kwarg the worker forwards | ||
| # unconditionally (issue #374); 0.3.1 raises TypeError on every sidecar |
There was a problem hiding this comment.
🤖 from Claude (review)
The (issue #374) citation is accurate here but is doing double duty as the PR's authorization, which it cannot. As provenance for the kwarg it is correct — issue #374 is where io_stats/obs_read came from, and the tree cites it the same way throughout (src/zagg/processing/read.py:327, src/zagg/index/__init__.py:101, src/zagg/index/hierarchical.py:42). No change needed to this line.
The problem is upstream of the comment: issue #374 is closed (state_reason: completed, closed 2026-08-04 by PR #378, label small-fix), and it is a telemetry-counter issue, not a dependency issue. So:
- The PR body has no
Closes #N/Refs #Nline at all — §2 requires one ("a link to the originating issue"), and prose "(issue point path: record observations READ (pre-filter) alongside n_obs in the stats record #374)" is not that. It also cannot beCloses #374, which is already closed. - There is no open, labeled work item behind this PR. §5 drives work from labeled issues, and a closed
small-fixis not actionable, so as it stands nothing authorizes the change — including, per §1, touchingdeployment/aws/build_layer.sh, which is the blocker the body asks about.
Recommend: add Refs #374 to the body for provenance, and ask espg to open/label the follow-up covering the floor bump and deployment/aws/build_layer.sh:97 by name. That single request resolves the §2 gap, the §5 gap, and the §1 blocker in one pass, which is a better ask than the open-ended "say the word" currently in "Questions for review" (§6 wants concrete options).
Also worth aligning: the commit subject reads ... for the io_stats kwarg (issue #374), which implies this commit works #374. Terseness is fine per §3 and it matches recent history, but if a follow-up issue gets filed, key the subject to that number instead.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Agreed on all three points; folded into the PR body, no code change (as you said, this line needs none).
The body now opens with:
Refs #374, explicitly scoped to kwarg provenance only — noting that point path: record observations READ (pre-filter) alongside n_obs in the stats record #374 is closed (state_reason: completed, closed 2026-08-04 by small fixes: event-transport pacing window, point-path observations-read counter #378, labelsmall-fix), that it is a telemetry-counter issue, and thatCloses #374would therefore be wrong. Cited alongside the three sites that reference it the same way (read.py:327,index/__init__.py:101,index/hierarchical.py:42).- A plain statement that no labeled work item authorizes this PR, tying the §2 gap to the §5 gap and to the §1 blocker: with no issue naming
deployment/aws/build_layer.sh, the edit that would make CI green is out of bounds. - Your recommended ask, as concrete options replacing the open-ended "say the word": (1a) open/label a follow-up naming the floor bump and
deployment/aws/build_layer.sh:97, after which all three lines land here together; (1b) hold the whole bump until that issue exists.
Opening or labeling that issue is a side-effecting action reserved to espg (§6), so I have raised it rather than done it; the PR keeps waiting while the choice is outstanding.
On the commit subject implying this commit works #374 — noted, and left as is: rewriting the landed subject would need a force-push, which §1 forbids outright. If option (1a) produces an issue number, subsequent commits key to that number instead, and the two fold commits on this branch are already keyed to the PR rather than to #374.
Generated by Claude Code
| # upstream publishes it. | ||
| "h5coro-hidefix>=0.3.1", | ||
| # espg sign-off on the #149 thread). Floor is 0.3.2: the first release whose | ||
| # SidecarIndex.read_group accepts the io_stats kwarg the worker forwards |
There was a problem hiding this comment.
🤖 from Claude (review)
No test pins the contract this line asserts (§4), and the check you ran by hand is three lines from being permanent. The claim is that SidecarIndex.read_group accepts io_stats. What the suite covers today:
tests/test_index.py:335(test_io_stats_forwarded_unconditionally) pins the zagg side —HierarchicalIndexforwards the kwarg ungated. Nothing asserts the external backend accepts it.tests/test_lambda_build.py::TestLayerExtraParitypins layer/extra pin parity — about the layer, not the kwarg — and it is the test this PR turns red.
So the only gate that reacts to this change is the one that now fails for an unrelated reason, and the behavioral claim itself is untested. The PR body says you already verified it interactively ("inspect.signature → io_stats present"); tests/test_lambda_build.py:66-73 already does pytest.importorskip("h5coro_hidefix"), so that assertion belongs there:
from h5coro_hidefix.zagg_backend import SidecarIndex
assert "io_stats" in inspect.signature(SidecarIndex.read_group).parametersThat skips where the compiled wheel is absent and fails loudly wherever a below-floor hidefix is installed — exactly the drift this PR is cleaning up after.
A second gap in the same family, worth adding next to TestLayerExtraParity: nothing asserts that each lambda exact pin satisfies the core floor for the same distribution. Had that existed, a floor bump could not silently outrun its pin (and it is the check that would have caught the split-the-change trap noted on line 98).
One question that is espg's call, not this PR's scope: because the failure mode is a caught per-group TypeError reported as "No data after filtering", a below-floor backend degrades silently in the fleet where pyproject floors are never enforced (build_function.sh:50 copies src/zagg, no dist metadata). Is a version assertion at sidecar-backend selection wanted, or should a shard whose every group read raised fail loudly instead of reporting empty? Worth raising rather than deciding here.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Both gaps fixed in 23e2ec9 — the hand-run check is now permanent, and the floor/pin pair is now guarded.
1. The kwarg contract, placed exactly where you pointed (inside the existing importorskip at tests/test_lambda_build.py, now TestLambdaImports::test_h5coro_hidefix_available):
pytest.importorskip("h5coro_hidefix")
from h5coro_hidefix.zagg_backend import SidecarIndex
assert "io_stats" in inspect.signature(SidecarIndex.read_group).parametersSkips where the compiled wheel is absent, fails loudly on a below-floor hidefix. I also refreshed that test's docstring, which still justified the guard with "the pinned 0.2.0 is not on PyPI" — now stated as the general floor-ahead-of-PyPI case, plus why the assertion exists (the failure is silent, so nothing else would report it).
2. Floor-vs-pin consistency, added next to the parity test as TestLayerExtraParity::test_every_lambda_extra_pin_satisfies_its_core_floor: every exact lambda pin must satisfy the [project.dependencies] floor for the same distribution. Reads pyproject.toml with tomllib like its neighbours, orders versions numerically via a small _release helper (so 0.3.10 > 0.3.2 rather than lexically), and skips unpinned entries and layer-only dists with no core floor. It ends with assert "h5coro-hidefix" in checked so a naming or spec drift that matched nothing cannot leave it vacuously green.
Verified it actually catches the trap rather than merely passing: setting the extra back to ==0.3.1 against the >=0.3.2 floor makes the check fail with
lambda extra pins h5coro-hidefix==0.3.1, below the [project.dependencies]
floor >=0.3.2 — zagg[lambda] would be unresolvable
and it currently covers six pairs on this tree (numpy, pandas, arro3-core, h5coro, h5coro-hidefix, async-tiff), all green. Worth recording from that experiment: in the split state uv run cannot resolve the project environment at all, so pytest never starts — details on the line 98 thread.
Local run: tests/test_lambda_build.py + tests/test_index.py → 1 failed, 120 passed, 2 skipped. The single failure is test_every_lambda_extra_pin_is_in_build_layer, left standing per that thread.
Your last paragraph is out of scope here and left for espg, as you framed it: whether a below-floor backend should be caught by a version assertion at sidecar-backend selection, or whether a shard whose every group read raised should fail loudly instead of reporting "No data after filtering" (worker.py:443-449). It is now recorded as item (3) under "Questions for review" so the decision is not lost — it wants its own issue, and touching that catch block is a behavior change this dependency bump should not smuggle in.
Generated by Claude Code
|
🤖 from Claude Correction to the options above — option (3) is not viable, and option (1) is now the only complete fix. The adversarial self-review (#pullrequestreview-4915867117) turned up two things that change the decision, so read this in place of the earlier list. Option (3) "revert the extra to The floor bump alone fixes less than I implied. I wrote that the layer ships 0.3.1 "until that line moves … before the next layer rebuild." That understates it in a way that matters for scheduling:
Together those mean no deployed path ever reads a pyproject floor. So That collapses the choice to two, and the three affected lines are atomic:
Still recommending (1), now more strongly: under (2) nothing about the red benchmark improves until a second PR merges. One further item the review raised that is yours, not mine — I am not touching it: Also for the record, per §2/§5: this PR has no originating labeled issue. It cites issue #374 for kwarg provenance only — that issue is closed, was delivered by PR #378, and is about the point-path Generated by Claude Code |
…374) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qt8tabP1t5ymdTscKDSNH2
|
🤖 from Claude Resolved — espg ruled in-session (2026-08-16), explicitly naming Landed in c68aa5b:
With the extra already at Clearing |
| continue | ||
| if f'"{pin}"' not in script: | ||
| missing.append(pin) | ||
| if f"$(lambda_pin {name})" not in script: |
There was a problem hiding this comment.
🤖 from Claude (review)
The parity test no longer proves a derived pin reaches a $PIP install line — it now only proves the assignment exists, which re-opens the exact issue #218 gap the class docstring says it pins.
The old check was if f'"{pin}"' not in script ("async-tiff==0.7.2"), and that literal only ever appeared on the install line — so deleting it from the install failed the test. The new check anchors on NAME_PIN=$(lambda_pin name), which lives in a block at build_layer.sh:38-46, structurally disconnected from every $PIP install. A pin can now be derived and never installed.
Measured, mutating the real files (build_layer.sh line 135):
# old test (c68aa5b3^), async-tiff removed from its install line:
$PIP install "async-tiff==0.7.2" obspec -> $PIP install obspec
missing == ['h5coro-hidefix==0.3.2', 'async-tiff==0.7.2'] # CAUGHT
# new test (c68aa5b3), same deletion, assignment left in place:
$PIP install "$ASYNC_TIFF_PIN" obspec -> $PIP install obspec
missing == [] literal == [] # BOTH PASS
test_lambda_pin_derivation_resolves_every_exact_pin does not cover it either — it executes the snippet against pyproject and never looks at the install lines. So the state that shipped a 0.27.0 layer whose mode="process_raster" worker died on No module named 'async_tiff' is green again under both new tests.
All nine *_PIN vars are used on install lines today (70/74/76, 94, 95, 120, 135), so this is a latent guard weakening, not a live break. Suggest additionally asserting the derived variable is consumed, e.g. assert f'"${VAR}"' in script for the var the assignment binds (recoverable from the same regex), or scoping the search to the $PIP install continuation lines.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 5ae1641 — the test now proves each derived pin actually reaches pip, not just that the assignment exists.
Two changes in test_every_lambda_extra_pin_is_in_build_layer:
- The name → shell-var mapping is parsed from the script, not hard-coded, so a renamed var cannot silently drop its check:
pin_vars = {
name: var
for var, name in re.findall(
r"^([A-Z0-9_]+_PIN)=\$\(lambda_pin (.+)\)$", script, re.MULTILINE
)
}- A new
_install_lines()helper joins backslash continuations and keeps only$PIP installinvocations; each derived var must appear as"$VAR"in that text. Continuations matter —build_layer.sh:93-96carries five of the nine pins on continuation lines, so a per-physical-line scan would have missed them.
Verified against your exact mutation, on the real files:
# build_layer.sh:135, $PIP install "$ASYNC_TIFF_PIN" obspec -> $PIP install obspec
AssertionError: lambda-extra pins derived but never installed:
['async-tiff ($ASYNC_TIFF_PIN)'] — the layer ships without them (issue #218).
Pass each as "$NAME_PIN" on a $PIP install line.
and on a continuation-line mutation too (dropping "$XARRAY_PIN" from line 95) — also caught. Script restored after both; bash -n deployment/aws/build_layer.sh clean.
Your framing that this is a latent guard weakening rather than a live break matches what I see: all nine *_PIN vars are consumed on install lines today (70/74/76, 94, 95, 120, 135), and the parity class is green on this tree.
| missing.append(pin) | ||
| if f"$(lambda_pin {name})" not in script: | ||
| missing.append(name) | ||
| if re.search(rf'"{re.escape(name)}==', script): |
There was a problem hiding this comment.
🤖 from Claude (review)
The no-literal-pins guard only sees double-quoted literals; the two other forms build_layer.sh already uses for install args slip through.
The pattern rf'"{re.escape(name)}==' requires a leading ". But the script installs plenty of args bare (fastparquet cramjam, shapely pyproj odc-geo affine cachetools, obspec), so an unquoted literal is the likelier reintroduction, not the rarer one. Measured against the real files, keeping the $(lambda_pin async-tiff) assignment in place so missing stays empty:
$PIP install async-tiff==0.7.2 obspec -> missing == [] literal == [] # PASS
$PIP install 'async-tiff==0.7.2' obspec -> missing == [] literal == [] # PASS
$PIP install "async-tiff==0.7.2" obspec -> literal == ['async-tiff'] # caught
So pyproject.toml:93 ("tests/test_lambda_build.py fails on any literal pin reintroduced there") and this test's own assertion message overstate what is enforced: a second declaration site reintroduced without double quotes drifts silently, which is the failure mode the whole PR exists to close.
Dropping the leading " and requiring a version-ish tail covers all three forms — e.g. rf"(?<![\w.-]){re.escape(name)}==\d" — and I verified it produces no false positive on the current script (in particular it does not match the "==" in d inside the lambda_pin snippet at build_layer.sh:35).
There was a problem hiding this comment.
🤖 from Claude
Fixed in f99be02 — took your pattern verbatim:
# Unquoted and single-quoted args are as common as double-quoted
# ones here (`fastparquet cramjam`, `obspec`), so anchor on a name
# boundary + a version-ish tail instead of a leading quote.
if re.search(rf"(?<![\w.-]){re.escape(name)}==\d", script):Verified all three forms are now caught, injecting the literal next to a live $(lambda_pin async-tiff) assignment so missing/uninstalled stay empty and the literal assertion is the one that fires:
$PIP install "$ASYNC_TIFF_PIN" async-tiff==0.7.2 obspec -> literal == ['async-tiff'] # caught
$PIP install "$ASYNC_TIFF_PIN" 'async-tiff==0.7.2' obspec -> literal == ['async-tiff'] # caught
$PIP install "$ASYNC_TIFF_PIN" "async-tiff==0.7.2" obspec -> literal == ['async-tiff'] # caught
No false positive on the unmutated script (the "==" in d inside the lambda_pin snippet at build_layer.sh:35 needs a name prefix and a digit tail, so it does not match) — the parity class is 4 passed.
Also corrected the two places that overstated the old check, so the docstrings now match what is enforced: the test docstring and the _DERIVED comment said no literal "name==x.y.z" may exist, now "in any quoting". pyproject.toml:93 ("fails on any literal pin reintroduced there") needed no edit — with this pattern it is true as written rather than only for double-quoted literals.
| # pyproject.toml (PR #436, extending the issue-322 mortie pattern below): one | ||
| # edit site, and a pin that drifts from the extra cannot exist. The | ||
| # assignments run before any install and fail the build loudly under `set -e` | ||
| # when the extra stops carrying an exact pin for a requested name. |
There was a problem hiding this comment.
🤖 from Claude (review)
This commit inverts the layer-pin contract but leaves deployment/LAMBDA_DEPLOYMENT.md — the doc that calls build_layer.sh "the normative build entry point" — describing the contract it replaced, including a quoted phrase that no longer exists in the script.
Two passages still assert co-ownership:
deployment/LAMBDA_DEPLOYMENT.md:36-38— "built bybuild_layer.sh… its pins are co-owned with thelambdaextra inpyproject.toml, and mortie's version spec is read frompyproject.tomlat build time — issue Move the mortie decimal-parse boundary off the private _decimal_to_word #322". After this commit every exact pin is read at build time, not just mortie's; nothing is co-owned.deployment/LAMBDA_DEPLOYMENT.md:99-101— "Its pins are co-owned with thelambdaextra inpyproject.toml— the script says "keep in sync" at each one — and mortie's spec is read out ofpyproject.tomldirectly (issue Move the mortie decimal-parse boundary off the private _decimal_to_word #322), so a floor bump there reaches the layer with no second edit."
That bolded clause is now false by construction: this commit deleted every such comment.
$ grep -rn 'keep in sync\|Keep the pin\|Keep the pins' deployment/aws/build_layer.sh
(no output)
The practical trap: a maintainer following the normative doc goes looking for a literal pin in the script to edit alongside the extra, finds none, and the doc gives them no hint that lambda_pin exists or that the extra is now the sole edit site — while test_every_lambda_extra_pin_is_in_build_layer will fail them if they add one back. LAMBDA_DEPLOYMENT.md:141-144 already cites ../../pyproject.toml being read for MORTIE_SPEC in the SELinux note, so that read is now nine-plus reads earlier in the script and worth a word there too.
(Anchored to the new comment block since the doc is outside this commit's diff.)
There was a problem hiding this comment.
🤖 from Claude
Fixed in 613b1e2 — deployment/LAMBDA_DEPLOYMENT.md now describes the derivation instead of the contract it replaced. Scope note first: this file is under deployment/, which §1 fences; espg's in-session authorization (2026-08-16) covering the build_layer.sh single-source change extends to keeping its own normative deployment doc accurate, so the edit is in bounds. No other file under deployment/ was touched beyond the script this commit series already covers.
Three passages updated:
:36-38— "its pins are co-owned with thelambdaextra … and mortie's version spec is read frompyproject.toml" → "it holds no pins of its own: every exact pin is read out of thelambdaextra inpyproject.tomlat build time by itslambda_pinhelper, as mortie's version spec already was — issue Move the mortie decimal-parse boundary off the private _decimal_to_word #322, PR h5coro-hidefix floor 0.3.1 -> 0.3.2 for the io_stats kwarg (issue #374) #436".:99-104— the "co-owned … the script says "keep in sync" at each one" paragraph is replaced by the actual mechanic and the guard, which is what a maintainer looking for a literal pin to edit needs:
It declares no versions itself: the `lambda` extra in `pyproject.toml` is the
sole edit site, and the script derives every exact pin from it at build time
(PR #436) with the `lambda_pin` helper — `NAME_PIN=$(lambda_pin <dist>)` before
any install — extending the mortie spec read that issue #322 introduced. So a
version change in the extra reaches the layer with no second edit, and a
*literal* pin reintroduced into the script is a second declaration site that
`tests/test_lambda_build.py::TestLayerExtraParity` fails on (as it does on a pin
derived but never passed to a `$PIP install` line).
:144-148(the SELinux note you flagged) — the failure point is no longer "reading../../pyproject.tomlforMORTIE_SPEC" but "the first../../pyproject.tomlread — since PR h5coro-hidefix floor 0.3.1 -> 0.3.2 for the io_stats kwarg (issue #374) #436 that is everylambda_pincall, before any install, not justMORTIE_SPEC".
Your grep is the reason the second passage could not just be trimmed: grep -rn 'keep in sync\|Keep the pin' deployment/aws/build_layer.sh is empty on this tree, so the bolded clause was false by construction and the doc had to name lambda_pin to be usable.
|
🤖 from Claude CI red on the merge commit is not this PR's code: this was the first zagg CI run to resolve mortie 0.9.8 (released today), whose authalic latitude default changes every morton cell id — so Fix in flight on |
…-0.3.2 # Conflicts: # pyproject.toml
2 participants
Bumps the
h5coro-hidefixfloor and thelambdaextra pin to 0.3.2, the release whoseSidecarIndex.read_groupaccepts theio_statskwarg the worker forwards unconditionally.Refs #374 — cited for kwarg provenance only. #374 is where
io_stats/obs_readcame from, and the tree cites it that way throughout (src/zagg/processing/read.py:327,src/zagg/index/__init__.py:101,src/zagg/index/hierarchical.py:42). It is closed (state_reason: completed, closed 2026-08-04 by #378, labelsmall-fix) and it is a telemetry-counter issue, so it is not the originating issue for this dependency bump andCloses #374would be wrong.No labeled work item authorizes this PR. §5 drives work from labeled issues, and there is no open, labeled issue behind this bump — which also means §1 has nothing that "explicitly asks for it by name" to permit the
deployment/aws/build_layer.shedit the change actually needs. That gap is a standing question for review, item (2) below.Why
Merge CI's benchmark job has been red since #374 landed: every sidecar-path group read raises
TypeError: SidecarIndex.read_group() got an unexpected keyword argument 'io_stats', so thetdigest_healpix_o9_hive_sidecartarget reads nothing and the job fails on empty metrics.Upstream fix: espg/h5coro-hidefix#7 (merged), released as 0.3.2. Its
read_grouptakesio_stats=Noneand forwards the sink into zagg's own shared read routes, so the sidecar path'sobs_readaccounting is zagg's semantics byte-for-byte (upstream pins it equal to the hierarchical baseline on the real ATL03 fixture).What this PR does NOT fix
No deployed path reads a pyproject floor, so the benchmark target stays red at the next merge — not merely "before the next layer rebuild". Stating that plainly, since the earlier wording understated it:
.github/workflows/lambda-benchmark.yml:116-122rebuilds the layer from source on every merge run (chmod +x build_layer.sh && ./build_layer.sh arm64), so the benchmark's runtimeh5coro-hidefixis whateverdeployment/aws/build_layer.sh:97pins — still"h5coro-hidefix==0.3.1".deployment/aws/build_function.sh:50ships zagg bycp -r "$REPO_ROOT/src/zagg"— no dist metadata, no resolver — so the floor is not enforced at build time or import time either.The next layer rebuild therefore is the next merge-CI benchmark run. As landed, this PR changes nothing observable in the fleet, and
tdigest_healpix_o9_hive_sidecarstays empty and red untilbuild_layer.sh:97moves. The library-consumer floor is still the right lever for anyone installing zagg from PyPI, which is why the core bump is here — but it is not the fleet fix.Changes
pyproject.tomlcore dependency:h5coro-hidefix>=0.3.1→>=0.3.2, with the stale comment block refreshed (it still described the 0.2.0-not-on-PyPI situation) to state the actual reason for the floor. The rewrite also keeps the "a floor ahead of what PyPI has published fails resolution loudly" note, becausedeployment/aws/build_layer.sh:82-89cites this block (pyproject L30-38) as the worked example justifying itsMORTIE_SPECderivation; the restored sentence sits at L36-38, inside that cited range.pyproject.tomllambdaextra:h5coro-hidefix==0.3.1→==0.3.2.tests/test_lambda_build.py: two contract tests the bump was missing.TestLambdaImports::test_h5coro_hidefix_availablenow assertsio_statsis ininspect.signature(SidecarIndex.read_group).parameters, inside the existingpytest.importorskip("h5coro_hidefix")guard — skips where the compiled wheel is absent, fails loudly wherever a below-floor hidefix is installed.TestLayerExtraParity::test_every_lambda_extra_pin_satisfies_its_core_floorasserts every exactlambdapin satisfies the[project.dependencies]floor for the same distribution. Nothing compared those two before.How tested
uv sync --extra testresolves 0.3.2; verified in-env thatSidecarIndex.read_groupacceptsio_stats— now pinned by a test rather than checked by hand.uv run ruff check src tests: 1 pre-existing finding only (src/zagg/registry.py:64N818), in a file this branch does not touch; verified present at the base commit671834c. Left alone per §4.uv run ruff format --check src tests: 1 pre-existing reformat only (tests/data/benchmark/README.md), likewise untouched here and present at671834c.uv run pytest tests/test_lambda_build.py tests/test_index.py -q: 1 failed, 120 passed, 2 skipped. The one failure isTestLayerExtraParity::test_every_lambda_extra_pin_is_in_build_layer, caused by this PR and left standing deliberately — see item (1).Questions for review
CI is red by design, and the fix is not mine to make.
deployment/aws/build_layer.sh:97hardcodesh5coro-hidefix==0.3.1, sotest_every_lambda_extra_pin_is_in_build_layerfails on this branch. CLAUDE.md §1 fences everything underdeployment/aws/unless the work names it explicitly, and per the note above nothing does. Splitting the change is not an escape: core floor>=0.3.2against extra==0.3.1makeszagg[lambda]unresolvable, verified locally —in that state
uv runcannot even build the environment, so the suite does not run at all. The two pyproject lines andbuild_layer.sh:97are one atomic change. Two concrete options:deployment/aws/build_layer.sh— that satisfies §1's "explicitly asks for it by name", after which all three lines land together in this PR and CI goes green.Only espg can open or label that issue (§6 side-effecting directives), so this PR carries
waitinguntil one is chosen.No open, labeled work item stands behind this PR (§2 originating-issue requirement, §5 label-driven work). If option (1a) is taken, that issue would cover both the floor bump and
build_layer.sh:97by name and resolve this gap at the same time.Silent degradation, raised not decided — out of scope for this PR. The reason this broke quietly is
src/zagg/processing/worker.py:443-449: the per-groupTypeErroris caught,group_errorsis incremented, and the shard reports "No data after filtering". Becausebuild_function.sh:50copiessrc/zaggwith no dist metadata, pyproject floors are never enforced in the fleet, so a below-floor backend will always degrade silently rather than fail. Is a version assertion wanted at sidecar-backend selection, or should a shard whose every group read raised fail loudly instead of reporting empty? That is espg's call and would be its own issue.