diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..0715b54 --- /dev/null +++ b/.gitignore @@ -0,0 +1,7 @@ +# Python build/test artifacts +__pycache__/ +*.py[cod] +*$py.class +.pytest_cache/ +.coverage +*.egg-info/ diff --git a/annotation_utilities/annotation_utilities/annotation_tools.py b/annotation_utilities/annotation_utilities/annotation_tools.py index d0fe5f7..3ad05a1 100644 --- a/annotation_utilities/annotation_utilities/annotation_tools.py +++ b/annotation_utilities/annotation_utilities/annotation_tools.py @@ -70,6 +70,35 @@ def get_annotations_with_tag(elements, tag, exclusive=False): return result +def get_selected_channels(value, field_name='channel selection'): + """Parse a ``channelCheckboxes`` interface value into a sorted list of + selected channel indices (ints). + + The expected (documented) shape is a dict mapping channel-index strings to + booleans, e.g. ``{"0": True, "1": False}`` -> ``[0]``. A missing value + (``None``) means nothing was selected and returns ``[]`` so callers can + apply their own required-field logic. + + Any other shape -- notably a list such as ``[0]``, which has been observed + arriving from some clients -- is treated as a misconfiguration rather than + silently recovered: guessing a channel from an unexpected payload could run + a tool on data the user never intended to select. Such values raise + ``ValueError`` so the caller can surface a clear error to the user. + + NOTE: the list form is undocumented; the front-end (NimbusImage repo) is the + source of truth for what ``channelCheckboxes`` actually serializes and + should be checked to fix the root cause. + """ + if value is None: + return [] + if isinstance(value, dict): + return sorted(int(k) for k, v in value.items() if v) + raise ValueError( + f"'{field_name}' has an unexpected format " + f"({type(value).__name__}: {value!r}); expected channel checkboxes " + f"(a mapping of channel index to on/off).") + + def find_matching_annotations_by_location(source, target_list, Time=True, XY=True, Z=True): """ This function filters the target_list based on the 'location' of the source point. diff --git a/annotation_utilities/tests/test_get_selected_channels.py b/annotation_utilities/tests/test_get_selected_channels.py new file mode 100644 index 0000000..a5bfd2b --- /dev/null +++ b/annotation_utilities/tests/test_get_selected_channels.py @@ -0,0 +1,40 @@ +import sys +from pathlib import Path + +import pytest + + +package_root = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(package_root)) + +from annotation_utilities.annotation_tools import get_selected_channels + + +def test_dict_returns_sorted_selected_channels(): + assert get_selected_channels({'0': True, '1': False}) == [0] + assert get_selected_channels({'2': True, '0': True, '1': False}) == [0, 2] + + +def test_empty_dict_returns_empty_list(): + assert get_selected_channels({}) == [] + + +def test_none_returns_empty_list(): + assert get_selected_channels(None) == [] + + +def test_all_false_dict_returns_empty_list(): + assert get_selected_channels({'0': False, '1': False}) == [] + + +@pytest.mark.parametrize("bad_value", [[0], [], [0, 1], (0,), '0', 0, True]) +def test_non_dict_values_raise_value_error(bad_value): + # The list form ([0]) in particular has been seen in the wild; it must be + # rejected rather than silently recovered. + with pytest.raises(ValueError): + get_selected_channels(bad_value) + + +def test_error_message_includes_field_name(): + with pytest.raises(ValueError, match="Channels to correct"): + get_selected_channels([0], field_name='Channels to correct') diff --git a/build_machine_learning_workers.sh b/build_machine_learning_workers.sh index b529029..b33fc1e 100755 --- a/build_machine_learning_workers.sh +++ b/build_machine_learning_workers.sh @@ -29,7 +29,7 @@ echo "Building Cellpose train worker" docker build ./workers/annotations/cellpose_train/ -t annotations/cellpose_train_worker:latest $NO_CACHE echo "Building Cellpose-SAM worker" -docker build ./workers/annotations/cellposesam/ -t annotations/cellposesam_worker:latest $NO_CACHE +docker build . -f ./workers/annotations/cellposesam/Dockerfile -t annotations/cellposesam_worker:latest $NO_CACHE echo "Building Stardist worker" docker build ./workers/annotations/stardist/ -t annotations/stardist_worker:latest $NO_CACHE diff --git a/todo/TODO_REGISTRY.md b/todo/TODO_REGISTRY.md index 7454a27..f93431f 100644 --- a/todo/TODO_REGISTRY.md +++ b/todo/TODO_REGISTRY.md @@ -13,6 +13,7 @@ Master index of deferred work, technical debt, and future improvements for the I | ID | Title | Status | Priority | File | Related PR | |----|-------|--------|----------|------|------------| | TODO-001 | ML worker build optimization (mamba + shared base images) | Deferred | Medium | [ml-worker-build-optimization.md](ml-worker-build-optimization.md) | [#132](https://github.com/arjunrajlaboratory/ImageAnalysisProject/pull/132) | +| TODO-002 | Malformed `channelCheckboxes` values from externally-authored tool configs | Open | Medium | [channelcheckboxes-serialization.md](channelcheckboxes-serialization.md) | _(this branch)_ | ## Completed TODOs diff --git a/todo/channelcheckboxes-serialization.md b/todo/channelcheckboxes-serialization.md new file mode 100644 index 0000000..b62eaac --- /dev/null +++ b/todo/channelcheckboxes-serialization.md @@ -0,0 +1,77 @@ +# TODO-002: Malformed `channelCheckboxes` values from externally-authored tool configs + +**Status:** Open (worker guardrail done; root-cause cleanup outstanding) +**Priority:** Medium +**Related PR:** _(this branch)_ + +## Summary + +The `channelCheckboxes` interface type is documented (in `CLAUDE.md` and the +`nimbus-interface` skill reference) as returning a dict mapping channel-index +strings to booleans, e.g. `{"0": True, "1": False}`. All worker code was written +against that assumption (`[int(k) for k, v in value.items() if v]`). + +A production Cellpose-SAM error log showed the values arriving as **lists** +instead: + +``` +"Channel for Slot 1": [0], "Channel for Slot 2": [], "Channel for Slot 3": [] +``` + +Calling `.items()` on a list raised an opaque +`AttributeError: 'list' object has no attribute 'items'` that crashed the worker +**before** its existing required-field check (`if not slot1...: sendError(...)`) +could run. So the safeguard wasn't missing — it was being skipped. + +## Root cause (confirmed) + +The list shape is **not** produced by the worker, the front-end UI, or the docs: + +- **Worker interface**: `Channel for Slot 1/2/3` declare no `default`, so + `getDefault("channelCheckboxes", undefined)` returns `{}` — an empty dict, + never `[0]`. +- **NimbusImage front-end**: `channelCheckboxes` is typed `Record` + from the first commit; the `ChannelCheckboxGroup` widget only ever emits dicts; + the empty value is `{}`, never `[]`. +- **Backend** (`server/helpers/tasks.py`): JSON-encodes `workerInterfaceValues` + verbatim into `--parameters` — no transformation. +- Nothing in either repo constructs these values as arrays. + +An empty `[]` for an unset slot is the tell: no code path generates `[]`, only +`{}`. These values were **persisted on a saved tool whose `workerInterfaceValues` +were written with the wrong shape** — channel slots encoded as arrays of selected +indices (the intuitive-but-wrong representation) instead of `{index: bool}` dicts. +The config is re-sent verbatim on every run, which is why it fails *consistently* +for those specific tools rather than randomly. + +Fingerprints (tag `mg_cellposesam_v2`, name `cellpose-sam zero-shot on +LCA5_P21_rep1`) point to programmatic/scripted batch tool creation via the +API/Swagger that bypassed the UI widget, guessing `[0]` for "channel 0 selected." + +**Conclusion:** the front-end never emits lists. A list always means +malformed/externally-authored input, never a real UI selection — so there is no +"list-form selection" semantic to preserve. + +## What was done (worker guardrail — DONE) + +Added a shared helper `annotation_utilities.annotation_tools.get_selected_channels()` +that parses the documented dict form, treats `None` as "nothing selected", and +**rejects any other shape (including lists) with a `ValueError`**. Each affected +worker catches it and calls `sendError(...)` with a clear "interface is out of +date or misconfigured, please re-select the channels" message instead of crashing. + +Affected workers: `cellposesam`, `registration`, `deconwolf`, +`histogram_matching`, `gaussian_blur`, `rolling_ball`. + +We chose to **reject** rather than normalize-and-run: since these values come +from an external script that may have guessed the wrong channel, failing loudly +is safer than silently running a tool on a channel the user never confirmed. + +## Outstanding (the real fix) + +1. Find and fix whatever created the `mg_cellposesam_v2` batch with array-shaped + channel values, so it writes `{index: bool}` dicts. +2. Optionally run a one-time normalization over saved tool configs to repair the + existing broken tools (otherwise they keep erroring until recreated). + +Until then, affected tools get a clear error instead of a crash, but won't run. diff --git a/workers/annotations/cellposesam/Dockerfile b/workers/annotations/cellposesam/Dockerfile index 738e7ed..7a721cc 100644 --- a/workers/annotations/cellposesam/Dockerfile +++ b/workers/annotations/cellposesam/Dockerfile @@ -32,7 +32,7 @@ RUN wget \ FROM base as build -COPY ./environment.yml / +COPY ./workers/annotations/cellposesam/environment.yml / RUN conda env create --file /environment.yml SHELL ["conda", "run", "-n", "worker", "/bin/bash", "-c"] @@ -45,16 +45,21 @@ RUN git clone https://github.com/Kitware/UPennContrast/ RUN pip install -r /UPennContrast/devops/girder/annotation_client/requirements.txt RUN pip install -e /UPennContrast/devops/girder/annotation_client/ -COPY ./download_models.py / +COPY ./workers/annotations/cellposesam/download_models.py / RUN python /download_models.py -COPY ./utils.py / -COPY ./girder_utils.py / -COPY ./entrypoint.py / +COPY ./workers/annotations/cellposesam/utils.py / +COPY ./workers/annotations/cellposesam/girder_utils.py / +COPY ./workers/annotations/cellposesam/entrypoint.py / -RUN git clone https://github.com/arjunrajlaboratory/ImageAnalysisProject/ -RUN pip install /ImageAnalysisProject/annotation_utilities -RUN pip install /ImageAnalysisProject/worker_client +# Install annotation_utilities and worker_client from the local build context +# (not a fresh clone of the default branch) so the entrypoint and these shared +# packages always come from the same checkout. Cloning master here would ship a +# stale package on branch/PR builds and break helpers added in the same change. +COPY ./annotation_utilities /annotation_utilities +RUN pip install /annotation_utilities +COPY ./worker_client /worker_client +RUN pip install /worker_client LABEL isUPennContrastWorker=True \ isAnnotationWorker=True \ diff --git a/workers/annotations/cellposesam/entrypoint.py b/workers/annotations/cellposesam/entrypoint.py index 414a207..260fe08 100644 --- a/workers/annotations/cellposesam/entrypoint.py +++ b/workers/annotations/cellposesam/entrypoint.py @@ -8,6 +8,8 @@ import annotation_client.workers as workers from annotation_client.utils import sendError, sendWarning, sendProgress +import annotation_utilities.annotation_tools as annotation_tools + import numpy as np # library for array manipulation import deeptile from deeptile.extensions.segmentation import cellpose_segmentation @@ -201,35 +203,48 @@ def compute(datasetId, apiUrl, token, params): padding = float(worker.workerInterface['Padding']) smoothing = float(worker.workerInterface['Smoothing']) - # Process new channel selections - slot1_channel_str_keys = [k for k, v in worker.workerInterface.get( - 'Channel for Slot 1', {}).items() if v] - slot2_channel_str_keys = [k for k, v in worker.workerInterface.get( - 'Channel for Slot 2', {}).items() if v] - slot3_channel_str_keys = [k for k, v in worker.workerInterface.get( - 'Channel for Slot 3', {}).items() if v] + # Process channel selections. Each slot is a channelCheckboxes field, which + # should arrive as a dict ({"0": True}). get_selected_channels rejects any + # other shape (e.g. a list [0]) with a ValueError so we report a clear error + # to the user instead of crashing with an opaque AttributeError on .items(). + try: + slot1_channels = annotation_tools.get_selected_channels( + worker.workerInterface.get('Channel for Slot 1'), 'Channel for Slot 1') + slot2_channels = annotation_tools.get_selected_channels( + worker.workerInterface.get('Channel for Slot 2'), 'Channel for Slot 2') + slot3_channels = annotation_tools.get_selected_channels( + worker.workerInterface.get('Channel for Slot 3'), 'Channel for Slot 3') + except ValueError as e: + sendError( + "Could not read the channel selections. This usually means the tool " + "interface is out of date or misconfigured. Please re-open the tool, " + "re-select the channels for each slot, and run it again.", + info=str(e)) + raise stack_channels = [] - if not slot1_channel_str_keys: - sendError("No channel selected for Slot 1. This is a required field.") + if not slot1_channels: + sendError( + "No channel selected for Slot 1. This is a required field. " + "Please select at least one channel for Slot 1 and run the tool again.") raise ValueError("No channel selected for Slot 1.") - if len(slot1_channel_str_keys) > 1: + if len(slot1_channels) > 1: sendWarning( - f"Multiple channels selected for Slot 1 ({slot1_channel_str_keys}). Using the first: {slot1_channel_str_keys[0]}.") - stack_channels.append(int(slot1_channel_str_keys[0])) + f"Multiple channels selected for Slot 1 ({slot1_channels}). Using the first: {slot1_channels[0]}.") + stack_channels.append(slot1_channels[0]) - if slot2_channel_str_keys: - if len(slot2_channel_str_keys) > 1: + if slot2_channels: + if len(slot2_channels) > 1: sendWarning( - f"Multiple channels selected for Slot 2 ({slot2_channel_str_keys}). Using the first: {slot2_channel_str_keys[0]}.") - stack_channels.append(int(slot2_channel_str_keys[0])) + f"Multiple channels selected for Slot 2 ({slot2_channels}). Using the first: {slot2_channels[0]}.") + stack_channels.append(slot2_channels[0]) - if slot3_channel_str_keys: - if len(slot3_channel_str_keys) > 1: + if slot3_channels: + if len(slot3_channels) > 1: sendWarning( - f"Multiple channels selected for Slot 3 ({slot3_channel_str_keys}). Using the first: {slot3_channel_str_keys[0]}.") - stack_channels.append(int(slot3_channel_str_keys[0])) + f"Multiple channels selected for Slot 3 ({slot3_channels}). Using the first: {slot3_channels[0]}.") + stack_channels.append(slot3_channels[0]) if not stack_channels: # Should technically be caught by slot 1 check, but as a safeguard. sendError("No channels were selected for processing.") diff --git a/workers/annotations/deconwolf/entrypoint.py b/workers/annotations/deconwolf/entrypoint.py index 60e4d17..e10d01e5 100644 --- a/workers/annotations/deconwolf/entrypoint.py +++ b/workers/annotations/deconwolf/entrypoint.py @@ -10,6 +10,8 @@ import annotation_client.workers as workers from annotation_client.utils import sendProgress, sendWarning, sendError +import annotation_utilities.annotation_tools as annotation_tools + import numpy as np import tifffile import large_image as li @@ -354,8 +356,16 @@ def compute(datasetId, apiUrl, token, params): workerInterface = params['workerInterface'] # Parse channel selection - allChannels = workerInterface.get('Channels to deconvolve', {}) - channels = [int(k) for k, v in allChannels.items() if v] + try: + channels = annotation_tools.get_selected_channels( + workerInterface.get('Channels to deconvolve'), 'Channels to deconvolve') + except ValueError as e: + sendError( + "Could not read the 'Channels to deconvolve' selection. This usually " + "means the tool interface is out of date or misconfigured. Please " + "re-open the tool, re-select the channels, and run it again.", + info=str(e)) + return print(f"Selected channels to deconvolve: {channels}") if len(channels) == 0: diff --git a/workers/annotations/gaussian_blur/entrypoint.py b/workers/annotations/gaussian_blur/entrypoint.py index 4041e94..5baa1f7 100644 --- a/workers/annotations/gaussian_blur/entrypoint.py +++ b/workers/annotations/gaussian_blur/entrypoint.py @@ -11,6 +11,8 @@ from annotation_client.utils import sendProgress, sendError +import annotation_utilities.annotation_tools as annotation_tools + import imageio import numpy as np @@ -118,12 +120,16 @@ def compute(datasetId, apiUrl, token, params): workerInterface = params['workerInterface'] sigma = float(workerInterface['Sigma']) channel = int(workerInterface['Channel']) - allChannels = workerInterface['All channels'] - - print("allChannels", allChannels) - # Output is allChannels {'1': True, '2': True} - # This means that channels 1 and 2 are being blurred - channels = [int(k) for k, v in allChannels.items() if v] + try: + channels = annotation_tools.get_selected_channels( + workerInterface.get('All channels'), 'All channels') + except ValueError as e: + sendError( + "Could not read the 'All channels' selection. This usually means the " + "tool interface is out of date or misconfigured. Please re-open the " + "tool, re-select the channels, and run it again.", + info=str(e)) + return print("channels", channels) tile = params['tile'] diff --git a/workers/annotations/histogram_matching/entrypoint.py b/workers/annotations/histogram_matching/entrypoint.py index c65b706..a483450 100644 --- a/workers/annotations/histogram_matching/entrypoint.py +++ b/workers/annotations/histogram_matching/entrypoint.py @@ -11,6 +11,8 @@ from annotation_client.utils import sendProgress, sendError +import annotation_utilities.annotation_tools as annotation_tools + import imageio import numpy as np @@ -105,12 +107,16 @@ def compute(datasetId, apiUrl, token, params): reference_Time = 0 else: reference_Time = int(workerInterface['Reference Time Coordinate']) - 1 - allChannels = workerInterface['Channels to correct'] - - print("allChannels", allChannels) - # Output is allChannels {'1': True, '2': True} - # This means that channels 1 and 2 are being blurred - channels = [int(k) for k, v in allChannels.items() if v] + try: + channels = annotation_tools.get_selected_channels( + workerInterface.get('Channels to correct'), 'Channels to correct') + except ValueError as e: + sendError( + "Could not read the 'Channels to correct' selection. This usually " + "means the tool interface is out of date or misconfigured. Please " + "re-open the tool, re-select the channels, and run it again.", + info=str(e)) + return print("channels", channels) if len(channels) == 0: sendError("No channels to correct") diff --git a/workers/annotations/registration/entrypoint.py b/workers/annotations/registration/entrypoint.py index 15d0f14..44122e5 100644 --- a/workers/annotations/registration/entrypoint.py +++ b/workers/annotations/registration/entrypoint.py @@ -216,12 +216,16 @@ def compute(datasetId, apiUrl, token, params): if reference_channel == "" or reference_channel == -1: reference_channel = 0 - allChannels = workerInterface['Channels to correct'] - - print("allChannels", allChannels) - # Output is allChannels {'1': True, '2': True} - # This means that channels 1 and 2 are being blurred - channels = [int(k) for k, v in allChannels.items() if v] + try: + channels = annotation_tools.get_selected_channels( + workerInterface.get('Channels to correct'), 'Channels to correct') + except ValueError as e: + sendError( + "Could not read the 'Channels to correct' selection. This usually " + "means the tool interface is out of date or misconfigured. Please " + "re-open the tool, re-select the channels, and run it again.", + info=str(e)) + return print("channels", channels) if len(channels) == 0: sendError("No channels to correct") diff --git a/workers/annotations/rolling_ball/entrypoint.py b/workers/annotations/rolling_ball/entrypoint.py index cec86b8..a9dcb8f 100644 --- a/workers/annotations/rolling_ball/entrypoint.py +++ b/workers/annotations/rolling_ball/entrypoint.py @@ -11,6 +11,8 @@ from annotation_client.utils import sendProgress, sendError +import annotation_utilities.annotation_tools as annotation_tools + import imageio import numpy as np @@ -111,12 +113,16 @@ def compute(datasetId, apiUrl, token, params): workerInterface = params['workerInterface'] radius = float(workerInterface['Radius']) - allChannels = workerInterface['Channels to correct'] - - print("allChannels", allChannels) - # Output is allChannels {'1': True, '2': True} - # This means that channels 1 and 2 are being blurred - channels = [int(k) for k, v in allChannels.items() if v] + try: + channels = annotation_tools.get_selected_channels( + workerInterface.get('Channels to correct'), 'Channels to correct') + except ValueError as e: + sendError( + "Could not read the 'Channels to correct' selection. This usually " + "means the tool interface is out of date or misconfigured. Please " + "re-open the tool, re-select the channels, and run it again.", + info=str(e)) + return print("channels", channels) tile = params['tile']