Rename log_save_interval, row_log_interval #3748

teddykoker · 2020-09-30T16:02:03Z

What does this PR do?

row_log_interval -> log_every_n_steps
log_save_interval -> flush_logs_every_n_steps

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 🙃

rohitgr7 · 2020-09-30T16:05:24Z

How about just log_interval and flush_logs_interval?

teddykoker · 2020-09-30T16:44:03Z

@rohitgr7 I believe this is what we decided on (cc @edenlightning). We can put up a poll in slack if people want though.

codecov · 2020-09-30T16:56:20Z

Codecov Report

Merging #3748 into master will increase coverage by 8%.
The diff coverage is 82%.

@@           Coverage Diff           @@
##           master   #3748    +/-   ##
=======================================
+ Coverage      83%     90%    +8%     
=======================================
  Files         108     108            
  Lines        8807    8388   -419     
=======================================
+ Hits         7284    7584   +300     
+ Misses       1523     804   -719

codecov · 2020-09-30T16:56:22Z

Codecov Report

Merging #3748 into master will increase coverage by 4%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #3748    +/-   ##
=======================================
+ Coverage      83%     88%    +4%     
=======================================
  Files         117     115     -2     
  Lines        9106    8913   -193     
=======================================
+ Hits         7587    7799   +212     
+ Misses       1519    1114   -405

awaelchli

If you guys already decided, then I'm happy with it. Though if there was a vote, I would vote for a shorter name with fewer underscores.

williamFalcon · 2020-10-01T11:38:26Z

interval is not clear enough sadly...

interval could be interpreted as: [1, 5] or 5 or 0.2

steps shows it's about steps

log_save_interval -> flush_logs_every_n_steps

Borda

add test for deprecated API, that with calling old ones the warning is raised...
https://github.com/PyTorchLightning/pytorch-lightning/blob/master/tests/test_deprecated.py
also, move deprecated API to a particular class
https://github.com/PyTorchLightning/pytorch-lightning/blob/master/pytorch_lightning/trainer/deprecated_api.py

pytorch_lightning/trainer/trainer.py

Borda · 2020-10-02T11:15:58Z

@teddykoker the label ready-to-go in meant for reliever not for PR author :]

mergify · 2020-10-03T16:34:54Z

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

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

pep8speaks · 2020-10-03T18:40:54Z

Hello @teddykoker! Thanks for updating this PR.

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

Comment last updated at 2020-10-06 09:33:23 UTC

…g into row_log_interval

mergify · 2020-10-04T01:22:14Z

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

mergify · 2020-10-04T21:38:24Z

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

rohitgr7 · 2020-10-06T05:39:17Z

need to update in #3881 files in case it gets merged.

Borda

still, some deprecation/compatibility is missing

CHANGELOG.md

pytorch_lightning/trainer/training_loop.py

pytorch_lightning/trainer/trainer.py

tests/trainer/test_trainer.py

Borda

fixed, shall be fine now :]

teddykoker · 2020-10-06T14:27:02Z

Thanks @Borda :)

rohitgr7 · 2020-10-06T15:54:03Z

@Borda There are still some places where this is not updated
https://github.com/PyTorchLightning/pytorch-lightning/blob/f55a9cf63afe3c710ad0ab69c5e4444e44a4b0ea/tests/trainer/model_hooks/test_model_hooks.py#L22
https://github.com/PyTorchLightning/pytorch-lightning/blob/f55a9cf63afe3c710ad0ab69c5e4444e44a4b0ea/tests/trainer/flags/test_overfit_batches.py#L31

few more...

teddykoker · 2020-10-06T16:26:35Z

Ugh I updated everything at the time of the PR, there must have been extra additions

teddykoker · 2020-10-06T16:27:34Z

I'll need to open a new PR i guess

rohitgr7 · 2020-10-06T16:29:07Z

maybe keep that open until the release to avoid this.

mergify bot requested a review from a team September 30, 2020 16:02

teddykoker changed the title ~~Rename~~ Rename log_save_interval, row_log_interval Sep 30, 2020

teddykoker added ready PRs ready to be merged priority: 0 High priority task labels Sep 30, 2020

teddykoker requested review from edenlightning, ananyahjha93, awaelchli and Borda September 30, 2020 17:01

awaelchli approved these changes Sep 30, 2020

View reviewed changes

mergify bot requested a review from a team September 30, 2020 22:05

SkafteNicki approved these changes Oct 1, 2020

View reviewed changes

mergify bot requested a review from a team October 1, 2020 08:30

teddykoker added 4 commits October 2, 2020 12:05

