Multichannel Model Support for Pre-Aligned Volumes#1896
Open
CavRiley wants to merge 5 commits intoProject-MONAI:mainfrom
Open
Multichannel Model Support for Pre-Aligned Volumes#1896CavRiley wants to merge 5 commits intoProject-MONAI:mainfrom
CavRiley wants to merge 5 commits intoProject-MONAI:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds multi-file and multichannel support: new Datastore API methods ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Endpoint
participant App as MONAILabelApp
participant Datastore
participant FS as FileSystem
Client->>Endpoint: POST add_image (path, image_file, metadata)
Endpoint->>App: prepare request (include multichannel, multi_file, input_channels)
App->>Datastore: if multi_file -> add_directory(id, path, info) else -> add_image(id, file, info)
alt multi_file branch
Datastore->>FS: copy directory/files (store metadata)
FS-->>Datastore: return directory_id
Datastore-->>App: directory_id
else single-file branch
Datastore->>FS: store single image file
FS-->>Datastore: image_id
Datastore-->>App: image_id
end
App-->>Endpoint: return id (image_id or directory_id)
Endpoint-->>Client: 200 { "id": id }
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can generate a title for your PR based on the changes.Add |
CavRiley
force-pushed
the
multiChannelSupport
branch
from
f112618 to
a412e66
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (6)
monailabel/datastore/cvat.py (1)
322-324: Parameter namedirectory_iddoesn't match abstract method signature.The abstract method in
monailabel/interfaces/datastore.py(line 205) usesidas the first parameter, but this implementation usesdirectory_id. While Python allows this (it's positional), it's better to match the interface signature for consistency and clarity.♻️ Proposed fix
- def add_directory(self, directory_id: str, filename: str, info: Dict[str, Any]) -> str: + def add_directory(self, id: str, filename: str, info: Dict[str, Any]) -> str: """Not Implemented""" raise NotImplementedError("This datastore does not support adding directories")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monailabel/datastore/cvat.py` around lines 322 - 324, The add_directory implementation in monailabel/datastore/cvat.py uses parameter name directory_id which mismatches the abstract interface signature (monailabel/interfaces/datastore.py) that expects id; rename the first parameter in def add_directory from directory_id to id to match the abstract method signature (keeping the same behavior and raising NotImplementedError) and update any internal references in the method body to use id (retain function name add_directory to locate it).monailabel/tasks/activelearning/first.py (1)
38-47: Missing logging for multichannel and multi_file cases.The early returns for
multichannelandmulti_fileskip thelogger.infostatement, making it harder to trace which image was selected in these scenarios. Consider adding logging before each early return for consistency.♻️ Proposed fix
# If the datastore contains 4d images send the multichannel flag to ensure images are loaded as sequences if datastore.get_is_multichannel(): + logger.info(f"First: Selected Image (multichannel): {image}") return {"id": image, "multichannel": True} # If the datastore is multi_file, each sample has a directory with multiple images if datastore.get_is_multi_file(): + logger.info(f"First: Selected Image (multi_file): {image}") return {"id": image, "multi_file": True} logger.info(f"First: Selected Image: {image}") return {"id": image}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monailabel/tasks/activelearning/first.py` around lines 38 - 47, The early returns in monailabel/tasks/activelearning/first.py skip the logger.info call so multichannel and multi_file selections aren't logged; before each early return that uses datastore.get_is_multichannel() and datastore.get_is_multi_file(), add a logger.info call matching the existing format (e.g., logger.info(f"First: Selected Image: {image}")) so every selection path logs the chosen image (ensure you place the log immediately before the return in the same conditional branches).sample-apps/radiology/README.md (1)
233-233: Consider using more descriptive link text.The link text "here" is not descriptive for accessibility purposes. Consider using the actual content description.
📝 Proposed fix
-- Network: This model uses the [UNet](https://docs.monai.io/en/latest/networks.html#unet) as the default network. Researchers can define their own network or use one of the listed [here](https://docs.monai.io/en/latest/networks.html) +- Network: This model uses the [UNet](https://docs.monai.io/en/latest/networks.html#unet) as the default network. Researchers can define their own network or use one of the [available MONAI networks](https://docs.monai.io/en/latest/networks.html)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-apps/radiology/README.md` at line 233, Replace the non-descriptive link text "here" with an explicit, accessible label like "available MONAI networks" (or "MONAI networks list") in the README sentence describing the default UNet so the link reads "...or use one of the listed available MONAI networks" and points to the same URL; update the sentence in the README (the Network description line) to use that descriptive link text.plugins/slicer/MONAILabel/MONAILabel.py (2)
1340-1352: Loop variablenameshadows outer scope variable; unused loop variableidx.
- The loop variable
name(line 1342) shadowsimage_nameassignment that usesnamefrom line 1308. This could cause confusion during maintenance.- The loop variable
idx(line 1348) is unused and should be renamed to_idxper Python convention.♻️ Proposed fix
# get valid image paths entries = sorted(os.listdir(dir_path)) image_paths = [] - for name in entries: - full_path = os.path.join(dir_path, name) + for entry_name in entries: + full_path = os.path.join(dir_path, entry_name) if os.path.isfile(full_path): image_paths.append(full_path) nodes = [] - for idx, image in enumerate(image_paths): + for _idx, image in enumerate(image_paths): image_base_name = os.path.basename(image) node = slicer.util.loadVolume(image) node.SetName(image_base_name) nodes.append(node)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/slicer/MONAILabel/MONAILabel.py` around lines 1340 - 1352, The for-loop that builds image_paths uses the loop variable name which shadows the outer variable image_name and the subsequent enumerate uses an unused idx; rename the inner loop variable from name to entry_name (or similar) when iterating entries and use entry_name to construct full_path, and change enumerate(image_paths) to for _idx, image in enumerate(image_paths): (or simply for image in image_paths:) and remove references to idx so the code no longer shadows image_name and follows the underscore convention for unused indices; update usages of node.SetName(os.path.basename(image))/image_base_name accordingly.
1306-1308: Variableidshadows Python built-in.Using
idas a variable name shadows the built-inid()function. Consider renaming toimage_idorsample_idfor clarity and to avoid potential issues.♻️ Proposed fix
- id = sample["id"] + image_id = sample["id"] image_file = sample.get("path") - image_name = sample.get("name", id) - node_name = sample.get("PatientID", sample.get("name", id)) + image_name = sample.get("name", image_id) + node_name = sample.get("PatientID", sample.get("name", image_id))Then update all subsequent references from
idtoimage_id(lines 1356, 1367, 1379).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/slicer/MONAILabel/MONAILabel.py` around lines 1306 - 1308, The local variable named id shadows the Python built-in; rename it to image_id (or sample_id) where it's assigned from sample["id"] and update every subsequent use of that variable in the same scope (e.g., the places that currently reference id when building image_name, image_file handling, and any later references in the same function/method in class MONAILabel) to image_id to avoid shadowing and maintain clarity.monailabel/endpoints/datastore.py (1)
71-71: Variableidshadows Python built-in.Similar to the Slicer plugin, using
idas a variable name shadows the built-inid()function. Consider usingimage_idfor consistency and clarity.♻️ Proposed fix
- id = image if image else os.path.basename(file.filename).replace(file_ext, "") + image_id = image if image else os.path.basename(file.filename).replace(file_ext, "")Then update references at lines 83 and 86.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monailabel/endpoints/datastore.py` at line 71, Rename the local variable named id to image_id to avoid shadowing the built-in id(); specifically change the assignment "id = image if image else os.path.basename(file.filename).replace(file_ext, "")" to use image_id and update all subsequent references in the same function/scope that use id (for example places where image or file.filename/file_ext values are combined or returned) to image_id so the code compiles and maintains consistency with the Slicer plugin naming.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@monailabel/datastore/dicom.py`:
- Around line 272-282: The two capability-query methods get_is_multichannel and
get_is_multi_file currently raise NotImplementedError which disrupts normal
control flow; update both methods to simply return False instead of raising so
callers can detect the datastore lacks these features (modify the
get_is_multichannel and get_is_multi_file implementations to return False and
leave their docstrings intact).
In `@monailabel/datastore/dsa.py`:
- Around line 277-287: The two capability methods get_is_multichannel and
get_is_multi_file currently raise NotImplementedError which breaks callers that
expect boolean flags; change both methods in class DSA (or wherever they are
defined) to return False instead of raising, so upstream logic querying
get_is_multichannel() and get_is_multi_file() receives False for unsupported
features rather than an exception.
In `@monailabel/datastore/local.py`:
- Around line 588-597: The multi-file branch currently treats any non-ignored
entry as a case image which allows stray files to be misinterpreted as cases;
update the logic in the method that uses self._multi_file (the branch iterating
"for file in files") to only consider entries that are actual case directories:
compute the full path for each file and require os.path.isdir(fullpath) (and
keep the existing ignore list check for "labels", ".lock", "datastore_v2.json")
before adding os.path.basename(file) to filtered; this ensures only directories
are discovered as cases while preserving the existing ignores and behavior for
self._multi_file.
- Around line 637-646: The new-image branch is passing the wrong keyword to the
ImageLabelModel constructor—it's using name=DataModel(...) which fails because
ImageLabelModel expects an image: DataModel; update the assignment in the block
that adds to self._datastore.objects (the code that uses self._filename(file_id,
file_ext_str) and constructs file_info) to pass the DataModel instance as
image=DataModel(info=file_info, ext=file_ext_str) (matching the usage found in
the other places like the lines that use image=), leaving the rest of the
created dict and invalidation logic unchanged.
- Around line 444-455: add_directory currently uses shutil.copy which raises
IsADirectoryError when filename is a directory; update add_directory (and its
use of FileLock/_lock_file and _datastore.image_path()) to detect whether
filename is a directory (os.path.isdir(filename)) and: if it's a file, continue
using shutil.copy to dest; if it's a directory, use shutil.copytree to copy the
entire directory tree into dest (or merge into dest if dest exists—create dest
parent and handle existing destination by removing or merging as appropriate),
ensuring proper locking via FileLock and preserving logging (logger.info/debug)
around the operation.
In `@monailabel/datastore/xnat.py`:
- Around line 393-403: The capability-check methods get_is_multichannel and
get_is_multi_file currently raise NotImplementedError; change them to return
False so callers receive a boolean capability response instead of an exception —
update both methods to simply return False and keep their docstrings intact so
the datastore reports "not supported" via False rather than raising.
In `@monailabel/interfaces/app.py`:
- Around line 288-292: The methods infer() and train() currently mutate the
incoming request by setting request["multi_file"], request["multichannel"], and
request["input_channels"] before doing copy.deepcopy(request); change the flow
to first make a deep copy (e.g., request_copy = copy.deepcopy(request)) and then
set those fields on the copy, and use that copy for subsequent processing; apply
the same change in both infer() and train() so the caller's original request is
never modified.
In `@monailabel/interfaces/datastore.py`:
- Around line 205-213: Update the abstract method signature of add_directory to
rename the first parameter from id to directory_id so it matches concrete
implementations (add_directory(self, directory_id: str, filename: str, info:
Dict[str, Any]) -> str), and update the docstring parameter names and
descriptions accordingly; ensure the method name add_directory and its return
type remain unchanged so callers and subclasses align with the new parameter
name.
In `@sample-apps/radiology/lib/trainers/segmentation_brats.py`:
- Around line 95-101: The pipeline always uses LoadDirectoryImagesd for "image"
and thus ignores the single-file (pre-stacked multichannel) case; restore the
runtime switch by checking context.multi_file and using LoadImaged(keys="image",
reader="ITKReader", ensure_channel_first=True) when multi_file is False,
otherwise use LoadDirectoryImagesd(keys="image",
target_spacing=self.target_spacing, channels=channels); apply the same
conditional pattern to the second occurrence (the block around the other
LoadDirectoryImagesd) so both places respect context.multi_file, and keep
target_spacing and channels parameters for the directory-loader path.
- Around line 107-110: The Spacingd transform calls are hardcoding
pixdim=(1.0,1.0,1.0) and thus ignore any provided target_spacing; update the
Spacingd invocations in segmentation_brats (the calls where
Spacingd(keys=["image","label"], pixdim=(1.0,1.0,1.0), mode=(...)) appears) to
use the actual target_spacing parameter (e.g., convert target_spacing to a
3-tuple and pass pixdim=tuple(target_spacing) or pixdim=target_spacing_tuple)
instead of the literal (1.0,1.0,1.0); apply the same change to both occurrences
so custom target_spacing is respected.
In `@sample-apps/radiology/lib/transforms/transforms.py`:
- Around line 104-112: The constructor stores channels but never uses it and
keeps a persistent self.resizer causing resize state to leak across samples;
update the transform flow (e.g., in the method that applies the pipeline after
__init__, such as the transform/call method used with
loader/ensure_channel_first) to 1) validate that the number of image modalities
matches self.channels (inspect the channel dimension after EnsureChannelFirst
and raise a clear error if mismatched) and 2) stop reusing a long-lived
self.resizer by creating the Spacing/Resize transform per-sample based on the
current sample shape (or reset self.resizer to None after each use) so each
sample computes its own target shape and no state carries over between samples.
Ensure references to self.resizer and channels are updated accordingly (use the
existing loader, EnsureChannelFirst, spacer/resizer symbols).
In `@sample-apps/radiology/main.py`:
- Around line 310-313: The CLI boolean flags defined with
parser.add_argument("-multi", "--multichannel", default=False) and
"-multif"/"--multi_file" are incorrect: replace their defaults with explicit
boolean flag behavior by using action='store_true' (or action='store_false' if
inverse) on the parser.add_argument calls for multichannel and multi_file; keep
the "--input_channels" argument as-is but ensure its type/required handling if
it should be an int/list. Update the parser.add_argument calls for
"multichannel" and "multi_file" to use action='store_true' so passing the flag
sets True without needing an explicit value.
In `@sample-apps/radiology/README.md`:
- Line 242: Fix the typo in the README line that currently reads "Dataset: The
model is trained over the adataset: https://www.med.upenn.edu/cbica/brats2020/"
by changing "adataset" to "a dataset" so the sentence reads "Dataset: The model
is trained over a dataset: https://www.med.upenn.edu/cbica/brats2020/". Ensure
only that single word is corrected in the README.md content.
---
Nitpick comments:
In `@monailabel/datastore/cvat.py`:
- Around line 322-324: The add_directory implementation in
monailabel/datastore/cvat.py uses parameter name directory_id which mismatches
the abstract interface signature (monailabel/interfaces/datastore.py) that
expects id; rename the first parameter in def add_directory from directory_id to
id to match the abstract method signature (keeping the same behavior and raising
NotImplementedError) and update any internal references in the method body to
use id (retain function name add_directory to locate it).
In `@monailabel/endpoints/datastore.py`:
- Line 71: Rename the local variable named id to image_id to avoid shadowing the
built-in id(); specifically change the assignment "id = image if image else
os.path.basename(file.filename).replace(file_ext, "")" to use image_id and
update all subsequent references in the same function/scope that use id (for
example places where image or file.filename/file_ext values are combined or
returned) to image_id so the code compiles and maintains consistency with the
Slicer plugin naming.
In `@monailabel/tasks/activelearning/first.py`:
- Around line 38-47: The early returns in
monailabel/tasks/activelearning/first.py skip the logger.info call so
multichannel and multi_file selections aren't logged; before each early return
that uses datastore.get_is_multichannel() and datastore.get_is_multi_file(), add
a logger.info call matching the existing format (e.g., logger.info(f"First:
Selected Image: {image}")) so every selection path logs the chosen image (ensure
you place the log immediately before the return in the same conditional
branches).
In `@plugins/slicer/MONAILabel/MONAILabel.py`:
- Around line 1340-1352: The for-loop that builds image_paths uses the loop
variable name which shadows the outer variable image_name and the subsequent
enumerate uses an unused idx; rename the inner loop variable from name to
entry_name (or similar) when iterating entries and use entry_name to construct
full_path, and change enumerate(image_paths) to for _idx, image in
enumerate(image_paths): (or simply for image in image_paths:) and remove
references to idx so the code no longer shadows image_name and follows the
underscore convention for unused indices; update usages of
node.SetName(os.path.basename(image))/image_base_name accordingly.
- Around line 1306-1308: The local variable named id shadows the Python
built-in; rename it to image_id (or sample_id) where it's assigned from
sample["id"] and update every subsequent use of that variable in the same scope
(e.g., the places that currently reference id when building image_name,
image_file handling, and any later references in the same function/method in
class MONAILabel) to image_id to avoid shadowing and maintain clarity.
In `@sample-apps/radiology/README.md`:
- Line 233: Replace the non-descriptive link text "here" with an explicit,
accessible label like "available MONAI networks" (or "MONAI networks list") in
the README sentence describing the default UNet so the link reads "...or use one
of the listed available MONAI networks" and points to the same URL; update the
sentence in the README (the Network description line) to use that descriptive
link text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf330973-8aa1-4a43-95be-aedf3f20fc1a
📒 Files selected for processing (18)
monailabel/datastore/cvat.pymonailabel/datastore/dicom.pymonailabel/datastore/dsa.pymonailabel/datastore/local.pymonailabel/datastore/xnat.pymonailabel/endpoints/datastore.pymonailabel/interfaces/app.pymonailabel/interfaces/datastore.pymonailabel/tasks/activelearning/first.pymonailabel/tasks/activelearning/random.pymonailabel/tasks/train/basic_train.pyplugins/slicer/MONAILabel/MONAILabel.pysample-apps/radiology/README.mdsample-apps/radiology/lib/configs/segmentation_brats.pysample-apps/radiology/lib/infers/segmentation_brats.pysample-apps/radiology/lib/trainers/segmentation_brats.pysample-apps/radiology/lib/transforms/transforms.pysample-apps/radiology/main.py
CavRiley
force-pushed
the
multiChannelSupport
branch
from
08269be to
382f417
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/slicer/MONAILabel/MONAILabel.py (1)
1356-1362:⚠️ Potential issue | 🟠 Major
multichannelis ignored for downloaded samples.The multichannel-specific loading path is only applied when
local_existsis true. For downloaded samples, execution always goes through single-volume loading, so behavior differs between shared-local and remote setups.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/slicer/MONAILabel/MONAILabel.py` around lines 1356 - 1362, The downloaded-sample path always loads a single volume; change it to honor multichannel the same way the local_exists path does: detect self.multichannel and, if true, call SampleData.SampleDataLogic.downloadFromURL in the multichannel form (pass per-channel nodeNames/fileNames/uris or call the existing multi-channel loader used for local samples), then assemble/assign the resulting multi-volume node to self._volumeNode; otherwise keep the single-volume behavior. Use symbols: serverUrl(), SampleData.SampleDataLogic.downloadFromURL, self.multichannel, and self._volumeNode to locate and modify the logic so downloaded samples follow the multichannel loading branch.
🧹 Nitpick comments (4)
monailabel/datastore/cvat.py (1)
328-335: Use module logger consistently.Line 334 uses
logging.info(...)while adjacent code useslogger.info(...); prefer a single logger style for consistent formatting/handlers.🔁 Proposed tiny cleanup
- logging.info("The function get_is_multichannel is not implemented for this datastore") + logger.info("The function get_is_multichannel is not implemented for this datastore")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monailabel/datastore/cvat.py` around lines 328 - 335, The method get_is_multichannel currently calls the stdlib logging module directly (logging.info(...)) while the rest of the file uses the module-level logger variable; change that call to use the existing logger (logger.info(...)) so formatting/handlers remain consistent—update the logging invocation inside get_is_multichannel to use logger.info and ensure no other direct logging imports are used in that function.monailabel/endpoints/datastore.py (1)
71-71: Avoid shadowing built-inid.Line 71 uses
idas a variable name; preferimage_idfor clarity and to avoid shadowing Python built-ins.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monailabel/endpoints/datastore.py` at line 71, Rename the local variable currently named id to image_id to avoid shadowing the built-in; update the assignment "id = image if image else os.path.basename(file.filename).replace(file_ext, "")" to set image_id instead and then replace all subsequent uses of id within the same function/scope with image_id (references involving image, file, and file_ext). Ensure any returned values, dictionary keys, or log messages that previously used id are updated to use image_id so behavior is unchanged.monailabel/tasks/train/basic_train.py (1)
495-497: Normalize request types before storing in training context.Line 495 and Line 496 currently assign raw request values. Explicit coercion avoids subtle type mismatches when values arrive as strings.
🔧 Proposed hardening
- context.multi_file = request.get("multi_file", False) - context.input_channels = request.get("input_channels", 1) + multi_file = request.get("multi_file", False) + context.multi_file = multi_file if isinstance(multi_file, bool) else str(multi_file).lower() == "true" + context.input_channels = int(request.get("input_channels", 1))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monailabel/tasks/train/basic_train.py` around lines 495 - 497, The request values assigned to context.multi_file and context.input_channels can arrive as strings; coerce them to the intended types before storing: convert multi_file to a boolean (handle truthy string forms like "true","1" vs "false","0") when setting context.multi_file, and convert input_channels to an int with a safe fallback when setting context.input_channels (use request.get("input_channels", 1) as default). Update the assignment sites (context.multi_file and context.input_channels) to perform these explicit conversions so the training context always contains the correct types.sample-apps/radiology/main.py (1)
310-312: Prevent ambiguous mode selection for CLI flags.Line 310 and Line 312 currently allow
--multichanneland--multi_filetogether; these modes are typically mutually exclusive and should be enforced at parse time.🛠️ Proposed refactor
- parser.add_argument("-multi", "--multichannel", action="store_true", help="Enable multichannel (4D) data loading") - parser.add_argument("-c", "--input_channels", type=int, default=1, help="Number of input channels") - parser.add_argument("-multif", "--multi_file", action="store_true", help="Enable multi-file data loading") + mode_group = parser.add_mutually_exclusive_group() + mode_group.add_argument( + "-multi", "--multichannel", action="store_true", help="Enable multichannel (4D) data loading" + ) + mode_group.add_argument( + "-multif", "--multi_file", action="store_true", help="Enable multi-file data loading" + ) + parser.add_argument("-c", "--input_channels", type=int, default=1, help="Number of input channels")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-apps/radiology/main.py` around lines 310 - 312, The CLI allows both --multichannel and --multi_file to be set together which should be forbidden; update the argument parsing by creating an argparse mutually exclusive group (or add a post-parse validation) that includes the --multichannel/--multichannel flag and the --multi_file/--multif flag so the parser raises an error if both are specified; locate the parser.add_argument calls for "-multi"/"--multichannel" and "-multif"/"--multi_file" and ensure they are added to the same mutually_exclusive_group (or add a check after parser.parse_args() to exit/raise with a clear message when args.multichannel and args.multi_file are both True).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@monailabel/endpoints/datastore.py`:
- Around line 82-87: The multi-file branch incorrectly passes a temp file path
to add_directory; instead ensure a directory is provided (e.g., if image_file is
a zip or tar archive, extract it to a temporary directory) and pass that
directory path to instance.datastore().add_directory(id, directory_path,
save_params); reference functions/getters: instance.datastore(),
get_is_multi_file(), add_directory(), add_image(), and variables id, image_file,
save_params; also ensure any created temp extraction directory is cleaned up
after add_directory completes or on error.
In `@monailabel/tasks/activelearning/random.py`:
- Around line 49-61: The returned "weight" is always taken from weights[0] which
can misalign with the sampled image; locate where the sample index (e.g., idx or
i) is used to pick "image" in the random sampling function in
monailabel/tasks/activelearning/random.py and replace weights[0] with the
corresponding weight for that sampled image (e.g., weights[idx] or weights[i]);
update all three return sites that currently use weights[0] (the multichannel,
multi_file, and default return paths) to use the matched weight variable.
In `@plugins/slicer/MONAILabel/MONAILabel.py`:
- Around line 1339-1355: The code assumes nodes[0] exists after attempting to
load files from image_paths; guard against empty or failed loads by checking the
nodes list before assigning self._volumeNode. In the block that builds nodes
(using slicer.util.loadVolume and setting node names via
node.SetName(image_base_name)), verify that nodes is non-empty and that loaded
node objects are valid; if nodes is empty, either raise a clear exception or
return an appropriate error/None and log an informative message so you don't
index nodes[0] when no volumes were loaded. Ensure this check occurs immediately
before the line that assigns self._volumeNode = nodes[0].
In `@sample-apps/radiology/lib/configs/segmentation_brats.py`:
- Around line 97-107: Replace the generic trainer instantiation
lib.trainers.Segmentation with the BraTS-specific trainer implemented in
sample-apps/radiology/lib/trainers/segmentation_brats.py (import the
Segmentation trainer from that module instead of lib.trainers), and ensure you
pass the BraTS-specific parameters: set roi_size=(224,224,144), use the
trainer's Adam optimizer and DiceLoss with sigmoid=True (the segmentation_brats
trainer implements these), and rely on its specialized multi-file loading/label
handling rather than the generic preprocessing; update the instantiation (the
variable currently named task and call site lib.trainers.Segmentation) to use
the Segmentation from segmentation_brats.
In `@sample-apps/radiology/README.md`:
- Line 233: Replace the non-descriptive "[here]" link text in the "Network:"
sentence with explicit descriptive link text (e.g., "MONAI networks" or
"available MONAI networks") so the link reads "use one of the listed MONAI
networks" and update the markdown link target accordingly; edit the line
containing the "Network:" sentence to swap "[here]" for the descriptive text
while keeping the same URL.
---
Outside diff comments:
In `@plugins/slicer/MONAILabel/MONAILabel.py`:
- Around line 1356-1362: The downloaded-sample path always loads a single
volume; change it to honor multichannel the same way the local_exists path does:
detect self.multichannel and, if true, call
SampleData.SampleDataLogic.downloadFromURL in the multichannel form (pass
per-channel nodeNames/fileNames/uris or call the existing multi-channel loader
used for local samples), then assemble/assign the resulting multi-volume node to
self._volumeNode; otherwise keep the single-volume behavior. Use symbols:
serverUrl(), SampleData.SampleDataLogic.downloadFromURL, self.multichannel, and
self._volumeNode to locate and modify the logic so downloaded samples follow the
multichannel loading branch.
---
Nitpick comments:
In `@monailabel/datastore/cvat.py`:
- Around line 328-335: The method get_is_multichannel currently calls the stdlib
logging module directly (logging.info(...)) while the rest of the file uses the
module-level logger variable; change that call to use the existing logger
(logger.info(...)) so formatting/handlers remain consistent—update the logging
invocation inside get_is_multichannel to use logger.info and ensure no other
direct logging imports are used in that function.
In `@monailabel/endpoints/datastore.py`:
- Line 71: Rename the local variable currently named id to image_id to avoid
shadowing the built-in; update the assignment "id = image if image else
os.path.basename(file.filename).replace(file_ext, "")" to set image_id instead
and then replace all subsequent uses of id within the same function/scope with
image_id (references involving image, file, and file_ext). Ensure any returned
values, dictionary keys, or log messages that previously used id are updated to
use image_id so behavior is unchanged.
In `@monailabel/tasks/train/basic_train.py`:
- Around line 495-497: The request values assigned to context.multi_file and
context.input_channels can arrive as strings; coerce them to the intended types
before storing: convert multi_file to a boolean (handle truthy string forms like
"true","1" vs "false","0") when setting context.multi_file, and convert
input_channels to an int with a safe fallback when setting
context.input_channels (use request.get("input_channels", 1) as default). Update
the assignment sites (context.multi_file and context.input_channels) to perform
these explicit conversions so the training context always contains the correct
types.
In `@sample-apps/radiology/main.py`:
- Around line 310-312: The CLI allows both --multichannel and --multi_file to be
set together which should be forbidden; update the argument parsing by creating
an argparse mutually exclusive group (or add a post-parse validation) that
includes the --multichannel/--multichannel flag and the --multi_file/--multif
flag so the parser raises an error if both are specified; locate the
parser.add_argument calls for "-multi"/"--multichannel" and
"-multif"/"--multi_file" and ensure they are added to the same
mutually_exclusive_group (or add a check after parser.parse_args() to exit/raise
with a clear message when args.multichannel and args.multi_file are both True).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b05e6ac-16e5-4966-a8df-7ad01eb7e15c
📒 Files selected for processing (18)
monailabel/datastore/cvat.pymonailabel/datastore/dicom.pymonailabel/datastore/dsa.pymonailabel/datastore/local.pymonailabel/datastore/xnat.pymonailabel/endpoints/datastore.pymonailabel/interfaces/app.pymonailabel/interfaces/datastore.pymonailabel/tasks/activelearning/first.pymonailabel/tasks/activelearning/random.pymonailabel/tasks/train/basic_train.pyplugins/slicer/MONAILabel/MONAILabel.pysample-apps/radiology/README.mdsample-apps/radiology/lib/configs/segmentation_brats.pysample-apps/radiology/lib/infers/segmentation_brats.pysample-apps/radiology/lib/trainers/segmentation_brats.pysample-apps/radiology/lib/transforms/transforms.pysample-apps/radiology/main.py
🚧 Files skipped from review as they are similar to previous changes (2)
- monailabel/datastore/dicom.py
- monailabel/interfaces/app.py
monailabel/endpoints/datastore.py
Outdated
Comment on lines
+82
to
+87
| if not instance.datastore().get_is_multi_file(): | ||
| image_id = instance.datastore().add_image(id, image_file, save_params) | ||
| return {"image": image_id} | ||
| else: | ||
| directory_id = instance.datastore().add_directory(id, image_file, save_params) | ||
| return {"image": directory_id} |
There was a problem hiding this comment.
Multi-file upload path is structurally incorrect for directory samples.
Line 86 passes a temp file path to add_directory(...). In multi-file mode, each sample is expected to be a directory of modality files, so this branch can’t preserve the required structure.
🧩 Proposed fix (accept zipped directory for multi-file mode)
+import zipfile
...
- if not instance.datastore().get_is_multi_file():
- image_id = instance.datastore().add_image(id, image_file, save_params)
- return {"image": image_id}
- else:
- directory_id = instance.datastore().add_directory(id, image_file, save_params)
- return {"image": directory_id}
+ if not instance.datastore().get_is_multi_file():
+ image_id = instance.datastore().add_image(id, image_file, save_params)
+ return {"image": image_id}
+
+ if not zipfile.is_zipfile(image_file):
+ raise HTTPException(status_code=400, detail="For multi_file datastore, upload a .zip with one sample directory")
+
+ extract_dir = tempfile.mkdtemp()
+ background_tasks.add_task(remove_file, extract_dir)
+ shutil.unpack_archive(image_file, extract_dir)
+
+ entries = [os.path.join(extract_dir, e) for e in os.listdir(extract_dir)]
+ sample_dir = entries[0] if len(entries) == 1 and os.path.isdir(entries[0]) else extract_dir
+ directory_id = instance.datastore().add_directory(id, sample_dir, save_params)
+ return {"image": directory_id}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@monailabel/endpoints/datastore.py` around lines 82 - 87, The multi-file
branch incorrectly passes a temp file path to add_directory; instead ensure a
directory is provided (e.g., if image_file is a zip or tar archive, extract it
to a temporary directory) and pass that directory path to
instance.datastore().add_directory(id, directory_path, save_params); reference
functions/getters: instance.datastore(), get_is_multi_file(), add_directory(),
add_image(), and variables id, image_file, save_params; also ensure any created
temp extraction directory is cleaned up after add_directory completes or on
error.
sample-apps/radiology/README.md
Outdated
| | preload | true, **false** | Preload model into GPU at startup | | ||
| | scribbles | **true**, false | Set to `false` to disable scribble-based interactive segmentation models | | ||
|
|
||
| - Network: This model uses the [UNet](https://docs.monai.io/en/latest/networks.html#unet) as the default network. Researchers can define their own network or use one of the listed [here](https://docs.monai.io/en/latest/networks.html) |
There was a problem hiding this comment.
Use descriptive link text instead of “here”.
Line 233 uses [here], which is non-descriptive and triggers MD059; use explicit link text for readability/accessibility.
📝 Proposed fix
-- Network: This model uses the [UNet](https://docs.monai.io/en/latest/networks.html#unet) as the default network. Researchers can define their own network or use one of the listed [here](https://docs.monai.io/en/latest/networks.html)
+- Network: This model uses the [UNet](https://docs.monai.io/en/latest/networks.html#unet) as the default network. Researchers can define their own network or use one of the listed [MONAI network architectures](https://docs.monai.io/en/latest/networks.html)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - Network: This model uses the [UNet](https://docs.monai.io/en/latest/networks.html#unet) as the default network. Researchers can define their own network or use one of the listed [here](https://docs.monai.io/en/latest/networks.html) | |
| - Network: This model uses the [UNet](https://docs.monai.io/en/latest/networks.html#unet) as the default network. Researchers can define their own network or use one of the listed [MONAI network architectures](https://docs.monai.io/en/latest/networks.html) |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 233-233: Link text should be descriptive
(MD059, descriptive-link-text)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sample-apps/radiology/README.md` at line 233, Replace the non-descriptive
"[here]" link text in the "Network:" sentence with explicit descriptive link
text (e.g., "MONAI networks" or "available MONAI networks") so the link reads
"use one of the listed MONAI networks" and update the markdown link target
accordingly; edit the line containing the "Network:" sentence to swap "[here]"
for the descriptive text while keeping the same URL.
9352473 to
7fa0fd7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
monailabel/endpoints/datastore.py (1)
82-90:⚠️ Potential issue | 🔴 CriticalMulti-file upload path is currently non-functional.
image_fileis always a temp file, so the directory check always fails andadd_directory()is never reached for multi-file datastores.🧩 Proposed fix (extract uploaded archive to temp dir, then call
add_directory)if not instance.datastore().get_is_multi_file(): image_id = instance.datastore().add_image(id, image_file, save_params) return {"image": image_id} - if not os.path.isdir(image_file): - raise HTTPException(status_code=400, detail="Multi-file datastore requires a directory, not a file") - - directory_id = instance.datastore().add_directory(id, image_file, save_params) + extract_dir = tempfile.mkdtemp() + background_tasks.add_task(remove_file, extract_dir) + try: + shutil.unpack_archive(image_file, extract_dir) + except (shutil.ReadError, ValueError): + raise HTTPException( + status_code=400, + detail="Multi-file datastore requires a directory archive (.zip/.tar/...)", + ) + + entries = [os.path.join(extract_dir, e) for e in os.listdir(extract_dir)] + sample_dir = entries[0] if len(entries) == 1 and os.path.isdir(entries[0]) else extract_dir + directory_id = instance.datastore().add_directory(id, sample_dir, save_params) return {"image": directory_id}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monailabel/endpoints/datastore.py` around lines 82 - 90, The multi-file branch currently fails because image_file is a temp file (an uploaded archive) so the isdir check prevents reaching add_directory; instead, when instance.datastore().get_is_multi_file() is True and image_file is a file, detect that it's an archive and extract it into a temporary directory (use tempfile.mkdtemp() and shutil.unpack_archive or appropriate zip/tar handlers), then call instance.datastore().add_directory(id, extracted_dir, save_params) and return that id; ensure you validate the archive type and perform cleanup (shutil.rmtree) in a finally block if extraction fails or after add_directory if ownership isn't transferred.
🧹 Nitpick comments (4)
sample-apps/radiology/lib/transforms/transforms.py (1)
104-111: Remove unused/instance-level mutable transform state inLoadDirectoryImagesd.
self.spacer/self.resizerare currently unused, andself.concatdoesn’t need to be instance state. Keeping these local improves readability and avoids accidental shared-state behavior.♻️ Proposed cleanup
class LoadDirectoryImagesd(MapTransform): def __init__(self, keys: KeysCollection, target_spacing=None, allow_missing_keys: bool = False, channels: int = 2): super().__init__(keys, allow_missing_keys) self.target_spacing = target_spacing self.loader = LoadImage(reader="ITKReader", image_only=False) self.ensure_channel_first = EnsureChannelFirst() - self.spacer = Spacing(pixdim=self.target_spacing, mode="bilinear") if target_spacing else None - self.resizer = None # initialized later self.channels = int(channels) @@ - self.concat = ConcatItemsd(keys=channel_keys, name=key, dim=0) - d = self.concat(d) + concat = ConcatItemsd(keys=channel_keys, name=key, dim=0) + d = concat(d)Also applies to: 159-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-apps/radiology/lib/transforms/transforms.py` around lines 104 - 111, The class initializer creates instance-level mutable transform attributes (self.spacer, self.resizer) and an unnecessary instance-level self.concat; remove these as instance fields and instead create spacer/resizer/concat as local variables where they are actually used (e.g., inside the methods that perform spacing/resizing/concatenation). Update the __init__ in LoadDirectoryImagesd (the method shown) to drop self.spacer and self.resizer and stop assigning self.concat, and modify the code at the other referenced location (around the code originally at lines ~159-160) to instantiate Spacing(pixdim=...) / resizing transforms and the concatenation operation locally before applying them so no shared mutable transform state remains on the object.monailabel/datastore/dsa.py (1)
281-297: Inconsistent logger usage.Line 287 uses
logging.info()while line 296 useslogger.info(). The rest of the file uses the module-levelloggerinstance.♻️ Proposed fix
def get_is_multichannel(self) -> bool: """ Not implemented for this datastore Returns whether the application's studies is directed at multichannel (4D) data """ - logging.info("The function get_is_multichannel is not implemented for this datastore") + logger.info("The function get_is_multichannel is not implemented for this datastore") return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monailabel/datastore/dsa.py` around lines 281 - 297, The function get_is_multichannel uses logging.info() while the rest of the module (including get_is_multi_file) uses the module-level logger; update get_is_multichannel to use the same module-level logger (logger.info) to make logging consistent with the rest of the file and keep both get_is_multichannel and get_is_multi_file using logger.info for their informational messages.monailabel/datastore/local.py (1)
269-273: Docstring copy-paste error.The docstring for
get_is_multi_file()says "multichannel" but should say "multi-file".📝 Proposed fix
def get_is_multi_file(self) -> bool: """ - Returns whether the dataset is multichannel or not + Returns whether the dataset is multi-file or not """ return self._multi_file🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monailabel/datastore/local.py` around lines 269 - 273, The docstring for get_is_multi_file() incorrectly says "multichannel"; update the docstring text in the get_is_multi_file method to accurately describe that it "Returns whether the dataset is multi-file or not" (referencing the method name get_is_multi_file and the attribute self._multi_file) so the comment matches the actual behavior.plugins/slicer/MONAILabel/MONAILabel.py (1)
1306-1309: Avoid shadowing the built-inid.Using
idas a variable name shadows Python's built-inid()function. While it works, it can cause confusion and subtle bugs ifid()is needed later in the method.♻️ Suggested rename
- id = sample["id"] + sample_id = sample["id"] image_file = sample.get("path") - image_name = sample.get("name", id) - node_name = sample.get("PatientID", sample.get("name", id)) + image_name = sample.get("name", sample_id) + node_name = sample.get("PatientID", sample.get("name", sample_id))Then update all subsequent references to
idin this method to usesample_id.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/slicer/MONAILabel/MONAILabel.py` around lines 1306 - 1309, Rename the local variable that currently uses the built-in name id to sample_id: change the assignment sample["id"] -> sample_id and update all subsequent references in this method (e.g., where image_name = sample.get("name", id) and node_name = sample.get("PatientID", sample.get("name", id))) to use sample_id instead; ensure every use of the old id variable in this function/method (including any logging, lookups, or function calls) is replaced with sample_id to avoid shadowing the built-in id() function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sample-apps/radiology/lib/infers/segmentation_brats.py`:
- Around line 70-84: pre_transforms and post_transforms call data.get(...)
without guarding against data being None; update both methods (pre_transforms,
post_transforms) to handle a None data arg by treating it as an empty mapping
before any .get calls (e.g. normalize to data = data or {} at the start of each
method or replace data.get(...) with (data or {}).get(...)), and ensure checks
like data.get("multi_file", False) and data.get("input_channels", 4) use that
guarded form so the functions no longer raise AttributeError when invoked with
the default None.
In `@sample-apps/radiology/lib/trainers/segmentation_brats.py`:
- Around line 172-177: The norm_labels() mapping should assign contiguous
channel indices to non-background classes regardless of where "background"
appears; update norm_labels to iterate self._labels.keys(), skip key_label ==
"background", and assign incremental indices using a counter (e.g., idx = 0; for
key_label in self._labels.keys(): if key_label == "background": continue;
new_label_nums[key_label] = idx; idx += 1) so channels are contiguous and
independent of background position; reference function norm_labels and attribute
self._labels when making the change.
---
Duplicate comments:
In `@monailabel/endpoints/datastore.py`:
- Around line 82-90: The multi-file branch currently fails because image_file is
a temp file (an uploaded archive) so the isdir check prevents reaching
add_directory; instead, when instance.datastore().get_is_multi_file() is True
and image_file is a file, detect that it's an archive and extract it into a
temporary directory (use tempfile.mkdtemp() and shutil.unpack_archive or
appropriate zip/tar handlers), then call instance.datastore().add_directory(id,
extracted_dir, save_params) and return that id; ensure you validate the archive
type and perform cleanup (shutil.rmtree) in a finally block if extraction fails
or after add_directory if ownership isn't transferred.
---
Nitpick comments:
In `@monailabel/datastore/dsa.py`:
- Around line 281-297: The function get_is_multichannel uses logging.info()
while the rest of the module (including get_is_multi_file) uses the module-level
logger; update get_is_multichannel to use the same module-level logger
(logger.info) to make logging consistent with the rest of the file and keep both
get_is_multichannel and get_is_multi_file using logger.info for their
informational messages.
In `@monailabel/datastore/local.py`:
- Around line 269-273: The docstring for get_is_multi_file() incorrectly says
"multichannel"; update the docstring text in the get_is_multi_file method to
accurately describe that it "Returns whether the dataset is multi-file or not"
(referencing the method name get_is_multi_file and the attribute
self._multi_file) so the comment matches the actual behavior.
In `@plugins/slicer/MONAILabel/MONAILabel.py`:
- Around line 1306-1309: Rename the local variable that currently uses the
built-in name id to sample_id: change the assignment sample["id"] -> sample_id
and update all subsequent references in this method (e.g., where image_name =
sample.get("name", id) and node_name = sample.get("PatientID",
sample.get("name", id))) to use sample_id instead; ensure every use of the old
id variable in this function/method (including any logging, lookups, or function
calls) is replaced with sample_id to avoid shadowing the built-in id() function.
In `@sample-apps/radiology/lib/transforms/transforms.py`:
- Around line 104-111: The class initializer creates instance-level mutable
transform attributes (self.spacer, self.resizer) and an unnecessary
instance-level self.concat; remove these as instance fields and instead create
spacer/resizer/concat as local variables where they are actually used (e.g.,
inside the methods that perform spacing/resizing/concatenation). Update the
__init__ in LoadDirectoryImagesd (the method shown) to drop self.spacer and
self.resizer and stop assigning self.concat, and modify the code at the other
referenced location (around the code originally at lines ~159-160) to
instantiate Spacing(pixdim=...) / resizing transforms and the concatenation
operation locally before applying them so no shared mutable transform state
remains on the object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1bac5010-f4d7-4640-a444-267c9abfe0aa
📒 Files selected for processing (18)
monailabel/datastore/cvat.pymonailabel/datastore/dicom.pymonailabel/datastore/dsa.pymonailabel/datastore/local.pymonailabel/datastore/xnat.pymonailabel/endpoints/datastore.pymonailabel/interfaces/app.pymonailabel/interfaces/datastore.pymonailabel/tasks/activelearning/first.pymonailabel/tasks/activelearning/random.pymonailabel/tasks/train/basic_train.pyplugins/slicer/MONAILabel/MONAILabel.pysample-apps/radiology/README.mdsample-apps/radiology/lib/configs/segmentation_brats.pysample-apps/radiology/lib/infers/segmentation_brats.pysample-apps/radiology/lib/trainers/segmentation_brats.pysample-apps/radiology/lib/transforms/transforms.pysample-apps/radiology/main.py
✅ Files skipped from review due to trivial changes (1)
- sample-apps/radiology/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- monailabel/tasks/activelearning/first.py
- monailabel/interfaces/app.py
- sample-apps/radiology/main.py
CavRiley
force-pushed
the
multiChannelSupport
branch
from
1b0fca3 to
3bb8ab2
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monailabel/endpoints/datastore.py (1)
72-76:⚠️ Potential issue | 🟠 MajorMove the
multi_filecheck before writing the temp file.Background tasks registered with
BackgroundTasks.add_task()only execute if a response is successfully returned. When you raiseHTTPException(or any exception) after registering a background task, that task will not run—causing temp files to leak to disk.The current code creates
image_file(line 72), registers cleanup (line 76), then raisesHTTPExceptionifget_is_multi_file()is true (lines 82-87). Rejected uploads leave temp files behind.Move the multi-file validation before temp file creation to avoid the need for error-path cleanup:
Proposed fix
def add_image( background_tasks: BackgroundTasks, image: Optional[str] = None, params: str = Form("{}"), file: UploadFile = File(...), user: Optional[str] = None, ): logger.info(f"Image: {image}; File: {file}; params: {params}") + instance: MONAILabelApp = app_instance() + if instance.datastore().get_is_multi_file(): + raise HTTPException( + status_code=400, + detail="Multi-file datastore does not support single-file uploads. " + "Data must be pre-staged as sample subdirectories on the server filesystem.", + ) + file_ext = "".join(pathlib.Path(file.filename).suffixes) if file.filename else ".nii.gz" id = image if image else os.path.basename(file.filename).replace(file_ext, "") image_file = tempfile.NamedTemporaryFile(suffix=file_ext).name @@ - instance: MONAILabelApp = app_instance() save_params: Dict[str, Any] = json.loads(params) if params else {} if user: save_params["user"] = user - if instance.datastore().get_is_multi_file(): - raise HTTPException( - status_code=400, - detail="Multi-file datastore does not support single-file uploads. " - "Data must be pre-staged as sample subdirectories on the server filesystem.", - )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monailabel/endpoints/datastore.py` around lines 72 - 76, Move the multi-file validation before creating the temp file to avoid leaking temp files when an HTTPException is raised; specifically, call get_is_multi_file() and raise the HTTPException (if true) before invoking tempfile.NamedTemporaryFile (the image_file creation) and before registering background_tasks.add_task(remove_file, image_file). Ensure the code path that creates image_file, opens it, copies file.file, and schedules remove_file only runs after the multi-file check passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@monailabel/datastore/local.py`:
- Around line 108-109: Validate that multichannel and multi_file are not both
enabled where those parameters are accepted (e.g., in LocalDatastore.__init__
and any other function that declares multichannel: bool = False, multi_file:
bool = False); if both are True, raise a clear ValueError (or similar) rejecting
the combination and include a descriptive message stating the modes are mutually
exclusive so callers fail fast instead of sending mixed downstream signals.
In `@monailabel/tasks/activelearning/random.py`:
- Around line 45-47: The weighted pick can raise ValueError when all entries in
weights sum to zero; before calling random.choices, check if sum(weights) == 0
(or use not any(weights)) and if so choose selected_idx uniformly from
range(len(images)) (e.g., via random.randrange or random.choice) so image and
selected_weight are set safely; otherwise proceed with the existing
random.choices using weights. Update the logic around
selected_idx/selected_weight/images in random.py to implement this fallback.
In `@plugins/slicer/MONAILabel/MONAILabel.py`:
- Around line 1312-1313: The remote-fallback logic currently assumes a
single-file sample and ignores the multichannel and multi_file flags when
sample["path"] is not locally visible; update the fallback in MONAILabel.py so
that before defaulting to "/datastore/image" it inspects
sample.get("multichannel") and sample.get("multi_file") and either (a) construct
and use the appropriate remote download target for that format (e.g., call the
existing multi-file/multichannel download helper or use the format-specific
remote path used elsewhere in this module) or (b) raise a clear, fast error
indicating shared storage is required for multichannel/multi_file samples;
change the same behavior at the other occurrence noted (around the 1361–1367
logic) so both code paths treat multichannel and multi_file consistently instead
of always choosing the single-volume fallback.
In `@sample-apps/radiology/lib/infers/segmentation_brats.py`:
- Around line 161-173: The pipeline currently collapses the multi-channel
overlap prediction into exclusive class labels via
ConvertFromMultiChannelBasedOnBratsClassesd, which loses the original TC/WT/ET
overlap semantics; instead, remove or replace
ConvertFromMultiChannelBasedOnBratsClassesd and preserve the multi-channel
prediction for downstream consumers by either (a) computing centroids per
channel from the original multi-channel "pred" (use GetCentroidsd on each
channel or update GetCentroidsd to accept multi-channel inputs), or (b) emit
both representations (keep the original multi-channel "pred" and produce a
separate single-label map if needed) before calling Restored so that
Restored(keys="pred", ref_image="image", config_labels=self.labels if
data.get("restore_label_idx", False) else None) and GetCentroidsd(keys="pred",
centroids_key="centroids") operate on the correct, semantically consistent
tensor; ensure references to ConvertFromMultiChannelBasedOnBratsClassesd,
Restored, GetCentroidsd, keys="pred", and restore_label_idx are updated
accordingly.
In `@sample-apps/radiology/lib/transforms/transforms.py`:
- Around line 140-151: The current loader builds image_files by sorting
directory entries and can misorder modalities; update the logic in transforms.py
(the image_files construction around dir_path) to use an explicit
modality-to-filename mapping instead of sorted filenames: define an expected
modality list (or suffix map) based on self.channels/modality names, search
dir_path for exactly one file matching each modality pattern, raise a clear
error if any modality is missing or duplicated, and then build image_files in
the explicit modality order so subsequent checks (e.g., the 0 < self.channels !=
len(image_files) validation) remain correct.
In `@sample-apps/radiology/main.py`:
- Around line 317-321: The CLI flags multichannel, input_channels and multi_file
are only set in the conf dict at startup but not forwarded in request payloads,
so infer, batch_infer and train flows still use defaults; update the request
construction logic (where requests are sent to the BraTS infer/train endpoints)
to include these flags from conf: add conf["multichannel"],
conf["input_channels"], and conf["multi_file"] into the payload/context for the
infer function, batch_infer function, and train function so the server-side
BraTS handlers read the intended values.
---
Outside diff comments:
In `@monailabel/endpoints/datastore.py`:
- Around line 72-76: Move the multi-file validation before creating the temp
file to avoid leaking temp files when an HTTPException is raised; specifically,
call get_is_multi_file() and raise the HTTPException (if true) before invoking
tempfile.NamedTemporaryFile (the image_file creation) and before registering
background_tasks.add_task(remove_file, image_file). Ensure the code path that
creates image_file, opens it, copies file.file, and schedules remove_file only
runs after the multi-file check passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ffa523c7-1516-45f7-94f0-3bbf4ed87c16
📒 Files selected for processing (18)
monailabel/datastore/cvat.pymonailabel/datastore/dicom.pymonailabel/datastore/dsa.pymonailabel/datastore/local.pymonailabel/datastore/xnat.pymonailabel/endpoints/datastore.pymonailabel/interfaces/app.pymonailabel/interfaces/datastore.pymonailabel/tasks/activelearning/first.pymonailabel/tasks/activelearning/random.pymonailabel/tasks/train/basic_train.pyplugins/slicer/MONAILabel/MONAILabel.pysample-apps/radiology/README.mdsample-apps/radiology/lib/configs/segmentation_brats.pysample-apps/radiology/lib/infers/segmentation_brats.pysample-apps/radiology/lib/trainers/segmentation_brats.pysample-apps/radiology/lib/transforms/transforms.pysample-apps/radiology/main.py
🚧 Files skipped from review as they are similar to previous changes (5)
- monailabel/tasks/train/basic_train.py
- monailabel/interfaces/app.py
- monailabel/datastore/cvat.py
- monailabel/interfaces/datastore.py
- sample-apps/radiology/README.md
Comment on lines
+161
to
+173
| t.extend( | ||
| [ | ||
| # Merge 3 binary channels → single-channel integer label map | ||
| # Must happen before Restored so spatial metadata is applied | ||
| # to the final (1, H, W, D) output, not the intermediate (3, H, W, D). | ||
| ConvertFromMultiChannelBasedOnBratsClassesd(keys="pred"), | ||
| Restored( | ||
| keys="pred", | ||
| ref_image="image", | ||
| config_labels=self.labels if data.get("restore_label_idx", False) else None, | ||
| ), | ||
| GetCentroidsd(keys="pred", centroids_key="centroids"), | ||
| ] |
There was a problem hiding this comment.
This post-process changes the task from multilabel regions to exclusive classes.
pred starts as TC/WT/ET, but ConvertFromMultiChannelBasedOnBratsClassesd collapses it into one voxel label per location. That drops the original overlap semantics and makes the returned mask inconsistent with the advertised labels (tumor core, whole tumor, enhancing tumor). Downstream consumers will read the output as region labels even though it now encodes a different class map.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sample-apps/radiology/lib/infers/segmentation_brats.py` around lines 161 -
173, The pipeline currently collapses the multi-channel overlap prediction into
exclusive class labels via ConvertFromMultiChannelBasedOnBratsClassesd, which
loses the original TC/WT/ET overlap semantics; instead, remove or replace
ConvertFromMultiChannelBasedOnBratsClassesd and preserve the multi-channel
prediction for downstream consumers by either (a) computing centroids per
channel from the original multi-channel "pred" (use GetCentroidsd on each
channel or update GetCentroidsd to accept multi-channel inputs), or (b) emit
both representations (keep the original multi-channel "pred" and produce a
separate single-label map if needed) before calling Restored so that
Restored(keys="pred", ref_image="image", config_labels=self.labels if
data.get("restore_label_idx", False) else None) and GetCentroidsd(keys="pred",
centroids_key="centroids") operate on the correct, semantically consistent
tensor; ensure references to ConvertFromMultiChannelBasedOnBratsClassesd,
Restored, GetCentroidsd, keys="pred", and restore_label_idx are updated
accordingly.
0c54551 to
9dc1c0a
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
plugins/slicer/MONAILabel/MONAILabel.py (1)
1361-1367:⚠️ Potential issue | 🟠 MajorRemote fallback still assumes a single-file sample.
When
sample["path"]is not locally visible, this branch always downloads/datastore/imageand feeds it through the single-volume loader. Remotemulti_filedirectories and remote 4Dmultichannelvolumes still bypass the new loading paths, so the feature only works with shared storage. Please add a format-specific download path here, or fail fast with a clear shared-storage requirement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/slicer/MONAILabel/MONAILabel.py` around lines 1361 - 1367, The remote-download branch always builds a single-file download URI and calls SampleData.SampleDataLogic.downloadFromURL, which breaks for remote multi_file or multichannel samples; update the logic in MONAILabel.py around download_uri/self._volumeNode to inspect the sample metadata (e.g., sample["format"], sample.get("multi_file"), sample.get("multichannel")) and choose a format-specific download endpoint or loader: for multi_file samples call the directory/multi-file download endpoint (or construct a URI that the existing multi-file loader expects) and pass the resulting file list to SampleData.SampleDataLogic.downloadFromURL, and for multichannel/4D volumes use the multi-channel download path; if a format-specific remote download is not available, raise a clear error indicating that shared storage is required. Ensure you reference and update the download_uri construction, the call to self.serverUrl(), and the call site self._volumeNode = SampleData.SampleDataLogic().downloadFromURL(...) to implement the branching or the fail-fast behavior.sample-apps/radiology/lib/infers/segmentation_brats.py (1)
161-172:⚠️ Potential issue | 🔴 CriticalThis post-process mislabels the returned BraTS regions.
ConvertFromMultiChannelBasedOnBratsClassesdcollapses[TC, WT, ET]into exclusive labels, butsample-apps/radiology/lib/configs/segmentation_brats.pystill advertises ids1/2/3astumor core / whole tumor / enhancing tumor. Downstream consumers then interpret the saved mask and centroids with the wrong names. Keep the 3-channel region output for downstream consumers, or emit a separate single-label map with label metadata that matches the collapsed classes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-apps/radiology/lib/infers/segmentation_brats.py` around lines 161 - 172, ConvertFromMultiChannelBasedOnBratsClassesd is collapsing the three BraTS channels into an exclusive single-label map which makes labels/centroids mismatched with the advertised ids; instead either preserve the original 3-channel output and produce a separate single-channel map with correct label metadata, or change the pipeline to produce both: keep the original "pred" as the (3, H, W, D) tensor, and add a new transform (e.g., ConvertFromMultiChannelBasedOnBratsClassesd -> keys="pred_collapsed", or create_pred_label_map) that writes a separate single-label map with config_labels=self.labels so Restored(keys="pred") and GetCentroidsd(keys="pred", centroids_key="centroids") operate on the 3-channel output while downstream consumers can use "pred_collapsed" (or update GetCentroidsd to point to the correct key) for single-label consumers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@monailabel/interfaces/datastore.py`:
- Around line 204-215: The Datastore public base class currently declares
add_directory(), get_is_multichannel(), and get_is_multi_file() as abstract
methods which breaks backward compatibility; change these to concrete methods on
the Datastore class by providing sensible default implementations (e.g.,
add_directory returns the provided directory_id or derives it from filename and
returns a string; get_is_multichannel and get_is_multi_file return False) so
existing out-of-tree subclasses can still instantiate without modification,
while built-in datastores (LocalDatastore, XNATDatastore, DSADatastore) may
override them when needed.
In `@monailabel/tasks/activelearning/random.py`:
- Around line 45-50: The current guard uses "if not any(weights)" which misses
cases where weights include negatives (e.g., future/corrupt timestamps) so
random.choices can be passed a non-positive total; update the weight computation
to clamp each computed delta to be >= 0 (replace negative deltas with 0), then
check the total sum (sum(weights) > 0) before calling random.choices; if the
total is zero fall back to uniform selection (random.randrange). Ensure you
update the logic around the "weights" list and the "selected_idx" selection to
use the clamped weights and the sum check.
In `@sample-apps/radiology/lib/configs/segmentation_brats.py`:
- Around line 53-58: The current check uses
strtobool(self.conf.get("use_pretrained_model", "false")) which defaults to
"false" and prevents downloading the BraTS checkpoint; change the default to
"true" (i.e., self.conf.get("use_pretrained_model", "true")) so the
download_file call (using PRE_TRAINED_PATH and saving to self.path[0]) runs by
default for the documented BraTS flow; ensure the "use_pretrained_model" key is
documented if you prefer keeping a different default.
In `@sample-apps/radiology/lib/transforms/transforms.py`:
- Around line 166-176: The loop that sets resizer on the first image and reuses
it (resizer, Resize) will silently interpolate later modalities to the first
volume shape and can scramble channel/affine alignment; update the logic in the
loader loop (image_files, loader, ensure_channel_first) to validate each image's
spatial geometry (shape, spacing, orientation/affine) against the first volume
and either (a) raise/log and skip/abort on mismatch or (b) perform an
orientation- and spacing-aware resample per-image using a proper affine-aware
resampling routine (not the current Resize) before stacking; ensure any
automatic resampling uses the image-specific affine/spacing and update/remove
the shared resizer variable so each modality is handled with explicit
geometry-aware transforms and failures are logged with dir_path and image path.
---
Duplicate comments:
In `@plugins/slicer/MONAILabel/MONAILabel.py`:
- Around line 1361-1367: The remote-download branch always builds a single-file
download URI and calls SampleData.SampleDataLogic.downloadFromURL, which breaks
for remote multi_file or multichannel samples; update the logic in MONAILabel.py
around download_uri/self._volumeNode to inspect the sample metadata (e.g.,
sample["format"], sample.get("multi_file"), sample.get("multichannel")) and
choose a format-specific download endpoint or loader: for multi_file samples
call the directory/multi-file download endpoint (or construct a URI that the
existing multi-file loader expects) and pass the resulting file list to
SampleData.SampleDataLogic.downloadFromURL, and for multichannel/4D volumes use
the multi-channel download path; if a format-specific remote download is not
available, raise a clear error indicating that shared storage is required.
Ensure you reference and update the download_uri construction, the call to
self.serverUrl(), and the call site self._volumeNode =
SampleData.SampleDataLogic().downloadFromURL(...) to implement the branching or
the fail-fast behavior.
In `@sample-apps/radiology/lib/infers/segmentation_brats.py`:
- Around line 161-172: ConvertFromMultiChannelBasedOnBratsClassesd is collapsing
the three BraTS channels into an exclusive single-label map which makes
labels/centroids mismatched with the advertised ids; instead either preserve the
original 3-channel output and produce a separate single-channel map with correct
label metadata, or change the pipeline to produce both: keep the original "pred"
as the (3, H, W, D) tensor, and add a new transform (e.g.,
ConvertFromMultiChannelBasedOnBratsClassesd -> keys="pred_collapsed", or
create_pred_label_map) that writes a separate single-label map with
config_labels=self.labels so Restored(keys="pred") and
GetCentroidsd(keys="pred", centroids_key="centroids") operate on the 3-channel
output while downstream consumers can use "pred_collapsed" (or update
GetCentroidsd to point to the correct key) for single-label consumers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92b1fd7d-7290-470c-93d3-037a5c8e0c2d
📒 Files selected for processing (18)
monailabel/datastore/cvat.pymonailabel/datastore/dicom.pymonailabel/datastore/dsa.pymonailabel/datastore/local.pymonailabel/datastore/xnat.pymonailabel/endpoints/datastore.pymonailabel/interfaces/app.pymonailabel/interfaces/datastore.pymonailabel/tasks/activelearning/first.pymonailabel/tasks/activelearning/random.pymonailabel/tasks/train/basic_train.pyplugins/slicer/MONAILabel/MONAILabel.pysample-apps/radiology/README.mdsample-apps/radiology/lib/configs/segmentation_brats.pysample-apps/radiology/lib/infers/segmentation_brats.pysample-apps/radiology/lib/trainers/segmentation_brats.pysample-apps/radiology/lib/transforms/transforms.pysample-apps/radiology/main.py
🚧 Files skipped from review as they are similar to previous changes (4)
- monailabel/datastore/dsa.py
- monailabel/datastore/dicom.py
- monailabel/tasks/train/basic_train.py
- monailabel/interfaces/app.py
Adds flags to the local datastore init signaling the data is either multichannel or multi-file (each directory has a sample with multiple volumes Signed-off-by: Cavan Riley <cavan-riley@uiowa.edu>
Adds ability to load multiple volumes per sample or load sequence data depending on request Signed-off-by: Cavan Riley <cavan-riley@uiowa.edu>
BraTS sample application addition using BraTS 2020 multi-volume data Signed-off-by: Cavan Riley <cavan-riley@uiowa.edu>
Signed-off-by: Cavan Riley <cavan-riley@uiowa.edu>
CavRiley
force-pushed
the
multiChannelSupport
branch
from
b83e07e to
041b610
Compare
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
1 participant
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.
Overview
Adds ability for training multichannel segmentation models on pre-aligned/co-registered volumes using the local datastore. I was working based off this issue #241
I added a working implementation for the BraTS 2020 dataset with individual volumes as a reference use case.
Changes
--input_channelsargument to support multichannel model configurations--multi_fileargument to handle multi-file volume inputs (each sample has a directory with multiple volumes)--multichannelargument to handle multichannel inputs (example sample has a single image with multiple volumes)Results (BraTS 2020 — 150 epochs)
Notes
I'd be happy to incorporate any feedback from maintainers so this could be added
Summary by CodeRabbit
New Features
Behavior Changes
Documentation