feat(wandb): add sync_step by borisdayma · Pull Request #5351 · Lightning-AI/pytorch-lightning

borisdayma · 2021-01-04T20:21:34Z

What does this PR do?

Add more flexiblitiy to WandbLogger with sync_step parameter.

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 your PR does only one thing, instead of bundling different changes together?
Did you make sure to update the documentation with your changes [if needed]?
Did you write any new necessary tests [no need for typos, docs]?
Did you verify new and existing tests pass locally with your changes?
If you made a notable change (that affects users), did you update the CHANGELOG?

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:

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self-explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified
Check that target branch and milestone are aligned!

Did you have fun?

Make sure you had fun coding 🙃

pep8speaks · 2021-01-04T20:21:38Z

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-01-24 16:18:30 UTC

codecov · 2021-01-05T00:23:05Z

Codecov Report

Merging #5351 (f650d72) into release/1.2-dev (0c9960b) will increase coverage by 4%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##           release/1.2-dev   #5351    +/-   ##
================================================
+ Coverage               89%     92%    +4%     
================================================
  Files                  153     153            
  Lines                10840   10845     +5     
================================================
+ Hits                  9628   10022   +394     
+ Misses                1212     823   -389

rohitgr7 · 2021-01-05T19:38:05Z

this shall go in release/1.2-dev?

Borda · 2021-01-06T00:23:34Z

@borisdayma mind rebase your change on release/1.2-dev 🐰 or do you prefer to have it as bugfix to master?

borisdayma · 2021-01-06T11:46:03Z

@Borda I tried to rebase, then just merge release/1.2-dev but there's a lot of conflicts (on files I didn't touch) so it scares me a little bit…
If it's ok with you a bugfix to master may be easier… at least for me!

justusschock · 2021-01-07T11:06:07Z

pytorch_lightning/loggers/wandb.py

        log_model: Save checkpoints in wandb dir to upload on W&B servers.
-        experiment: WandB experiment object.
        prefix: A string to put at the beginning of metric keys.
+        sync_step: Sync Trainer step with wandb step.


Can you put this after the experiment flag, so that old args order is still working?

I changed the order within the function itself.
For the description of args, I let this order because I thought it made more sense. For example id and version are the same thing. Is that ok?

pytorch_lightning/loggers/wandb.py

Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

borisdayma · 2021-01-20T14:14:38Z

Hi, just checking if I need to do anything else on my side?
I know of a few people eager to see this PR merged.

pytorch_lightning/loggers/wandb.py

akihironitta · 2021-01-24T08:30:54Z

pytorch_lightning/loggers/wandb.py

        name: Optional[str] = None,
        save_dir: Optional[str] = None,
-        offline: bool = False,
+        offline: Optional[bool] = False,


I don't think the change here is necessary. Optional[bool] is equivalent to Union[bool, None], and offline only accepts bool, right?

It should work but actually I want to clean up the way we pass init parameters in a follow-up PR and suggest to just use kwargs.
The possible parameters has evolved and we could just refer to wandb.init doc

That makes sense.

tchaton

LGTM !

borisdayma mentioned this pull request Jan 4, 2021

WandbLogger: add more flexibility with logging step #5194

Closed

borisdayma marked this pull request as ready for review January 5, 2021 08:44

borisdayma requested review from Borda, SeanNaren, awaelchli, justusschock, tchaton and williamFalcon as code owners January 5, 2021 08:44

Borda added this to the 1.2 milestone Jan 5, 2021

Borda changed the base branch from master to release/1.2-dev January 6, 2021 00:15

Borda requested review from ananyahjha93, edenlightning and teddykoker as code owners January 6, 2021 00:15

borisdayma added 7 commits January 6, 2021 17:45

docs(wandb): add details to args

f356c60

feat(wandb): no sync between trainer and W&B steps

5b0a769

style: pep8

486c29b

tests(wandb): test sync_step

9f2a995

docs(wandb): add references

1ae326a

docs(wandb): fix typo

223fcab

feat(wandb): more explicit warning

121a8c0

rohitgr7 force-pushed the feat-sync_step branch from be44f3c to 121a8c0 Compare January 6, 2021 12:18

