Start accumulate gradients schedule at epoch 0

[HHousen]

What does this PR do?

Fixes #2480. If the pl.Trainer option accumulate_grad_batches was an integer then the first epoch (epoch 0) would use accumulate_grad_batches=1 while the remaining epochs would use accumulate_grad_batches=<user_value>. This was caused by #2289.

Before:

After:

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 🙃

[awaelchli]

Hi, thanks for the PR. The test that you changed is for LRFinder, it looks ok, but do you have an idea why the other test test_gradient_accumulation_scheduling in test_trainer.py did not fail? It seems like an important test but it is invariant to this change. It might be because this test has a very specific example in which this is not detected. I wonder if it could be parameterized to cover several cases.

[awaelchli]

Suggested change

	assert lrfinder._total_batch_idx == 100 * 2, \
	'Accumulation parameter did not work'
	assert lrfinder._total_batch_idx == 100 * 2, \
	'Accumulation parameter did not work'
	assert trainer.total_batch_idx == 0

What do you think? Should we also add this assertion?

[HHousen]

Does this essentially ensure that the trainer never starts to train the model?

[awaelchli]

yes, since there is accumulation happening in trainer and lr_finder, I would just include this assert for clarity.

[Borda]

The tests/trainer/test_lr_finder.py::test_accumulation_and_early_stopping test failed because the _total_batch_idx assert value was changed in #2375 from 100 * 2 to 190. I think this change was a mistake. The _total_batch_idx changed to 190 because the first epoch had accumulate_grad_batches set to 1. Since this pull request fixes the accumulate_grad_batches bug for the first epoch, I have changed this test back to the previous value.

this test has a bug, it shall be 200, pls correct it :]

[HHousen]

Hi, thanks for the PR. The test that you changed is for LRFinder, it looks ok, but do you have an idea why the other test test_gradient_accumulation_scheduling in test_trainer.py did not fail? It seems like an important test but it is invariant to this change. It might be because this test has a very specific example in which this is not detected. I wonder if it could be parameterized to cover several cases.

Prior to this pull request the test_gradient_accumulation_scheduling test did not fail because the schedule is {1: 2, 3: 4}, which falls under the dict section of the if...else block in configure_accumulated_gradients(). In the GradientAccumulationScheduler callback the line scheduling.update({0: 1}) set the accumulate_grad_batches to 1 for the first epoch, which is tested in test_gradient_accumulation_scheduling in the line assert self.trainer.accumulate_grad_batches == 1 which will pass. This did not raise an error because the 0th epoch in the {1: 2, 3: 4} schedule is not set and because a dict was tested, not an int. If an int were tested (lets say 3) and the asserts were updated to always check for a value of 3, then the test would have failed because epoch zero would have accumulate_grad_batches=1. Epoch zero would have accumulate_grad_batches=1 because the line schedule = {1: accumulate_grad_batches} in configure_accumulated_gradients() did not define the first epoch, which scheduling.update({0: 1}) in GradientAccumulationScheduler sets to 1.

After this pull request the test_gradient_accumulation_scheduling test did not fail either because I modified the code in the if...else block in configure_accumulated_gradients() that handles ints, but a dict is tested in this test.

The test_gradient_accumulation_scheduling test should check an int as the value for accumulate_grad_batches. This addition to the test would have caught this error.

[awaelchli]

@HHousen would you like to modify that test so that it would fail on master and pass here? Otherwise the error could sneak back in the future without us noticing :) I can help if you need

[HHousen]

@HHousen would you like to modify that test so that it would fail on master and pass here? Otherwise the error could sneak back in the future without us noticing :) I can help if you need

Sure. I'll parametrize the test so an int can be tested as well as a dict.

[HHousen]

In the else block of _optimizer_step in the current test, why is 3 added to self.prev_called_batch_idx. Shouldn't 4 be added since the accumulate_grad_batches is 4?

[pep8speaks]

Hello @HHousen! Thanks for updating this PR.

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

Comment last updated at 2020-07-05 00:05:47 UTC

[HHousen]

In the else block of _optimizer_step in the current test, why is 3 added to self.prev_called_batch_idx. Shouldn't 4 be added since the accumulate_grad_batches is 4?

Nevermind, it should be 4.

[HHousen]

