refactor imports of logger dependencies #4860

Borda · 2020-11-25T22:04:34Z

What does this PR do?

Refactoring imports around optional loggers, @awaelchli
resolves partially #2266

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 your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
Did you make sure to update the documentation with your changes?
Did you write any new necessary tests?
Did you verify new and existing tests pass locally with your changes?
If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self-explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified; Bugfixes should be including in bug-fix release milestones (m.f.X) and features should be included in (m.X.b) releases.

Did you have fun?

Make sure you had fun coding 🙃

pytorch_lightning/utilities/__init__.py

Borda · 2020-11-26T19:35:29Z

pytorch_lightning/loggers/__init__.py

+from pytorch_lightning.loggers.comet import COMET_AVAILABLE, CometLogger
+from pytorch_lightning.loggers.mlflow import MLFLOW_AVAILABLE, MLFlowLogger
+from pytorch_lightning.loggers.neptune import NEPTUNE_AVAILABLE, NeptuneLogger
+from pytorch_lightning.loggers.test_tube import TESTTUBE_AVAILABLE, TestTubeLogger
+from pytorch_lightning.loggers.wandb import WANDB_AVAILABLE, WandbLogger


I guess alco this one split per logger, right? @tchaton @SeanNaren
even this one is too large :]

Borda · 2020-11-26T22:45:22Z

@awaelchli do you have suggestions why the mocking tests for loggers are failing?

pytorch_lightning/loggers/wandb.py

Borda · 2020-12-17T09:35:56Z

@awaelchli it seems to be still falling, so far I see it seems that with mocking we cannot use the actual _module_available so not sure what would be better if the func also accepts mock packages, but then it degrades the functions or redoes logger mocking...
Well, an alternative would be to add extra arg to the _module_available(..., strict: bool = True) and with not strict it would also, work with mocked packages... What do you think?

awaelchli · 2020-12-17T10:34:12Z

so far I see it seems that with mocking we cannot use the actual _module_available so not sure what would be better if the func also accepts mock packages, but then it degrades the functions or redoes logger mocking.

of course it can :)
you deleted some lines, that's why tests couldn't mock the experiment objects. See my commits, I reverted it and now it should be working.

You can test this also locally, all you have to do is uninstall the loggers (pip uninstall wandb, for example) and then run the tests and they should still pass.

codecov · 2021-01-01T21:42:59Z

Codecov Report

Merging #4860 (2706064) into master (410d67f) will increase coverage by 0%.
The diff coverage is 98%.

@@          Coverage Diff           @@
##           master   #4860   +/-   ##
======================================
  Coverage      93%     93%           
======================================
  Files         134     134           
  Lines        9976    9992   +16     
======================================
+ Hits         9294    9309   +15     
- Misses        682     683    +1

Borda · 2021-01-01T21:56:50Z

@awaelchli mind recheck? 🐰

pytorch_lightning/loggers/mlflow.py

.github/workflows/ci_test-conda.yml

pytorch_lightning/loggers/__init__.py

pytorch_lightning/loggers/comet.py

pytorch_lightning/loggers/mlflow.py

pytorch_lightning/loggers/wandb.py

…Lightning/pytorch-lightning into refactor/imports-loggers

.github/workflows/ci_test-base.yml

README.md

rohitgr7

looks like some tests are failing on master.

tests/loggers/test_mlflow.py

Borda · 2021-01-05T22:22:28Z

looks like some tests are failing on master.

yes, but the last commit here touching code b2cbf7c is fine...

* refactor imports of logger dependencies * fix * fix * fix * name * fix * mocks * fix tests * fix mlflow * fix test tube * fix wandb import check * whitespace * name * name * hack * hack * rev * fix * update mlflow import check * try without installing conda dep * . * . * . * . * . * . * . * . * . Co-authored-by: Adrian Wälchli <[email protected]> Co-authored-by: Adrian Wälchli <[email protected]> (cherry picked from commit ec0fb7a)

Borda added feature Is an improvement or enhancement refactor logger Related to the Loggers 3rd party Related to a 3rd-party labels Nov 25, 2020

Borda added this to the 1.1 milestone Nov 25, 2020

Borda requested a review from awaelchli November 25, 2020 22:04

Borda added the help wanted Open to be worked on label Nov 25, 2020

Borda mentioned this pull request Nov 26, 2020

Import optional packages in a nicer way Lightning-Universe/lightning-bolts#346

Open

Borda commented Nov 26, 2020

