search

Project-MONAI / MONAI

AI Toolkit for Healthcare Imaging
https://monai.io/
Apache License 2.0
5.5k stars 1.01k forks source link

Integrating a Tailored Auto-Encoder Model into Generative Model Application #7861

Open dongyang0122 opened 1 week ago

dongyang0122 commented 1 week ago

Fixes #7858.

Description

Integrating a tailored auto-encoder model into the generative model application to improve the production of high-dimensional 3D images, specifically sized at 512 x 512 x 512.

Types of changes

ericspod commented 1 week ago

The requirements for this app aren't being installed by the tests, this is why they are failing. The generative models repo is being integrated into Core by @virginiafdez currently, shall we wait for that to complete and then we can eliminate the requirement here for the external repo? Thanks!

dongyang0122 commented 1 week ago

Hi @Nic-Ma @KumoLiu @ericspod @yiheng-wang-nv , the PR is dependent on another monai related project. I am not sure how to pass the some tests without installing the package. Thanks.

dongyang0122 commented 1 week ago

Hi @ericspod, we are currently preparing to release a new MONAI feature within the next few days. Initially, we may need to enable this application, followed by integrating the changes once another pull request is ready for incorporation.

ericspod commented 6 days ago

Hi @ericspod, we are currently preparing to release a new MONAI feature within the next few days. Initially, we may need to enable this application, followed by integrating the changes once another pull request is ready for incorporation.

I feel this application being added now, before the generative integration is done, is problematic because of its external dependency. As I mentioned in another comment, calling pip on import is unexpected and error-prone I think.

What we can do is add the dependency package to one of the requirements files as a temporary fix so that it's installed, this will be removed when the integration is done and no longer needed. The imports in this application would then be changed but should require no other changes. @Nic-Ma @KumoLiu thoughts?

dongyang0122 commented 6 days ago

Hi @ericspod, we are currently preparing to release a new MONAI feature within the next few days. Initially, we may need to enable this application, followed by integrating the changes once another pull request is ready for incorporation.

I feel this application being added now, before the generative integration is done, is problematic because of its external dependency. As I mentioned in another comment, calling pip on import is unexpected and error-prone I think.

What we can do is add the dependency package to one of the requirements files as a temporary fix so that it's installed, this will be removed when the integration is done and no longer needed. The imports in this application would then be changed but should require no other changes. @Nic-Ma @KumoLiu thoughts?

@ericspod Thanks. I am wondering if it will affect CI/CD testing if we add the library to one of the requirement .txt files.

dongyang0122 commented 6 days ago

@KumoLiu @Nic-Ma @ericspod I am not clear how to pass these failed checks. Please share your suggestion. Thanks.

ericspod commented 6 days ago

I think the fails are to do with the fact that you're running pip inside a running Python environment. This is potentially corrupting the running environment since new packages are appearing. I really don't think running pip during import is a good idea at all. This will also force such an install for any user of MONAI and possibly cause other unexpected behaviours. We need to either modify the requirements, modify the CICD environment, or put this code into a separate repository for the time being until the integration of generative is complete.

KumoLiu commented 4 days ago

Hi @ericspod, we are currently preparing to release a new MONAI feature within the next few days. Initially, we may need to enable this application, followed by integrating the changes once another pull request is ready for incorporation.

I feel this application being added now, before the generative integration is done, is problematic because of its external dependency. As I mentioned in another comment, calling pip on import is unexpected and error-prone I think.

What we can do is add the dependency package to one of the requirements files as a temporary fix so that it's installed, this will be removed when the integration is done and no longer needed. The imports in this application would then be changed but should require no other changes. @Nic-Ma @KumoLiu thoughts?

I agree with @ericspod here, we can add it in requirement-dev.txt as a workaround for now and remove it after the generative part is merged into dev.

dongyang0122 commented 1 day ago

I am not sure what the following error (premerge / flake8-py3 (codeformat) (pull_request) Failing after 3m) is from. This is not from my scripts.

tests/test_pad_collation.py:116:12: E721 Use is and is not for type comparisons, or isinstance() for isinstance checks 114 self.assertTrue(len(set(shapes)) > 1) # inverted shapes must be different because of random xforms 115 116 if t_type == dict: ^^^^^^^^^^^^^^ E721 117 batch_inverse = BatchInverseTransform(dataset.transform, loader) 118 for data in loader:

Found 3 errors. Check failed! Please run auto style fixes: ./runtests.sh --autofix Error: Process completed with exit code 1.

KumoLiu commented 1 day ago

I am not sure what the following error (premerge / flake8-py3 (codeformat) (pull_request) Failing after 3m) is from. This is not from my scripts.

tests/test_pad_collation.py:116:12: E721 Use is and is not for type comparisons, or isinstance() for isinstance checks | 114 | self.assertTrue(len(set(shapes)) > 1) # inverted shapes must be different because of random xforms 115 | 116 | if t_type == dict: | ^^^^^^^^^^^^^^ E721 117 | batch_inverse = BatchInverseTransform(dataset.transform, loader) 118 | for data in loader: |

Found 3 errors. Check failed! Please run auto style fixes: ./runtests.sh --autofix Error: Process completed with exit code 1.

Hi @dongyang0122, you can merge dev to solve the error, thanks!