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

ivadomed to nnunetv2 backend migration #800

Draft
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

hermancollin
Copy link
Member

Checklist

  • I've given this PR a concise, self-descriptive, and meaningful title
  • I've linked relevant issues in the PR body
  • I've applied the relevant labels to this PR
  • I've added relevant tests for my contribution
  • I've updated the documentation and/or added correct docstrings
  • I've assigned a reviewer
  • I've consulted ADS's internal developer documentation to ensure my contribution is in line with any relevant design decisions

Description

This PR replaces the ivadomed dependency with nnunetv2. The old models will unfortunately no longer be supported directly, but they can always be used by reverting to an earlier commit. The main changes are

  • addition of model cards to list all available models with download links, intended use, training data details
  • refactoring download_models.py where URLs to the 3 standard ivadomed models were hardcoded; this script now only downloads one model at a time (generalist model by default)
  • refactoring of segment.py where some ivadomed-related options are no longer available. Input resampling is also applied on our side now (it was previously done in ivadomed itself based on the input px size VS training px size)
  • refactoring of apply_model.py; here, we now use the nnunet inference function instead of ivadomed.inference.imed_inference()

Linked issues

Resolves #765 (master issue)
Addresses #797 (download_model refactoring)
Addresses #798 (segmentation CLI refactoring)
Addresses #799 (model cards)

@hermancollin hermancollin added dependencies Pull requests that update a dependency file refactoring context: code refactoring models context cli context: the changes related to ADS's command line interface nnunetv2 labels Apr 23, 2024
'''
import pprint

MODELS = {
Copy link
Member Author

Choose a reason for hiding this comment

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

for testing, check if URLs exist

@hermancollin hermancollin mentioned this pull request May 22, 2024
7 tasks
@hermancollin
Copy link
Member Author

Had to bump python version to 3.11 but this currently causes problems with CI...

@hermancollin
Copy link
Member Author

For some reason imageio 2.26 is downloaded, which makes all test fail. Despite the pinned imageio<2.28.0, on the master branch, 2.34 is downloaded:
Screenshot_20240523_095248

Not sure I understand what's going on.

@mathieuboudreau
Copy link
Member

For some reason imageio 2.26 is downloaded, which makes all test fail. Despite the pinned imageio<2.28.0, on the master branch, 2.34 is downloaded: Screenshot_20240523_095248

Not sure I understand what's going on.

Likely a loose version condition in one of the pip dependencies that we install in the conda env file - those won't enfoce our own conditions (annoyingly, and we never found a better solution to this). IVADOMED maybe? I'd remove all pip installations in the file, and see if the correct version gets installed, and then re-added them one at a time

@hermancollin
Copy link
Member Author

Likely a loose version condition in one of the pip dependencies that we install in the conda env file - those won't enfoce our own conditions (annoyingly, and we never found a better solution to this). IVADOMED maybe? I'd remove all pip installations in the file, and see if the correct version gets installed, and then re-added them one at a time

Unpinning imageio fixed it. Now it gets 2.33 I think which is good enough.

Comment on lines 80 to 85
predictor.predict_from_files(
list_of_lists_or_source_folder=input_list,
output_folder_or_list_of_truncated_output_files=output_list,
save_probabilities=False,
overwrite=False,
)
Copy link
Member Author

Choose a reason for hiding this comment

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

damn this is something I didn't expect... we trained all nnunet models on grayscale images, meaning they have a single channel. While trying axon_segmentation(), I encountered an image that, although looking black and white, was saved in RGB format (meaning 3 channels). This makes nnunet fail directly and unfortunately, from my experience, this is a very common occurence.

We thus have 2 options:

  1. before inference, read all images and if they are in RGB format, save a grayscale conversion in PNG format + update input filenames list
  2. use nnUNetPredictor.predict_from_list_of_npy_arrays instead of nnUNetPredictor.predict_from_list_of_files. I don't link this option because it adds a lot of complexity to this PR: we will need to manage the RAM on our side and it will be a problem for large number of inputs. By comparison, I currently use predict_from_list_of_files which is memory efficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli context: the changes related to ADS's command line interface dependencies Pull requests that update a dependency file models context nnunetv2 refactoring context: code refactoring
Projects
None yet
2 participants