Deprecate weights_save_path from the Trainer constructor (#12084)

Co-authored-by: Kaushik B <[email protected]>
Co-authored-by: Rohit Gupta <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Adrian Wälchli <[email protected]>

Author: 5 people

CHANGELOG.md

@@ -442,6 +442,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 - Deprecated `LightningLoggerBase.agg_and_log_metrics` in favor of `LightningLoggerBase.log_metrics` ([#11832](https://github.com/PyTorchLightning/pytorch-lightning/pull/11832))
 
 
+- Deprecated passing `weights_save_path` to the `Trainer` constructor in favor of adding the `ModelCheckpoint` callback with `dirpath` directly to the list of callbacks ([#12084](https://github.com/PyTorchLightning/pytorch-lightning/pull/12084))
+
+
 - Deprecated `BaseProfiler.profile_iterable` ([#12102](https://github.com/PyTorchLightning/pytorch-lightning/pull/12102))
 
 

docs/source/common/trainer.rst

@@ -1584,6 +1584,12 @@ Can specify as float or int.
 weights_save_path
 ^^^^^^^^^^^^^^^^^
 
+
+.. warning:: `weights_save_path` has been deprecated in v1.6 and will be removed in v1.8. Please pass
+   ``dirpath`` directly to the :class:`~pytorch_lightning.callbacks.model_checkpoint.ModelCheckpoint`
+   callback.
+
+
 .. raw:: html
 
     <video width="50%" max-width="400px" controls

pytorch_lightning/callbacks/model_checkpoint.py

@@ -579,18 +579,20 @@ def __resolve_ckpt_dir(self, trainer: "pl.Trainer") -> None:
 
         1.  Checkpoint callback's path (if passed in)
         2.  The default_root_dir from trainer if trainer has no logger
-        3.  The weights_save_path from trainer, if user provides it
+        3.  The weights_save_path from trainer, if user provides it (deprecated)
         4.  User provided weights_saved_path
 
         The base path gets extended with logger name and version (if these are available)
         and subfolder "checkpoints".
         """
         if self.dirpath is not None:
             return  # short circuit
+
+        # TODO: Remove weights_save_path logic here in v1.8
         if trainer.loggers:
-            if trainer.weights_save_path != trainer.default_root_dir:
+            if trainer._weights_save_path_internal != trainer.default_root_dir:
                 # the user has changed weights_save_path, it overrides anything
-                save_dir = trainer.weights_save_path
+                save_dir = trainer._weights_save_path_internal
             elif len(trainer.loggers) == 1:
                 save_dir = trainer.logger.save_dir or trainer.default_root_dir
             else:
@@ -602,7 +604,7 @@ def __resolve_ckpt_dir(self, trainer: "pl.Trainer") -> None:
 
             ckpt_path = os.path.join(save_dir, str(name), version, "checkpoints")
         else:
-            ckpt_path = os.path.join(trainer.weights_save_path, "checkpoints")
+            ckpt_path = os.path.join(trainer._weights_save_path_internal, "checkpoints")
 
         ckpt_path = trainer.strategy.broadcast(ckpt_path)
 

pytorch_lightning/trainer/configuration_validator.py

@@ -365,5 +365,5 @@ def _check_deprecated_callback_hooks(trainer: "pl.Trainer") -> None:
             if is_overridden(method_name=hook, instance=callback):
                 rank_zero_deprecation(
                     f"The `Callback.{hook}` hook has been deprecated in v1.6 and"
-                    f" will be removed in v1.8. Please use `Callback.on_fit_start` instead."
+                    " will be removed in v1.8. Please use `Callback.on_fit_start` instead."
                 )

pytorch_lightning/trainer/connectors/callback_connector.py

@@ -53,6 +53,12 @@ def on_trainer_init(
     ):
         # init folder paths for checkpoint + weights save callbacks
         self.trainer._default_root_dir = default_root_dir or os.getcwd()
+        if weights_save_path:
+            rank_zero_deprecation(
+                "Setting `Trainer(weights_save_path=)` has been deprecated in v1.6 and will be"
+                " removed in v1.8. Please pass ``dirpath`` directly to the `ModelCheckpoint` callback"
+            )
+
         self.trainer._weights_save_path = weights_save_path or self.trainer._default_root_dir
         if stochastic_weight_avg:
             rank_zero_deprecation(

pytorch_lightning/trainer/connectors/checkpoint_connector.py

@@ -55,11 +55,12 @@ def __init__(self, trainer: "pl.Trainer", resume_from_checkpoint: Optional[_PATH
 
     @property
     def _hpc_resume_path(self) -> Optional[str]:
-        weights_save_path = self.trainer.weights_save_path
-        fs = get_filesystem(weights_save_path)
-        if not fs.isdir(weights_save_path):
+        # TODO: in v1.8 set this equal to self.trainer.default_root_dir
+        dir_path_hpc = self.trainer._weights_save_path_internal
+        fs = get_filesystem(dir_path_hpc)
+        if not fs.isdir(dir_path_hpc):
             return None
-        dir_path_hpc = str(weights_save_path)
+        dir_path_hpc = str(dir_path_hpc)
         max_version = self.__max_ckpt_version_in_folder(dir_path_hpc, "hpc_ckpt_")
         if max_version is not None:
             return os.path.join(dir_path_hpc, f"hpc_ckpt_{max_version}.ckpt")

pytorch_lightning/trainer/connectors/signal_connector.py

@@ -68,7 +68,8 @@ def slurm_sigusr1_handler_fn(self, signum: _SIGNUM, frame: FrameType) -> None:
         # save logger to make sure we get all the metrics
         for logger in self.trainer.loggers:
             logger.finalize("finished")
-        hpc_save_path = self.trainer._checkpoint_connector.hpc_save_path(self.trainer.weights_save_path)
+        # TODO: in v1.8 change this to use self.trainer.default_root_dir
+        hpc_save_path = self.trainer._checkpoint_connector.hpc_save_path(self.trainer._weights_save_path_internal)
         self.trainer.save_checkpoint(hpc_save_path)
 
         if self.trainer.is_global_zero:

pytorch_lightning/trainer/trainer.py

@@ -169,7 +169,7 @@ def __init__(
         precision: Union[int, str] = 32,
         enable_model_summary: bool = True,
         weights_summary: Optional[str] = "top",
-        weights_save_path: Optional[str] = None,
+        weights_save_path: Optional[str] = None,  # TODO: Remove in 1.8
         num_sanity_val_steps: int = 2,
         resume_from_checkpoint: Optional[Union[Path, str]] = None,
         profiler: Optional[Union[BaseProfiler, str]] = None,
@@ -447,6 +447,11 @@ def __init__(
                 Can be remote file paths such as `s3://mybucket/path` or 'hdfs://path/'
                 Defaults to `default_root_dir`.
 
+                .. deprecated:: v1.6
+                    ``weights_save_path`` has been deprecated in v1.6 and will be removed in v1.8. Please pass
+                    ``dirpath`` directly to the :class:`~pytorch_lightning.callbacks.model_checkpoint.ModelCheckpoint`
+                    callback.
+
             move_metrics_to_cpu: Whether to force internal logged metrics to be moved to cpu.
                 This can save some gpu memory, but can make training slower. Use with attention.
                 Default: ``False``.
@@ -2210,6 +2215,20 @@ def weights_save_path(self) -> str:
         """
         The default root location to save weights (checkpoints), e.g., when the
         :class:`~pytorch_lightning.callbacks.model_checkpoint.ModelCheckpoint` does not define a file path.
+
+        .. deprecated:: v1.6
+            `Trainer.weights_save_path` has been deprecated in v1.6 and will be removed in v1.8.
+        """
+        rank_zero_deprecation("`Trainer.weights_save_path` has been deprecated in v1.6 and will be removed in v1.8.")
+        return self._weights_save_path_internal
+
+    # TODO: Remove _weights_save_path_internal in v1.8
+    @property
+    def _weights_save_path_internal(self) -> str:
+        """This is an internal implementation of weights_save_path which allows weights_save_path to be used
+        internally by the framework without emitting a deprecation warning.
+
+        To be removed in v1.8.
         """
         if get_filesystem(self._weights_save_path).protocol == "file":
             return os.path.normpath(self._weights_save_path)

tests/deprecated_api/test_remove_1-8.py

@@ -590,7 +590,7 @@ def on_pretrain_routine_start(self, trainer, pl_module):
         default_root_dir=tmpdir,
     )
     with pytest.deprecated_call(
-        match="The `Callback.on_pretrain_routine_start` hook has been deprecated in v1.6" " and will be removed in v1.8"
+        match="The `Callback.on_pretrain_routine_start` hook has been deprecated in v1.6 and will be removed in v1.8"
     ):
         trainer.fit(model)
 
@@ -607,11 +607,18 @@ def on_pretrain_routine_end(self, trainer, pl_module):
         default_root_dir=tmpdir,
     )
     with pytest.deprecated_call(
-        match="The `Callback.on_pretrain_routine_end` hook has been deprecated in v1.6" " and will be removed in v1.8"
+        match="The `Callback.on_pretrain_routine_end` hook has been deprecated in v1.6 and will be removed in v1.8"
     ):
         trainer.fit(model)
 
 
+def test_v1_8_0_weights_save_path(tmpdir):
+    with pytest.deprecated_call(match=r"Setting `Trainer\(weights_save_path=\)` has been deprecated in v1.6"):
+        trainer = Trainer(weights_save_path=tmpdir)
+    with pytest.deprecated_call(match=r"`Trainer.weights_save_path` has been deprecated in v1.6"):
+        _ = trainer.weights_save_path
+
+
 @pytest.mark.flaky(reruns=3)
 @pytest.mark.parametrize(["action", "expected"], [("a", [3, 1]), ("b", [2]), ("c", [1])])
 def test_simple_profiler_iterable_durations(tmpdir, action: str, expected: list):

tests/loggers/test_all.py

@@ -208,26 +208,28 @@ def name(self):
     logger = TestLogger(**_get_logger_args(TestLogger, save_dir))
     trainer = Trainer(**trainer_args, logger=logger, weights_save_path=weights_save_path)
     trainer.fit(model)
-    assert trainer.weights_save_path == trainer.default_root_dir
+    assert trainer._weights_save_path_internal == trainer.default_root_dir
     assert trainer.checkpoint_callback.dirpath == os.path.join(logger.save_dir, "name", "version", "checkpoints")
     assert trainer.default_root_dir == tmpdir
 
     # with weights_save_path given, the logger path and checkpoint path should be different
     save_dir = tmpdir / "logs"
     weights_save_path = tmpdir / "weights"
     logger = TestLogger(**_get_logger_args(TestLogger, save_dir))
-    trainer = Trainer(**trainer_args, logger=logger, weights_save_path=weights_save_path)
+    with pytest.deprecated_call(match=r"Setting `Trainer\(weights_save_path=\)` has been deprecated in v1.6"):
+        trainer = Trainer(**trainer_args, logger=logger, weights_save_path=weights_save_path)
     trainer.fit(model)
-    assert trainer.weights_save_path == weights_save_path
+    assert trainer._weights_save_path_internal == weights_save_path
     assert trainer.logger.save_dir == save_dir
     assert trainer.checkpoint_callback.dirpath == weights_save_path / "name" / "version" / "checkpoints"
     assert trainer.default_root_dir == tmpdir
 
     # no logger given
     weights_save_path = tmpdir / "weights"
-    trainer = Trainer(**trainer_args, logger=False, weights_save_path=weights_save_path)
+    with pytest.deprecated_call(match=r"Setting `Trainer\(weights_save_path=\)` has been deprecated in v1.6"):
+        trainer = Trainer(**trainer_args, logger=False, weights_save_path=weights_save_path)
     trainer.fit(model)
-    assert trainer.weights_save_path == weights_save_path
+    assert trainer._weights_save_path_internal == weights_save_path
     assert trainer.checkpoint_callback.dirpath == weights_save_path / "checkpoints"
     assert trainer.default_root_dir == tmpdir
 

tests/models/data/horovod/test_train_script.py

@@ -19,7 +19,6 @@ def test_horovod_model_script(tmpdir):
     """This just for testing/debugging horovod script without horovod..."""
     trainer_options = dict(
         default_root_dir=str(tmpdir),
-        weights_save_path=str(tmpdir),
         gradient_clip_val=1.0,
         enable_progress_bar=False,
         max_epochs=1,

tests/models/data/horovod/train_default_model.py

@@ -49,7 +49,7 @@ def run_test_from_config(trainer_options, on_gpu, check_size=True):
     set_random_main_port()
     reset_seed()
 
-    ckpt_path = trainer_options["weights_save_path"]
+    ckpt_path = trainer_options["default_root_dir"]
     trainer_options.update(callbacks=[ModelCheckpoint(dirpath=ckpt_path)])
 
     class TestModel(BoringModel):

tests/models/test_cpu.py

@@ -66,7 +66,7 @@ def test_cpu_slurm_save_load(tmpdir):
     # save logger to make sure we get all the metrics
     if logger:
         logger.finalize("finished")
-    hpc_save_path = trainer._checkpoint_connector.hpc_save_path(trainer.weights_save_path)
+    hpc_save_path = trainer._checkpoint_connector.hpc_save_path(trainer.default_root_dir)
     trainer.save_checkpoint(hpc_save_path)
     assert os.path.exists(hpc_save_path)
 

tests/models/test_horovod.py

@@ -80,7 +80,6 @@ def test_horovod_cpu(tmpdir):
     """Test Horovod running multi-process on CPU."""
     trainer_options = dict(
         default_root_dir=str(tmpdir),
-        weights_save_path=str(tmpdir),
         gradient_clip_val=1.0,
         enable_progress_bar=False,
         max_epochs=1,
@@ -110,7 +109,6 @@ def test_horovod_cpu_clip_grad_by_value(tmpdir):
     """Test Horovod running multi-process on CPU."""
     trainer_options = dict(
         default_root_dir=str(tmpdir),
-        weights_save_path=str(tmpdir),
         gradient_clip_val=1.0,
         gradient_clip_algorithm="value",
         enable_progress_bar=False,
@@ -127,7 +125,6 @@ def test_horovod_cpu_implicit(tmpdir):
     """Test Horovod without specifying a backend, inferring from env set by `horovodrun`."""
     trainer_options = dict(
         default_root_dir=str(tmpdir),
-        weights_save_path=str(tmpdir),
         gradient_clip_val=1.0,
         enable_progress_bar=False,
         max_epochs=1,
@@ -142,7 +139,6 @@ def test_horovod_multi_gpu(tmpdir):
     """Test Horovod with multi-GPU support."""
     trainer_options = dict(
         default_root_dir=str(tmpdir),
-        weights_save_path=str(tmpdir),
         gradient_clip_val=1.0,
         enable_progress_bar=False,
         max_epochs=1,
@@ -193,7 +189,6 @@ def test_horovod_multi_gpu_grad_by_value(tmpdir):
     """Test Horovod with multi-GPU support."""
     trainer_options = dict(
         default_root_dir=str(tmpdir),
-        weights_save_path=str(tmpdir),
         gradient_clip_val=1.0,
         gradient_clip_algorithm="value",
         enable_progress_bar=False,
@@ -216,7 +211,6 @@ def test_horovod_apex(tmpdir):
     """Test Horovod with multi-GPU support using apex amp."""
     trainer_options = dict(
         default_root_dir=str(tmpdir),
-        weights_save_path=str(tmpdir),
         gradient_clip_val=1.0,
         enable_progress_bar=False,
         max_epochs=1,
@@ -236,7 +230,6 @@ def test_horovod_amp(tmpdir):
     """Test Horovod with multi-GPU support using native amp."""
     trainer_options = dict(
         default_root_dir=str(tmpdir),
-        weights_save_path=str(tmpdir),
         gradient_clip_val=1.0,
         enable_progress_bar=False,
         max_epochs=1,
@@ -256,7 +249,6 @@ def test_horovod_gather(tmpdir):
     """Test Horovod with multi-GPU support using native amp."""
     trainer_options = dict(
         default_root_dir=str(tmpdir),
-        weights_save_path=str(tmpdir),
         gradient_clip_val=1.0,
         enable_progress_bar=False,
         max_epochs=1,

tests/trainer/test_trainer_cli.py

@@ -43,7 +43,7 @@ def test_default_args(mock_argparse, tmpdir):
     assert trainer.max_epochs == 5
 
 
-@pytest.mark.parametrize("cli_args", [["--accumulate_grad_batches=22"], ["--weights_save_path=./"], []])
+@pytest.mark.parametrize("cli_args", [["--accumulate_grad_batches=22"], []])
 def test_add_argparse_args_redefined(cli_args: list):
     """Redefines some default Trainer arguments via the cli and tests the Trainer initialization correctness."""
     parser = ArgumentParser(add_help=False)
@@ -64,7 +64,7 @@ def test_add_argparse_args_redefined(cli_args: list):
     assert isinstance(trainer, Trainer)
 
 
-@pytest.mark.parametrize("cli_args", [["--accumulate_grad_batches=22"], ["--weights_save_path=./"], []])
+@pytest.mark.parametrize("cli_args", [["--accumulate_grad_batches=22"], []])
 def test_add_argparse_args(cli_args: list):
     """Simple test ensuring Trainer.add_argparse_args works."""
     parser = ArgumentParser(add_help=False)
@@ -128,7 +128,6 @@ def _raise():
                 # They should not be changed by the argparse interface.
                 "min_steps": None,
                 "accelerator": None,
-                "weights_save_path": None,
                 "profiler": None,
             },
         ),

tests/utilities/test_cli.py

@@ -75,7 +75,7 @@ def test_default_args(mock_argparse):
     assert trainer.max_epochs == 5
 
 
-@pytest.mark.parametrize("cli_args", [["--accumulate_grad_batches=22"], ["--weights_save_path=./"], []])
+@pytest.mark.parametrize("cli_args", [["--accumulate_grad_batches=22"], []])
 def test_add_argparse_args_redefined(cli_args):
     """Redefines some default Trainer arguments via the cli and tests the Trainer initialization correctness."""
     parser = LightningArgumentParser(add_help=False, parse_as_dict=False)
@@ -139,7 +139,6 @@ def _raise():
                 # interface.
                 min_steps=None,
                 accelerator=None,
-                weights_save_path=None,
                 profiler=None,
             ),
         ),