[WIP] [discussion] Re-raise logger import warnings when not installed

[awaelchli]

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure to update the docs?
• Did you write any new necessary tests?
• If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

When importing a logger this way
from pytorch_lightning.loggers.wandb import WandbLogger
an error message will be printed if the package is not installed.
However, if we import from the top level module, i.e.,
from pytorch_lightning.loggers import WandbLogger
the error message is discarded.
This PR fixes that.

Probably what is happening in #1487.

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[Borda]

no, we do not force a user to install all loggers, that is also the reason why we have extras not all loggers in requirements...

[codecov]

Codecov Report

Merging #1489 into master will decrease coverage by 0%.
The diff coverage is 0%.

@@          Coverage Diff           @@
##           master   #1489   +/-   ##
======================================
- Coverage      91%     91%   -0%     
======================================
  Files          67      67           
  Lines        3745    3747    +2     
======================================
  Hits         3403    3403           
- Misses        342     344    +2     

[awaelchli]

ok I see, I thought these errors are only raised when the user imports it.
So is it not possible to tell the user that there is a missing package?

[awaelchli]

ok I understand now that my fix won't work, but my intention was simply to allow the custom error message to pass to the top level in case the user imports
from pytorch_lightning.loggers import WandbLogger
instead of
from pytorch_lightning.loggers.wandb import WandbLogger
I'm sure questions like this #1487 will be asked more.

[Borda]

I see your point and agree that we shall raise import error even when user explicitly imports logger from pytorch_lightning.loggers.__init__ just not sure how to avoids raising errors from other loggers which are not requested...

[Borda]

the actual error message is:

>>> from pytorch_lightning.loggers import NeptuneLogger
/usr/lib/python3/dist-packages/apport/report.py:13: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
  import fnmatch, glob, traceback, errno, sys, atexit, locale, imp
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: cannot import name 'NeptuneLogger' from 'pytorch_lightning.loggers' (/home/jb/.local/lib/python3.7/site-packages/pytorch_lightning/loggers/__init__.py)
Error in sys.excepthook:
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/apport_python_hook.py", line 63, in apport_excepthook
    from apport.fileutils import likely_packaged, get_recent_crashes
  File "/usr/lib/python3/dist-packages/apport/__init__.py", line 5, in <module>
    from apport.report import Report
  File "/usr/lib/python3/dist-packages/apport/report.py", line 30, in <module>
    import apport.fileutils
  File "/usr/lib/python3/dist-packages/apport/fileutils.py", line 23, in <module>
    from apport.packaging_impl import impl as packaging
  File "/usr/lib/python3/dist-packages/apport/packaging_impl.py", line 24, in <module>
    import apt
  File "/usr/lib/python3/dist-packages/apt/__init__.py", line 23, in <module>
    import apt_pkg
ModuleNotFoundError: No module named 'apt_pkg'

Original exception was:
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: cannot import name 'NeptuneLogger' from 'pytorch_lightning.loggers' (/home/jb/.local/lib/python3.7/site-packages/pytorch_lightning/loggers/__init__.py)

[awaelchli]

I think it is not possible. The only thing we could do is add a note to the documentation that the user needs to manually install the logger. Should I do this? Can directly add it to this PR #1484 . What do you think?

[Borda]

ok, pls add a note to the docs-logger PR and let's also keep this open for a while if we find another way to using loggers...

[awaelchli]

this is probably an unsolvable problem, let's close it @Borda ?

[justusschock]

I think, what we could do here: have all imports to these packages locally inside the logger classes. This way we can normally import them and don't have to catch ImportErrors and the User still gets errors during instantiation (which would be fine I guess).

[justusschock]

@awaelchli If you enable edit from contributors for your PR I can do it. Should be a rather quick fix :)

[awaelchli]

It's already turned on but I think it won't work because here I was still using my own fork when I submitted the PR. Feel free to submit a fresh PR since I have not really changed anything here :)