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

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

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.

"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

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.

@vectorvp
Copy link
Contributor

@KumoLiu, hello, I want to help with merging this PR. How can I do so? Make a new PR with changes you requested?

@KumoLiu
Copy link
Contributor

KumoLiu commented Nov 22, 2024

@KumoLiu, hello, I want to help with merging this PR. How can I do so? Make a new PR with changes you requested?

Sure. @ytl0623, if you’re short on time to make the update, perhaps @vectorvp could assist with it. Thanks!

@vectorvp
Copy link
Contributor

@KumoLiu, do we want to keep modifications in module.py?

@KumoLiu
Copy link
Contributor

KumoLiu commented Nov 26, 2024

resolve_writer

No, we can just extend the existing resolve_writer as well.

@vectorvp vectorvp mentioned this pull request Nov 26, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Print what package should be installed when suitable writer is missing
5 participants