Add deprecation warning to ModelCheckpoint when logging val_loss with no monitor (#6012)

* Add deprecation warning when logging val_loss with no monitor

* EOF

* Update CHANGELOG

* Clear warning cache before testing

* pep8

Co-authored-by: Jirka Borovec <[email protected]>

Author: carmocca

CHANGELOG.md

@@ -218,6 +218,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
     * `xla_device_utils` >> `xla_device`
 
 
+- Deprecated using `'val_loss'` to set the `ModelCheckpoint` monitor ([#6012](https://github.com/PyTorchLightning/pytorch-lightning/pull/6012))
+
+
 ### Removed
 
 - Removed deprecated checkpoint argument `filepath` ([#5321](https://github.com/PyTorchLightning/pytorch-lightning/pull/5321))

pytorch_lightning/callbacks/model_checkpoint.py

@@ -34,6 +34,9 @@
 from pytorch_lightning.utilities import rank_zero_info, rank_zero_only, rank_zero_warn
 from pytorch_lightning.utilities.cloud_io import get_filesystem
 from pytorch_lightning.utilities.exceptions import MisconfigurationException
+from pytorch_lightning.utilities.warnings import WarningCache
+
+warning_cache = WarningCache()
 
 
 class ModelCheckpoint(Callback):
@@ -185,9 +188,6 @@ def __init__(
         self.save_function = None
         self.warned_result_obj = False
 
-        if save_top_k is None and monitor is not None:
-            self.save_top_k = 1
-
         if prefix:
             rank_zero_warn(
                 'Argument `prefix` is deprecated in v1.1 and will be removed in v1.3.'
@@ -460,17 +460,23 @@ def __resolve_ckpt_dir(self, trainer):
 
     def _add_backward_monitor_support(self, trainer):
         metrics = trainer.logger_connector.callback_metrics
+        deprecation_warning = False
 
-        # backward compatibility... need to deprecate
         if self.monitor is None and 'val_loss' in metrics:
             self.monitor = 'val_loss'
-
-        if self.monitor is None and 'checkpoint_on' in metrics:
-            self.monitor = 'checkpoint_on'
+            deprecation_warning = True
 
         if self.save_top_k is None and self.monitor is not None:
+            # TODO: Remove `Optional` from `save_top_k` when this is deleted in v1.4
             self.save_top_k = 1
 
+        if deprecation_warning:
+            warning_cache.warn(
+                "Relying on `self.log('val_loss', ...)` to set the ModelCheckpoint monitor is deprecated in v1.2"
+                " and will be removed in v1.4. Please, create your own `mc = ModelCheckpoint(monitor='your_monitor')`"
+                " and use it as `Trainer(callbacks=[mc])`.", DeprecationWarning
+            )
+
     def _validate_monitor_key(self, trainer):
         metrics = trainer.logger_connector.callback_metrics
 

pytorch_lightning/trainer/connectors/data_connector.py

@@ -96,7 +96,7 @@ def attach_dataloaders(
         train_dataloader=None,
         val_dataloaders=None,
         test_dataloaders=None,
-        predict_dataloaders=None
+        predict_dataloaders=None,
     ):
         # when dataloader is passed via fit, patch the train_dataloader
         # functions to overwrite with these implementations

pytorch_lightning/utilities/warnings.py

@@ -19,10 +19,10 @@ class WarningCache:
     def __init__(self):
         self.warnings = set()
 
-    def warn(self, m):
+    def warn(self, m, *args, **kwargs):
         if m not in self.warnings:
             self.warnings.add(m)
-            rank_zero_warn(m)
+            rank_zero_warn(m, *args, **kwargs)
 
     def clear(self):
         self.warnings.clear()

tests/deprecated_api/test_remove_1-4.py

@@ -215,3 +215,19 @@ def automatic_optimization(self):
         match="`optimizer` argument to `manual_backward` is deprecated in v1.2 and will be removed in v1.4"
     ):
         trainer.fit(model)
+
+
+def test_v1_4_0_deprecated_checkpoint_on(tmpdir):
+    from pytorch_lightning.callbacks.model_checkpoint import warning_cache
+    warning_cache.clear()
+
+    class TestModel(BoringModel):
+
+        def training_step(self, batch, batch_idx):
+            self.log("val_loss", -batch_idx)
+            return super().training_step(batch, batch_idx)
+
+    trainer = Trainer(default_root_dir=tmpdir, checkpoint_callback=True, max_epochs=1)
+
+    with pytest.warns(DeprecationWarning, match=r"Relying on.*is deprecated in v1.2 and will be removed in v1.4"):
+        trainer.fit(TestModel())