Skip to content
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

[PW41] Add MRSegmentator Model #90

Open
wants to merge 35 commits into
base: main
Choose a base branch
from

Conversation

FJDorfner
Copy link

Pull Request to add the MRSegmentator model to mhub.

Copy link
Member

@LennyN95 LennyN95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great Work!!

I suggested some minor changes. I'll update segdb and once we have that we can continue with the test routine. Thx @FJDorfner, very clean and to the point :)

models/mrsegmentator/utils/MRSegmentatorMLRunner.py Outdated Show resolved Hide resolved
models/mrsegmentator/meta.json Outdated Show resolved Hide resolved
{
"type": "Segmentation",
"classes": [
"SPLEEN",
Copy link
Member

@LennyN95 LennyN95 Jul 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add SPINE to SegDB. I can lookup the codes, but feel free to add SCT code's if you already have them ;))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FJDorfner We're adding a quite broad label here, can you explain weather the model delineates the entire spine or the bone-structure of the spine?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @LennyN95, the model segments the bodies of the vertebrae individually. The disk between is not included in the label. This would be the yellow label in the attached Screenshot as a visual:)

Screenshot 2024-07-24 at 10 24 47

"label": "Input Image",
"description": "The MR or CT scan of a patient (Thorax, Abdomen and Pelvis).",
"format": "DICOM",
"modality": "MRI|CT",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • TODO @LennyN95: Add support for multi-modality inputs. Dash separation is a good idea and in line with DTQ syntax.

models/mrsegmentator/config/low_memory.yml Outdated Show resolved Hide resolved
models/mrsegmentator/config/default.yml Outdated Show resolved Hide resolved
models/mrsegmentator/config/default.yml Outdated Show resolved Hide resolved
models/mrsegmentator/config/low_memory.yml Outdated Show resolved Hide resolved
@LennyN95
Copy link
Member

@FJDorfner since all comments from the previous review are now implemented, we can continue with the testing procedure.

@FJDorfner
Copy link
Author

@LennyN95 Thanks! I will continue with the testing.

@LennyN95
Copy link
Member

Please note, we updated our base image. All mhub dependencies are now installed in a virtual environment under /app/.venv running Python 3.11. Python, virtual environment and dependencies are now managed with uv. If required, you can create custom virtual environments, e.g., uv venv -p 3.8 .venv38 and use uv pip install -p .venv38 packge-name to install dependencies and uv run -p .venv3.8 python script.py to run a python script.

We also simplified our test routine. Sample and reference data now have to be uploaded to Zenodo and provided in a mhub.tom file at the project root. The process how to create and provide these sample data is explained in the updated testing phase article of our documentation. Under doi.org/10.5281/zenodo.13785615 we provide sample data as a reference.

@FJDorfner
Copy link
Author

@LennyN95 Thanks for letting me know about the mhub update. I am currently trying to adapt the configuration to these changes in order to complete the PR. Unfortunately running the model is not working at the moment. The DicomImporter step comes up empty even though the correct paths are configured.

This is what the output looks like:

Start DicomImporter

source input dir: input_data --> /app/data/input_data
import sort dir: sorted_data --> /app/data/sorted_data

sorting dicom data

input dir: /app/data/input_data
output dir: /app/data/sorted_data
schema: %SeriesInstanceUID/dicom/%SOPInstanceUID.dcm
creating output folder: /app/data/sorted_data

run: dicomsort -k -u /app/data/input_data /app/data/sorted_data/%SeriesInstanceUID/dicom/%SOPInstanceUID.dcm

0it [00:00, ?it/s]
0it [00:00, ?it/s]
Files sorted

related instances: 0
Done in 0.506747 seconds.

. Instance [/app/data/_global]
├── id: global
├── sid: global


When I run the container interactively, I can run `dicomsort -k -u /app/data/input_data /app/data/sorted_data/%SeriesInstanceUID/dicom/%SOPInstanceUID.dcm' and it successfully finds and sorts the files. (After activating the venv at /app/.venv)

Do you have any idea what the problem might be here? Thanks!

RUN buildutils/import_mhub_model.sh mrsegmentator ${MHUB_MODELS_REPO}

# Install MRSegmentator
RUN uv pip install -p /app/.venv mrsegmentator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI; uv should use .venv by default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to make sure there's no dependency conflict between mhubio and mrsegmentator you can use uv to create a custom venv and write a small cli wrapper running in that venv that can be called from within your Runner IO Module via self.subprocess.

Example:
https://github.com/MHubAI/models/blob/main/models/pyradiomics/dockerfiles/Dockerfile#L3-L8

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I don't think there is a dependency conflict, as the container and dicomsort work in interactive mode. Is there anything that I need to do to activate the .venv? When I run the container interactively dicomsort only works after activating the .venv.

Comment on lines 16 to 18
RUN touch .temp_image.nii.gz
RUN mrsegmentator -i .temp_image.nii.gz; exit 0
RUN rm .temp_image.nii.gz
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be the issue? I am not sure if mrsegmentator is available, try uv run mrsegmentator if it's installed in the .venv.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this would be the issue, these lines seem to run fine when building the container, and there is no error message produced. The problem mainly is that the dicomsort module does not seem to find any data. I will still revise and use uv run mrsegmentator.

@FJDorfner
Copy link
Author

FJDorfner commented Nov 27, 2024

@LennyN95 I have tried to troubleshoot this some more and found that running:
sudo docker run --gpus all --shm-size=1g -it -v /home/ubuntu/test_folder/default/sample/dicom:/app/data/input_data:ro -v $MHUB_TEST_DIR/$MHUB_WORKFLOW_NAME/reference:/app/data/output_data --entrypoint /bin/bash mhubai-dev/$MHUB_MODEL_NAME:latest

and then inside running the entrypoint command as defined in the dockerfile:
mhub.run --config /app/models/mrsegmentator/config/default.yml

works perfectly fine.

Whereas when I run:
sudo docker run mhubai-dev/$MHUB_MODEL_NAME:latest --gpus all --shm-size=1g -v /home/ubuntu/test_folder/default/sample/dicom:/app/data/input_data:ro -v $MHUB_TEST_DIR/$MHUB_WORKFLOW_NAME/reference:/app/data/output_data -w $MHUB_WORKFLOW_NAME

it doesn't work. (see attached screenshot)
Screenshot 2024-11-27 at 17 16 27

Edit:
Running mhub.run -w default inside the docker container in interactive mode also works as expected.

Edit2:
I have resolved the problem. The issue was with the docker fun command, the workflow specification was not properly passed through to the entrypoint command. This was resolved by changing the order like this:
sudo docker run --gpus all --shm-size=1g \ -v $MHUB_TEST_DIR/$MHUB_WORKFLOW_NAME/sample/:/app/data/input_data:ro \ -v $MHUB_TEST_DIR/$MHUB_WORKFLOW_NAME/reference:/app/data/output_data \ mhubai-dev/$MHUB_MODEL_NAME:latest -w $MHUB_WORKFLOW_NAME

@FJDorfner
Copy link
Author

Thank you @LennyN95! The PR should now be compliant with the updated submission and testing procedure.

@LennyN95
Copy link
Member

Hey @FJDorfner, oh.. yes all "inside the container commands" need to be after the image name. However, I see you specify one extra cli argument --shm-size=1g. This is not supported by MHub (all models must share the same run command). Are you using multiprocessing during inference?

@FJDorfner
Copy link
Author

Unfortunately, --shm-size=1g is needed for the standard configuration of mrsegmentator to run properly inside the docker container. Is there a way to make this work without having an extra CLI argument?

@LennyN95
Copy link
Member

Can you identify where exactly MRSegmentator uses this? Then we can have a look together. We ran into a similar issue with NNUnetV2 and ended up providing a patch PR to allow single processing inference. So likely there is a way to make it work, ideal scenario you can set some num_worker=0, worst it might require some changes.

@FJDorfner
Copy link
Author

Thanks! MRSegmentator is based upon nnUNetv2, the multiprocessing happens in there I think. What did you end up doing to get nnUNetv2 working? I think this would likely also fix the problem here.

@LennyN95
Copy link
Member

We have an open PR with a patch here MIC-DKFZ/nnUNet#2614. However, this is based on the latest version which as of now is nnunetv2==2.5.1. Can you check which version MRSegmentator uses and if it's compatible with 2.5.1?
One easy way to do this is to clone nnunet from my public fork, mount it into the docker container under /nnunet-src and then run uv pip install -e /nnunet-src.

@FJDorfner
Copy link
Author

Thank you for investigating and already opening a PR with nnUnetv2. Currently MRSegmentator is based on version nnunetv2==2.2.1. For now we would suggest to use the current fast configuration, which uses the split_level 1 flag, as the default configuration. In our tests this has very little impact on performance and runs perfectly without any changes to shm-size.

@LennyN95
Copy link
Member

LennyN95 commented Dec 2, 2024

This is great, thx @FJDorfner. Then, we are all-set to move on with the testing?

@FJDorfner FJDorfner requested a review from LennyN95 December 3, 2024 12:40
@FJDorfner
Copy link
Author

Thanks! @LennyN95 yes we are all set to move on with the testing. I have updated the zenodo upload to accurately reflect the new configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready for Testing
Development

Successfully merging this pull request may close these issues.

2 participants