Skip to content

Report actionable error for out-of-range batch coordinates#143

Open
arjunrajlab wants to merge 7 commits into
masterfrom
fix-batch-coordinate-out-of-range
Open

Report actionable error for out-of-range batch coordinates#143
arjunrajlab wants to merge 7 commits into
masterfrom
fix-batch-coordinate-out-of-range

Conversation

@arjunrajlab

Copy link
Copy Markdown
Collaborator

Summary

Workers on the WorkerClient.process() path crashed with a bare KeyError
from coordinatesToFrameIndex (self.map[channel][T][Z][XY]) when a user
entered a Batch XY/Z/Time range containing coordinates not present in the
dataset (e.g. Batch XY: "80-90" on a single-XY dataset). The user saw only a
raw stack trace.

Now the worker reports an actionable error and stops cleanly:

Batch XY out of range — Positions 80-90 do not exist

What changed

  • New pure validator annotation_utilities/coordinate_validation.py:
    find_out_of_range(index_range, xys, zs, times) and
    format_out_of_range_message(invalid). Messages are rendered 1-indexed to
    match the UI's Batch fields (the parser converts 1→0 on input). Missing
    IndexRange/sub-keys default a dimension's size to 1 (existing convention).
  • WorkerClient: new public validate_coordinates(...) (strict — any
    out-of-range coordinate → sendError + raise ValueError), called at the top
    of process() before the loop. Batch ranges are materialized to lists in
    __init__ (also closes a latent generator double-consumption hazard). Stack-aware:
    a dimension the worker stacks is validated against the current (valid) tile,
    so only batched dimensions are really checked.
  • cellposesam: one-line early validate_coordinates() before the GPU model
    loads, so it fails instantly instead of after a multi-second model load.

This covers the 6 workers on the WorkerClient.process() path (cellposesam,
cellpose, condensatenet, laplacian_of_gaussian, random_squares,
sample_interface). The ~26 workers that parse batch ranges directly are deferred
to TODO-002 (todo/out-of-range-coordinate-validation-rollout.md), with a
full file/line map. Low-level hardening of coordinatesToFrameIndex itself is
tracked in arjunrajlaboratory/NimbusImage#1185.

Design spec: docs/superpowers/specs/2026-05-30-out-of-range-coordinate-validation-design.md

Note: the REGISTRY.md commit is unrelated pre-existing description drift,
regenerated only to satisfy the repo's pre-PR doc-sync hook.

Test Plan

  • annotation_utilities: pytest tests — 17 passing (pure validator: index
    math, defaults, negatives, dedup, 1-indexed/singular/plural/contiguous
    message formatting, multi-dimension)
  • worker_client: pytest tests — 11 passing (out-of-range XY/Z through
    process() raises + sendError and never enters the loop; multi-dimension
    combined message; in-range proceeds; validate_coordinates direct;
    stacked-dimension skip)
  • Reported scenario verified: Batch XY "80-90" on IndexXY=1
    "Batch XY out of range" / "Positions 80-90 do not exist"
  • Reviewer: confirm strict-error policy (vs. filter-and-warn used by some
    property workers) is the desired UX for batch annotation workers

🤖 Generated with Claude Code

arjunrajlab and others added 6 commits May 30, 2026 15:00
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Workers using WorkerClient.process() crashed with a bare KeyError from
coordinatesToFrameIndex when a user's Batch XY/Z/Time range included
coordinates not present in the dataset (e.g. Batch XY '80-90' on a
single-XY dataset). The user saw only a stack trace.

Add a pure validator in annotation_utilities.coordinate_validation that
checks requested coordinates against the dataset's IndexRange and renders
a short, 1-indexed message matching the UI Batch fields. Wire it into a
new WorkerClient.validate_coordinates() that process() calls before the
loop (strict: any out-of-range coordinate -> sendError + raise). Call it
early in cellposesam.compute() so it fails before the GPU model loads.

Covers the 6 workers on the WorkerClient.process() path. The ~26 workers
that parse batch ranges directly can adopt the shared helper later; the
low-level coordinatesToFrameIndex hardening is tracked in
arjunrajlaboratory/NimbusImage#1185.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Closes a coverage gap noted in code review: process()-level validation
was only exercised via Batch XY. Add integration tests for an
out-of-range Batch Z and for the combined multi-dimension error message.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Triggered by the internal-only cellposesam entrypoint edit. The diff is
pre-existing description drift between REGISTRY.md and current worker
interfaces; it is unrelated to the coordinate-validation fix and is
included only to satisfy the repo's doc-sync hooks.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0895756c87

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

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".


# Fail fast with an actionable error if the requested Batch XY/Z/Time
# coordinates are out of range, before loading the (slow) Cellpose-SAM model.
worker.validate_coordinates()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Install the matching worker_client before calling validate_coordinates

When building the Cellpose-SAM image from its Dockerfile, /entrypoint.py is copied from this checkout but worker_client is installed from a fresh GitHub clone of ImageAnalysisProject (workers/annotations/cellposesam/Dockerfile lines 53-57). In PR or branch builds where that clone does not yet contain this commit, this new call raises AttributeError for every compute request before any validation or model loading can happen. Install worker_client from the build context, or otherwise guard this call until the image is guaranteed to contain the matching package version.

Useful? React with 👍 / 👎.

…kewed worker_client

The cellposesam Dockerfile installs worker_client from a fresh master
clone while entrypoint.py is copied from the build context. A pre-merge
branch build therefore pairs the new entrypoint with an older
worker_client lacking validate_coordinates(), raising AttributeError on
every compute request. Guard the early call with hasattr; process() still
performs the same validation once worker_client is current.

Addresses Codex review (P2) on PR #143.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant