Repository: Lightning-AI/pytorch-lightning
Pull request: Bugfix/5487 auto lr ordering
## What does this PR do?

before: auto learning rate finder fails AFTER optimization due to misconfiguration
after: auto learning rate finder fails BEFORE optimization due to misconfiguration, and user can fail faster.

Fixes #5487

## Before submitting
- [x] Was this discussed/approved via a GitHub issue? (not for typos and docs)
- [x] Did you read the [contributor guideline](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/.github/CONTRIBUTING.md), **Pull Request** section?
- [x] Did you make sure your PR does only one thing, instead of bundling different changes together?
- [x] Did you make sure to update the documentation with your changes? (if necessary)
- [x] Did you write any new necessary tests? (not for typos and docs)
- [x] Did you verify new and existing tests pass locally with your changes?
- [x] Did you update the [CHANGELOG](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/CHANGELOG.md)? (not for typos, docs, test updates, or internal minor changes/refactorings)

<!-- For CHANGELOG separate each item in the unreleased section by a blank line to reduce collisions -->

## PR review
Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read [Review guidelines](https://github.com/PyTorchLightning/pytorch-lightning/wiki/Review-guidelines). In short, see the following bullet-list:

 - [x] Is this pull request ready for review? (if not, please submit in draft mode)
 - [x] Check that all items from **Before submitting** are resolved
 - [x] Make sure the title is self-explanatory and the description concisely explains the PR
 - [x] Add labels and milestones (and optionally projects) to the PR so it can be classified
 - [x] **Check that target branch and milestone match!**


## Did you have fun?
Make sure you had fun coding 🙃


Problem statement:
auto learning rate finder fails AFTER optimization due to misconfiguration
## 🐛 Bug
Optimizing learning rate using auto_lr_find=True fails **AFTER** a learning rate was found with error

`pytorch_lightning.utilities.exceptions.MisconfigurationException: When auto_lr_find is set to True, expects that `model` or `model.hparams` either has field `lr` or `learning_rate` that can overridden`

**AFTER** having waited for several minutes.

This can be caught **BEFORE** the long run to save time.

## Please reproduce using the BoringModel

I can just copy paste everything to here... the only lacking line is `self.lr = 999` under `model::__init__()`, if it is not present the error happens, otherwise it doesn't happen.

Reproducing this a single time takes me 15 minutes (due to the lr finding process), but I still wanted to report this because it seems important and I don't have time right now for a full reproduction on the template.

`
trainer = Trainer(auto_lr_find=True)
trainer.tune(model)
`

then for a while I see the progress bar

`Finding best initial lr:  27%|██▋       | 27/100 [02:43<07:37,  6.27s/it]`

and then the error after a lr has been found.

### Expected behavior

`pytorch_lightning.utilities.exceptions.MisconfigurationException: When auto_lr_find is set to True, expects that `model` or `model.hparams` either has field `lr` or `learning_rate` that can overridden`

should hit BEFORE the long calculation.

### Environment

 - PyTorch Version (e.g., 1.0): 1.7.1
 - OS (e.g., Linux): Windows 
 - How you installed PyTorch (`conda`, `pip`, source): pip
 - Build command you used (if compiling from source): -
 - Python version: 3.7
 - CUDA/cuDNN version: 101
 - GPU models and configuration: single GPU NVIDIA quadro P4000
 - Any other relevant information:-

Hints from issue discussion:
good catch! Mind send a PR with a fix?
> 
> 
> good catch! Mind send a PR with a fix?

I actually never contributed to open source before :)

How does this work? I get assigned and there's a deadline?
is there a readme?

I want to do it!
We have a basic guide [here](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/.github/CONTRIBUTING.md)!

There's no deadline! Any contribution is welcome :smile: 
Can the same issue be done twice by some people w/o them knowing about it until the latter does a PR?
Yes. For this reason, you might want to create the PR in draft mode while you are working on it, so others can know about it

https://github.blog/2019-02-14-introducing-draft-pull-requests/
Thanks for contributing @noamzilo! I have assigned this ticket to you, so other contributors know that this is already in progress. please let us know if you need any help!
How to link this post to the PR?
PR at:

https://github.com/PyTorchLightning/pytorch-lightning/pull/5638

currently my test doesn't pass due to lack of 

`pip install pytest-timeout` (from https://stackoverflow.com/questions/19527320/how-can-i-limit-the-maximum-running-time-for-a-unit-test) in the test server.

Please allow this library, or advise how I should limit test time.

Thanks :)

Commit message: Update pytorch_lightning/tuner/lr_finder.py

