Validate WorkerClient batch coordinates before image access - #165
Merged
Conversation
arjunrajlab
marked this pull request as ready for review
August 7, 2026 10:57
This was referenced Aug 7, 2026
arjunrajlab
added a commit
that referenced
this pull request
Aug 7, 2026
Resolves three conflicts between this branch's dataset-aware `all` batch parsing and master's batch-coordinate validation (#165) / select-field validation (#168). worker_client.py: master added `_parse_batch_values`, which reported malformed Batch input with sendError before the clients were built. This branch needs the dataset's IndexRange to expand `all`, so parsing has to happen after datasetClient exists. Folded master's per-field error message into the shared `get_batch_ranges()` so it raises a ValueError naming the offending field, and WorkerClient catches it, sends the banner, and re-raises. `_parse_batch_values` is gone; the direct SAM batching loops now inherit the clear message too, which they never had. CELLPOSESAM.md / CROP.md: kept master's more precise wording (1-indexed, empty-selection error, size-one dimension note) and added this branch's `all` documentation alongside it rather than replacing either. Also mirrored the branch's skill updates into .claude/skills/, which it had only applied to .agents/, and fixed the duplicate "### 7" the textual auto-merge produced in the hardening catalog. Corrected the stale sweep counts there (claimed ~6 WorkerClient / ~26 direct; actually 43 / 10). Added two regression tests for the merged behavior: get_batch_ranges names the field that failed to parse, and returns re-iterable lists rather than one-shot generators. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Summary
IndexRangebefore image accesssendErrorinstead of leaking parser exceptionsRoot cause
Batch fields are entered as 1-indexed positions and converted to zero-based coordinates. The production value
Batch Z: "0-34"therefore became[-1, ..., 33].WorkerClientpassed-1directly tocoordinatesToFrameIndex, where the dataset map raisedKeyError: -1without first sending a useful worker error.The earlier
Failed to get docker networkhostname-resolution message was non-fatal: the container started, detected CUDA, and reached image loading before the coordinate failure.Impact
Invalid batch coordinates now fail before frame lookup, with a message such as:
Batch Z contains invalid position 0. Batch positions start at 1; ... valid range is 1-35.Cellpose-SAM performs this validation before constructing the large model, and all workers usingWorkerClient.process()inherit the shared guard.Validation
pytest -q worker_client/tests— 19 passedpytest -q annotation_utilities/tests— 61 passedpytest -q workers/annotations/cellposesam/tests— 13 passedpython -m py_compilefor the changed Python modulesgit diff --check./build_workers.sh --build-and-run-tests cellposesamsuccessfully rebuilt the shared base images but did not run a worker test becausecellposesam/cellposesam_testare absent from Compose; the script nevertheless returned zero. That pre-existing build-runner issue is documented as TODO-005 in this PR.