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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(wandb): allow custom init args #6989
Conversation
Hello @borisdayma! Thanks for updating this PR. There are currently no PEP 8 issues detected in this Pull Request. Cheers! 馃嵒 Comment last updated at 2021-05-04 08:40:14 UTC |
Codecov Report
@@ Coverage Diff @@
## master #6989 +/- ##
========================================
- Coverage 91% 83% -8%
========================================
Files 200 200
Lines 12875 13594 +719
========================================
- Hits 11765 11337 -428
- Misses 1110 2257 +1147 |
pytorch_lightning/loggers/wandb.py
Outdated
@@ -89,13 +88,11 @@ class WandbLogger(LightningLoggerBase): | |||
|
|||
def __init__( | |||
self, | |||
name: Optional[str] = None, |
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.
this will fail if a user uses positional arguments
pls, maintain a proper deprecation process...
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.
Ok I聽put back name
and project
at their previous positions.
pytorch_lightning/loggers/wandb.py
Outdated
self._kwargs = kwargs | ||
# set wandb init arguments | ||
self._wandb_init = dict(resume='allow', id=version or id, dir=save_dir, | ||
anonymous='allow' if anonymous is True else None if anonymous is False else anonymous) |
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.
can we rather define this as look-up-table aka dictionary?
an_lut = {True: 'allow', False: None}
anonymous = an_lut.get(anonymous, anonymous)
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.
Great idea!
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.
nice
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you need further help see our docs: https://pytorch-lightning.readthedocs.io/en/latest/generated/CONTRIBUTING.html#pull-request or ask the assistance of a core contributor here or on Slack. Thank you for your contributions. |
Hi, any update on this PR? |
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.
LGTM
I think this should go into 1.3 because it's really more a fix than a feature.
Changed milestone. Thanks @borisdayma
@Borda check if your comments were addressed?
@@ -93,7 +92,7 @@ def __init__( | |||
save_dir: Optional[str] = None, | |||
offline: Optional[bool] = False, | |||
id: Optional[str] = None, | |||
anonymous: Optional[bool] = False, | |||
anonymous: Optional[bool] = None, |
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.
Why change the default?
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.
This is actually still the same behavior but is more in line with the W&B documentation so less prone to potential bugs.
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.
LGTM !
What does this PR do?
Fixes #6788
This PR lets users define any
wandb.init()
parameter, includingresume
which was previously forced toallow
.In addition, it handles the case where arguments are passed as
kwargs
vs as direct arguments.Before submitting
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:
Did you have fun?
Make sure you had fun coding 馃檭