Patch to review:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index e614476071eaa..cea118de512a5 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -35,6 +35,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 - Remove unnecessary intermediate layers in Dockerfiles ([#5697](https://github.com/PyTorchLightning/pytorch-lightning/pull/5697))
 
 
+- Fixed auto learning rate ordering ([#5638](https://github.com/PyTorchLightning/pytorch-lightning/pull/5638))
+
+
 ## [1.1.6] - 2021-01-26
 
 ### Changed
diff --git a/pytorch_lightning/tuner/lr_finder.py b/pytorch_lightning/tuner/lr_finder.py
index c037a51f88d38..cf0adc5a83653 100644
--- a/pytorch_lightning/tuner/lr_finder.py
+++ b/pytorch_lightning/tuner/lr_finder.py
@@ -14,7 +14,7 @@
 import importlib
 import logging
 import os
-from typing import List, Optional, Sequence, Union, Callable
+from typing import List, Optional, Sequence, Union, Callable, Any
 from functools import wraps
 
 import numpy as np
@@ -42,34 +42,38 @@
 log = logging.getLogger(__name__)
 
 
+def _determine_lr_attr_name(trainer, model: LightningModule) -> str:
+    if isinstance(trainer.auto_lr_find, str):
+        if not lightning_hasattr(model, trainer.auto_lr_find):
+            raise MisconfigurationException(
+                f'`auto_lr_find` was set to {trainer.auto_lr_find}, however'
+                ' could not find this as a field in `model` or `model.hparams`.'
+            )
+        return trainer.auto_lr_find
+
+    attr_options = ('lr', 'learning_rate')
+    for attr in attr_options:
+        if lightning_hasattr(model, attr):
+            return attr
+
+    raise MisconfigurationException(
+        'When `auto_lr_find=True`, either `model` or `model.hparams` should'
+        f' have one of these fields: {attr_options} overridden.'
+    )
+
+
 def _run_lr_finder_internally(trainer, model: LightningModule):
     """ Call lr finder internally during Trainer.fit() """
+    lr_attr_name = _determine_lr_attr_name(trainer, model)
     lr_finder = lr_find(trainer, model)
-
     if lr_finder is None:
         return
 
     lr = lr_finder.suggestion()
 
     # TODO: log lr.results to self.logger
-    if isinstance(trainer.auto_lr_find, str):
-        # Try to find requested field, may be nested
-        if lightning_hasattr(model, trainer.auto_lr_find):
-            lightning_setattr(model, trainer.auto_lr_find, lr)
-        else:
-            raise MisconfigurationException(
-                f'`auto_lr_find` was set to {trainer.auto_lr_find}, however'
-                ' could not find this as a field in `model` or `model.hparams`.')
-    else:
-        if lightning_hasattr(model, 'lr'):
-            lightning_setattr(model, 'lr', lr)
-        elif lightning_hasattr(model, 'learning_rate'):
-            lightning_setattr(model, 'learning_rate', lr)
-        else:
-            raise MisconfigurationException(
-                'When auto_lr_find is set to True, expects that `model` or'
-                ' `model.hparams` either has field `lr` or `learning_rate`'
-                ' that can overridden')
+    lightning_setattr(model, lr_attr_name, lr)
+
     log.info(f'Learning rate set to {lr}')
 
 
diff --git a/tests/trainer/flags/test_fast_dev_run.py b/tests/trainer/flags/test_fast_dev_run.py
index 2eaa6fd7f888d..e94c607866ecf 100644
--- a/tests/trainer/flags/test_fast_dev_run.py
+++ b/tests/trainer/flags/test_fast_dev_run.py
@@ -15,6 +15,7 @@ def test_skip_on_fast_dev_run_tuner(tmpdir, tuner_alg):
     """ Test that tuner algorithms are skipped if fast dev run is enabled """
 
     model = BoringModel()
+    model.lr = 0.1  # avoid no-lr-found exception
     trainer = Trainer(
         default_root_dir=tmpdir,
         max_epochs=2,
diff --git a/tests/trainer/test_dataloaders.py b/tests/trainer/test_dataloaders.py
index ef8a39c5d8abf..e7dde65fecedd 100644
--- a/tests/trainer/test_dataloaders.py
+++ b/tests/trainer/test_dataloaders.py
@@ -1148,7 +1148,6 @@ def test_replace_sampler_with_multiprocessing_context(tmpdir):
     train = DataLoader(train, batch_size=32, num_workers=2, multiprocessing_context=context, shuffle=True)
 
     class ExtendedBoringModel(BoringModel):
-
         def train_dataloader(self):
             return train
 
diff --git a/tests/trainer/test_lr_finder.py b/tests/trainer/test_lr_finder.py
index 401584d920c9b..c53ff5df7f4f8 100755
--- a/tests/trainer/test_lr_finder.py
+++ b/tests/trainer/test_lr_finder.py
@@ -13,11 +13,13 @@
 # limitations under the License.
 import os
 from copy import deepcopy
+
 import pytest
 import torch
 
 from pytorch_lightning import Trainer
 from pytorch_lightning.utilities.exceptions import MisconfigurationException
+from tests.base import BoringModel
 from tests.base import EvalModelTemplate
 from tests.base.datamodules import TrialMNISTDataModule
 
@@ -262,3 +264,12 @@ def test_suggestion_with_non_finite_values(tmpdir):
 
     assert before_lr == after_lr, \
         'Learning rate was altered because of non-finite loss values'
+
+
+def test_lr_finder_fails_fast_on_bad_config(tmpdir):
+    """ Test that tune fails if the model does not have a lr BEFORE running lr find """
+    # note: this did not raise an exception before #5638 because lr_find is skipped
+    # during fast_dev_run and the lr attribute check was done after lr_find
+    trainer = Trainer(default_root_dir=tmpdir, fast_dev_run=True, auto_lr_find=True)
+    with pytest.raises(MisconfigurationException, match='either has one of these fields'):
+        trainer.tune(BoringModel())


Code review comments:
diff_hunk: @@ -262,3 +264,12 @@ def test_suggestion_with_non_finite_values(tmpdir):
 
     assert before_lr == after_lr, \
         'Learning rate was altered because of non-finite loss values'
+
+
+def test_lr_finder_fails_fast_on_bad_config(tmpdir):
+    """ Test that tune fails if the model does not have a lr BEFORE running lr find """
+    # note: this did not raise an exception before #5638 because lr_find is skipped
+    # during fast_dev_run and the lr attribute check was done after lr_find
+    trainer = Trainer(default_root_dir=tmpdir, fast_dev_run=True, auto_lr_find=True)
+    with pytest.raises(MisconfigurationException, match='either has one of these fields'):
path: tests/trainer/test_lr_finder.py
text: @user1:
```suggestion
    with pytest.raises(MisconfigurationException, match='should have one of these fields'):
```