Update LR schedulers only when their corresponding Optimizer is being…

[hemildesai]

… used.

In the case when optimizer frequencies are specified,
the LR scheduler corresponding to a particular optimizer is updated
only when that optimizer is being used in the training loop or epoch.

What does this PR do?

Fixes #4815

Also updates several old tests to use BoringModel

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

• 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.

[pep8speaks]

Hello @hemildesai! Thanks for updating this PR.

• In the file pytorch_lightning/trainer/optimizers.py:

Line 51:13: W503 line break before binary operator

Comment last updated at 2021-05-04 00:57:35 UTC

[codecov]

Codecov Report

Merging #4868 (f1fab63) into master (c6a171b) will decrease coverage by 6%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #4868    +/-   ##
=======================================
- Coverage      91%     85%    -6%     
=======================================
  Files         200     200            
  Lines       12894   13252   +358     
=======================================
- Hits        11783   11312   -471     
- Misses       1111    1940   +829     

[rohitgr7]

what if I set:

def configure_optimizer(self):
    optimizer1 = {'optimizer': SGD, 'frequency': 10}
    optimizer2 = {'optimizer': ADAM, 'frequency': 10}
    scheduler1 = scheduler(optimizer1)
    scheduler2 = scheduler(optimizer2)
    return [optimizer1, optimizer2], [scheduler1, scheduler2]

the fix won't work, right?

[hemildesai]

@rohitgr7 It mentions in the docs at https://pytorch-lightning.readthedocs.io/en/stable/lightning_module.html#configure-optimizers that if multiple optimizers are passed in a list, all of them will run at each optimization step. So I assumed that we don't want to change that behavior.

[rohitgr7]

There is a difference between passing multiple optimizers in a list, and passing multiple optimizers in
dictionaries with a frequency of 1: In the former case, all optimizers will operate on the given batch in
each optimization step. In the latter, only one optimizer will operate on the given batch at every step.

I did pass them in a dictionary within a list with a specified frequency. My concern here is the optimizer will update correctly as per the frequency, but I am concerned that lr_scheduler won't follow.

[hemildesai]

@rohitgr7 I think the example will error out in the current version as well because when two lists are passed, it assumes that each element of the first list is an optimizer from torch.optim. Although there's no explicit check for it.

https://github.com/PyTorchLightning/pytorch-lightning/blob/master/pytorch_lightning/trainer/optimizers.py#L44:L47

[rohitgr7]

True. not a valid configuration. Can you add a condition here: https://github.com/PyTorchLightning/pytorch-lightning/blob/204a0a2d03ce7dfb014e347f1d34a49ef5e86902/pytorch_lightning/trainer/optimizers.py#L44

[carmocca]

Can you replace this with BoringModel?

[edenlightning]

@carmocca mind help getting this merged?

Request no longer applies

[ananthsub]

is there an issue for converting all of the old test utilities to boring model / boring datamodule equivalents? Maybe something we can guarantee for v1.4 to simplify testing?

[carmocca]

We don't have an issue for it. Just doing it little by little and making sure all new code uses BoringModel

Maybe something we can guarantee for v1.4 to simplify testing?

The benefit is a considerable speedup, more than simplification

[awaelchli]

lgtm
thanks for the fix.
the pr could have avoided the refactors in the tests. IMO should have been done in a separate pr for a more careful review.