Raise MisconfigurationException if trainer.eval is missing required methods (#10016)

Author: rohitgr7

CHANGELOG.md

@@ -343,6 +343,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 - Updated several places in the loops and trainer to access `training_type_plugin` directly instead of `accelerator` ([#9901](https://github.com/PyTorchLightning/pytorch-lightning/pull/9901))
 
 
+- Raise `MisconfigurationException` instead of warning if `trainer.{validate/test}` is missing required methods ([#10016](https://github.com/PyTorchLightning/pytorch-lightning/pull/10016))
+
+
 - Changed default value of the `max_steps` Trainer argument from `None` to -1 ([#9460](https://github.com/PyTorchLightning/pytorch-lightning/pull/9460))
 
 

pytorch_lightning/trainer/configuration_validator.py

@@ -29,16 +29,16 @@ def verify_loop_configurations(trainer: "pl.Trainer", model: "pl.LightningModule
 
     """
     if trainer.state.fn in (TrainerFn.FITTING, TrainerFn.TUNING):
-        __verify_train_loop_configuration(trainer, model)
-        __verify_eval_loop_configuration(model, "val")
+        __verify_train_val_loop_configuration(trainer, model)
         __verify_manual_optimization_support(trainer, model)
         __check_training_step_requires_dataloader_iter(model)
     elif trainer.state.fn == TrainerFn.VALIDATING:
-        __verify_eval_loop_configuration(model, "val")
+        __verify_eval_loop_configuration(trainer, model, "val")
     elif trainer.state.fn == TrainerFn.TESTING:
-        __verify_eval_loop_configuration(model, "test")
+        __verify_eval_loop_configuration(trainer, model, "test")
     elif trainer.state.fn == TrainerFn.PREDICTING:
-        __verify_predict_loop_configuration(trainer, model)
+        __verify_eval_loop_configuration(trainer, model, "predict")
+
     __verify_dp_batch_transfer_support(trainer, model)
     _check_add_get_queue(model)
     # TODO(@daniellepintz): Delete _check_progress_bar in v1.7
@@ -51,7 +51,7 @@ def verify_loop_configurations(trainer: "pl.Trainer", model: "pl.LightningModule
     _check_dl_idx_in_on_train_batch_hooks(trainer, model)
 
 
-def __verify_train_loop_configuration(trainer: "pl.Trainer", model: "pl.LightningModule") -> None:
+def __verify_train_val_loop_configuration(trainer: "pl.Trainer", model: "pl.LightningModule") -> None:
     # -----------------------------------
     # verify model has a training step
     # -----------------------------------
@@ -83,24 +83,15 @@ def __verify_train_loop_configuration(trainer: "pl.Trainer", model: "pl.Lightnin
         )
 
     # ----------------------------------------------
-    # verify model does not have
-    # - on_train_dataloader
-    # - on_val_dataloader
+    # verify model does not have on_train_dataloader
     # ----------------------------------------------
     has_on_train_dataloader = is_overridden("on_train_dataloader", model)
     if has_on_train_dataloader:
         rank_zero_deprecation(
-            "Method `on_train_dataloader` in DataHooks is deprecated and will be removed in v1.7.0."
+            "Method `on_train_dataloader` is deprecated in v1.5.0 and will be removed in v1.7.0."
             " Please use `train_dataloader()` directly."
         )
 
-    has_on_val_dataloader = is_overridden("on_val_dataloader", model)
-    if has_on_val_dataloader:
-        rank_zero_deprecation(
-            "Method `on_val_dataloader` in DataHooks is deprecated and will be removed in v1.7.0."
-            " Please use `val_dataloader()` directly."
-        )
-
     trainer.overriden_optimizer_step = is_overridden("optimizer_step", model)
     trainer.overriden_optimizer_zero_grad = is_overridden("optimizer_zero_grad", model)
     automatic_optimization = model.automatic_optimization
@@ -110,8 +101,30 @@ def __verify_train_loop_configuration(trainer: "pl.Trainer", model: "pl.Lightnin
     if has_overriden_optimization_functions and going_to_accumulate_grad_batches and automatic_optimization:
         rank_zero_warn(
             "When using `Trainer(accumulate_grad_batches != 1)` and overriding"
-            "`LightningModule.optimizer_{step,zero_grad}`, the hooks will not be called on every batch"
-            "(rather, they are called on every optimization step)."
+            " `LightningModule.optimizer_{step,zero_grad}`, the hooks will not be called on every batch"
+            " (rather, they are called on every optimization step)."
+        )
+
+    # -----------------------------------
+    # verify model for val loop
+    # -----------------------------------
+
+    has_val_loader = trainer._data_connector._val_dataloader_source.is_defined()
+    has_val_step = is_overridden("validation_step", model)
+
+    if has_val_loader and not has_val_step:
+        rank_zero_warn("You passed in a `val_dataloader` but have no `validation_step`. Skipping val loop.")
+    if has_val_step and not has_val_loader:
+        rank_zero_warn("You defined a `validation_step` but have no `val_dataloader`. Skipping val loop.")
+
+    # ----------------------------------------------
+    # verify model does not have on_val_dataloader
+    # ----------------------------------------------
+    has_on_val_dataloader = is_overridden("on_val_dataloader", model)
+    if has_on_val_dataloader:
+        rank_zero_deprecation(
+            "Method `on_val_dataloader` is deprecated in v1.5.0 and will be removed in v1.7.0."
+            " Please use `val_dataloader()` directly."
         )
 
 
@@ -143,52 +156,43 @@ def _check_on_post_move_to_device(model: "pl.LightningModule") -> None:
         )
 
 
-def __verify_eval_loop_configuration(model: "pl.LightningModule", stage: str) -> None:
+def __verify_eval_loop_configuration(trainer: "pl.Trainer", model: "pl.LightningModule", stage: str) -> None:
     loader_name = f"{stage}_dataloader"
-    step_name = "validation_step" if stage == "val" else "test_step"
+    step_name = "validation_step" if stage == "val" else f"{stage}_step"
+    trainer_method = "validate" if stage == "val" else stage
+    on_eval_hook = f"on_{loader_name}"
 
-    has_loader = is_overridden(loader_name, model)
+    has_loader = getattr(trainer._data_connector, f"_{stage}_dataloader_source").is_defined()
     has_step = is_overridden(step_name, model)
-
-    if has_loader and not has_step:
-        rank_zero_warn(f"you passed in a {loader_name} but have no {step_name}. Skipping {stage} loop")
-    if has_step and not has_loader:
-        rank_zero_warn(f"you defined a {step_name} but have no {loader_name}. Skipping {stage} loop")
+    has_on_eval_dataloader = is_overridden(on_eval_hook, model)
 
     # ----------------------------------------------
-    # verify model does not have
-    # - on_val_dataloader
-    # - on_test_dataloader
+    # verify model does not have on_eval_dataloader
     # ----------------------------------------------
-    has_on_val_dataloader = is_overridden("on_val_dataloader", model)
-    if has_on_val_dataloader:
+    if has_on_eval_dataloader:
         rank_zero_deprecation(
-            "Method `on_val_dataloader` in DataHooks is deprecated and will be removed in v1.7.0."
-            " Please use `val_dataloader()` directly."
+            f"Method `{on_eval_hook}` is deprecated in v1.5.0 and will"
+            f" be removed in v1.7.0. Please use `{loader_name}()` directly."
         )
 
-    has_on_test_dataloader = is_overridden("on_test_dataloader", model)
-    if has_on_test_dataloader:
-        rank_zero_deprecation(
-            "Method `on_test_dataloader` in DataHooks is deprecated and will be removed in v1.7.0."
-            " Please use `test_dataloader()` directly."
-        )
-
-
-def __verify_predict_loop_configuration(trainer: "pl.Trainer", model: "pl.LightningModule") -> None:
-    has_predict_dataloader = trainer._data_connector._predict_dataloader_source.is_defined()
-    if not has_predict_dataloader:
-        raise MisconfigurationException("Dataloader not found for `Trainer.predict`")
-    # ----------------------------------------------
-    # verify model does not have
-    # - on_predict_dataloader
-    # ----------------------------------------------
-    has_on_predict_dataloader = is_overridden("on_predict_dataloader", model)
-    if has_on_predict_dataloader:
-        rank_zero_deprecation(
-            "Method `on_predict_dataloader` in DataHooks is deprecated and will be removed in v1.7.0."
-            " Please use `predict_dataloader()` directly."
-        )
+    # -----------------------------------
+    # verify model has an eval_dataloader
+    # -----------------------------------
+    if not has_loader:
+        raise MisconfigurationException(f"No `{loader_name}()` method defined to run `Trainer.{trainer_method}`.")
+
+    # predict_step is not required to be overridden
+    if stage == "predict":
+        if model.predict_step is None:
+            raise MisconfigurationException("`predict_step` cannot be None to run `Trainer.predict`")
+        elif not has_step and not is_overridden("forward", model):
+            raise MisconfigurationException("`Trainer.predict` requires `forward` method to run.")
+    else:
+        # -----------------------------------
+        # verify model has an eval_step
+        # -----------------------------------
+        if not has_step:
+            raise MisconfigurationException(f"No `{step_name}()` method defined to run `Trainer.{trainer_method}`.")
 
 
 def __verify_dp_batch_transfer_support(trainer: "pl.Trainer", model: "pl.LightningModule") -> None:

tests/callbacks/test_pruning.py

@@ -30,8 +30,6 @@
 
 
 class TestModel(BoringModel):
-    test_step = None
-
     def __init__(self):
         super().__init__()
         self.layer = Sequential(

tests/callbacks/test_quantization.py

@@ -224,8 +224,8 @@ def test_quantization_val_test_predict(tmpdir):
         max_epochs=4,
     )
     trainer.fit(val_test_predict_qmodel, datamodule=dm)
-    trainer.validate(model=val_test_predict_qmodel, verbose=False)
-    trainer.test(model=val_test_predict_qmodel, verbose=False)
+    trainer.validate(model=val_test_predict_qmodel, datamodule=dm, verbose=False)
+    trainer.test(model=val_test_predict_qmodel, datamodule=dm, verbose=False)
     trainer.predict(
         model=val_test_predict_qmodel, dataloaders=[torch.utils.data.DataLoader(RandomDataset(num_features, 16))]
     )

tests/deprecated_api/test_remove_1-7.py

@@ -163,27 +163,27 @@ def _run(model, task="fit"):
     model = CustomBoringModel()
 
     with pytest.deprecated_call(
-        match="Method `on_train_dataloader` in DataHooks is deprecated and will be removed in v1.7.0."
+        match="Method `on_train_dataloader` is deprecated in v1.5.0 and will be removed in v1.7.0."
     ):
         _run(model, "fit")
 
     with pytest.deprecated_call(
-        match="Method `on_val_dataloader` in DataHooks is deprecated and will be removed in v1.7.0."
+        match="Method `on_val_dataloader` is deprecated in v1.5.0 and will be removed in v1.7.0."
     ):
         _run(model, "fit")
 
     with pytest.deprecated_call(
-        match="Method `on_val_dataloader` in DataHooks is deprecated and will be removed in v1.7.0."
+        match="Method `on_val_dataloader` is deprecated in v1.5.0 and will be removed in v1.7.0."
     ):
         _run(model, "validate")
 
     with pytest.deprecated_call(
-        match="Method `on_test_dataloader` in DataHooks is deprecated and will be removed in v1.7.0."
+        match="Method `on_test_dataloader` is deprecated in v1.5.0 and will be removed in v1.7.0."
     ):
         _run(model, "test")
 
     with pytest.deprecated_call(
-        match="Method `on_predict_dataloader` in DataHooks is deprecated and will be removed in v1.7.0."
+        match="Method `on_predict_dataloader` is deprecated in v1.5.0 and will be removed in v1.7.0."
     ):
         _run(model, "predict")
 

tests/helpers/test_models.py

@@ -40,7 +40,9 @@ def test_models(tmpdir, data_class, model_class):
     trainer = Trainer(default_root_dir=tmpdir, max_epochs=1)
 
     trainer.fit(model, datamodule=dm)
-    trainer.test(model, datamodule=dm)
+
+    if dm is not None:
+        trainer.test(model, datamodule=dm)
 
     model.to_torchscript()
     if data_class:

tests/models/test_restore.py

@@ -331,11 +331,11 @@ def test_running_test_pretrained_model_distrib_dp(tmpdir):
 
     # correct result and ok accuracy
     assert trainer.state.finished, f"Training failed with {trainer.state}"
-    pretrained_model = ClassificationModel.load_from_checkpoint(trainer.checkpoint_callback.best_model_path)
+    pretrained_model = CustomClassificationModelDP.load_from_checkpoint(trainer.checkpoint_callback.best_model_path)
 
     # run test set
     new_trainer = Trainer(**trainer_options)
-    new_trainer.test(pretrained_model)
+    new_trainer.test(pretrained_model, datamodule=dm)
     pretrained_model.cpu()
 
     dataloaders = dm.test_dataloader()
@@ -383,7 +383,7 @@ def test_running_test_pretrained_model_distrib_ddp_spawn(tmpdir):
 
     # run test set
     new_trainer = Trainer(**trainer_options)
-    new_trainer.test(pretrained_model)
+    new_trainer.test(pretrained_model, datamodule=dm)
     pretrained_model.cpu()
 
     dataloaders = dm.test_dataloader()

tests/plugins/test_deepspeed_plugin.py

@@ -915,8 +915,9 @@ def test_dataloader(self):
         gpus=1,
         fast_dev_run=True,
     )
-    trainer.fit(model, datamodule=TestSetupIsCalledDataModule())
-    trainer.test(model)
+    dm = TestSetupIsCalledDataModule()
+    trainer.fit(model, datamodule=dm)
+    trainer.test(model, datamodule=dm)
 
 
 @mock.patch("torch.optim.lr_scheduler.StepLR.step", autospec=True)

tests/trainer/test_config_validator.py

@@ -52,50 +52,63 @@ def test_fit_val_loop_config(tmpdir):
     trainer = Trainer(default_root_dir=tmpdir, max_epochs=1)
 
     # no val data has val loop
-    with pytest.warns(UserWarning, match=r"you passed in a val_dataloader but have no validation_step"):
+    with pytest.warns(UserWarning, match=r"You passed in a `val_dataloader` but have no `validation_step`"):
         model = BoringModel()
         model.validation_step = None
         trainer.fit(model)
 
     # has val loop but no val data
-    with pytest.warns(UserWarning, match=r"you defined a validation_step but have no val_dataloader"):
+    with pytest.warns(UserWarning, match=r"You defined a `validation_step` but have no `val_dataloader`"):
         model = BoringModel()
         model.val_dataloader = None
         trainer.fit(model)
 
 
-def test_test_loop_config(tmpdir):
-    """When either test loop or test data are missing."""
+def test_eval_loop_config(tmpdir):
+    """When either eval step or eval data is missing."""
     trainer = Trainer(default_root_dir=tmpdir, max_epochs=1)
 
+    # has val step but no val data
+    model = BoringModel()
+    model.val_dataloader = None
+    with pytest.raises(MisconfigurationException, match=r"No `val_dataloader\(\)` method defined"):
+        trainer.validate(model)
+
+    # has test data but no val step
+    model = BoringModel()
+    model.validation_step = None
+    with pytest.raises(MisconfigurationException, match=r"No `validation_step\(\)` method defined"):
+        trainer.validate(model)
+
     # has test loop but no test data
-    with pytest.warns(UserWarning, match=r"you defined a test_step but have no test_dataloader"):
-        model = BoringModel()
-        model.test_dataloader = None
+    model = BoringModel()
+    model.test_dataloader = None
+    with pytest.raises(MisconfigurationException, match=r"No `test_dataloader\(\)` method defined"):
         trainer.test(model)
 
-    # has test data but no test loop
-    with pytest.warns(UserWarning, match=r"you passed in a test_dataloader but have no test_step"):
-        model = BoringModel()
-        model.test_step = None
+    # has test data but no test step
+    model = BoringModel()
+    model.test_step = None
+    with pytest.raises(MisconfigurationException, match=r"No `test_step\(\)` method defined"):
         trainer.test(model)
 
+    # has predict step but no predict data
+    model = BoringModel()
+    model.predict_dataloader = None
+    with pytest.raises(MisconfigurationException, match=r"No `predict_dataloader\(\)` method defined"):
+        trainer.predict(model)
 
-def test_val_loop_config(tmpdir):
-    """When either validation loop or validation data are missing."""
-    trainer = Trainer(default_root_dir=tmpdir, max_epochs=1)
-
-    # has val loop but no val data
-    with pytest.warns(UserWarning, match=r"you defined a validation_step but have no val_dataloader"):
-        model = BoringModel()
-        model.val_dataloader = None
-        trainer.validate(model)
+    # has predict data but no predict_step
+    model = BoringModel()
+    model.predict_step = None
+    with pytest.raises(MisconfigurationException, match=r"`predict_step` cannot be None."):
+        trainer.predict(model)
 
-    # has val data but no val loop
-    with pytest.warns(UserWarning, match=r"you passed in a val_dataloader but have no validation_step"):
-        model = BoringModel()
-        model.validation_step = None
-        trainer.validate(model)
+    # has predict data but no forward
+    model = BoringModel()
+    model.forward = None
+    with pytest.raises(MisconfigurationException, match=r"requires `forward` method to run."):
+        trainer.predict(model)
 
 
 @pytest.mark.parametrize("datamodule", [False, True])
@@ -130,11 +143,6 @@ def predict_dataloader(self):
     assert len(results) == 2
     assert results[0][0].shape == torch.Size([1, 2])
 
-    model.predict_dataloader = None
-
-    with pytest.raises(MisconfigurationException, match="Dataloader not found for `Trainer.predict`"):
-        trainer.predict(model)
-
 
 def test_trainer_manual_optimization_config(tmpdir):
     """Test error message when requesting Trainer features unsupported with manual optimization."""