Fix accumulate_grad_batches for last batch #2853

ydcjeff · 2020-08-06T17:52:14Z

What does this PR do?

Fixes #1549

EDIT:

Issue to reproduce – Colab

https://colab.research.google.com/drive/1iMMKPMeghhfWD092lT3l9XKClY2YJYNQ?usp=sharing

Fixed issue to reproduce – Colab

https://colab.research.google.com/drive/1udmCrqYfJz2t8pEDi5pAIJKcyH_jHLLF?usp=sharing

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 · 2020-08-06T17:52:18Z

Hello @ydcjeff! Thanks for updating this PR.

In the file pytorch_lightning/trainer/training_loop.py:

Line 594:17: W503 line break before binary operator
Line 742:17: W503 line break before binary operator
Line 884:25: W503 line break before binary operator

Comment last updated at 2020-08-15 13:46:13 UTC

ydcjeff · 2020-08-06T17:57:26Z

Should I write new tests?

Borda · 2020-08-06T19:00:09Z

Should I write new tests?

yes, please, add a new test for this case...

pytorch_lightning/trainer/training_loop.py

ydcjeff · 2020-08-07T18:42:05Z

It is ready to review.

tests/trainer/test_trainer.py

pytorch_lightning/trainer/training_loop.py

mergify · 2020-08-08T14:51:52Z

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

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>

mergify · 2020-08-15T12:57:45Z

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

rohitgr7

LGTM ✌️. Not sure about circleci BuildError

awaelchli · 2020-08-15T13:11:47Z

Try to merge master to get rid of the build error

rohitgr7 · 2020-08-15T13:27:15Z

not sure if I merged it correctly but still the same BuildError.

williamFalcon · 2020-08-15T13:33:10Z

ummm... ok, we seem to have broken this haha.

Can we go back to the commit without rebasing master?
Then rebase master.

williamFalcon · 2020-08-15T13:38:43Z

ok... i need to run but. Maybe the easiest thing is to start from master again and apply your changes using master as the base?

i don't know why this is trying to pull changes from weeks ago

awaelchli · 2020-08-15T13:39:07Z

@rohitgr7 can force push the branch with the local changes, then it should be back to how it was.
I recommend merging master into the branch, otherwise you lose changes if you make a mistake in rebasing.

awaelchli

looking good now, this build config error must come from master.

ydcjeff · 2020-08-15T14:14:23Z

not sure what just happened, but thanks for fixing @rohitgr7

williamFalcon · 2020-08-15T18:11:16Z

awesome!

does the test fail on master?
fix pep8?

rohitgr7 · 2020-08-15T18:14:54Z

pep8 is not a problem here I think https://www.flake8rules.com/rules/W503.html

Borda · 2020-08-15T18:25:17Z

@ydcjeff seems you do not run CirclCI tests on our side which is the reason why all TPU test are missing

ydcjeff · 2020-08-15T18:40:21Z

@Borda I fetched from upstream and worked on separate branch, not sure why TPU tests are missing.
Even there is circleci config

williamFalcon · 2020-08-15T19:06:29Z

ok... i tested this on master and the tests do fail. let's merge so we can get rid of the circle ci thing and run tpu tests

@awaelchli

* first attempt * update changelog * fix pep8 and tests * Apply suggestions from code review Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com> * added new tests * fixed tests * Apply suggestions from code review * used num_training_batches * fixed pep8 * fixed with is_last_batch suggested by @awaelchli * fixed with num_training_batches * fixed with num_training_batches * cleanup * fix test and update docs * fixed for alignment, update docs * minor changes * update doc Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com> Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

mergify bot requested a review from a team August 6, 2020 17:53

Borda added this to the 0.9.0 milestone Aug 6, 2020

rohitgr7 reviewed Aug 6, 2020

View reviewed changes

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

mergify bot requested a review from a team August 6, 2020 19:08

Borda added the bug Something isn't working label Aug 6, 2020

ydcjeff changed the title ~~Fix accumulate_grad_batches for last batch~~ [WIP] Fix accumulate_grad_batches for last batch Aug 7, 2020

ydcjeff changed the title ~~[WIP] Fix accumulate_grad_batches for last batch~~ Fix accumulate_grad_batches for last batch Aug 7, 2020

Borda reviewed Aug 7, 2020

View reviewed changes

mergify bot requested a review from a team August 7, 2020 19:41

