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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Import optional packages in a nicer way #346

Open
akihironitta opened this issue Nov 9, 2020 · 5 comments
Open

Import optional packages in a nicer way #346

akihironitta opened this issue Nov 9, 2020 · 5 comments
Labels
discussion enhancement New feature or request help wanted Extra attention is needed refactoring
Milestone

Comments

@akihironitta
Copy link
Contributor

akihironitta commented Nov 9, 2020

馃殌 Feature

I would like to suggest to implement a context manager class in order to handle optional imports better. With the context manager implemented, importing optional packages and deferring raising ModuleNotFoundError will look like:

with ContextManager() as cm:
    import torchvision  # doesn't raise `ModuleNotFoundError` here even if it fails.

class SomeClass:
    def __init__(self, ...):
        cm.check()  # raises `ModuleNotFoundError` if `import torchvision` failed.
        ...

Motivation

The suggested context manager reduces duplicate information in the codebase. Currently, there are redundant lines such as try: ... except ModuleNotFoundErorr: ..., _PACKAGE_AVAILABLE and raise ModuleNotFoundError(...).

Example

https://github.com/PyTorchLightning/pytorch-lightning-bolts/blob/4d254fde6112b21436003028d553a726bf7ea6ef/pl_bolts/datamodules/cifar10_datamodule.py#L12-L20
https://github.com/PyTorchLightning/pytorch-lightning-bolts/blob/4d254fde6112b21436003028d553a726bf7ea6ef/pl_bolts/datamodules/cifar10_datamodule.py#L86-L89

Pitch

Here is a minimal implementation of context manager and its behaviour.

Example

class ContextManager:
    def __init__(self):
        self.exc = None
        
    def __enter__(self):
        return self

    def __exit__(self, exc_type, exc_value, traceback):
        self.exc = exc_type, exc_value, traceback
        return True  # Ignores exception if True and reraises exception if False

    def check(self):
        if self.exc is not None:
            exc_type, exc_value, traceback = self.exc
            raise exc_type(exc_value).with_traceback(traceback)

# Here's how it behaves
print("Try to import.")
with ContextManager() as cm:
    import aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

print("Tried to import.")
cm.check()
Try to import.
Tried to import.
Traceback (most recent call last):
  File "main.py", line 22, in <module>
    cm.check()
  File "main.py", line 15, in check
    raise exc_type(exc_value).with_traceback(traceback)
  File "main.py", line 19, in <module>
    import aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
ModuleNotFoundError: No module named 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'

Alternatives

As suggested in #338 (comment) by @Borda, we could define global variables like _TORCHVISION_AVAILABLE in pl_bolts/__init__.py, but in that way, we will have to add new variables like _NEW_PACKAGE_AVAILABLE as we add new optional packages to Bolts.

See also

Built-in Types - Context Manager Types
Stack Overflow - How to safely handle an exception inside a context manager

Other comments

We can implement the suggested context manager in PL and use it from Bolts. If it should be implemented in PL, could you transfer this issue to PL repo? GitHub Docs - Transferring an issue to another repository

Let me know if it sounds reasonable or too much engineering!

@akihironitta akihironitta added enhancement New feature or request help wanted Extra attention is needed labels Nov 9, 2020
@Borda Borda added refactoring let's do it! Looking forward to have it implemented discussion and removed let's do it! Looking forward to have it implemented labels Nov 9, 2020
@Borda
Copy link
Member

Borda commented Nov 9, 2020

I would agree with the first usage example, define _TORCHVISION_AVAILABLE in relevant upper level init or even in pl_bolts.__init__ but then we do not need to define extra Contex class, then the limiting importing is handled inside
https://github.com/PyTorchLightning/pytorch-lightning-bolts/blob/f00e85dd204fa2434c47d9ae20614b0a2baeb1f0/pl_bolts/utils/warnings.py#L7
but looking at it again, it looks cute: @ananyahjha93 what do you think?

@akihironitta
Copy link
Contributor Author

Found some related PRs in other repos, so let me leave them here:

@akihironitta
Copy link
Contributor Author

@ananyahjha93 Any thoughts on this? :]

@akihironitta

This comment has been minimized.

@Borda

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion enhancement New feature or request help wanted Extra attention is needed refactoring
Development

No branches or pull requests

2 participants