Add lightweight reviewer app and review endpoints - #1907
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 a review-only sample app, datastore-backed review APIs, report generation, Slicer image-loading integration, ChangesReviewer workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant LightweightReviewClient
participant ReviewAPI
participant LocalDatastore
LightweightReviewClient->>ReviewAPI: Request review cases or reports
ReviewAPI->>LocalDatastore: Read image and label metadata
LocalDatastore-->>ReviewAPI: Return review data
ReviewAPI-->>LightweightReviewClient: Return cases, versions, or reports
LightweightReviewClient->>ReviewAPI: Submit label metadata or label
ReviewAPI->>LocalDatastore: Persist review data
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@monailabel/datastore/local.py`:
- Around line 228-232: Update save_label to derive the label identifier and
extension through _to_label_id instead of _to_id, preserving the .seg.nrrd
suffix when re-saving labels discovered by _add_non_existing_labels. Keep the
existing save behavior unchanged for other extensions.
In `@monailabel/endpoints/datastore_review.py`:
- Around line 156-172: Update _render_csv to sanitize free-text CSV values
before writer.writerow serializes them, especially comment and reviewer, by
prefixing values beginning with =, +, -, or @ so spreadsheet applications treat
them as text. Apply the same protection consistently to other user-controlled
fields while preserving normal values and CSV formatting.
- Around line 88-101: Ensure label updates populate last_reviewed so date
filtering can match reviewed cases. Update LocalDatastore.update_label_info to
preserve an existing timestamp or stamp the server-side update time, with a
documented fallback to the label’s existing ts when appropriate. Ensure
_reviews_case exposes that populated value through
label_info.get("last_reviewed") without changing unrelated case fields.
In `@plugins/slicer/MONAILabelReviewer/MONAILabelReviewer.py`:
- Around line 1729-1731: Constrain the server-derived image_name in the download
flow around getPathToStore before writing response.content, ensuring the
resolved destination remains within self.temp_dir.name; reject or safely
normalize traversal and absolute-path components, while preserving valid
filenames and the existing file-write behavior.
In `@sample-apps/reviewer/app.py`:
- Around line 76-84: Update the mode assignment in the configuration-loading
flow to use the existing configured mode as the fallback when
MONAI_LABEL_REVIEW_MODE is unset. Preserve an explicitly provided environment
override while ensuring conf_dict’s mode, such as “standalone”, is not replaced
by “review”.
In `@sample-apps/reviewer/client.py`:
- Around line 171-174: Update the label download method around the response
handling to return response.content instead of calling response.json(), since
/datastore/label returns binary NIfTI/NRRD data. Change the method’s return type
annotation to reflect bytes while preserving the existing successful-response
logging and return flow.
- Around line 254-259: Apply self.timeout to both write requests: add the
configured timeout to the label-info PUT and the label-upload PUT in
sample-apps/reviewer/client.py at lines 254-259 and 308-314, respectively. No
other request behavior should change.
- Line 416: Update create_client() and the LightweightReviewClient constructor
flow so the default None server_url is replaced with the appropriate usable
default before __init__ calls server_url.rstrip("/"). Preserve explicit
server_url values while ensuring calling create_client() without arguments
succeeds.
In `@sample-apps/reviewer/README.md`:
- Around line 209-211: Update the README configuration example under “Mode” to
use the environment variable read by the app, MONAI_LABEL_REVIEW_MODE, instead
of the unused MONAI_LABEL_REVIEW_ENABLED.
- Around line 181-184: Update the documented PUT /datastore/label/info request
in the README to submit the JSON payload using the server-supported info form
field instead of approvals, while preserving the existing payload contents.
In `@test_reviewer_server.py`:
- Around line 1-16: Prevent pytest from auto-discovering the manual smoke-test
functions in test_reviewer_server.py by relocating the script under scripts/ and
renaming it without a test_ prefix, such as manual_reviewer_smoke_test.py.
Preserve the existing functions test_list_images, test_download_image,
test_download_label, test_update_labelinfo, and test_generate_report as manual
checks rather than converting them into CI tests.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 28bdeb2d-21cb-44fa-8481-f781e6f709dd
📒 Files selected for processing (15)
README.mdmonailabel/app.pymonailabel/datastore/local.pymonailabel/endpoints/datastore_review.pyplugins/slicer/MONAILabelReviewer/MONAILabelReviewer.pyplugins/slicer/MONAILabelReviewer/MONAILabelReviewerLib/ImageDataController.pyplugins/slicer/MONAILabelReviewer/MONAILabelReviewerLib/MonaiServerREST.pysample-apps/reviewer/README.mdsample-apps/reviewer/app.pysample-apps/reviewer/client.pysample-apps/reviewer/lib/__init__.pysample-apps/reviewer/lib/config.pysample-apps/reviewer/main.pytest_reviewer_server.pytests/unit/datastore/test_local.py
|
Note Docstrings generation - SUCCESS |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 7 file(s) based on 11 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Docstrings generation was requested by @chezhia. * #1907 (comment) The following files were modified: * `monailabel/datastore/local.py` * `monailabel/endpoints/datastore_review.py` * `plugins/slicer/MONAILabelReviewer/MONAILabelReviewer.py` * `plugins/slicer/MONAILabelReviewer/MONAILabelReviewerLib/ImageDataController.py` * `plugins/slicer/MONAILabelReviewer/MONAILabelReviewerLib/MonaiServerREST.py` * `sample-apps/reviewer/app.py` * `sample-apps/reviewer/client.py` * `sample-apps/reviewer/lib/config.py`
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@monailabel/datastore/local.py`:
- Around line 561-573: Update the last_reviewed handling around the existing
metadata fields so explicit incoming info["last_reviewed"] values remain
unchanged, while updates containing review metadata such as status, level,
comment, reviewer, or reviewer_name receive datetime.now().isoformat() instead
of reusing the prior timestamp. Retain the existing fallback behavior for
updates without review metadata.
In `@tests/unit/sample_apps/test_reviewer_app.py`:
- Around line 29-30: Update the patch.dict call surrounding
app._load_review_config in the test to use clear=True instead of clear=False,
ensuring inherited environment variables such as MONAI_LABEL_REVIEW_MODE are
removed while preserving the test’s provided inputs.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bbf738aa-f472-4c88-b23e-2ee84839586d
📒 Files selected for processing (9)
monailabel/datastore/local.pymonailabel/endpoints/datastore_review.pyplugins/slicer/MONAILabelReviewer/MONAILabelReviewer.pysample-apps/reviewer/README.mdsample-apps/reviewer/app.pysample-apps/reviewer/client.pytests/unit/datastore/test_local.pytests/unit/sample_apps/test_reviewer_app.pytests/unit/sample_apps/test_reviewer_client.py
🚧 Files skipped from review as they are similar to previous changes (4)
- sample-apps/reviewer/README.md
- sample-apps/reviewer/app.py
- plugins/slicer/MONAILabelReviewer/MONAILabelReviewer.py
- monailabel/endpoints/datastore_review.py
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/manual_reviewer_smoke_test.py`:
- Around line 32-38: Remove the unnecessary f-string prefixes from the literal
arguments in the command construction around the app, studies, port, and conf
options, including the corresponding block near the additionally referenced
lines. Preserve f-strings only for values that interpolate variables such as
TEST_DATA_DIR or PORT.
- Around line 47-71: Update the server startup flow around the subprocess.Popen
readiness loop to use nonblocking merged stdout/stderr handling, check
proc.poll() and fail immediately when the child exits, and enforce an unused
test port before startup and readiness polling. Validate that the readiness
response belongs to the spawned process rather than accepting an existing
service on port 8079, preventing review metadata from being sent to another
server.
- Around line 157-161: Update the download success checks in the image and label
download paths to require both HTTP status 200 and non-empty response content
before returning success. Apply this consistently to the checks around the image
download and the additional locations at the indicated response-handling
branches, preserving the existing success logging for valid non-empty downloads.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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 Plus
Run ID: 5a89550a-de23-49aa-b85c-e59ba858c2ce
📒 Files selected for processing (3)
sample-apps/reviewer/README.mdscripts/manual_reviewer_smoke_test.pytests/unit/sample_apps/test_reviewer_app.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/sample_apps/test_reviewer_app.py
- sample-apps/reviewer/README.md
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
scripts/manual_reviewer_smoke_test.py (3)
32-38: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove extraneous f-string prefixes (still unresolved).
Ruff F541 still fires on
f"--app",f"--studies",f"--port",f"--conf"(L32,34,36,38) andf"✅ Successfully listed images",f"\n Sample images:"(L105,110) — none contain placeholders.Also applies to: 105-110
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/manual_reviewer_smoke_test.py` around lines 32 - 38, Remove the unnecessary f-string prefixes from the constant CLI argument strings and fixed output messages in the smoke test, including the arguments near the command construction and the success/listing messages in the relevant output section; preserve their exact string contents.Source: Linters/SAST tools
47-73: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winServer readiness check can still block indefinitely and validate the wrong server (still unresolved).
proc.stdout.readline()can block, theexcept: passswallows all errors silently, andwait_for_server_readynever callsproc.poll()nor confirms the responding server on port 8079 is the spawned child (a port collision could make this script mutate a different server's review metadata).Also applies to: 76-88
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/manual_reviewer_smoke_test.py` around lines 47 - 73, Update wait_for_server_ready to use non-blocking readiness checks with a hard timeout, report subprocess errors instead of swallowing them, and call proc.poll() to detect early process exit. Confirm the spawned server itself is serving on port 8079 before returning, preventing an existing process on that port from being treated as ready.Source: Linters/SAST tools
157-161: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEmpty download bodies still pass as success (still unresolved).
download_response.status_code == 200alone is treated as success for image/label downloads; an empty body would silently pass, masking a broken datastore response.Also applies to: 198-212
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/manual_reviewer_smoke_test.py` around lines 157 - 161, Update the download success checks around the image and label response handlers to require both HTTP 200 and a non-empty response body before returning success. Apply the same validation to the additional block referenced by the comment, preserving the existing logging and failure behavior for empty downloads.
🧹 Nitpick comments (3)
tests/unit/datastore/test_local.py (1)
99-120: 📐 Maintainability & Code Quality | 🔵 TrivialTest relies on
get_label_inforeturning a mutable internal reference.The legacy-
ts-fallback scenario is set up by mutating the dict returned fromget_label_infoin place, rather than going through a public write path. This is a fragile assumption about the return value's identity; ifget_label_infois later changed to return a defensive copy, this test would stop exercising the fallback behavior without failing loudly.Consider setting up the legacy state through
save_label/update_label_info(or another public API) instead of mutating the returned dict directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/datastore/test_local.py` around lines 99 - 120, Update test_update_label_info_falls_back_to_existing_ts_for_legacy_reviews so the legacy ts and approved status are persisted through LocalDatastore’s public APIs, such as save_label or update_label_info, rather than mutating the dictionary returned by get_label_info. Keep the test focused on verifying that update_label_info preserves the existing timestamp as last_reviewed for legacy reviews.monailabel/endpoints/datastore_review.py (2)
104-133: 🚀 Performance & Scalability | 🔵 Trivial
/review/casesand/review/reportdo a full datastore scan on every request.
_list_review_casesiterates every image and issuesget_image_info/get_label_info/get_labels_by_image_idper image, with no caching, on every call. For large datastores this makes both/review/cases(likely polled by the Slicer UI) and/review/reportexpensive per-request I/O operations. Consider caching aggregated case metadata and invalidating on label updates if this is expected to scale beyond small review sets.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monailabel/endpoints/datastore_review.py` around lines 104 - 133, Update _list_review_cases to reuse a cache of aggregated review-case metadata instead of scanning the datastore and fetching per-image details on every request. Add cache invalidation tied to label updates so /review/cases and /review/report observe updated statuses, reviewers, dates, and tags while avoiding repeated full datastore I/O.
80-101: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConfirm
get_label_infolookup key is correct for non-Local datastores.
_review_casepassesimage_idas thelabel_idargument todatastore.get_label_info(image_id, tag), whileapi_review_versionscorrectly resolveslabel_idfromget_labels_by_image_id()first. Per the Datastore interface,get_label_info(label_id, label_tag)expects the label id, which only coincides withimage_idforLocalDatastore. For DSA/XNAT-backed reviewers this could silently return{}for every case.For consistency (and to be safe if this app is ever pointed at a non-local datastore), consider resolving
label_id = labels.get(tag)before the lookup, mirroringapi_review_versions.Also applies to: 234-258
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monailabel/endpoints/datastore_review.py` around lines 80 - 101, Update _review_case to resolve label_id from labels.get(tag) before calling _safe_label_info, and pass that label ID to the label-info lookup instead of image_id. Preserve the existing fallback behavior for missing labels and keep the remaining status and response construction unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@monailabel/endpoints/datastore_review.py`:
- Around line 261-287: Update api_review_report so fmt=csv and fmt=html return
raw rendered content with the appropriate text media type instead of JSON
envelopes; preserve the existing JSON response structure for fmt=json and the
invalid-format error behavior.
In `@sample-apps/reviewer/app.py`:
- Line 62: Update the logger.info call in the Reviewer App initialization to use
a regular string literal instead of an f-string, since it contains no
interpolation placeholders.
In `@sample-apps/reviewer/client.py`:
- Around line 234-265: Update update_labelinfo’s review_info construction so
reviewer_name and workflow_id are only included when they have values, matching
the conditional handling of level, comment, and reviewer_email. Preserve the
existing payload and update behavior while preventing absent arguments from
overwriting stored label information with null.
- Around line 290-310: Update the multipart files payload in the request flow
around label_file and files so the uploaded label uses label_file.name, or
another explicit filename preserving the source file’s extension, instead of
image_id. Keep the existing upload endpoint and request behavior unchanged.
---
Duplicate comments:
In `@scripts/manual_reviewer_smoke_test.py`:
- Around line 32-38: Remove the unnecessary f-string prefixes from the constant
CLI argument strings and fixed output messages in the smoke test, including the
arguments near the command construction and the success/listing messages in the
relevant output section; preserve their exact string contents.
- Around line 47-73: Update wait_for_server_ready to use non-blocking readiness
checks with a hard timeout, report subprocess errors instead of swallowing them,
and call proc.poll() to detect early process exit. Confirm the spawned server
itself is serving on port 8079 before returning, preventing an existing process
on that port from being treated as ready.
- Around line 157-161: Update the download success checks around the image and
label response handlers to require both HTTP 200 and a non-empty response body
before returning success. Apply the same validation to the additional block
referenced by the comment, preserving the existing logging and failure behavior
for empty downloads.
---
Nitpick comments:
In `@monailabel/endpoints/datastore_review.py`:
- Around line 104-133: Update _list_review_cases to reuse a cache of aggregated
review-case metadata instead of scanning the datastore and fetching per-image
details on every request. Add cache invalidation tied to label updates so
/review/cases and /review/report observe updated statuses, reviewers, dates, and
tags while avoiding repeated full datastore I/O.
- Around line 80-101: Update _review_case to resolve label_id from
labels.get(tag) before calling _safe_label_info, and pass that label ID to the
label-info lookup instead of image_id. Preserve the existing fallback behavior
for missing labels and keep the remaining status and response construction
unchanged.
In `@tests/unit/datastore/test_local.py`:
- Around line 99-120: Update
test_update_label_info_falls_back_to_existing_ts_for_legacy_reviews so the
legacy ts and approved status are persisted through LocalDatastore’s public
APIs, such as save_label or update_label_info, rather than mutating the
dictionary returned by get_label_info. Keep the test focused on verifying that
update_label_info preserves the existing timestamp as last_reviewed for legacy
reviews.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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 Plus
Run ID: 21163d30-4e15-40ed-86d2-b6fdb05067ce
📒 Files selected for processing (17)
README.mdmonailabel/app.pymonailabel/datastore/local.pymonailabel/endpoints/datastore_review.pyplugins/slicer/MONAILabelReviewer/MONAILabelReviewer.pyplugins/slicer/MONAILabelReviewer/MONAILabelReviewerLib/ImageDataController.pyplugins/slicer/MONAILabelReviewer/MONAILabelReviewerLib/MonaiServerREST.pysample-apps/reviewer/README.mdsample-apps/reviewer/app.pysample-apps/reviewer/client.pysample-apps/reviewer/lib/__init__.pysample-apps/reviewer/lib/config.pysample-apps/reviewer/main.pyscripts/manual_reviewer_smoke_test.pytests/unit/datastore/test_local.pytests/unit/sample_apps/test_reviewer_app.pytests/unit/sample_apps/test_reviewer_client.py
🚧 Files skipped from review as they are similar to previous changes (6)
- sample-apps/reviewer/main.py
- plugins/slicer/MONAILabelReviewer/MONAILabelReviewerLib/ImageDataController.py
- monailabel/app.py
- sample-apps/reviewer/lib/config.py
- tests/unit/sample_apps/test_reviewer_app.py
- monailabel/datastore/local.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sample-apps/reviewer/client.py (1)
359-379: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle non-JSON report formats separately.
generate_report()supportscsvandhtml, but the successful response is always passed through_json_dict(). CSV/HTML responses will raiseresponse.json()failures and bubble back as{"status": "error", ...}; use raw payload handling or restrict the client method to JSON.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sample-apps/reviewer/client.py` around lines 359 - 379, Update generate_report to handle fmt values csv and html without passing successful responses through _json_dict: return the raw response payload in the appropriate form for non-JSON formats, while preserving _json_dict for json reports. Alternatively, explicitly restrict supported formats to json and reject csv/html before making the request, keeping the method’s documented behavior consistent.
🧹 Nitpick comments (1)
sample-apps/reviewer/client.py (1)
355-357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the broad exception handler.
Catching
Exceptionmasks programming errors and makes unexpected failures indistinguishable from request failures. Catchrequests.exceptions.RequestException(plus the specific JSON-decoding exception if required by the response contract).Proposed fix
- except Exception as e: + except requests.exceptions.RequestException as e:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sample-apps/reviewer/client.py` around lines 355 - 357, Update the exception handler surrounding the version request in the image-version retrieval flow to catch requests.exceptions.RequestException instead of broad Exception; include the specific JSON-decoding exception only if the response contract requires handling malformed JSON. Preserve the existing logger.error message and error response for the handled request failures.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@sample-apps/reviewer/client.py`:
- Around line 359-379: Update generate_report to handle fmt values csv and html
without passing successful responses through _json_dict: return the raw response
payload in the appropriate form for non-JSON formats, while preserving
_json_dict for json reports. Alternatively, explicitly restrict supported
formats to json and reject csv/html before making the request, keeping the
method’s documented behavior consistent.
---
Nitpick comments:
In `@sample-apps/reviewer/client.py`:
- Around line 355-357: Update the exception handler surrounding the version
request in the image-version retrieval flow to catch
requests.exceptions.RequestException instead of broad Exception; include the
specific JSON-decoding exception only if the response contract requires handling
malformed JSON. Preserve the existing logger.error message and error response
for the handled request failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ba0e6095-1a31-4f71-ac9b-e15fca884d09
📒 Files selected for processing (4)
plugins/slicer/MONAILabelReviewer/MONAILabelReviewer.pysample-apps/reviewer/app.pysample-apps/reviewer/client.pyscripts/manual_reviewer_smoke_test.py
💤 Files with no reviewable changes (1)
- plugins/slicer/MONAILabelReviewer/MONAILabelReviewer.py
🚧 Files skipped from review as they are similar to previous changes (2)
- sample-apps/reviewer/app.py
- scripts/manual_reviewer_smoke_test.py
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
There was a problem hiding this comment.
Pull request overview
This PR adds a lightweight “reviewer” workflow to MONAILabel for reviewing existing segmentations without loading AI inference/training tasks. It introduces a reviewer sample app and client, new authenticated /review/* aggregate endpoints, and corresponding 3D Slicer reviewer plugin fixes, plus datastore updates to better handle reviewer label conventions.
Changes:
- Added a new
sample-apps/reviewerapp + lightweight client for review-only workflows. - Introduced
/review/cases,/review/versions, and/review/reportendpoints and mounted them in the main FastAPI app. - Updated local datastore reconciliation for
.seg.nrrdlabels and enhanced review timestamp tracking; updated Slicer plugin URL normalization and image download behavior; added tests and docs.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/sample_apps/test_reviewer_client.py | Unit tests validating reviewer client timeout and URL normalization behavior |
| tests/unit/sample_apps/test_reviewer_app.py | Unit tests validating reviewer app config/env override behavior |
| tests/unit/datastore/test_local.py | Unit tests for .seg.nrrd label handling and last_reviewed behavior in LocalDatastore |
| scripts/manual_reviewer_smoke_test.py | Manual end-to-end smoke test script for the reviewer server endpoints |
| sample-apps/reviewer/README.md | Reviewer sample app documentation and workflow guide |
| sample-apps/reviewer/main.py | MONAILabel app entrypoint for the reviewer sample app |
| sample-apps/reviewer/lib/config.py | Reviewer app configuration helper module |
| sample-apps/reviewer/lib/init.py | Package init for reviewer sample’s lib module |
| sample-apps/reviewer/client.py | Lightweight Python client for reviewer workflow routes |
| sample-apps/reviewer/app.py | Reviewer MONAILabelApp implementation with no AI tasks |
| README.md | Top-level documentation updated to include the reviewer workflow |
| plugins/slicer/MONAILabelReviewer/MONAILabelReviewerLib/MonaiServerREST.py | Slicer plugin REST layer: URL normalization and direct image request helper |
| plugins/slicer/MONAILabelReviewer/MONAILabelReviewerLib/ImageDataController.py | Slicer plugin controller: added image request passthrough |
| plugins/slicer/MONAILabelReviewer/MONAILabelReviewer.py | Slicer plugin UI/logic updates: URL normalization, safer temp-file handling, direct image downloads |
| monailabel/endpoints/datastore_review.py | New /review aggregate endpoints (cases, versions, report) |
| monailabel/datastore/local.py | LocalDatastore updates for .seg.nrrd label IDs/extensions and last_reviewed handling |
| monailabel/app.py | Main FastAPI app updated to mount the new review router |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
monailabel/endpoints/datastore_review.py (2)
40-48: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not swallow datastore failures as empty metadata.
Only
LabelNotFoundExceptionrepresents an expected missing-label case. Catching every other exception makes I/O, corruption, or storage failures look like unlabeled/pending cases and silently produces incorrect reports. Re-raise unexpected exceptions or catch only documented datastore exceptions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monailabel/endpoints/datastore_review.py` around lines 40 - 48, Update _safe_label_info to handle only LabelNotFoundException as an empty metadata result; remove the broad Exception catch and allow unexpected datastore failures to propagate, or narrowly catch only documented datastore exceptions without converting them to {}.
65-77: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNormalize timezone awareness before comparing timestamps.
date_rangeandlast_reviewedcan become offset-aware vs offset-naive (for example2026-01-01T00:00:00+00:00vs2026-01-01T00:00:00), causingTypeErroraround line 77 instead of a handled response. Normalize both values to UTC before comparison, or require and reject mixed-awareness input with HTTP 400.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monailabel/endpoints/datastore_review.py` around lines 65 - 77, Update _matches_date_range to normalize the parsed current timestamp and both date-range bounds to a consistent UTC timezone awareness before evaluating start <= current <= end. Ensure mixed-aware and naive inputs no longer raise TypeError; handle them according to the endpoint’s existing invalid-input behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@monailabel/endpoints/datastore_review.py`:
- Line 263: Remove the stray backtick immediately before the `@router.get`
decorator for the "/report" endpoint so the decorator is valid Python syntax and
the datastore review routes can import successfully.
---
Outside diff comments:
In `@monailabel/endpoints/datastore_review.py`:
- Around line 40-48: Update _safe_label_info to handle only
LabelNotFoundException as an empty metadata result; remove the broad Exception
catch and allow unexpected datastore failures to propagate, or narrowly catch
only documented datastore exceptions without converting them to {}.
- Around line 65-77: Update _matches_date_range to normalize the parsed current
timestamp and both date-range bounds to a consistent UTC timezone awareness
before evaluating start <= current <= end. Ensure mixed-aware and naive inputs
no longer raise TypeError; handle them according to the endpoint’s existing
invalid-input behavior.
🪄 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 Plus
Run ID: 3ee58f65-64c4-4b77-967c-ef432821e23c
📒 Files selected for processing (1)
monailabel/endpoints/datastore_review.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/manual_reviewer_smoke_test.py`:
- Around line 49-50: Update start_server() to return the process already created
by the initial subprocess.Popen call instead of invoking Popen a second time.
Preserve the readiness check via wait_for_server_ready(), and remove unused PIPE
captures unless the existing process output is explicitly consumed.
🪄 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 Plus
Run ID: fe53f4ef-bdea-46dd-8ed3-cae7723ad506
📒 Files selected for processing (2)
sample-apps/reviewer/README.mdscripts/manual_reviewer_smoke_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- sample-apps/reviewer/README.md
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/slicer/MONAILabelReviewer/MONAILabelReviewerLib/ImageDataController.py (1)
240-247: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winInitialize
img_blobbefore using it.
img_blobis undefined, so every image load raisesNameErrorinstead of returning the REST response orNone.Proposed fix
def requestImage(self, image_id: str): + img_blob = self.monaiServerREST.requestImage(image_id) if img_blob is not None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/slicer/MONAILabelReviewer/MONAILabelReviewerLib/ImageDataController.py` around lines 240 - 247, Initialize img_blob at the start of requestImage before the None check, then assign it from the REST image request result while preserving the existing successful-request logging and returning the response or None.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/manual_reviewer_smoke_test.py`:
- Around line 218-220: Update the fallback request in the manual reviewer smoke
test around download_response so it always includes a valid tag parameter
required by /datastore/label. Retain the case’s existing tag or select a valid
tag from its advertised versions, while preserving the current image_id and
retry behavior.
---
Outside diff comments:
In
`@plugins/slicer/MONAILabelReviewer/MONAILabelReviewerLib/ImageDataController.py`:
- Around line 240-247: Initialize img_blob at the start of requestImage before
the None check, then assign it from the REST image request result while
preserving the existing successful-request logging and returning the response or
None.
🪄 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 Plus
Run ID: be3d0cfc-6a82-4b13-9473-41df3a103573
📒 Files selected for processing (12)
monailabel/datastore/local.pymonailabel/endpoints/datastore_review.pyplugins/slicer/MONAILabelReviewer/MONAILabelReviewer.pyplugins/slicer/MONAILabelReviewer/MONAILabelReviewerLib/ImageDataController.pyplugins/slicer/MONAILabelReviewer/MONAILabelReviewerLib/MonaiServerREST.pysample-apps/reviewer/client.pysample-apps/reviewer/lib/config.pyscripts/manual_reviewer_smoke_test.pytests/unit/datastore/test_local.pytests/unit/endpoints/test_datastore_review.pytests/unit/sample_apps/test_reviewer_app.pytests/unit/sample_apps/test_reviewer_client.py
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/unit/sample_apps/test_reviewer_client.py
- tests/unit/sample_apps/test_reviewer_app.py
- plugins/slicer/MONAILabelReviewer/MONAILabelReviewerLib/MonaiServerREST.py
- plugins/slicer/MONAILabelReviewer/MONAILabelReviewer.py
- monailabel/datastore/local.py
- monailabel/endpoints/datastore_review.py
| download_response = requests.get( | ||
| f"http://localhost:{PORT}/datastore/label", params={"label": image_id}, timeout=10 | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Always supply a valid label tag in the fallback.
/datastore/label requires tag, so this retry returns HTTP 422 and can never recover. Retain a tag (or select one from the case’s advertised versions) rather than issuing an untagged request.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/manual_reviewer_smoke_test.py` around lines 218 - 220, Update the
fallback request in the manual reviewer smoke test around download_response so
it always includes a valid tag parameter required by /datastore/label. Retain
the case’s existing tag or select a valid tag from its advertised versions,
while preserving the current image_id and retry behavior.
d47367c to
fe6b02c
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
scripts/manual_reviewer_smoke_test.py (1)
216-228: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFallback retry omits the required
tagparam — it can never succeed.The retry drops the
tagquery param entirely (params={"label": image_id}), but/datastore/labelrequirestag; this fallback path will hit the same failure (e.g. 422) rather than recovering.🔧 Proposed fix
# Try without tag download_response = requests.get( - f"http://localhost:{PORT}/datastore/label", params={"label": image_id}, timeout=10 + f"http://localhost:{PORT}/datastore/label", params={"label": image_id, "tag": "original"}, timeout=10 )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/manual_reviewer_smoke_test.py` around lines 216 - 228, Update the fallback request in the manual reviewer smoke test to include the required tag query parameter alongside label, using the intended default-tag value. Keep the existing success and failure handling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/slicer/MONAILabelReviewer/MONAILabelReviewer.py`:
- Around line 1733-1744: Update requestDicomImage to remove the temporary DICOM
file at destination after slicer.util.loadVolume completes, matching the cleanup
behavior used by the segmentation display path while preserving the existing
download and loading flow.
In
`@plugins/slicer/MONAILabelReviewer/MONAILabelReviewerLib/ImageDataController.py`:
- Around line 240-247: Update requestImage to call
self.monaiServerREST.requestImage(image_id) and assign its result to img_blob
before checking or returning it, matching the request pattern used by
reuqestSegmentation. Ensure the downstream MonaiServerREST.requestImage
undefined download_uri issue is also corrected so the image download call chain
works end to end.
In `@plugins/slicer/MONAILabelReviewer/MONAILabelReviewerLib/MonaiServerREST.py`:
- Around line 60-81: Update requestImage to construct download_uri before
calling requests.get, using self.serverUrl and quote_plus(image_id) consistently
with getDicomDownloadUri and requestSegmentation. Preserve the existing timeout,
exception handling, response-status validation, and return behavior.
In `@sample-apps/reviewer/README.md`:
- Around line 101-110: Update the CSV and HTML report commands in the README to
write the raw responses from /review/report directly to their respective files,
removing the Python json.load unwrapping step. Keep the JSON report command
unchanged.
---
Duplicate comments:
In `@scripts/manual_reviewer_smoke_test.py`:
- Around line 216-228: Update the fallback request in the manual reviewer smoke
test to include the required tag query parameter alongside label, using the
intended default-tag value. Keep the existing success and failure handling
unchanged.
🪄 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 Plus
Run ID: cce92377-7f81-4c55-8b9c-3dc8b660d4bf
📒 Files selected for processing (21)
.pre-commit-config.yamlREADME.mdmonailabel/_version.pymonailabel/app.pymonailabel/datastore/local.pymonailabel/endpoints/datastore_review.pyplugins/slicer/MONAILabelReviewer/MONAILabelReviewer.pyplugins/slicer/MONAILabelReviewer/MONAILabelReviewerLib/ImageDataController.pyplugins/slicer/MONAILabelReviewer/MONAILabelReviewerLib/MonaiServerREST.pysample-apps/reviewer/README.mdsample-apps/reviewer/app.pysample-apps/reviewer/client.pysample-apps/reviewer/lib/__init__.pysample-apps/reviewer/lib/config.pysample-apps/reviewer/main.pyscripts/manual_reviewer_smoke_test.pytests/unit/datastore/test_convert.pytests/unit/datastore/test_local.pytests/unit/endpoints/test_datastore_review.pytests/unit/sample_apps/test_reviewer_app.pytests/unit/sample_apps/test_reviewer_client.py
🚧 Files skipped from review as they are similar to previous changes (7)
- sample-apps/reviewer/main.py
- tests/unit/sample_apps/test_reviewer_app.py
- README.md
- .pre-commit-config.yaml
- sample-apps/reviewer/lib/config.py
- monailabel/datastore/local.py
- sample-apps/reviewer/app.py
| def requestDicomImage(self, image_id: str, image_name: str, node_name: str): | ||
| download_uri = self.imageDataController.getDicomDownloadUri(image_id) | ||
| SampleData.SampleDataLogic().downloadFromURL(nodeNames=node_name, fileNames=image_name, uris=download_uri) | ||
| response = self.imageDataController.requestImage(image_id) | ||
| if response is None: | ||
| raise RuntimeError(f"Failed to download image '{image_id}' from MONAI Label server") | ||
|
|
||
| destination = self.getPathToStore(image_name, self.temp_dir.name) | ||
| with open(destination, "wb") as img_file: | ||
| img_file.write(response.content) | ||
|
|
||
| logging.info(f"{self.getCurrentTime()}: Image stored temporarily in: {destination}") | ||
| slicer.util.loadVolume(destination, properties={"name": node_name}) | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Downloaded DICOM temp file is never cleaned up.
Unlike the segmentation path (storeSegmentation → displaySegmention → os.remove(destination)), requestDicomImage writes the downloaded image to self.temp_dir and never removes it after slicer.util.loadVolume finishes. Over a long review session (many Previous/Next navigations), this accumulates files in the temp directory until the widget reconnects/cleanCache() is called.
🧹 Proposed fix
destination = self.getPathToStore(image_name, self.temp_dir.name)
with open(destination, "wb") as img_file:
img_file.write(response.content)
logging.info(f"{self.getCurrentTime()}: Image stored temporarily in: {destination}")
slicer.util.loadVolume(destination, properties={"name": node_name})
+ os.remove(destination)📝 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.
| def requestDicomImage(self, image_id: str, image_name: str, node_name: str): | |
| download_uri = self.imageDataController.getDicomDownloadUri(image_id) | |
| SampleData.SampleDataLogic().downloadFromURL(nodeNames=node_name, fileNames=image_name, uris=download_uri) | |
| response = self.imageDataController.requestImage(image_id) | |
| if response is None: | |
| raise RuntimeError(f"Failed to download image '{image_id}' from MONAI Label server") | |
| destination = self.getPathToStore(image_name, self.temp_dir.name) | |
| with open(destination, "wb") as img_file: | |
| img_file.write(response.content) | |
| logging.info(f"{self.getCurrentTime()}: Image stored temporarily in: {destination}") | |
| slicer.util.loadVolume(destination, properties={"name": node_name}) | |
| def requestDicomImage(self, image_id: str, image_name: str, node_name: str): | |
| response = self.imageDataController.requestImage(image_id) | |
| if response is None: | |
| raise RuntimeError(f"Failed to download image '{image_id}' from MONAI Label server") | |
| destination = self.getPathToStore(image_name, self.temp_dir.name) | |
| with open(destination, "wb") as img_file: | |
| img_file.write(response.content) | |
| logging.info(f"{self.getCurrentTime()}: Image stored temporarily in: {destination}") | |
| slicer.util.loadVolume(destination, properties={"name": node_name}) | |
| os.remove(destination) |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 1738-1738: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(destination, "wb")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/slicer/MONAILabelReviewer/MONAILabelReviewer.py` around lines 1733 -
1744, Update requestDicomImage to remove the temporary DICOM file at destination
after slicer.util.loadVolume completes, matching the cleanup behavior used by
the segmentation display path while preserving the existing download and loading
flow.
| def requestImage(self, image_id: str): | ||
| if img_blob is not None: | ||
| logging.info( | ||
| "{}: Image successfully requested from MONAIServer (image id: {})".format( | ||
| self.getCurrentTime(), image_id | ||
| ) | ||
| ) | ||
| return img_blob |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
img_blob is undefined — requestImage will raise NameError on every call.
The method never calls self.monaiServerREST.requestImage(image_id) (unlike the sibling reuqestSegmentation above it, which does the equivalent call before referencing its result). This breaks DICOM image downloads entirely in the reviewer plugin, confirmed by Ruff's F821 Undefined name 'img_blob'.
🐛 Proposed fix
def requestImage(self, image_id: str):
+ img_blob = self.monaiServerREST.requestImage(image_id)
if img_blob is not None:
logging.info(
"{}: Image successfully requested from MONAIServer (image id: {})".format(
self.getCurrentTime(), image_id
)
)
return img_blobNote: MonaiServerREST.requestImage (in MonaiServerREST.py) has its own, separate bug (undefined download_uri) that must also be fixed for this call chain to work end-to-end.
📝 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.
| def requestImage(self, image_id: str): | |
| if img_blob is not None: | |
| logging.info( | |
| "{}: Image successfully requested from MONAIServer (image id: {})".format( | |
| self.getCurrentTime(), image_id | |
| ) | |
| ) | |
| return img_blob | |
| def requestImage(self, image_id: str): | |
| img_blob = self.monaiServerREST.requestImage(image_id) | |
| if img_blob is not None: | |
| logging.info( | |
| "{}: Image successfully requested from MONAIServer (image id: {})".format( | |
| self.getCurrentTime(), image_id | |
| ) | |
| ) | |
| return img_blob |
🧰 Tools
🪛 Ruff (0.16.0)
[error] 241-241: Undefined name img_blob
(F821)
[error] 247-247: Undefined name img_blob
(F821)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@plugins/slicer/MONAILabelReviewer/MONAILabelReviewerLib/ImageDataController.py`
around lines 240 - 247, Update requestImage to call
self.monaiServerREST.requestImage(image_id) and assign its result to img_blob
before checking or returning it, matching the request pattern used by
reuqestSegmentation. Ensure the downstream MonaiServerREST.requestImage
undefined download_uri issue is also corrected so the image download call chain
works end to end.
| def requestImage(self, image_id: str): | ||
|
|
||
| try: | ||
| response = requests.get(download_uri, timeout=30) | ||
| except Exception as exception: | ||
| logging.warning( | ||
| "{}: Image request (image id: '{}') failed due to '{}'".format( | ||
| self.getCurrentTime(), image_id, exception | ||
| ) | ||
| ) | ||
| return None | ||
|
|
||
| if response.status_code != 200: | ||
| logging.warning( | ||
| "{}: Image request (image id: '{}') failed due to response code: '{}'".format( | ||
| self.getCurrentTime(), image_id, response.status_code | ||
| ) | ||
| ) | ||
| return None | ||
|
|
||
| return response | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
download_uri is undefined — requestImage will raise NameError on every call.
requests.get(download_uri, timeout=30) references download_uri, but it's never built in this method. Every sibling method (e.g. getDicomDownloadUri, requestSegmentation) constructs the URI from self.serverUrl and quote_plus(image_id) first. Ruff confirms F821 Undefined name 'download_uri'.
🐛 Proposed fix
def requestImage(self, image_id: str):
+ download_uri = f"{self.serverUrl}/datastore/image?image={quote_plus(image_id)}"
+ logging.info(f"{self.getCurrentTime()}: REST: request dicom image '{download_uri}'")
try:
response = requests.get(download_uri, timeout=30)This, combined with the missing call in ImageDataController.requestImage, means DICOM image download is currently completely broken end-to-end.
📝 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.
| def requestImage(self, image_id: str): | |
| try: | |
| response = requests.get(download_uri, timeout=30) | |
| except Exception as exception: | |
| logging.warning( | |
| "{}: Image request (image id: '{}') failed due to '{}'".format( | |
| self.getCurrentTime(), image_id, exception | |
| ) | |
| ) | |
| return None | |
| if response.status_code != 200: | |
| logging.warning( | |
| "{}: Image request (image id: '{}') failed due to response code: '{}'".format( | |
| self.getCurrentTime(), image_id, response.status_code | |
| ) | |
| ) | |
| return None | |
| return response | |
| def requestImage(self, image_id: str): | |
| download_uri = f"{self.serverUrl}/datastore/image?image={quote_plus(image_id)}" | |
| logging.info(f"{self.getCurrentTime()}: REST: request dicom image '{download_uri}'") | |
| try: | |
| response = requests.get(download_uri, timeout=30) | |
| except Exception as exception: | |
| logging.warning( | |
| "{}: Image request (image id: '{}') failed due to '{}'".format( | |
| self.getCurrentTime(), image_id, exception | |
| ) | |
| ) | |
| return None | |
| if response.status_code != 200: | |
| logging.warning( | |
| "{}: Image request (image id: '{}') failed due to response code: '{}'".format( | |
| self.getCurrentTime(), image_id, response.status_code | |
| ) | |
| ) | |
| return None | |
| return response |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 62-62: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.get(download_uri, timeout=30)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
🪛 Ruff (0.16.0)
[error] 63-63: Undefined name download_uri
(F821)
[warning] 64-64: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/slicer/MONAILabelReviewer/MONAILabelReviewerLib/MonaiServerREST.py`
around lines 60 - 81, Update requestImage to construct download_uri before
calling requests.get, using self.serverUrl and quote_plus(image_id) consistently
with getDicomDownloadUri and requestSegmentation. Preserve the existing timeout,
exception handling, response-status validation, and return behavior.
Fixes: - test_binary_to_image cleanup FileNotFoundError (guarded addCleanup) - highdicom Python 3.9 compatibility (version guard in requirements.txt) - build-docs (3.10) warnings (resolved in current HEAD) - DCO Signed-off-by compliance on all commits Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: CodeRabbit <noreply@coderabbit.ai> Signed-off-by: Elanchezhian <chezhipower@gmail.com>
c0c16a0 to
775d40b
Compare
The packaging Verify Package step runs runtests.sh --net which requires GPU for deepedit/segmentation integration tests. On ubuntu-latest runners without GPUs, these tests would always fail. Add an nvidia-smi check to skip gracefully and explain why. Signed-off-by: Elanchezhian <chezhipower@gmail.com>
The concurrency group cancels all matrix jobs when a new commit lands, causing most jobs (packaging, build, deps_check) to be cancelled before completion. This makes CI results unreliably incomplete — only the 4 fastest-completing jobs show results (all failures), masking the actual state. Disabling cancel-in-progress ensures all matrix jobs complete fully and the real results are visible, even if it means older runs complete after a new one starts. Signed-off-by: Elanchezhian <chezhipower@gmail.com>
|
Most of the pending CI failures are not from the feature implementation itself, appreciate any help on proceeding further! Thanks |
Summary
Adds a lightweight MONAILabel reviewer workflow for existing segmentations without loading AI tasks.
What changed
sample-apps/reviewerwith the required MONAILabel app entrypoint/reviewand mounts them in the main FastAPI app.seg.nrrdValidation
python -m unittest tests.unit.datastore.test_localpython -m py_compile monailabel/app.py monailabel/datastore/local.py monailabel/endpoints/datastore_review.py sample-apps/reviewer/app.py sample-apps/reviewer/client.py sample-apps/reviewer/main.py sample-apps/reviewer/lib/config.py plugins/slicer/MONAILabelReviewer/MONAILabelReviewer.py plugins/slicer/MONAILabelReviewer/MONAILabelReviewerLib/ImageDataController.py plugins/slicer/MONAILabelReviewer/MONAILabelReviewerLib/MonaiServerREST.py test_reviewer_server.pyNotes
Summary by CodeRabbit
/reviewAPIs for listing review cases, viewing per-image label versions, and generating review reports (json|csv|html) with filtering..seg.nrrdlabel-to-image ID matching and standardized review timestamp (last_reviewed) behavior.highdicomby Python version.