-
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
WandbLogger to log model topology by default #8662
WandbLogger to log model topology by default #8662
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8662 +/- ##
======================================
Coverage 89% 89%
======================================
Files 169 169
Lines 14075 14075
======================================
Hits 12468 12468
Misses 1607 1607 |
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 ! Any idea of implication on training performance ?
I don't think it will have any negative impacts on training performance, because it's just extra logging. This was the default behavior for wandb some time ago (before wandb client commit #2395 on July 16) I just found out that wandb added Should we raise the minimum version requirement for wandb logger? Or we discard my PR and leave the user handle this behavior by themselves? (which is what I'm doing for my project) |
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 great. Impact is minor since this needs to be called manually anyway.
@gau-nernst could you add a changelog entry in CHANGELOG.md ?
@gau-nernst if you merge master into this branch the failing gpu test will pass and we can merge. |
@awaelchli I have rebased from master and added an entry to CHANGELOG.md |
What does this PR do?
Fixes #8638
log_graph=True
parameter toWandbLogger.watch()
and pass it towandb.watch()
log_graph
Recently wandb introduces the parameter
log_graph
inwandb.watch()
. By default, it isFalse
, so model topology won't be logged (see here)Does your PR introduce any breaking changes? If yes, please list them.
Before submitting
PR review
Anyone in the community is welcome to review the PR.
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 🙃