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.

Sign up for GitHub

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

Jump to bottom

Print what package should be installed when suitable writer is missing #8001

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Print what package should be installed when suitable writer is missing #8001

wants to merge 2 commits into from

Conversation

ytl0623
Copy link
Contributor

@ytl0623 ytl0623 commented Aug 7, 2024

Fixes #7980

Description

To implement these changes:

  1. Add the from typing import Dict, List import.
  2. Add the WRITER_PACKAGE_MAP dictionary after the existing imports.
  3. Replace the existing OptionalImportError class with the new version provided above.

For example, if you try to save a PNG file without Pillow installed, you'll now get an error message like:

# Before
monai.utils.module.OptionalImportError: No ImageWriter backend found for png.

# Now
monai.utils.module.OptionalImportError: No ImageWriter backend found for png. Please install one of the following packages: pillow

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

dzenanz
dzenanz reviewed Aug 12, 2024
View reviewed changes
monai/utils/module.py
"nii": ["nibabel"],
"nii.gz": ["nibabel"],
"dcm": ["pydicom"],
# Add more mappings as needed
Copy link
Contributor

Choose a reason for hiding this comment

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

Who is supposed to add the rest of the entries on this list? ITK supports quite a few, including all of the ones you listed above.

Copy link
Member

Choose a reason for hiding this comment

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

We can add ITK for a number of these types though the listed libraries are the primary ones used. If you can identify what other libraries load each of these types (and other types that can be loaded) please add them here as well.

@KumoLiu
Copy link
Contributor

KumoLiu commented Aug 15, 2024 •
edited
Loading

Hi @ytl0623, thanks for the pr!

Given that we already have a resolve_writer function to determine the appropriate writer based on the suffix, do you think it would be feasible to extend this function by adding a required_pkg list which can also resolve @dzenanz's concern here.

def resolve_writer():
    ...
    requried_pkg = []
        default_writers = SUPPORTED_WRITERS.get(EXT_WILDCARD, ())
        for _writer in look_up_option(fmt, SUPPORTED_WRITERS, default=default_writers):
            try:
                _writer()  # this triggers `monai.utils.module.require_pkg` to check the system availability
                avail_writers.append(_writer)
            except OptionalImportError as e:
                requried_pkg.append(re.search(r'`(.*?)`', str(e)).group(1))
                continue
            except Exception:  # other writer init errors indicating it exists
                avail_writers.append(_writer)
        if not avail_writers and error_if_not_found:
            raise OptionalImportError(f"No ImageWriter backend found for {fmt}, install one of the required packages in {requried_pkg}.")
    ...

@KumoLiu KumoLiu requested a review from ericspod August 15, 2024 07:53
@KumoLiu
Copy link
Contributor

KumoLiu commented Aug 26, 2024

Hi @ytl0623, Do you plan to address the comments on the PR? If not, I’d be happy to assist with it. I’m hoping we can get this merged into version 1.4, but we only have about one or two weeks left to add new features.

@ytl0623
Copy link
Contributor Author

ytl0623 commented Aug 26, 2024

Hi @KumoLiu, I apologize for the delay in responding. I’m willing to assist it. It might take me a little time, but I’ll make sure to work on it and keep you updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dzenanz dzenanz dzenanz left review comments

@ericspod ericspod Awaiting requested review from ericspod

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Print what package should be installed when suitable writer is missing
4 participants
@ytl0623 @KumoLiu @dzenanz @ericspod