From 4c303b4dd294ca985f06d1927c4c521db7b7d58d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrian=20W=C3=A4lchli?= Date: Thu, 12 Aug 2021 00:02:43 +0200 Subject: [PATCH 1/6] only update plateau at end of epoch --- pytorch_lightning/loops/epoch/training_epoch_loop.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pytorch_lightning/loops/epoch/training_epoch_loop.py b/pytorch_lightning/loops/epoch/training_epoch_loop.py index 52b31b67edfa9..213be0de21e6d 100644 --- a/pytorch_lightning/loops/epoch/training_epoch_loop.py +++ b/pytorch_lightning/loops/epoch/training_epoch_loop.py @@ -227,7 +227,8 @@ def on_run_end(self) -> List[List[STEP_OUTPUT]]: self.trainer.call_hook("on_epoch_end") self.trainer.logger_connector.on_epoch_end() - self.update_lr_schedulers("epoch", update_plateau_schedulers=True) + if self._num_training_batches_reached(self.is_last_batch): + self.update_lr_schedulers("epoch", update_plateau_schedulers=True) epoch_output = self._epoch_output # free memory From 106626260d894c7e81aa42dde8009036e884aae0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrian=20W=C3=A4lchli?= Date: Thu, 12 Aug 2021 00:34:37 +0200 Subject: [PATCH 2/6] add test --- tests/trainer/optimization/test_optimizers.py | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/trainer/optimization/test_optimizers.py b/tests/trainer/optimization/test_optimizers.py index c0541f1c142b3..56d24e318673b 100644 --- a/tests/trainer/optimization/test_optimizers.py +++ b/tests/trainer/optimization/test_optimizers.py @@ -405,6 +405,33 @@ def test_lr_scheduler_strict(tmpdir): trainer.fit(model) +@pytest.mark.parametrize("complete_epoch", [True, False]) +@mock.patch("torch.optim.lr_scheduler.ReduceLROnPlateau.step") +def test_lr_scheduler_strict_incomplete_epoch(step_mock, tmpdir, complete_epoch): + model = BoringModel() + optimizer = optim.Adam(model.parameters()) + scheduler = optim.lr_scheduler.ReduceLROnPlateau(optimizer) + max_epochs = 1 if complete_epoch else None + max_steps = None if complete_epoch else 1 + trainer = Trainer(default_root_dir=tmpdir, max_epochs=max_epochs, max_steps=max_steps) + + model.configure_optimizers = lambda: { + "optimizer": optimizer, + "lr_scheduler": {"scheduler": scheduler, "monitor": "giraffe", "strict": True}, + } + + if complete_epoch: + with pytest.raises( + MisconfigurationException, + match=r"ReduceLROnPlateau conditioned on metric .* which is not available\. Available metrics are:", + ): + trainer.fit(model) + else: + trainer.fit(model) + + assert step_mock.call_count == 0 + + def test_unknown_configure_optimizers_raises(tmpdir): """ Test exception with an unsupported configure_optimizers return From f92815608c6c608ad5b420dfa45fd2321c5538ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrian=20W=C3=A4lchli?= Date: Thu, 12 Aug 2021 00:44:02 +0200 Subject: [PATCH 3/6] update changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 77163b62457b6..06ff2e9da167e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -153,6 +153,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). - Fixed truncated backprop through time enablement when set as a property on the LightningModule and not the Trainer ([#8804](https://github.com/PyTorchLightning/pytorch-lightning/pull/8804/)) +- Fixed plateau scheduler stepping on incomplete epoch ([#8861](https://github.com/PyTorchLightning/pytorch-lightning/pull/8861)) + + ## [1.4.0] - 2021-07-27 ### Added From 61fd92d6eeb73e3659fe2214f15d5559320a47ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrian=20W=C3=A4lchli?= Date: Thu, 12 Aug 2021 00:49:25 +0200 Subject: [PATCH 4/6] add test docstring --- tests/trainer/optimization/test_optimizers.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/trainer/optimization/test_optimizers.py b/tests/trainer/optimization/test_optimizers.py index 56d24e318673b..2a99d9f0794c5 100644 --- a/tests/trainer/optimization/test_optimizers.py +++ b/tests/trainer/optimization/test_optimizers.py @@ -408,6 +408,7 @@ def test_lr_scheduler_strict(tmpdir): @pytest.mark.parametrize("complete_epoch", [True, False]) @mock.patch("torch.optim.lr_scheduler.ReduceLROnPlateau.step") def test_lr_scheduler_strict_incomplete_epoch(step_mock, tmpdir, complete_epoch): + """Tests that plateau scheduler does not attempt to step in an incomplete epoch (stopped early).""" model = BoringModel() optimizer = optim.Adam(model.parameters()) scheduler = optim.lr_scheduler.ReduceLROnPlateau(optimizer) From 008c1cd421803254fed0a738d9a7728e9b8be9cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrian=20W=C3=A4lchli?= Date: Thu, 12 Aug 2021 00:50:21 +0200 Subject: [PATCH 5/6] update boring model usage --- tests/trainer/optimization/test_optimizers.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/trainer/optimization/test_optimizers.py b/tests/trainer/optimization/test_optimizers.py index 2a99d9f0794c5..e5d9badd12b3d 100644 --- a/tests/trainer/optimization/test_optimizers.py +++ b/tests/trainer/optimization/test_optimizers.py @@ -20,7 +20,6 @@ from pytorch_lightning import Callback, Trainer from pytorch_lightning.callbacks import ModelCheckpoint from pytorch_lightning.utilities.exceptions import MisconfigurationException -from tests.base import EvalModelTemplate from tests.helpers.boring_model import BoringModel from tests.helpers.runif import RunIf @@ -79,7 +78,7 @@ def test_reducelronplateau_with_no_monitor_raises(tmpdir): """ Test exception when a ReduceLROnPlateau is used with no monitor """ - model = EvalModelTemplate() + model = BoringModel() optimizer = optim.Adam(model.parameters()) model.configure_optimizers = lambda: ([optimizer], [optim.lr_scheduler.ReduceLROnPlateau(optimizer)]) trainer = Trainer(default_root_dir=tmpdir, fast_dev_run=True) @@ -93,7 +92,7 @@ def test_reducelronplateau_with_no_monitor_in_lr_scheduler_dict_raises(tmpdir): """ Test exception when lr_scheduler dict has a ReduceLROnPlateau with no monitor """ - model = EvalModelTemplate() + model = BoringModel() optimizer = optim.Adam(model.parameters()) model.configure_optimizers = lambda: { "optimizer": optimizer, @@ -380,7 +379,7 @@ def test_lr_scheduler_strict(tmpdir): """ Test "strict" support in lr_scheduler dict """ - model = EvalModelTemplate() + model = BoringModel() optimizer = optim.Adam(model.parameters()) scheduler = optim.lr_scheduler.ReduceLROnPlateau(optimizer) trainer = Trainer(default_root_dir=tmpdir, max_epochs=1) From c487c0d82146441a5a6eb069d32991f5a5a04c9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrian=20W=C3=A4lchli?= Date: Thu, 12 Aug 2021 09:59:55 +0200 Subject: [PATCH 6/6] combine tests --- tests/trainer/optimization/test_optimizers.py | 48 +++++++------------ 1 file changed, 17 insertions(+), 31 deletions(-) diff --git a/tests/trainer/optimization/test_optimizers.py b/tests/trainer/optimization/test_optimizers.py index e5d9badd12b3d..3c8a3d5ae8e68 100644 --- a/tests/trainer/optimization/test_optimizers.py +++ b/tests/trainer/optimization/test_optimizers.py @@ -375,42 +375,15 @@ def configure_optimizers(self): trainer.fit(model) -def test_lr_scheduler_strict(tmpdir): +@pytest.mark.parametrize("complete_epoch", [True, False]) +@mock.patch("torch.optim.lr_scheduler.ReduceLROnPlateau.step") +def test_lr_scheduler_strict(step_mock, tmpdir, complete_epoch): """ Test "strict" support in lr_scheduler dict """ model = BoringModel() optimizer = optim.Adam(model.parameters()) scheduler = optim.lr_scheduler.ReduceLROnPlateau(optimizer) - trainer = Trainer(default_root_dir=tmpdir, max_epochs=1) - - model.configure_optimizers = lambda: { - "optimizer": optimizer, - "lr_scheduler": {"scheduler": scheduler, "monitor": "giraffe", "strict": True}, - } - with pytest.raises( - MisconfigurationException, - match=r"ReduceLROnPlateau conditioned on metric .* which is not available\. Available metrics are:", - ): - trainer.fit(model) - - model.configure_optimizers = lambda: { - "optimizer": optimizer, - "lr_scheduler": {"scheduler": scheduler, "monitor": "giraffe", "strict": False}, - } - with pytest.warns( - RuntimeWarning, match=r"ReduceLROnPlateau conditioned on metric .* which is not available but strict" - ): - trainer.fit(model) - - -@pytest.mark.parametrize("complete_epoch", [True, False]) -@mock.patch("torch.optim.lr_scheduler.ReduceLROnPlateau.step") -def test_lr_scheduler_strict_incomplete_epoch(step_mock, tmpdir, complete_epoch): - """Tests that plateau scheduler does not attempt to step in an incomplete epoch (stopped early).""" - model = BoringModel() - optimizer = optim.Adam(model.parameters()) - scheduler = optim.lr_scheduler.ReduceLROnPlateau(optimizer) max_epochs = 1 if complete_epoch else None max_steps = None if complete_epoch else 1 trainer = Trainer(default_root_dir=tmpdir, max_epochs=max_epochs, max_steps=max_steps) @@ -429,7 +402,20 @@ def test_lr_scheduler_strict_incomplete_epoch(step_mock, tmpdir, complete_epoch) else: trainer.fit(model) - assert step_mock.call_count == 0 + step_mock.assert_not_called() + + model.configure_optimizers = lambda: { + "optimizer": optimizer, + "lr_scheduler": {"scheduler": scheduler, "monitor": "giraffe", "strict": False}, + } + + if complete_epoch: + with pytest.warns( + RuntimeWarning, match=r"ReduceLROnPlateau conditioned on metric .* which is not available but strict" + ): + trainer.fit(model) + + step_mock.assert_not_called() def test_unknown_configure_optimizers_raises(tmpdir):