Improved docs for Loggers #1484

awaelchli · 2020-04-14T06:56:52Z

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?

A continuation of a series of doc improvements. Follows #1389, #1370, #1483

typos
added links for classes, methods, modules etc. for easier navigation within the page.
converted many code snippets to doctests and fixed failing ones.
added instructions how to install the loggers ImportError: cannot import name 'WandbLogger' from 'pytorch_lightning.loggers' #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 🙃

pep8speaks · 2020-04-14T06:56:58Z

Hello @awaelchli! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-04-16 13:19:02 UTC

Borda

Great work!

not sure but I think that doctest in RST files are not executed, so there is no need of converting them here

pytorch_lightning/loggers/mlflow.py

pytorch_lightning/loggers/comet.py

pytorch_lightning/loggers/trains.py

awaelchli · 2020-04-14T07:21:20Z

@Borda
Good point about doctest. I was wondering.
I can run it locally like this for example:
python -m doctest docs/source/experiment_logging.rst
Maybe we could add it to CI like this?
I will check the links you posted.

Borda · 2020-04-14T07:22:45Z

Maybe we could add it to CI like this?

sure add it to CI :]

awaelchli · 2020-04-14T13:01:07Z

Hey @jakubczakon do you know how I could make the doctest pass in online mode without complaining about an invalid api key?

api_key="ANONYMOUS" doesn't seem to work, and I noticed the tests use api_key="test" but for some reason that doesn't work in doctests.

jakubczakon · 2020-04-14T15:24:45Z

Hi @awaelchli,

I think the reason could be:

the project is not specified correctly. api_token="ANONYMOUS" works for some projects in the "shared" organization. We need to add that project by hand. That said "shared/pytorch-lightning-integration" is working.
you are using an older version of neptune-client that doesn't yet have "ANONYMOUS" token hardcoded -> updating the project should fix it

Also, the fact that api_token="test" works in offline mode is because in offline mode nothing gets sent to Neptune so no token is checked etc.

Could you please paste the particular piece of code that is causing the problem?

awaelchli · 2020-04-14T15:47:42Z

@jakubczakon AWESOME. Your tipp with the version update did the trick! I didn't see it fail locally because I had the new version and the CI tests with minimal requirements.
Thanks for your help!!

awaelchli · 2020-04-14T16:32:14Z

@Borda The doctest extension is already in conf.py. It can be run with make doctest. If I do locally, I get many import errors. Let's maybe fix it in another PR?

Borda · 2020-04-14T17:10:27Z

sounds fair to do it extra PR... I would maybe run in pytest then sphinx...

UPDATE: #1511

mergify · 2020-04-15T00:34:11Z

