Bugfix/5487 auto lr ordering (#5638)

* started to write failing test. just getting into the framework...

* started to write failing test. just getting into the framework...

* added failing test for misconfiguration of lr finder

* made test startup quickly. making sure without the fix it also fails slowly

* improved test

* fixed for linter

* fixed for linter

* yet another fix for the linter

* yet another fix for the linter

* fixed comment by @carmocca

* fixed comment by @carmocca

* Fix test

* chlog

* Apply suggestions from code review

* Fix test

* Update pytorch_lightning/tuner/lr_finder.py

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

* Update pytorch_lightning/tuner/lr_finder.py

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

* Update tests/trainer/test_lr_finder.py

* Update pytorch_lightning/tuner/lr_finder.py

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

* Update pytorch_lightning/tuner/lr_finder.py

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

* Update pytorch_lightning/tuner/lr_finder.py

* Update tests/trainer/test_lr_finder.py

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
Co-authored-by: Jirka Borovec <jirka.borovec@seznam.cz>
Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>

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

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}')
 
 

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,

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
 

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='should have one of these fields'):
+        trainer.tune(BoringModel())