feat(wandb): offset logging step when resuming #5050

borisdayma · 2020-12-10T03:12:25Z

What does this PR do?

When resuming a run, the step was not being offset to account for previous logged data.
It will now be correctly offset.

In addition, I took advantage to give users a hint when they have related issues trying to log "back in time". It is usually due to them logging manually other data (images, etc) and causing the wandb step to automatically increase. This will close wandb/wandb#1507

I added the same details in the documentation.

Before submitting

Was this discussed/approved via a Github issue? discussed in wandb issue
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? Since the other modification is just a warning
Did you make sure to update the documentation with your changes?
Did you write any new necessary tests?
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; Bugfixes should be including in bug-fix release milestones (m.f.X) and features should be included in (m.X.b) releases.

Did you have fun?

Make sure you had fun coding 🙃

pep8speaks · 2020-12-10T03:12:29Z

Hello @borisdayma! Thanks for updating this PR.

In the file pytorch_lightning/loggers/wandb.py:

Line 165:121: E501 line too long (122 > 120 characters)

Comment last updated at 2020-12-19 12:33:15 UTC

pytorch_lightning/loggers/wandb.py

codecov · 2020-12-10T04:17:13Z

Codecov Report

Merging #5050 (78e0d19) into master (4c34855) will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #5050   +/-   ##
======================================
  Coverage      93%     93%           
======================================
  Files         134     134           
  Lines        9928    9933    +5     
======================================
+ Hits         9242    9247    +5     
  Misses        686     686

Co-authored-by: Adrian Wälchli <[email protected]>

Borda

can we add a test for the multiple call?

pytorch_lightning/loggers/wandb.py

Co-authored-by: Jirka Borovec <[email protected]>

borisdayma · 2020-12-10T16:34:41Z

can we add a test for the multiple call?

@Borda Sure, you're talking about the warning?

borisdayma · 2020-12-11T01:24:03Z

@Borda I added a test for warnings displayed.

tchaton · 2020-12-14T07:30:14Z

pytorch_lightning/loggers/wandb.py

@@ -154,6 +161,9 @@ def log_metrics(self, metrics: Dict[str, float], step: Optional[int] = None) ->
        assert rank_zero_only.rank == 0, 'experiment tried to log from global_rank != 0'

        metrics = self._add_prefix(metrics)
+        if not self.WARNING_DISPLAYED and (step is not None and step + self._step_offset < self.experiment.step):


I think we have a better way to handle only once displayed Warning. Could be WarningCache. Need to check.

shall we reimplement this one?
https://github.com/PyTorchLightning/pytorch-lightning-bolts/blob/13863ccf1f96cb912406c029c50f4253f4599aee/pl_bolts/utils/warnings.py#L7

borisdayma · 2020-12-14T19:51:39Z

I'm now using WarningCache as suggested by @tchaton but it breaks my tests.
In particular I'm having trouble capturing the warnings. I tried with caplog, capsys and capfd but no output/err seems to be captured, even if I just do a direct call within the test to warnings.warn or logging.log.

Edit: looks like I can use recwarn in this case

borisdayma · 2020-12-15T00:45:01Z

I was able to make the tests work with WarningCache.
I had to rely on recwarn instead of caplog (not sure why) and made a little helper function.

…orch-lightning into feat_wandb_resume

* feat(wandb): offset logging step when resuming * feat(wandb): output warnings * fix(wandb): allow step to be None * test(wandb): update tests * feat(wandb): display warning only once * style: fix PEP issues * tests(wandb): fix tests * tests(wandb): improve test * style: fix whitespace * feat: improve warning Co-authored-by: Adrian Wälchli <[email protected]> * feat(wandb): use variable from class instance Co-authored-by: Jirka Borovec <[email protected]> * tests(wandb): check warnings * feat(wandb): use WarningCache * tests(wandb): fix tests * style: fix formatting Co-authored-by: Adrian Wälchli <[email protected]> Co-authored-by: Jirka Borovec <[email protected]>

borisdayma added 5 commits December 9, 2020 09:00

feat(wandb): offset logging step when resuming

f7c7994

feat(wandb): output warnings

a93cd0a

fix(wandb): allow step to be None

d00e57a

test(wandb): update tests

dc3794a

feat(wandb): display warning only once

eb75676