This was linked to issues Jan 6, 2021

wandb logger problem with on_step log on validation #4980

Closed

Logging validation metrics with val_check_interval #5070

Closed

rohitgr7 added bug Something isn't working logger Related to the Loggers labels Jan 6, 2021

rohitgr7 added feature Is an improvement or enhancement and removed bug Something isn't working labels Jan 6, 2021

justusschock approved these changes Jan 7, 2021

View reviewed changes

feat(wandb): order of args

e77cadd

awaelchli reviewed Jan 8, 2021

View reviewed changes

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

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

pytorch_lightning/loggers/wandb.py Show resolved Hide resolved

awaelchli approved these changes Jan 8, 2021

View reviewed changes

style: Apply suggestions from code review

7b1b9e1

Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

borisdayma mentioned this pull request Jan 13, 2021

"Step must only increase in log calls" when adding W&B logger after some training wandb/wandb#1626

Closed

awaelchli mentioned this pull request Jan 14, 2021

Ignore step param in Neptune logger's log_metric method #5510

Merged

12 tasks

awaelchli added the ready to be merged PRs ready to be merged label Jan 15, 2021

Merge branch 'release/1.2-dev' into feat-sync_step

c71c0fb

awaelchli mentioned this pull request Jan 18, 2021

inconsistent step counting for train and val after resuming from checkpoint #5547

Closed

borisdayma added 2 commits January 20, 2021 07:55

style: long line

e4a6245

Merge branch 'release/1.2-dev' into feat-sync_step

2a0d8ad

rohitgr7 reviewed Jan 20, 2021

View reviewed changes

pytorch_lightning/loggers/wandb.py Show resolved Hide resolved

rohitgr7 approved these changes Jan 20, 2021

View reviewed changes

jeffydc approved these changes Jan 20, 2021

View reviewed changes

awaelchli enabled auto-merge (squash) January 21, 2021 13:01

Merge branch 'release/1.2-dev' into feat-sync_step

10133bb

SeanNaren disabled auto-merge January 23, 2021 19:27

SeanNaren enabled auto-merge (squash) January 23, 2021 19:27

akihironitta reviewed Jan 24, 2021

View reviewed changes

tchaton approved these changes Jan 24, 2021

View reviewed changes

mergify bot added 3 commits January 24, 2021 10:47

Merge branch 'release/1.2-dev' into feat-sync_step

175541c

Merge branch 'release/1.2-dev' into feat-sync_step

b42673d

Merge branch 'release/1.2-dev' into feat-sync_step

f650d72

SeanNaren merged commit f0fafa2 into Lightning-AI:release/1.2-dev Jan 24, 2021

borisdayma mentioned this pull request Jan 25, 2021

WandB-Logger drops all the logged values in training step for PyTorch Lightning wandb/wandb#1507

Closed

borisdayma mentioned this pull request Feb 12, 2021

fix(wandb): prevent WandbLogger from dropping values #5931

Merged

12 tasks

Conversation

borisdayma commented Jan 4, 2021 • edited by awaelchli Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Before submitting

PR review

Did you have fun?

Uh oh!

pep8speaks commented Jan 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2021-01-24 16:18:30 UTC

Uh oh!

codecov bot commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rohitgr7 commented Jan 5, 2021

Uh oh!

Borda commented Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

borisdayma commented Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justusschock Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

borisdayma Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

borisdayma commented Jan 20, 2021

Uh oh!

Uh oh!

akihironitta Jan 24, 2021

Choose a reason for hiding this comment

Uh oh!

borisdayma Jan 24, 2021

Choose a reason for hiding this comment

Uh oh!

akihironitta Jan 24, 2021

Choose a reason for hiding this comment

Uh oh!

tchaton left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

borisdayma commented Jan 4, 2021 •

edited by awaelchli

Loading

pep8speaks commented Jan 4, 2021 •

edited

Loading

codecov bot commented Jan 5, 2021 •

edited

Loading

Borda commented Jan 6, 2021 •

edited

Loading

borisdayma commented Jan 6, 2021 •

edited

Loading