View reviewed changes

pytorch_lightning/utilities/__init__.py Outdated Show resolved Hide resolved

Borda removed the help wanted Open to be worked on label Nov 26, 2020

Borda marked this pull request as ready for review November 26, 2020 12:32

Borda requested review from ananyahjha93, justusschock, nateraw, SeanNaren, tchaton, teddykoker and williamFalcon as code owners November 26, 2020 12:32

Borda mentioned this pull request Nov 26, 2020

Backend unit tests #4856

Merged

11 tasks

Borda commented Nov 26, 2020

View reviewed changes

Borda added the priority: 1 Medium priority task label Nov 26, 2020

awaelchli reviewed Nov 27, 2020

View reviewed changes

pytorch_lightning/loggers/wandb.py Outdated Show resolved Hide resolved

pytorch_lightning/loggers/wandb.py Outdated Show resolved Hide resolved

Borda modified the milestones: 1.1, 1.1.x Nov 30, 2020

mergify bot requested a review from a team December 12, 2020 14:58

Borda force-pushed the refactor/imports-loggers branch from 0a58bd1 to b8ad06c Compare December 17, 2020 09:09

Borda force-pushed the refactor/imports-loggers branch from 4eb387a to c360fa8 Compare December 20, 2020 07:25

Borda enabled auto-merge (squash) January 1, 2021 21:56

awaelchli approved these changes Jan 1, 2021

View reviewed changes

Borda added the ready PRs ready to be merged label Jan 1, 2021

tchaton reviewed Jan 2, 2021

View reviewed changes

pytorch_lightning/loggers/mlflow.py Show resolved Hide resolved

Borda requested a review from tchaton January 2, 2021 11:07

tchaton requested changes Jan 4, 2021

View reviewed changes

Borda requested a review from tchaton January 4, 2021 07:55

tchaton approved these changes Jan 4, 2021

View reviewed changes

Merge branch 'master' into refactor/imports-loggers

7395f0b

s-rog approved these changes Jan 4, 2021

View reviewed changes

Merge branch 'master' into refactor/imports-loggers

1fe2f1e

SkafteNicki approved these changes Jan 5, 2021

View reviewed changes

Borda and others added 7 commits January 5, 2021 11:23

Merge branch 'master' into refactor/imports-loggers

63b22f4

Merge branch 'master' into refactor/imports-loggers

7e2664a

flake8

b2cbf7c

Merge branch 'master' into refactor/imports-loggers

cda0f87

try 3.7

9ec8616

Merge branch 'refactor/imports-loggers' of https://github.com/PyTorch…

10e4be5

…Lightning/pytorch-lightning into refactor/imports-loggers

Merge branch 'master' into refactor/imports-loggers

2706064

edenlightning reviewed Jan 5, 2021

View reviewed changes

.github/workflows/ci_test-base.yml Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

Borda requested a review from edenlightning January 5, 2021 19:09

Borda commented Jan 5, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

Apply suggestions from code review

87de356

rohitgr7 approved these changes Jan 5, 2021

View reviewed changes

tests/loggers/test_mlflow.py Show resolved Hide resolved

Borda merged commit ec0fb7a into master Jan 5, 2021

Borda deleted the refactor/imports-loggers branch January 5, 2021 19:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor imports of logger dependencies #4860

refactor imports of logger dependencies #4860

Borda commented Nov 25, 2020 •

edited

Loading

Borda Nov 26, 2020 •

edited

Loading

Borda commented Nov 26, 2020

Borda commented Dec 17, 2020

awaelchli commented Dec 17, 2020

codecov bot commented Jan 1, 2021 •

edited

Loading

Borda commented Jan 1, 2021

rohitgr7 left a comment

Borda commented Jan 5, 2021

refactor imports of logger dependencies #4860

refactor imports of logger dependencies #4860

Conversation

Borda commented Nov 25, 2020 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

Borda Nov 26, 2020 • edited Loading

Choose a reason for hiding this comment

Borda commented Nov 26, 2020

Borda commented Dec 17, 2020

awaelchli commented Dec 17, 2020

codecov bot commented Jan 1, 2021 • edited Loading

Codecov Report

Borda commented Jan 1, 2021

rohitgr7 left a comment

Choose a reason for hiding this comment

Borda commented Jan 5, 2021

Borda commented Nov 25, 2020 •

edited

Loading

Borda Nov 26, 2020 •

edited

Loading

codecov bot commented Jan 1, 2021 •

edited

Loading