borisdayma requested review from awaelchli, Borda, justusschock, SeanNaren, tchaton and williamFalcon as code owners December 10, 2020 03:12

borisdayma added 5 commits December 9, 2020 21:15

style: fix PEP issues

69bf4bb

Merge branch 'master' into feat_wandb_resume

891acf4

tests(wandb): fix tests

5978805

tests(wandb): improve test

dbc15f2

style: fix whitespace

ea4f88e

awaelchli added 3rd party Related to a 3rd-party feature Is an improvement or enhancement logger Related to the Loggers labels Dec 10, 2020

awaelchli approved these changes Dec 10, 2020

View reviewed changes

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

awaelchli added this to the 1.1.x milestone Dec 10, 2020

feat: improve warning

1c3bc82

Co-authored-by: Adrian Wälchli <[email protected]>

Borda approved these changes Dec 10, 2020

View reviewed changes

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

Borda added the ready PRs ready to be merged label Dec 10, 2020

feat(wandb): use variable from class instance

6c02a2c

Co-authored-by: Jirka Borovec <[email protected]>

tests(wandb): check warnings

63dcd70

Merge branch 'master' into feat_wandb_resume

c9066cd

mergify bot requested a review from a team December 12, 2020 14:56

tchaton reviewed Dec 14, 2020

View reviewed changes

feat(wandb): use WarningCache

61cb199

tests(wandb): fix tests

f7d832e

borisdayma added 4 commits December 14, 2020 18:45

Merge branch 'master' into feat_wandb_resume

b996d2a

style: fix formatting

d4d500e

Merge branch 'feat_wandb_resume' of https://github.com/borisdayma/pyt…

e157ce3

…orch-lightning into feat_wandb_resume

Merge branch 'master' into feat_wandb_resume

792a94a

Borda modified the milestones: 1.1.x, 1.2 Dec 16, 2020

Borda added bug Something isn't working and removed feature Is an improvement or enhancement labels Dec 16, 2020

Merge branch 'master' into feat_wandb_resume

f930f27

rohitgr7 approved these changes Dec 18, 2020

View reviewed changes

awaelchli approved these changes Dec 18, 2020

View reviewed changes

Merge branch 'master' into feat_wandb_resume

91e089d

SkafteNicki approved these changes Dec 19, 2020

View reviewed changes

Merge branch 'master' into feat_wandb_resume

78e0d19

Borda requested a review from tchaton December 19, 2020 12:51

Borda merged commit e89764e into Lightning-AI:master Dec 19, 2020

borisdayma mentioned this pull request Dec 19, 2020

WandbLogger: add more flexibility with logging step #5194

Closed

awaelchli mentioned this pull request Jan 18, 2021

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(wandb): offset logging step when resuming #5050

feat(wandb): offset logging step when resuming #5050

borisdayma commented Dec 10, 2020

pep8speaks commented Dec 10, 2020 •

edited

Loading

codecov bot commented Dec 10, 2020 •

edited

Loading

Borda left a comment

borisdayma commented Dec 10, 2020 •

edited

Loading

borisdayma commented Dec 11, 2020

tchaton Dec 14, 2020

Borda Dec 14, 2020

borisdayma commented Dec 14, 2020 •

edited

Loading

borisdayma commented Dec 15, 2020

feat(wandb): offset logging step when resuming #5050

feat(wandb): offset logging step when resuming #5050

Conversation

borisdayma commented Dec 10, 2020

What does this PR do?

Before submitting

PR review

Did you have fun?

pep8speaks commented Dec 10, 2020 • edited Loading

Comment last updated at 2020-12-19 12:33:15 UTC

codecov bot commented Dec 10, 2020 • edited Loading

Codecov Report

Borda left a comment

Choose a reason for hiding this comment

borisdayma commented Dec 10, 2020 • edited Loading

borisdayma commented Dec 11, 2020

tchaton Dec 14, 2020

Choose a reason for hiding this comment

Borda Dec 14, 2020

Choose a reason for hiding this comment

borisdayma commented Dec 14, 2020 • edited Loading

borisdayma commented Dec 15, 2020

pep8speaks commented Dec 10, 2020 •

edited

Loading

codecov bot commented Dec 10, 2020 •

edited

Loading

borisdayma commented Dec 10, 2020 •

edited

Loading

borisdayma commented Dec 14, 2020 •

edited

Loading