This pull request is now in conflict... :(

codecov · 2020-04-15T06:13:04Z

Codecov Report

Merging #1484 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1484   +/-   ##
======================================
  Coverage      91%     91%           
======================================
  Files          67      67           
  Lines        3754    3754           
======================================
  Hits         3408    3408           
  Misses        346     346

mergify · 2020-04-15T15:16:06Z

This pull request is now in conflict... :(

This reverts commit a45aeb4.

Borda · 2020-04-16T11:55:15Z

@awaelchli rebase to master shall fix it...
unfortunately I cannot do it for you...
graphql error: 'Could not resolve to a PullRequest with the number of 1599.'

Borda

add tests of RST files in GH action and CircleCI

pytorch_lightning/loggers/mlflow.py

awaelchli · 2020-04-16T12:43:26Z

@Borda about RST files: could I do that in another PR? on master "make doctest" has many errors (including RST) that would need to be solved and I think it would be better to do in another PR. What do you think?
#1484 (comment)

Borda

so pls make it an issue linked to #714 so we do not forget...
because now it seems as these examples shall be tested (because they are formatted as doctest) but in fact, they are not... lol

awaelchli · 2020-04-16T12:54:29Z

I already added a note in #714 as a reminder 2 days ago.
and the tests that I added here in rst I tested locally, with "make doctest docs/source/file.rst"

* improve __init__ * improve logger base * improve comet logger docs * improved docs for mlflow * improved nepune logger docs * fix matplotlib import issue * improve tensorboard docs * improve docs for test tube * improved trains logger docs * improve wandb logger docs * improved docs in experiment_logging.rst * added MLflow to the list of loggers * fix too long lines * fix trains doctest * fix neptune doctest * fix mlflow doctest * Apply suggestions from code review Co-Authored-By: Jirka Borovec <Borda@users.noreply.github.com> * Apply suggestions from code review * fix whitespace * try bypass mode for neptune (fix doctest api key error) * try "test" as api key * Revert "try "test" as api key" This reverts commit fd77db2. * try test as api key * update neptune docs * bump neptune minimal version * revert unnecessary bypass code * test if CI runs doctests in .rst files * Revert "test if CI runs doctests in .rst files" This reverts commit a45aeb4. * add doctest directive * neptune demo links * added tutorial link for W&B * fix line too long * fix merge error * fix merge error * add instructions how to install loggers * add instructions how to install the loggers * hide _abc_impl property from docs * review Borda, 4 spaces * indentation in example sections * blank Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

mergify bot requested a review from a team April 14, 2020 06:57

Borda added the docs Documentation related label Apr 14, 2020

Borda reviewed Apr 14, 2020

View reviewed changes

pytorch_lightning/loggers/mlflow.py Show resolved Hide resolved

pytorch_lightning/loggers/comet.py Show resolved Hide resolved

pytorch_lightning/loggers/trains.py Show resolved Hide resolved

mergify bot requested a review from a team April 14, 2020 07:14

justusschock approved these changes Apr 14, 2020

View reviewed changes

mergify bot requested a review from a team April 14, 2020 08:30

awaelchli mentioned this pull request Apr 14, 2020

convert examples to doctests #714

Closed

awaelchli mentioned this pull request Apr 15, 2020

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

Closed

5 tasks

Adrian Wälchli added 11 commits April 15, 2020 18:18

improve __init__

7708f81

improve logger base

3f5271d

improve comet logger docs

7296409

improved docs for mlflow

c090f5b

improved nepune logger docs

4f1247a

fix matplotlib import issue

c609eae

improve tensorboard docs

075f202

improve docs for test tube

fd7ab9a

improved trains logger docs

e3f0dd2

improve wandb logger docs

4cd9d9a

improved docs in experiment_logging.rst

8de3daa

Adrian Wälchli added 12 commits April 15, 2020 18:20

revert unnecessary bypass code

44ca48c

test if CI runs doctests in .rst files

95a8224

add doctest directive

182d67e

Revert "test if CI runs doctests in .rst files"

ee0e14b

This reverts commit a45aeb4.

neptune demo links

7254bd6

added tutorial link for W&B

19f02be

fix line too long

330bb39

fix merge error

7f5a057

fix merge error

0f31635

add instructions how to install loggers

e2bc300

add instructions how to install the loggers

5f65c0c

hide _abc_impl property from docs

21596bb

Borda requested changes Apr 16, 2020

View reviewed changes

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

mergify bot requested a review from a team April 16, 2020 12:07

review Borda, 4 spaces

41081b5

Borda approved these changes Apr 16, 2020

View reviewed changes

Borda added the ready PRs ready to be merged label Apr 16, 2020

Borda added this to the 0.7.4 milestone Apr 16, 2020

mergify bot requested a review from a team April 16, 2020 12:48

Adrian Wälchli added 2 commits April 16, 2020 15:14

indentation in example sections

bb42628

blank

d1944e6

williamFalcon merged commit 6e1d72d into Lightning-AI:master Apr 16, 2020

awaelchli mentioned this pull request Apr 16, 2020

doctest for .rst files #1511

Merged

5 tasks

awaelchli deleted the docs/improve_loggers branch April 24, 2020 01:03

Borda modified the milestones: 0.7.4, v0.7.x Apr 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved docs for Loggers #1484

Improved docs for Loggers #1484

awaelchli commented Apr 14, 2020 •

edited

Loading

pep8speaks commented Apr 14, 2020 •

edited

Loading

Borda left a comment •

edited

Loading

awaelchli commented Apr 14, 2020

Borda commented Apr 14, 2020 •

edited

Loading

awaelchli commented Apr 14, 2020 •

edited

Loading

jakubczakon commented Apr 14, 2020

awaelchli commented Apr 14, 2020

awaelchli commented Apr 14, 2020

Borda commented Apr 14, 2020 •

edited

Loading

mergify bot commented Apr 15, 2020

codecov bot commented Apr 15, 2020 •

edited

Loading

mergify bot commented Apr 15, 2020

Borda commented Apr 16, 2020 •

edited

Loading

Borda left a comment

awaelchli commented Apr 16, 2020

Borda left a comment

awaelchli commented Apr 16, 2020

Improved docs for Loggers #1484

Improved docs for Loggers #1484

Conversation

awaelchli commented Apr 14, 2020 • edited Loading

Before submitting

What does this PR do?

PR review

Did you have fun?

pep8speaks commented Apr 14, 2020 • edited Loading

Comment last updated at 2020-04-16 13:19:02 UTC

Borda left a comment • edited Loading

Choose a reason for hiding this comment

awaelchli commented Apr 14, 2020

Borda commented Apr 14, 2020 • edited Loading

awaelchli commented Apr 14, 2020 • edited Loading

jakubczakon commented Apr 14, 2020

awaelchli commented Apr 14, 2020

awaelchli commented Apr 14, 2020

Borda commented Apr 14, 2020 • edited Loading

mergify bot commented Apr 15, 2020

codecov bot commented Apr 15, 2020 • edited Loading

Codecov Report

mergify bot commented Apr 15, 2020

Borda commented Apr 16, 2020 • edited Loading

Borda left a comment

Choose a reason for hiding this comment

awaelchli commented Apr 16, 2020

Borda left a comment

Choose a reason for hiding this comment

awaelchli commented Apr 16, 2020

awaelchli commented Apr 14, 2020 •

edited

Loading

pep8speaks commented Apr 14, 2020 •

edited

Loading

Borda left a comment •

edited

Loading

Borda commented Apr 14, 2020 •

edited

Loading

awaelchli commented Apr 14, 2020 •

edited

Loading

Borda commented Apr 14, 2020 •

edited

Loading

codecov bot commented Apr 15, 2020 •

edited

Loading

Borda commented Apr 16, 2020 •

edited

Loading