Rename row_log_interval -> log_every_n_steps

c475f3e

log_save_interval -> flush_logs_every_n_steps

Changelog

ac54b2c

fixed title underline length

b0b7409

typo

13dbc3f

Borda added the design Includes a design discussion label Oct 2, 2020

Borda requested changes Oct 2, 2020

View reviewed changes

pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved

pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved

pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved

pytorch_lightning/trainer/trainer.py Show resolved Hide resolved

Borda removed the ready PRs ready to be merged label Oct 2, 2020

mergify bot requested a review from a team October 2, 2020 11:15

teddykoker and others added 2 commits October 3, 2020 14:40

Update pytorch_lightning/trainer/trainer.py

6245166

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

Update pytorch_lightning/trainer/trainer.py

7076e91

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

teddykoker added 4 commits October 3, 2020 14:45

Merge branch 'master' of github.com:PyTorchLightning/pytorch-lightnin…

4ed6984

…g into row_log_interval

pep8 + deprecation test

db40668

'todo: remove in 1.1 comment'

674df2c

1.1 -> 0.11

86ca5bf

Merge branch 'master' into row_log_interval

7b54b27

teddykoker requested a review from Borda October 4, 2020 18:35

Merge branch 'master' into row_log_interval

bed11d1

edenlightning mentioned this pull request Oct 6, 2020

[blocked by #3748] update logging docs #3886

Closed

7 tasks

Borda requested changes Oct 6, 2020

View reviewed changes

mergify bot requested a review from a team October 6, 2020 08:22

Borda added 6 commits October 6, 2020 10:23

log

629bafc

docs

a5707b2

depr API

4f1e473

add depr tests

85d6c91

note

7433cdd

miss

a12abd7

Borda approved these changes Oct 6, 2020

View reviewed changes

teddykoker merged commit 9600926 into Lightning-AI:master Oct 6, 2020

Borda added this to the 0.10.0 milestone Oct 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename log_save_interval, row_log_interval #3748

Rename log_save_interval, row_log_interval #3748

teddykoker commented Sep 30, 2020 •

edited

Loading

rohitgr7 commented Sep 30, 2020

teddykoker commented Sep 30, 2020

codecov bot commented Sep 30, 2020

codecov bot commented Sep 30, 2020 •

edited

Loading

awaelchli left a comment •

edited

Loading

williamFalcon commented Oct 1, 2020

Borda left a comment

Borda commented Oct 2, 2020

mergify bot commented Oct 3, 2020

pep8speaks commented Oct 3, 2020 •

edited

Loading

mergify bot commented Oct 4, 2020

mergify bot commented Oct 4, 2020

rohitgr7 commented Oct 6, 2020

Borda left a comment

Borda left a comment

teddykoker commented Oct 6, 2020

rohitgr7 commented Oct 6, 2020 •

edited

Loading

teddykoker commented Oct 6, 2020

teddykoker commented Oct 6, 2020

rohitgr7 commented Oct 6, 2020

Rename log_save_interval, row_log_interval #3748

Rename log_save_interval, row_log_interval #3748

Conversation

teddykoker commented Sep 30, 2020 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

rohitgr7 commented Sep 30, 2020

teddykoker commented Sep 30, 2020

codecov bot commented Sep 30, 2020

Codecov Report

codecov bot commented Sep 30, 2020 • edited Loading

Codecov Report

awaelchli left a comment • edited Loading

Choose a reason for hiding this comment

williamFalcon commented Oct 1, 2020

Borda left a comment

Choose a reason for hiding this comment

Borda commented Oct 2, 2020

mergify bot commented Oct 3, 2020

pep8speaks commented Oct 3, 2020 • edited Loading

Comment last updated at 2020-10-06 09:33:23 UTC

mergify bot commented Oct 4, 2020

mergify bot commented Oct 4, 2020

rohitgr7 commented Oct 6, 2020

Borda left a comment

Choose a reason for hiding this comment

Borda left a comment

Choose a reason for hiding this comment

teddykoker commented Oct 6, 2020

rohitgr7 commented Oct 6, 2020 • edited Loading

teddykoker commented Oct 6, 2020

teddykoker commented Oct 6, 2020

rohitgr7 commented Oct 6, 2020

teddykoker commented Sep 30, 2020 •

edited

Loading

codecov bot commented Sep 30, 2020 •

edited

Loading

awaelchli left a comment •

edited

Loading

pep8speaks commented Oct 3, 2020 •

edited

Loading

rohitgr7 commented Oct 6, 2020 •

edited

Loading