From 8376c814c1bc31f2b02811de451c09ec08462f22 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 30 May 2026 10:27:11 +0000 Subject: [PATCH 1/5] Make cellposesam channel parsing robust and report clear errors The channelCheckboxes slot values were assumed to be dicts and read via .items(), but the value can arrive as a plain list (e.g. [0]). This caused an opaque 'list object has no attribute items' AttributeError to crash the worker before the friendly validation checks could run. Add a get_selected_channels() helper that normalizes dict, list, and empty/None inputs into a sorted list of channel indices, and emit a clear sendError when Slot 1 is unset or the selections can't be parsed. --- workers/annotations/cellposesam/entrypoint.py | 71 +++++++++++++------ 1 file changed, 51 insertions(+), 20 deletions(-) diff --git a/workers/annotations/cellposesam/entrypoint.py b/workers/annotations/cellposesam/entrypoint.py index 414a207..dac1aac 100644 --- a/workers/annotations/cellposesam/entrypoint.py +++ b/workers/annotations/cellposesam/entrypoint.py @@ -23,6 +23,26 @@ BASE_MODELS = ['cellpose-sam'] +def get_selected_channels(value): + """Normalize a ``channelCheckboxes`` interface value into a sorted list of + selected channel indices (ints). + + Depending on the client, this value may arrive either as a dict mapping + channel-index strings to booleans (e.g. ``{"0": True, "1": False}``) or as + a plain list of selected channel indices (e.g. ``[0]``). It may also be + empty or missing when the user did not select anything. This helper accepts + all of those shapes and always returns a list of ints (possibly empty). + """ + if not value: + return [] + if isinstance(value, dict): + return sorted(int(k) for k, v in value.items() if v) + if isinstance(value, (list, tuple)): + return sorted(int(c) for c in value) + raise TypeError( + f"Unexpected channelCheckboxes value of type {type(value).__name__}: {value!r}") + + def interface(image, apiUrl, token): client = workers.UPennContrastWorkerPreviewClient( apiUrl=apiUrl, token=token) @@ -201,35 +221,46 @@ 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 + # may arrive as a dict ({"0": True}) or a list ([0]); get_selected_channels + # normalizes both (and the empty/unset case) so we never crash on the raw + # value and can report a clear error to the user instead. + try: + slot1_channels = get_selected_channels( + worker.workerInterface.get('Channel for Slot 1')) + slot2_channels = get_selected_channels( + worker.workerInterface.get('Channel for Slot 2')) + slot3_channels = get_selected_channels( + worker.workerInterface.get('Channel for Slot 3')) + except (TypeError, ValueError) as e: + sendError( + "Could not read the channel selections. Please re-select the channels for each slot and try 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.") From 7ef96f1c1f919e1f5e9da8689b187f06a1c35d3e Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 30 May 2026 11:10:46 +0000 Subject: [PATCH 2/5] Reject malformed channelCheckboxes values with a clear error across workers channelCheckboxes is documented to return a dict ({"0": True}), but a production log showed it arriving as a list ([0]), which crashed workers with an opaque AttributeError on .items() before any validation ran. Add a shared, tested annotation_tools.get_selected_channels() helper that parses the documented dict form, treats a missing value as 'nothing selected', and rejects any other shape (notably lists) with a ValueError. We reject rather than silently recover, since a [0] could be an un-chosen default and running a tool on the wrong channel is worse than failing loudly. Wire cellposesam, registration, deconwolf, histogram_matching, gaussian_blur, and rolling_ball to the helper so they sendError() with a clear 'interface out of date or misconfigured' message instead of crashing. Add TODO-002 to verify against the NimbusImage front-end what channelCheckboxes actually serializes and fix the root cause there. --- .../annotation_utilities/annotation_tools.py | 29 +++++++++ .../tests/test_get_selected_channels.py | 40 +++++++++++++ todo/TODO_REGISTRY.md | 1 + todo/channelcheckboxes-serialization.md | 59 +++++++++++++++++++ workers/annotations/cellposesam/entrypoint.py | 46 +++++---------- workers/annotations/deconwolf/entrypoint.py | 14 ++++- .../annotations/gaussian_blur/entrypoint.py | 18 ++++-- .../histogram_matching/entrypoint.py | 18 ++++-- .../annotations/registration/entrypoint.py | 16 +++-- .../annotations/rolling_ball/entrypoint.py | 18 ++++-- 10 files changed, 202 insertions(+), 57 deletions(-) create mode 100644 annotation_utilities/tests/test_get_selected_channels.py create mode 100644 todo/channelcheckboxes-serialization.md 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/todo/TODO_REGISTRY.md b/todo/TODO_REGISTRY.md index 7454a27..1b61753 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 | Verify `channelCheckboxes` serialization against the front-end | 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..997e664 --- /dev/null +++ b/todo/channelcheckboxes-serialization.md @@ -0,0 +1,59 @@ +# TODO-002: Verify `channelCheckboxes` serialization against the front-end + +**Status:** Open +**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]`). + +However, 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 any user-facing validation could run. Notably, Slot 1 here was **not** +empty (it held channel `0`), so the list form is not simply the "nothing +selected" representation. + +## What was done in the meantime + +A shared helper `annotation_utilities.annotation_tools.get_selected_channels()` +now parses the value: it accepts the documented dict form, treats `None` as +"nothing selected", and **rejects any other shape (including lists) with a +`ValueError`**. Each affected worker catches this and calls `sendError(...)` with +a clear "the tool interface is out of date or misconfigured, please re-select +the channels" message instead of crashing. + +Affected workers updated: `cellposesam`, `registration`, `deconwolf`, +`histogram_matching`, `gaussian_blur`, `rolling_ball`. + +We deliberately chose to **reject** the list form rather than silently recover +it, because a `[0]` could be an un-chosen UI default rather than an intentional +selection, and running a tool on the wrong channel is worse than failing loudly. + +## What still needs investigating (the actual TODO) + +The **front-end (NimbusImage repo) is the source of truth** for what +`channelCheckboxes` actually serializes. We need to confirm there: + +1. Does the front-end ever legitimately send a list for `channelCheckboxes`? If + so, under what conditions (e.g. interface not initialized, older cached tool + definition, a specific widget state)? +2. If the list form is a front-end bug, fix it there so the value is always the + documented dict. +3. If the list form is intentional/valid, revisit the decision above — we may + want the workers to accept and normalize it instead of erroring, and the docs + (`CLAUDE.md`, `nimbus-interface` skill) should be updated to document both + shapes. + +Until this is resolved, users whose front-end sends the list form will get a +clear error rather than a crash, but their jobs will not run. diff --git a/workers/annotations/cellposesam/entrypoint.py b/workers/annotations/cellposesam/entrypoint.py index dac1aac..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 @@ -23,26 +25,6 @@ BASE_MODELS = ['cellpose-sam'] -def get_selected_channels(value): - """Normalize a ``channelCheckboxes`` interface value into a sorted list of - selected channel indices (ints). - - Depending on the client, this value may arrive either as a dict mapping - channel-index strings to booleans (e.g. ``{"0": True, "1": False}``) or as - a plain list of selected channel indices (e.g. ``[0]``). It may also be - empty or missing when the user did not select anything. This helper accepts - all of those shapes and always returns a list of ints (possibly empty). - """ - if not value: - return [] - if isinstance(value, dict): - return sorted(int(k) for k, v in value.items() if v) - if isinstance(value, (list, tuple)): - return sorted(int(c) for c in value) - raise TypeError( - f"Unexpected channelCheckboxes value of type {type(value).__name__}: {value!r}") - - def interface(image, apiUrl, token): client = workers.UPennContrastWorkerPreviewClient( apiUrl=apiUrl, token=token) @@ -222,19 +204,21 @@ def compute(datasetId, apiUrl, token, params): smoothing = float(worker.workerInterface['Smoothing']) # Process channel selections. Each slot is a channelCheckboxes field, which - # may arrive as a dict ({"0": True}) or a list ([0]); get_selected_channels - # normalizes both (and the empty/unset case) so we never crash on the raw - # value and can report a clear error to the user instead. + # 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 = get_selected_channels( - worker.workerInterface.get('Channel for Slot 1')) - slot2_channels = get_selected_channels( - worker.workerInterface.get('Channel for Slot 2')) - slot3_channels = get_selected_channels( - worker.workerInterface.get('Channel for Slot 3')) - except (TypeError, ValueError) as e: + 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. Please re-select the channels for each slot and try again.", + "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 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'] From c44f19044b712ab0757d58df6926dcdb25b3d4d3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 30 May 2026 11:11:33 +0000 Subject: [PATCH 3/5] Add .gitignore for Python build/test artifacts --- .gitignore | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .gitignore 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/ From 7e17e24ce199920ab87b8931e7a6407e79096434 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 30 May 2026 20:15:01 +0000 Subject: [PATCH 4/5] Update TODO-002 with confirmed root cause (malformed external tool config) --- todo/TODO_REGISTRY.md | 2 +- todo/channelcheckboxes-serialization.md | 84 +++++++++++++++---------- 2 files changed, 52 insertions(+), 34 deletions(-) diff --git a/todo/TODO_REGISTRY.md b/todo/TODO_REGISTRY.md index 1b61753..f93431f 100644 --- a/todo/TODO_REGISTRY.md +++ b/todo/TODO_REGISTRY.md @@ -13,7 +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 | Verify `channelCheckboxes` serialization against the front-end | Open | Medium | [channelcheckboxes-serialization.md](channelcheckboxes-serialization.md) | _(this branch)_ | +| 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 index 997e664..b62eaac 100644 --- a/todo/channelcheckboxes-serialization.md +++ b/todo/channelcheckboxes-serialization.md @@ -1,6 +1,6 @@ -# TODO-002: Verify `channelCheckboxes` serialization against the front-end +# TODO-002: Malformed `channelCheckboxes` values from externally-authored tool configs -**Status:** Open +**Status:** Open (worker guardrail done; root-cause cleanup outstanding) **Priority:** Medium **Related PR:** _(this branch)_ @@ -11,8 +11,8 @@ The `channelCheckboxes` interface type is documented (in `CLAUDE.md` and the 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]`). -However, a production Cellpose-SAM error log showed the values arriving as -**lists** instead: +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": [] @@ -20,40 +20,58 @@ However, a production Cellpose-SAM error log showed the values arriving as Calling `.items()` on a list raised an opaque `AttributeError: 'list' object has no attribute 'items'` that crashed the worker -before any user-facing validation could run. Notably, Slot 1 here was **not** -empty (it held channel `0`), so the list form is not simply the "nothing -selected" representation. +**before** its existing required-field check (`if not slot1...: sendError(...)`) +could run. So the safeguard wasn't missing — it was being skipped. -## What was done in the meantime +## Root cause (confirmed) -A shared helper `annotation_utilities.annotation_tools.get_selected_channels()` -now parses the value: it accepts the documented dict form, treats `None` as -"nothing selected", and **rejects any other shape (including lists) with a -`ValueError`**. Each affected worker catches this and calls `sendError(...)` with -a clear "the tool interface is out of date or misconfigured, please re-select -the channels" message instead of crashing. +The list shape is **not** produced by the worker, the front-end UI, or the docs: -Affected workers updated: `cellposesam`, `registration`, `deconwolf`, -`histogram_matching`, `gaussian_blur`, `rolling_ball`. +- **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. -We deliberately chose to **reject** the list form rather than silently recover -it, because a `[0]` could be an un-chosen UI default rather than an intentional -selection, and running a tool on the wrong channel is worse than failing loudly. +## 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`. -## What still needs investigating (the actual TODO) +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. -The **front-end (NimbusImage repo) is the source of truth** for what -`channelCheckboxes` actually serializes. We need to confirm there: +## Outstanding (the real fix) -1. Does the front-end ever legitimately send a list for `channelCheckboxes`? If - so, under what conditions (e.g. interface not initialized, older cached tool - definition, a specific widget state)? -2. If the list form is a front-end bug, fix it there so the value is always the - documented dict. -3. If the list form is intentional/valid, revisit the decision above — we may - want the workers to accept and normalize it instead of erroring, and the docs - (`CLAUDE.md`, `nimbus-interface` skill) should be updated to document both - shapes. +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 this is resolved, users whose front-end sends the list form will get a -clear error rather than a crash, but their jobs will not run. +Until then, affected tools get a clear error instead of a crash, but won't run. From fc97d26ba5a018aa15d63e06ffc748c0d502b5f4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 30 May 2026 20:29:36 +0000 Subject: [PATCH 5/5] Build cellposesam from repo-root context, install shared packages from local checkout cellposesam installed annotation_utilities/worker_client via a fresh clone of the default branch rather than the build checkout, so branch/PR builds shipped the new entrypoint against a stale package and would raise AttributeError on get_selected_channels before the intended sendError path. Switch the worker to the repo-root build context (matching deconwolf/sam2) and COPY+install both shared packages from the local checkout so the entrypoint and packages stay in lockstep. --- build_machine_learning_workers.sh | 2 +- workers/annotations/cellposesam/Dockerfile | 21 +++++++++++++-------- 2 files changed, 14 insertions(+), 9 deletions(-) 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/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 \