fix: handle frames that omit index keys for size-one dimensions - #159
Merged
Conversation
Girder omits an index key from every entry of tiles['frames'] when a
dataset has a single position along that dimension. A single-channel
dataset's frames are {'Channel': 'Default', 'Frame': 3, 'Index': 3,
'IndexT': 3} -- no 'IndexC' at all -- so `frame['IndexC']` raises
KeyError on a perfectly valid dataset. Time lapse registration hit this
in production after ~20 minutes of successful work.
Four workers had the identical crash on any single-channel dataset:
registration, gaussian_blur, histogram_matching and rolling_ball. Each
one was reproduced against its pre-fix image before being changed.
Add two helpers to annotation_utilities.annotation_tools:
- get_frame_index(frame, dimension, default=0) treats an absent
dimension as coordinate 0 and raises ValueError on an unknown
dimension name, so a typo cannot silently read as channel 0.
- frame_to_large_image_params(frame) replaces the addTile keyword
comprehension that was copy-pasted into seven workers (eight call
sites). It keeps the original pass-through predicate rather than
whitelisting the four known axes: dropping an unusual axis would
collide frames in the sink, which is worse than an addTile error.
Also report batch selections that match nothing, instead of failing
silently:
- registration: an "Apply to XY coordinates" range intersecting no
existing position previously raised KeyError on the matrix lookup.
- crop: an out-of-range range either wrote and uploaded an *empty*
image (for a dimension the dataset has) or was ignored outright and
cropped everything (for a size-one dimension, absent from frames).
crop's per-frame filter now checks coordinate 0 for absent dimensions so
it behaves the same whether or not a dimension exists.
deconwolf, crop and h_and_e_deconvolution were not crashing -- crop
guards with `in`, h_and_e requires 3 channels, deconwolf already used
.get(..., 0) -- and are moved onto the shared helpers to remove the
duplication.
Tests: 22 new (12 helper unit tests, 10 worker tests), every one
confirmed failing before the fix. annotation_utilities 30 passed,
worker_client 12 passed, and all seven affected worker Docker suites
pass: registration 23, gaussian_blur 17, rolling_ball 19,
histogram_matching 18, crop 17, h_and_e_deconvolution 9, deconwolf 38.
Deploy note: image-processing-base carries the new shared helpers, so it
must be pushed before these workers.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S2kq6wZENNiD1eYUG7CXUh
…gle-dimension # Conflicts: # annotation_utilities/annotation_utilities/annotation_tools.py # workers/annotations/gaussian_blur/tests/test_gaussian_blur.py # workers/annotations/histogram_matching/entrypoint.py # workers/annotations/histogram_matching/tests/test_histogram_matching.py # workers/annotations/rolling_ball/tests/test_rolling_ball.py
Addresses review feedback on #159. get_selected_channels validates the *shape* of a channelCheckboxes value but not its range, so a saved config selecting channel 1 parses cleanly when it is run against a single-channel dataset and then matches no frame. The worker processed nothing, uploaded a byte-identical copy of its input, and reported success — the same silent no-op this PR already fixed for crop's XY/Z/Time ranges and registration's "Apply to XY coordinates", left in place for the channel dimension. New shared helper annotation_tools.split_channel_selection(selected, num_channels) partitions a selection against the dataset's real channel count. Five workers that filter frames by channel now report the result: an error when nothing selected exists, a warning naming the missing indices when only part of the selection is unusable. An *empty* selection still means "process nothing" where that was already deliberate (gaussian_blur, rolling_ball), so the helper reports nothing missing for it. In deconwolf the check also prevents an IndexError in parse_wavelengths, and it runs before any PSF is built. Also from the review: - drop the duplicated annotation_tools import in gaussian_blur, rolling_ball - correct the "front end sends {...} or [1, 2]" comment in all five workers: get_selected_channels rejects the list shape rather than accepting it - cover h_and_e_deconvolution's frame handling, which had no test asserting that only Index* keys reach addTile - reword CROP.md, whose new size-one bullet described a path the emptiness check now reports before any frame is examined - use call.kwargs rather than call[1] consistently in the new tests - catalog the failure mode in nimbus-worker-hardening, including which get_selected_channels callers are covered and which remain unaudited Tests: 12 helper unit tests, 10 worker tests, every one confirmed failing against the pre-fix entrypoints. annotation_utilities 61, worker_client 12, gaussian_blur 20, rolling_ball 22, histogram_matching 21, registration 26, crop 17, deconwolf 42, h_and_e_deconvolution 10 — all pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DGVianXJET5VVja76jahvj
Collaborator
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
Time lapse registration crashed in production after ~20 minutes of successful work:
Girder omits an index key from every entry of
tiles['frames']when a dataset has a single position along that dimension. The failing dataset's frames were{'Channel': 'Default', 'Frame': 3, 'Index': 3, 'IndexT': 3}— noIndexCwhatsoever. This is the per-frame twin of theIndexRangeguard the repo already had, and it was missed because the existing sweep greps all key onIndexRange, which this form never mentions.Four workers had the identical crash on any single-channel dataset —
registration:408,gaussian_blur:151,histogram_matching:132,rolling_ball:136. Each was reproduced against its pre-fix image (mounting the new test file over/testsin the old image) before being touched.Shared helpers
Two idioms were copy-pasted across seven workers; both now live in
annotation_utilities.annotation_tools:get_frame_index(frame, dimension, default=0)— treats an absent dimension as coordinate 0, and raisesValueErroron an unknown dimension name so a typo like'Channel'fails loudly instead of silently reading as channel 0.frame_to_large_image_params(frame)— replaces the{f'{k.lower()[5:]}': v ... len(k) > 5}addTilecomprehension (7 workers, 8 call sites). It deliberately keeps the original pass-through predicate rather than whitelisting the four known axes: a whitelist would silently drop an unusual axis and collide frames in the sink, which is worse than theaddTileerror it would prevent.Silent-failure fixes
Three selection paths failed without telling anyone:
registration — an
Apply to XY coordinatesrange intersecting no existing position wouldKeyErroron the matrix lookup. Now sends an error naming the request and the dataset's actual position count.crop — an out-of-range range either wrote and uploaded an empty image (for a dimension the dataset has) or was ignored outright and cropped everything (for a size-one dimension, absent from frames). Both now report. Its per-frame filter also checks coordinate 0 for absent dimensions, so it behaves identically whether or not a dimension exists.
the channel dimension, in five workers (added after review) —
get_selected_channelsvalidates the shape of achannelCheckboxesvalue but not its range, so a saved config selecting channel 1 parses cleanly against a single-channel dataset and then matches no frame. Each worker processed nothing, uploaded a byte-identical copy of its input, and reported success. New shared helpersplit_channel_selection(selected, num_channels)partitions the selection against the dataset's real channel count;gaussian_blur,rolling_ball,histogram_matching,registrationanddeconwolfnow error when nothing selected exists and warn, naming the missing indices, when only part of the selection is unusable.An empty selection stays a separate case:
gaussian_blurandrolling_balldeliberately still write an unprocessed copy when the user deselects everything, so the helper reports nothing missing for it. Indeconwolfthe check also prevents anIndexErrorinparse_wavelengths, and it runs before any PSF is built; inhistogram_matchingmissing channels are dropped before the reference images are collected.cellposesam,cellposesam_trainandsample_interfacealso callget_selected_channelsbut use it differently (per-slot merge inputs, and a demo worker) — they are untouched and noted as unaudited in the skill catalog.Not crashing, just deduplicated
deconwolf,cropandh_and_e_deconvolutionwere never broken —cropguards within,h_and_erequires 3 channels,deconwolfalready used.get(..., 0). They carried the duplicated comprehension and are moved onto the shared helper.Testing
44 new tests (24 helper unit tests, 20 worker tests), every one confirmed failing before its fix — each was run against the pre-fix
entrypoint.pymounted into the old image.annotation_utilitiesworker_clientThe
h_and_e_deconvolutionaddition covers a gap rather than a bug: it had no test asserting that onlyIndex*keys reachaddTile, so its move ontoframe_to_large_image_paramswas previously unverified. Note that CI does not run these — the Dockertestjob intest-workers.ymlis commented out as too expensive — so this local run is their only pre-merge coverage. The package suites do run in CI'spackage-testsjob.Deploy note
image-processing-basecarries the new shared helpers, so it must be pushed before the six workers that inherit from it —crop,gaussian_blur,histogram_matching,h_and_e_deconvolution,registration,rolling_ball— or they willAttributeErroron the new helper names.deconwolfis not affected: itpip installsannotation_utilitiesfrom the repo in its own Dockerfile, so its own rebuild picks the helpers up.Docs
REGISTRATION.mdandCROP.mdgained sections on size-one dimensions and the new error behaviour (CROP.md's "values outside the range are silently ignored" line was now wrong, and its replacement was reworded again after review — the emptiness check reports before any frame is examined, so the size-one exclusion it described is unreachable).GAUSSIAN_BLUR.md,ROLLING_BALL.md,HISTOGRAM_MATCHING.mdandDECONWOLF.mddocument the size-one note plus the new channel-range error and warning. Thenimbus-worker-hardeningskill catalog gained this per-frame variant, its sweep greps, and a warning that frame-index bugs are invisible on multi-channel fixtures — a test frame needs noIndexCkey, notIndexC: 0.REGISTRY.mdneeds no change: no workers added, removed, or renamed.Post-review cleanup
Merged
master(PR #162'schannelCheckboxeswork touched the same five files; all conflicts were both-sides-appended and resolved by keeping both). Then, from review:annotation_toolsimport ingaussian_blurandrolling_ball{...}or[1, 2]" comment in all five workers —get_selected_channelsrejects the list shape rather than accepting it (the comment arrived with fix(channelCheckboxes): report malformed values instead of crashing #162)call.kwargsinstead ofcall[1]consistently in the new testsOne pre-existing nit was left alone as unrelated drift: four SAM workers have a duplicated
from shapely.geometry import Polygon. No files in this PR.🤖 Generated with Claude Code
https://claude.ai/code/session_01S2kq6wZENNiD1eYUG7CXUh