I updated the test_trainer.py::test_gradient_accumulation_scheduling test. It had a few issues that prevented it from properly testing what it was designed to test. Namely, the batches per epoch was too low and only two epochs were processed but there were tests designed for greater than 3 epochs. I have updated and parametrized the test. The master branch should now have 2 fail and 1 succeed, whereas this pull request should have 3/3 succeed.

[awaelchli]

ok, that explains a lot :) so the test was never really running the function?
Good that you saw this!

[HHousen]

Yep.

[williamFalcon]

@HHousen solid catch!!

[HHousen]

Sorry. I tried to remove the two dataset files that were downloaded when I ran the tests since I accidentally committed them. This did not work since force-pushing closed this pull request. I undid the force-push by reverting back to e0cb028, but I still can't reopen since GitHub says the branch was "force-pushed or recreated."

git reflog show bugfix/2480_grad-accumulate-0-epoch:

e0cb0285 (HEAD -> bugfix/2480_grad-accumulate-0-epoch, origin/bugfix/2480_grad-accumulate-0-epoch, refs/original/refs/heads/bugfix/2480_grad-accumulate-0-epoch) bugfix/2480_grad-accumulate-0-epoch@{0}: reset: moving to origin/bugfix/2480_grad-accumulate-0-epoch@{1}
c3b8cca4 bugfix/2480_grad-accumulate-0-epoch@{1}: filter-branch: rewrite
e0cb0285 (HEAD -> bugfix/2480_grad-accumulate-0-epoch, origin/bugfix/2480_grad-accumulate-0-epoch, refs/original/refs/heads/bugfix/2480_grad-accumulate-0-epoch) bugfix/2480_grad-accumulate-0-epoch@{2}: commit: Fix pep8 formatting
6420265c bugfix/2480_grad-accumulate-0-epoch@{3}: commit: Update test_trainer.py::test_gradient_accumulation_scheduling
f1f9b6f8 bugfix/2480_grad-accumulate-0-epoch@{4}: commit: Undo change in #2375
2e199c84 bugfix/2480_grad-accumulate-0-epoch@{5}: commit: Start accumulate gradients schedule at epoch 0
e5a97999 (origin/master, origin/HEAD, master) bugfix/2480_grad-accumulate-0-epoch@{6}: branch: Created from HEAD

It looks like everything should have been reverted (the GitHub compare looks good). Any suggestions? @awaelchli

[awaelchli]

I suggest you simply submit an new PR. You can set the remote branch to a new name and push your commits there.

[Borda]

Can you rest your branch to master and commit the change again... Then we reopen this PR (it would be a sad to lose all this valuable discussion and contribution)

git checkout bugfix/2480_grad-accumulate-0-epoch
git reset upstream/master 
.... add the changes...... 
git commit.... 
git push -f

When any change appears simply reopen this PR

[HHousen]

I have to open a new pull request since GitHub will not let me reopen this one even if the branch is at the state right before the request was automatically closed. Moved to #2513. It is strange though since it's the same exact branch in both requests.

[Borda]

I have to open a new pull request since GitHub will not let me reopen this one even if the branch is at the state right before the request was automatically closed.

It was closed because the PR state was indetical with target branch... So if there is any difference it allows you to reopen the PR...

[Borda]

I have to open a new pull request since GitHub will not let me reopen this one even if the branch is at the state right before the request was automatically closed.

It was closed because the PR state was indetical with target branch... So if there is any difference it allows you to reopen the PR...

[HHousen]

I have to open a new pull request since GitHub will not let me reopen this one even if the branch is at the state right before the request was automatically closed.

It was closed because the PR state was indetical with target branch... So if there is any difference it allows you to reopen the PR...

I rewrote history by accident which closed the PR because there were no commits in common with master. I then reset back to the commit directly before the rewrite, so everything was in the same state as it was before the rewrite. It would still not let me open after this. There are currently 5 additional commits on this branch, which #2513 recognizes but this PR will not. GitHub should let me reopen a PR if the branch to be merged is in the same state that it was in directly before the closure. Since it would not let me do this, I added a commit to remove the accidentally committed files and opened #2513. Both PRs are for the same branch, which should not be possible to my understanding. The only reason it is possible is because this one was closed and is no longer tracking changes.

Essentially, this PR would not recognize that the state of the branch was reverted and thus would not let me reopen.

I did try resetting to master and redoing the commits, but that produced identical commits and resulted in the same state as doing a git reset HEAD@{1} after my mistaken push.