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

feat: print packages to install #8240

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

feat: print packages to install #8240

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions monai/data/image_writer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

from __future__ import annotations

import re
from collections.abc import Mapping, Sequence
from typing import TYPE_CHECKING, Any, cast

Expand Down Expand Up @@ -106,17 +107,23 @@ def resolve_writer(ext_name, error_if_not_found=True) -> Sequence:
if fmt.startswith("."):
fmt = fmt[1:]
avail_writers = []
required_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:
except OptionalImportError as e:
error_match = re.search(r"`(.*?)`", str(e))
if error_match:
required_pkg.append(error_match.group(1))
Comment on lines +117 to +119
Copy link
Member

Choose a reason for hiding this comment

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

I suggest you modify OptionalImportError to have a member recording the name of the package that isn't present so you don't have to do a re search. This would be used here like this raise OptionalImportError(err_msg, pkg_name) in place of the original line. The other place this is raised is here where the second argument could just be None.

Copy link
Contributor Author

@vectorvp vectorvp Nov 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@ericspod, If I understand you correctly:

  1. The line here should be modified to
    raise OptionalImportError(err_msg, pkg_name)
  2. In image_writer I change this line to
    raise OptionalImportError(f"No ImageWriter backend found for {fmt}.", None)
  3. Remove exception lines with re.search in image_writer.
  4. And to make it all works I should modify OptionalImportError module in monai/utils/module.py by adding a functionality to track package names that aren't present?

Copy link
Member

Choose a reason for hiding this comment

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

@ericspod, If I understand you correctly:

1. The line [here](https://github.com/Project-MONAI/MONAI/blob/e73257caa79309dcce1e93abf1632f4bfd75b11f/monai/utils/module.py#L473) should be modified to
   `raise OptionalImportError(err_msg, pkg_name)`

2. In image_writer I change this [line](https://github.com/Project-MONAI/MONAI/blob/dev/monai/data/image_writer.py#L119) to
   `raise OptionalImportError(f"No ImageWriter backend found for {fmt}.", None)`

3. Remove exception lines with re.search in image_writer.

4. And to make it all works I should modify `OptionalImportError` module in monai/utils/module.py by adding a functionality to track package names that aren't present?

I think that's it, that'll allow you to get the name of the package without assumptions about the exception's error string, it'll be a more robust way of doing this going forward. Thanks!

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}.")
raise OptionalImportError(
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please also include the test case in test_image_rw.py to ensure the change works as expected? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KumoLiu, should it be added as a separate class?

f"No ImageWriter backend found for {fmt}, install one of the required packages in {required_pkg}."
)
writer_tuple = ensure_tuple(avail_writers)
SUPPORTED_WRITERS[fmt] = writer_tuple
return writer_tuple
Expand Down
Loading