Borda requested review from rohitgr7, ethanwharris, jeremyjordan, justusschock and Borda August 7, 2020 19:41

rohitgr7 reviewed Aug 7, 2020

View reviewed changes

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

mergify bot requested a review from a team August 7, 2020 21:54

awaelchli requested changes Aug 8, 2020

View reviewed changes

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

mergify bot requested a review from a team August 8, 2020 13:57

ydcjeff and others added 7 commits August 8, 2020 21:27

first attempt

954a870

update changelog

7265aa5

fix pep8 and tests

b33bef8

Apply suggestions from code review

a53f224

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>

added new tests

09c0a08

fixed tests

a675ba4

Apply suggestions from code review

df917cc

mergify bot requested a review from a team August 15, 2020 10:06

update doc

a8b82d6

rohitgr7 approved these changes Aug 15, 2020

View reviewed changes

mergify bot requested a review from a team August 15, 2020 13:02

Merge remote-tracking branch 'origin/master' into HEAD

ac4f4c0

awaelchli approved these changes Aug 15, 2020

View reviewed changes

mergify bot requested a review from a team August 15, 2020 14:01

williamFalcon merged commit 73ebd10 into Lightning-AI:master Aug 15, 2020

ydcjeff deleted the accumulate-grad-batches-1549 branch August 16, 2020 08:39

awaelchli mentioned this pull request Aug 16, 2020

fix Misstep #2478

Closed

7 tasks

Borda modified the milestones: 0.9.0, 1.0.0 Aug 20, 2020

ydcjeff mentioned this pull request Oct 3, 2020

Steps not incremented correctly with accumulate gradients #2242

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix accumulate_grad_batches for last batch #2853

Fix accumulate_grad_batches for last batch #2853

ydcjeff commented Aug 6, 2020 •

edited

Loading

pep8speaks commented Aug 6, 2020 •

edited

Loading

ydcjeff commented Aug 6, 2020

Borda commented Aug 6, 2020

ydcjeff commented Aug 7, 2020

mergify bot commented Aug 8, 2020

mergify bot commented Aug 15, 2020

rohitgr7 left a comment •

edited

Loading

awaelchli commented Aug 15, 2020 •

edited

Loading

rohitgr7 commented Aug 15, 2020

williamFalcon commented Aug 15, 2020

williamFalcon commented Aug 15, 2020

awaelchli commented Aug 15, 2020 •

edited

Loading

awaelchli left a comment

ydcjeff commented Aug 15, 2020

williamFalcon commented Aug 15, 2020

rohitgr7 commented Aug 15, 2020

Borda commented Aug 15, 2020

ydcjeff commented Aug 15, 2020 •

edited

Loading

williamFalcon commented Aug 15, 2020

Fix accumulate_grad_batches for last batch #2853

Fix accumulate_grad_batches for last batch #2853

Conversation

ydcjeff commented Aug 6, 2020 • edited Loading

What does this PR do?

Issue to reproduce – Colab

Fixed issue to reproduce – Colab

Before submitting

PR review

Did you have fun?

pep8speaks commented Aug 6, 2020 • edited Loading

Comment last updated at 2020-08-15 13:46:13 UTC

ydcjeff commented Aug 6, 2020

Borda commented Aug 6, 2020

ydcjeff commented Aug 7, 2020

mergify bot commented Aug 8, 2020

mergify bot commented Aug 15, 2020

rohitgr7 left a comment • edited Loading

Choose a reason for hiding this comment

awaelchli commented Aug 15, 2020 • edited Loading

rohitgr7 commented Aug 15, 2020

williamFalcon commented Aug 15, 2020

williamFalcon commented Aug 15, 2020

awaelchli commented Aug 15, 2020 • edited Loading

awaelchli left a comment

Choose a reason for hiding this comment

ydcjeff commented Aug 15, 2020

williamFalcon commented Aug 15, 2020

rohitgr7 commented Aug 15, 2020

Borda commented Aug 15, 2020

ydcjeff commented Aug 15, 2020 • edited Loading

williamFalcon commented Aug 15, 2020

ydcjeff commented Aug 6, 2020 •

edited

Loading

pep8speaks commented Aug 6, 2020 •

edited

Loading

rohitgr7 left a comment •

edited

Loading

awaelchli commented Aug 15, 2020 •

edited

Loading

awaelchli commented Aug 15, 2020 •

edited

Loading

ydcjeff commented Aug 15, 2020 •

edited

Loading