log/save_interval based on global step

[awaelchli]

What does this PR do?

This issue came up frequently: nothing is getting logged, what's the problem?
If number of batches in training is < row_log_interval, nothing gets logged.
This PR will fix this by making row_log_interval based on global_step.

The current logging behaviour does not change, except in the edge case where epochs are shorter than logging interval.

Fixes #3466
Fixes #3506

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? Otherwise, we ask you to create a separate PR for every change.
• 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.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[pep8speaks]

Hello @awaelchli! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-09-29 14:19:21 UTC

[rohitgr7]

Global step updates only on the training step right? I tried to log it after every training_epoch_end with batches=50 and getting results:

Train epoch end: Epoch: 0 Global Step: 49
Train epoch end: Epoch: 1 Global Step: 100
Train epoch end: Epoch: 2 Global Step: 151
Train epoch end: Epoch: 3 Global Step: 202

It is incrementing by an extra +1 after every epoch. Is this expected behavior or a bug?

[awaelchli]

@rohitgr7 Yes I remember discussing this before with someone, I can't find the issue or PR unfortunately.
I don't remember what the conclusion was, whether it is a bug or not. The +1 comes from the training_epoch_end, if we log there it creates an extra step. As far as I know, it has always been like this. Ideally, the logs in epoch end should be combined with the last log call of the training loop.

[rohitgr7]

Ideally, the logs in epoch end should be combined with the last log call of the training loop.

Agreed.

@awaelchli Ideally IMO training_epoch_end is not doing any leaning step and since we are now logging w.r.t global_step, it should not increment at training_epoch_end.

[awaelchli]

so you say we should first fix this global step problem before we can switch to logging on global step? ok, that makes sense.
not sure yet how to approach it.

[rohitgr7]

maybe just removing this line will do the job only if it meant to not update in training_epoch_end.
https://github.com/PyTorchLightning/pytorch-lightning/blob/a94728c99b68085b861f7384c8ecc163b2d401b0/pytorch_lightning/trainer/connectors/logger_connector.py#L237

[awaelchli]

@rohitgr7 I'm trying your suggestion and see if any tests fail #3673

[Borda]

@rohitgr7 I'm trying your suggestion and see if any tests fail #3673

what do you mean by all, we may have the bad sound in some other places already in tests...

[awaelchli]

It is incrementing by an extra +1 after every epoch. Is this expected behavior or a bug?

@rohitgr7 william fixed it, it should be good now and we can trust global_step to be aligned in all epochs.

[mergify]

This pull request is now in conflict... :(

[rohitgr7]

LGTM ✌️

[Borda]

LGTM

[williamFalcon]

awesome fix!

[codecov]

Codecov Report

Merging #3667 into master will increase coverage by 5%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #3667    +/-   ##
=======================================
+ Coverage      85%     90%    +5%     
=======================================
  Files         110     110            
  Lines        8355    8849   +494     
=======================================
+ Hits         7141    7982   +841     
+ Misses       1214     867   -347