-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Feature: wandb logger #627
Conversation
There was a problem hiding this 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
pytorch_lightning/logging/wandb.py
Outdated
|
||
from pytorch_lightning.logging import WandbLogger | ||
wandb_logger = WandbLogger( | ||
name="my_new_nun", # Optional, display name |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
pytorch_lightning/logging/wandb.py
Outdated
|
||
.. code-block:: python | ||
|
||
from pytorch_lightning.logging import WandbLogger |
There was a problem hiding this comment.
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__() |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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__()
tests/test_logging.py
Outdated
tutils.reset_seed() | ||
|
||
try: | ||
from pytorch_lightning.logging import WandbLogger |
There was a problem hiding this comment.
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
tests/test_logging.py
Outdated
tutils.reset_seed() | ||
|
||
try: | ||
from pytorch_lightning.logging import WandbLogger |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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...
It seems the failed Travis tests are not related to this PR. |
True, could you pls double number epochs in the two tests... |
This is so cool, thanks @borisdayma! @Borda do you need anything else before we can merge? |
Hi, I'm just checking if you need anything else from me? |
Hey @Borda do you think we'll be able to get this merged soon? |
pls ping @williamFalcon , only he can do it... |
Before submitting
Pull request created as documented in logging page
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!