-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CCHMC Ped Abd CT Seg Example App #525
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: bluna301 <luna.bryanr@gmail.com>
Signed-off-by: bluna301 <luna.bryanr@gmail.com>
Signed-off-by: bluna301 <luna.bryanr@gmail.com>
HI @bluna301, Thanks for the pull request. I will review it by the end of the week. |
Thanks Vikash! |
|
import argparse | ||
import logging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just merged a pull request that fixed the issues of typos, formatting, and import orders in many modules. A rebase will be needed if there are conflicts.
MONGODB_IP_DOCKER=172.17.0.1 # default Docker bridge network IP | ||
``` | ||
|
||
Prior to packaging into a MAP, the MongoDB credentials should be harcoded into the `MongoDBWriterOperator`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: harcoded -> hardcoded
# series selection rule in JSON, which selects for axial CT series; flexible ST choices: | ||
# StudyDescription: matches any value | ||
# Modality: matches "CT" value (case-insensitive); filters out non-CT modalities | ||
# ImageType: matches value that conatins "PRIMARY", "ORIGINAL", and "AXIAL"; filters out most cor and sag views |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: conatins -> contains
# ImageType: matches value that conatins "PRIMARY", "ORIGINAL", and "AXIAL"; filters out most cor and sag views | ||
# SeriesDescription: matches any values that do not contain "cor" or "sag" (case-insensitive); filters out cor and sag views | ||
# SliceThickness: supports list, string, and numerical matching: | ||
# [3, 5]: matches ST values beween 3 and 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: beween -> between
"Modality": "(?i)CT", | ||
"ImageType": ["PRIMARY", "ORIGINAL", "AXIAL"], | ||
"SeriesDescription": "(?i)^(?!.*(cor|sag)).*$", | ||
"SliceThickness": [3, 5] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This range support looks good once we publish the next version of MD SDK package (as your PR had already been merged). In the meantime, the editable SDK with the recent code will be needed. Planning to do the next release of MD App SDK later in the month and bumping the version to 3.0, following the release of holoscan v3.0, but we can discuss if a minor 2.x version needs to be squeezed in.
from typing import Any, Dict, Union | ||
|
||
import pydicom | ||
import pytz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding package types-pytz
to <project root>/requirements-examples.txt
should get rid of the mypy
complaint.
In the CI workflow, the test setup pip installs this requirements file, and others, before running all the checks.
pytz>=2024.1 | ||
|
||
# holoscan installation; python 3.8-3.11 is required to install V2.0.0 | ||
holoscan==2.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Known issues in holoscan has been fixed in the latest v2.9.0, and the latest monai-deploy-app-sdk (at 2.0.0) pulls in the latest holoscan v2.x. So pinning holoscan to avoid issue is not needed now, if it is the main reason. Though the caveat is that monai-deploy CLI is now a standalone package, not part of holoscan from v2.9.0.
Another thing worth mentioning is that there are more advanced operator execution conditions and soon dynamic workflow support in the SDK, which can be used for conditional inclusion and execution of some branches of the workflow, e.g. the Mongo DB integration. Examples of their use are not in the repo yet, and we can think about adding such in the next version of this app. |
This MAP pipeline is based on the CCHMC Pediatric Abdominal CT Segmentation MONAI Bundle.
Some unique features of this MAP pipeline include:
AbdomenSegOperator
enables either PyTorch or TorchScript model loadingDICOMSecondaryCaptureWriterOperator
writes a DICOM SC with organ contoursapp.py
MongoDBEntryCreatorOperator
andMongoDBWriterOperator
) allow for writing to the MongoDB database associated with MONAI Deploy ExpressThere is also some dependency cleanup of the
hugging_face_integration_app
example app that @vikashg created; there were some unused dependencies and formatting fixes that were highlighted when running the linting, type, and test checks.As described in the
README.md
, when executing the pipeline pythonically, a.env
file that contains the relevant MongoDB connection information is recommended. However, I am unsure of the best way to implement this sensitive information into the MAP without hardcoding. @MMelQin and @vikashg, any thoughts on the best way to achieve this?