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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: wandb logger #627

Merged
merged 13 commits into from
Jan 14, 2020
Merged

Conversation

borisdayma
Copy link
Contributor

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos, doc improvements)
    Pull request created as documented in logging page
  • Did you read the contributor guideline?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

Allows for logging through Weights & Biases

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?

Yep!

Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

Good addition, just remove try/except in tests


from pytorch_lightning.logging import WandbLogger
wandb_logger = WandbLogger(
name="my_new_nun", # Optional, display name
Copy link
Member

Choose a reason for hiding this comment

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

If all are optimal, there is no need to write it

Copy link
Contributor Author

@borisdayma borisdayma Dec 14, 2019

Choose a reason for hiding this comment

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

Here the intent was just to show sample use if anybody wanted to use those args as it does not appear in the doc and they would have to look at the docstring of logging.wandb.WandbLogger
I'm going to remove it.


.. code-block:: python

from pytorch_lightning.logging import WandbLogger
Copy link
Member

Choose a reason for hiding this comment

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

Consider write it as doctest


def __init__(self, name=None, save_dir=None, offline=False, id=None, anonymous=False,
version=None, project=None, tags=None):
super().__init__()
Copy link
Member

Choose a reason for hiding this comment

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

Is it correct way? I am use to write the child class name...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default super will look for one class above the current class, so here it will call LightningLoggerBase.__init__()

tutils.reset_seed()

try:
from pytorch_lightning.logging import WandbLogger
Copy link
Member

Choose a reason for hiding this comment

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

Remove try/except here, it has to be tested

tutils.reset_seed()

try:
from pytorch_lightning.logging import WandbLogger
Copy link
Member

Choose a reason for hiding this comment

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

Remove try/except here, it has to be tested

hparams = tutils.get_hparams()
model = LightningTestModel(hparams)

wandb_dir = os.path.join(tmpdir, "wandb")
Copy link
Member

Choose a reason for hiding this comment

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

I think that the sub folder is not needed, the temp dir is always clean and unique...

@borisdayma
Copy link
Contributor Author

It seems the failed Travis tests are not related to this PR.

@Borda
Copy link
Member

Borda commented Dec 14, 2019

True, could you pls double number epochs in the two tests...

@vanpelt
Copy link
Contributor

vanpelt commented Dec 17, 2019

This is so cool, thanks @borisdayma! @Borda do you need anything else before we can merge?

@borisdayma
Copy link
Contributor Author

Hi, I'm just checking if you need anything else from me?
Since this PR also affects some other tests that were failing it may be good to integrate it quickly to limit any future impact.

@vanpelt
Copy link
Contributor

vanpelt commented Jan 3, 2020

Hey @Borda do you think we'll be able to get this merged soon?

@Borda
Copy link
Member

Borda commented Jan 3, 2020

pls ping @williamFalcon , only he can do it...

@williamFalcon williamFalcon merged commit ec7fc97 into Lightning-AI:master Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants