Repository: Lightning-AI/pytorch-lightning
Pull request: Fix weights_save_path when logger is used + simplify path handling + better docs
## What does this PR do?

<!--
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
-->

Fixes #2527, #882, #1812 and continues effort to document and protect Trainer attributes #1497 better.

1. If user defines the Logger save dir, it will now properly work with the setting for `weights_save_path`. 
2. Makes sure that the ` weights_save_path` only gets picked if it actually was changed by user.
3. Fixed `weights_save_path` not getting used when logger=False
4. Deprecated `trainer.ckpt_path` since it was never actually different from `weights_save_path`, and never used anywhere in the code base.
5. Made sure that the checkpoint callback does not set the properties like `weights_save_path`, it's not needed and prevented multiple checkpoint callbacks being used.
6. Added detailed docs about path settings 

# Before submitting

- [x] Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
- [x] Did you read the [contributor guideline](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/.github/CONTRIBUTING.md), Pull Request section?
- [x] Did you make sure your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
- [x] Did you make sure to update the documentation with your changes?
- [x] Did you write any new necessary tests? 
- [x] Did you verify new and existing tests pass locally with your changes?
- [x] If you made a notable change (that affects users), did you update the [CHANGELOG](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/CHANGELOG.md)?

Problem statement:
checkpoint save dir is not correctly set when _save_dir is given by wandb logger
## 🐛 Bug

When using ModelCheckpoint with default parameter and Wandb Logger with save_dir set to some dir,
The checkpoint is still dumped to os.getcwd()

### To Reproduce
........
logger = WandbLogger(save_dir='/path/to/experiment')
trainer = Trainer.from_argparse_args(other_args, logger = logger)

### Expected behavior
The checkpoint should be saved under /path/to/experiment defined by Wandb logger's save_dir argument.

### Additional context
The pl version i am use is by pip install, i.e. 0.8.4

I think the problem is related to the logic in on_train_start() function in model_checkpoint.py,
```
        if trainer.logger is not None:
            # weights_save_path overrides anything
            if getattr(trainer, 'weights_save_path', None) is not None:
                save_dir = trainer.weights_save_path
            else:
                save_dir = (getattr(trainer.logger, 'save_dir', None)
                            or getattr(trainer.logger, '_save_dir', None)
                            or trainer.default_root_dir)

```
Unfortunately, the default of "weights_save_path" is not None; it is set to default_root_dir which is os.getcwd() (See pytorch_lightning/trainer/callback_config.py, line 57): 
```
        # if weights_save_path is still none here, set to current working dir
        if self.weights_save_path is None:
            self.weights_save_path = self.default_root_dir
```

Thus, the ckpt_path is always set to weights_save_path instead of save_dir from logger.

### Fix
A quick patch for this might be as follows:
```
        if trainer.logger is not None:
            # weights_save_path overrides anything
            # unless if it is os.getcwd() and we have a logger set its save_dir to other folder
            weights_save_path = getattr(trainer, 'weights_save_path', None)
            loggers_save_path = (getattr(trainer.logger, 'save_dir', None)
                            or getattr(trainer.logger, '_save_dir', None)
                            or trainer.default_root_dir)
            avoid_weights_save_path = (weight_save_path == trainer.default_root_dir and loggers_save_path != trainer.default_root_dir)

            if (weights_save_path is not None and not avoid_weights_save_path):
                save_dir = weights_save_path
            else:
                save_dir = loggers_save_path

```
I would be happy to fork the code and submit a PR, btw.

Hints from issue discussion:
@pableeto I don't think we want to make this part of the code even more complicated. 
Wouldn't the real fix be to remove this if block here: 
https://github.com/PyTorchLightning/pytorch-lightning/blob/25ee51bc570503f331dceecc610d0eb355e22327/pytorch_lightning/trainer/callback_config.py#L57

I think it is a left-over from a recent refactor.
Well, I guess that would be fine :)
Not sure if remove that block will cause other issues, though.


> @pableeto I don't think we want to make this part of the code even more complicated.
> Wouldn't the real fix be to remove this if block here:
> https://github.com/PyTorchLightning/pytorch-lightning/blob/25ee51bc570503f331dceecc610d0eb355e22327/pytorch_lightning/trainer/callback_config.py#L57
> 
> I think it is a left-over from a recent refactor.

I just found if we remove this block, then it will made the pipeline crush at line 391 of pytorch_lightning/trainer/training_io.py:
```
        folderpath = self.weights_save_path              # This will become None, next line will crush
        if os.path.exists(folderpath):
            files = os.listdir(folderpath)
```


@pableeto try:
```
pip install pytorch-lightning==0.8.5rc1
```

We can re-open if it is not fixed
> @pableeto try:
> 
> ```
> pip install pytorch-lightning==0.8.5rc1
> ```
> 
> We can re-open if it is not fixed

@williamFalcon Just tried 0.8.5rc1, problem still exists.

Commit message: Merge branch 'master' into bugfix/ckpt_path

Patch to review:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5ed0abbcfa801..83beb5b680fb6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -19,6 +19,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 
 ### Deprecated
 
+- Deprecated Trainer attribute `ckpt_path`, which will now be set by `weights_save_path` ([#2681](https://github.com/PyTorchLightning/pytorch-lightning/pull/2681))
 
 ### Removed
 
@@ -29,6 +30,10 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 
 - Fixed `dtype` and `device` properties not getting updated in submodules ([#2657](https://github.com/PyTorchLightning/pytorch-lightning/pull/2657))
 
+- Fixed `save_dir` in loggers getting ignored by default value of `weights_save_path` when user did not specify `weights_save_path` ([#2681](https://github.com/PyTorchLightning/pytorch-lightning/pull/2681))
+
+- Fixed `weights_save_path` getting ignored when `logger=False` is passed to Trainer ([#2681](https://github.com/PyTorchLightning/pytorch-lightning/pull/2681))
+
 ## [0.8.5] - 2020-07-09
 
 ### Added
diff --git a/pytorch_lightning/callbacks/model_checkpoint.py b/pytorch_lightning/callbacks/model_checkpoint.py
index b6a92efc53321..bfade6f024ba8 100644
--- a/pytorch_lightning/callbacks/model_checkpoint.py
+++ b/pytorch_lightning/callbacks/model_checkpoint.py
@@ -41,8 +41,11 @@ class ModelCheckpoint(Callback):
                 ...     filepath='my/path/{epoch}-{val_loss:.2f}-{other_metric:.2f}'
                 ... )
 
-            Can also be set to `None`, then it will be set to default location
-            during trainer construction.
+            By default, filepath is `None` and will be set at runtime to the location
+            specified by :class:`~pytorch_lightning.trainer.trainer.Trainer`'s
+            :paramref:`~pytorch_lightning.trainer.trainer.Trainer.default_root_dir` or
+            :paramref:`~pytorch_lightning.trainer.trainer.Trainer.weights_save_path` arguments,
+            and if the Trainer uses a logger, the path will also contain logger name and version.
 
         monitor: quantity to monitor.
         verbose: verbosity mode. Default: ``False``.
@@ -233,8 +236,17 @@ def format_checkpoint_name(self, epoch, metrics, ver=None):
     @rank_zero_only
     def on_train_start(self, trainer, pl_module):
         """
-        Determine model checkpoint save directory at runtime. References attributes from the
-        Trainer's logger to determine where to save checkpoints.
+        Determines model checkpoint save directory at runtime. References attributes from the
+        trainer's logger to determine where to save checkpoints.
+        The base path for saving weights is set in this priority:
+
+        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
+        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
@@ -242,10 +254,11 @@ def on_train_start(self, trainer, pl_module):
         self.filename = '{epoch}'
 
         if trainer.logger is not None:
-            # weights_save_path overrides anything
-            save_dir = (getattr(trainer, 'weights_save_path', None)
-                        or getattr(trainer.logger, 'save_dir', None)
-                        or trainer.default_root_dir)
+            if trainer.weights_save_path != trainer.default_root_dir:
+                # the user has changed weights_save_path, it overrides anything
+                save_dir = trainer.weights_save_path
+            else:
+                save_dir = trainer.logger.save_dir or trainer.default_root_dir
 
             version = trainer.logger.version if isinstance(
                 trainer.logger.version, str) else f'version_{trainer.logger.version}'
@@ -256,15 +269,12 @@ def on_train_start(self, trainer, pl_module):
                 "checkpoints"
             )
         else:
-            ckpt_path = os.path.join(trainer.default_root_dir, "checkpoints")
+            ckpt_path = os.path.join(trainer.weights_save_path, "checkpoints")
 
         self.dirpath = ckpt_path
 
         assert trainer.global_rank == 0, 'tried to make a checkpoint from non global_rank=0'
-
         os.makedirs(self.dirpath, exist_ok=True)
-        trainer.ckpt_path = ckpt_path
-        trainer.weights_save_path = ckpt_path
 
     @rank_zero_only
     def on_validation_end(self, trainer, pl_module):
diff --git a/pytorch_lightning/trainer/__init__.py b/pytorch_lightning/trainer/__init__.py
index b8f02a36c6806..7e188ab97492c 100644
--- a/pytorch_lightning/trainer/__init__.py
+++ b/pytorch_lightning/trainer/__init__.py
@@ -291,12 +291,12 @@ def on_train_end(self, trainer, pl_module):
     )
 
 default_root_dir
-^^^^^^^^^^^^^^^^^
+^^^^^^^^^^^^^^^^
 
 Default path for logs and weights when no logger
 or :class:`pytorch_lightning.callbacks.ModelCheckpoint` callback passed.
 On certain clusters you might want to separate where logs and checkpoints
-are stored. If you don't then use this method for convenience.
+are stored. If you don't then use this argument for convenience.
 
 Example::
 
diff --git a/pytorch_lightning/trainer/callback_config.py b/pytorch_lightning/trainer/callback_config.py
index b65dc37ef8b1b..8600449d86a94 100644
--- a/pytorch_lightning/trainer/callback_config.py
+++ b/pytorch_lightning/trainer/callback_config.py
@@ -33,12 +33,6 @@ def is_overridden(self, *args):
         """Warning: this is just empty shell for code implemented in other class."""
 
     def configure_checkpoint_callback(self, checkpoint_callback):
-        """
-        Weight path set in this priority:
-        Checkpoint_callback's path (if passed in).
-        User provided weights_saved_path
-        Otherwise use os.getcwd()
-        """
         if checkpoint_callback is True:
             # when no val step is defined, use 'loss' otherwise 'val_loss'
             train_step_only = not self.is_overridden('validation_step')
@@ -53,10 +47,6 @@ def configure_checkpoint_callback(self, checkpoint_callback):
         if checkpoint_callback:
             checkpoint_callback.save_function = self.save_checkpoint
 
-        # if weights_save_path is still none here, set to current working dir
-        if self.weights_save_path is None:
-            self.weights_save_path = self.default_root_dir
-
         return checkpoint_callback
 
     def configure_early_stopping(self, early_stop_callback):
diff --git a/pytorch_lightning/trainer/deprecated_api.py b/pytorch_lightning/trainer/deprecated_api.py
index 6cb160f1d26b2..88bbbb4bf2aae 100644
--- a/pytorch_lightning/trainer/deprecated_api.py
+++ b/pytorch_lightning/trainer/deprecated_api.py
@@ -45,6 +45,8 @@ class TrainerDeprecatedAPITillVer0_10(ABC):
     limit_test_batches: Union[int, float]
     limit_train_batches: Union[int, float]
     overfit_batches: Union[int, float]
+    weights_save_path: str
+    is_global_zero: bool
 
     def __init__(self):
         super().__init__()  # mixin calls super too
@@ -118,3 +120,17 @@ def proc_rank(self, rank):
         rank_zero_warn("Attribute `proc_rank` is now set by `global_rank` since v0.8.0"
                        " and this method will be removed in v0.10.0", DeprecationWarning)
         self.global_rank = rank
+
+    @property
+    def ckpt_path(self) -> str:
+        """Back compatibility, will be removed in v0.10.0"""
+        rank_zero_warn("Attribute `ckpt_path` is now set by `weights_save_path` since v0.9.0"
+                       " and this method will be removed in v0.10.0", DeprecationWarning)
+        return self.weights_save_path if self.is_global_zero else None
+
+    @ckpt_path.setter
+    def ckpt_path(self, path: str):
+        """Back compatibility, will be removed in v0.10.0"""
+        rank_zero_warn("Attribute `ckpt_path` is now set by `weights_save_path` since v0.9.0"
+                       " and this method will be removed in v0.10.0", DeprecationWarning)
+        self.weights_save_path = path
diff --git a/pytorch_lightning/trainer/trainer.py b/pytorch_lightning/trainer/trainer.py
index e19636a76bdae..cdebbd2ac74c2 100644
--- a/pytorch_lightning/trainer/trainer.py
+++ b/pytorch_lightning/trainer/trainer.py
@@ -419,10 +419,7 @@ def __init__(
         self.should_stop = False
         self.running_sanity_check = False
 
-        # set default save path if user didn't provide one
-        if default_root_dir is None:
-            default_root_dir = os.getcwd()
-        self.default_root_dir = default_root_dir
+        self._default_root_dir = default_root_dir
 
         # init callbacks
         self.callbacks = callbacks or []
@@ -436,7 +433,7 @@ def __init__(
         # configure checkpoint callback
         # it is important that this is the last callback to run
         # pass through the required args to figure out defaults
-        self.weights_save_path = weights_save_path
+        self._weights_save_path = weights_save_path
         checkpoint_callback = self.configure_checkpoint_callback(checkpoint_callback)
         if checkpoint_callback:
             self.callbacks.append(checkpoint_callback)
@@ -894,6 +891,28 @@ def enable_validation(self) -> bool:
         val_loop_enabled = (self.is_overridden('validation_step') and self.limit_val_batches > 0)
         return val_loop_enabled or self.fast_dev_run
 
+    @property
+    def default_root_dir(self) -> str:
+        """
+        The default location to save artifacts of loggers, checkpoints etc.
+        It is used as a fallback if logger or checkpoint callback do not define specific save paths.
+        Defaults to ``os.getcwd()``.
+        """
+        path = self._default_root_dir or os.getcwd()
+        path = os.path.normpath(path)
+        return path
+
+    @property
+    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.
+        It defaults to :paramref:`~pytorch_lightning.trainer.trainer.Trainer.default_root_dir`.
+        """
+        path = self._weights_save_path or self.default_root_dir
+        path = os.path.normpath(path)
+        return path
+
     # -----------------------------
     # MODEL TRAINING
     # -----------------------------
diff --git a/tests/callbacks/test_model_checkpoint.py b/tests/callbacks/test_model_checkpoint.py
index bb575494c3148..4cb52a54610e3 100644
--- a/tests/callbacks/test_model_checkpoint.py
+++ b/tests/callbacks/test_model_checkpoint.py
@@ -28,9 +28,7 @@ def test_model_checkpoint_with_non_string_input(tmpdir, save_top_k):
         max_epochs=2,
     )
     trainer.fit(model)
-
-    # These should be different if the dirpath has be overridden
-    assert trainer.ckpt_path != trainer.default_root_dir
+    assert checkpoint.dirpath == tmpdir / trainer.logger.name / f'version_0' / 'checkpoints'
 
 
 @pytest.mark.parametrize(
@@ -51,7 +49,7 @@ def test_model_checkpoint_path(tmpdir, logger_version, expected):
     )
     trainer.fit(model)
 
-    ckpt_version = Path(trainer.ckpt_path).parent.name
+    ckpt_version = Path(trainer.checkpoint_callback.dirpath).parent.name
     assert ckpt_version == expected
 
 
diff --git a/tests/loggers/test_all.py b/tests/loggers/test_all.py
index b64119078c6dd..e3d6202d05932 100644
--- a/tests/loggers/test_all.py
+++ b/tests/loggers/test_all.py
@@ -1,5 +1,6 @@
 import atexit
 import inspect
+import os
 import pickle
 import platform
 from unittest import mock
@@ -82,6 +83,68 @@ def log_metrics(self, metrics, step):
                                 (1, ['epoch', 'test_acc', 'test_loss'])]
 
 
+@pytest.mark.parametrize("logger_class", [
+    TensorBoardLogger,
+    CometLogger,
+    MLFlowLogger,
+    TestTubeLogger,
+    WandbLogger,
+])
+@mock.patch('pytorch_lightning.loggers.wandb.wandb')
+def test_loggers_save_dir_and_weights_save_path(wandb, tmpdir, monkeypatch, logger_class):
+    """ Test the combinations of save_dir, weights_save_path and default_root_dir.  """
+    if logger_class == CometLogger:
+        # prevent comet logger from trying to print at exit, since
+        # pytest's stdout/stderr redirection breaks it
+        monkeypatch.setattr(atexit, 'register', lambda _: None)
+
+    class TestLogger(logger_class):
+        # for this test it does not matter what these attributes are
+        # so we standardize them to make testing easier
+        @property
+        def version(self):
+            return 'version'
+
+        @property
+        def name(self):
+            return 'name'
+
+    model = EvalModelTemplate()
+    trainer_args = dict(
+        default_root_dir=tmpdir,
+        max_steps=1,
+    )
+
+    # no weights_save_path given
+    save_dir = tmpdir / 'logs'
+    weights_save_path = None
+    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.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)
+    trainer.fit(model)
+    assert trainer.weights_save_path == 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)
+    trainer.fit(model)
+    assert trainer.weights_save_path == weights_save_path
+    assert trainer.checkpoint_callback.dirpath == weights_save_path / 'checkpoints'
+    assert trainer.default_root_dir == tmpdir
+
+
 @pytest.mark.parametrize("logger_class", [
     TensorBoardLogger,
     CometLogger,
diff --git a/tests/loggers/test_comet.py b/tests/loggers/test_comet.py
index a89840163fe7a..a3ba883a65ae3 100644
--- a/tests/loggers/test_comet.py
+++ b/tests/loggers/test_comet.py
@@ -103,5 +103,5 @@ def test_comet_logger_dirs_creation(tmpdir, monkeypatch):
     trainer = Trainer(default_root_dir=tmpdir, logger=logger, max_epochs=1, limit_val_batches=3)
     trainer.fit(model)
 
-    assert trainer.ckpt_path == trainer.weights_save_path == (tmpdir / 'test' / version / 'checkpoints')
-    assert set(os.listdir(trainer.ckpt_path)) == {'epoch=0.ckpt'}
+    assert trainer.checkpoint_callback.dirpath == (tmpdir / 'test' / version / 'checkpoints')
+    assert set(os.listdir(trainer.checkpoint_callback.dirpath)) == {'epoch=0.ckpt'}
diff --git a/tests/loggers/test_mlflow.py b/tests/loggers/test_mlflow.py
index ec9bc8db332a4..31b580f33f6d4 100644
--- a/tests/loggers/test_mlflow.py
+++ b/tests/loggers/test_mlflow.py
@@ -37,5 +37,5 @@ def test_mlflow_logger_dirs_creation(tmpdir):
     assert set(os.listdir(tmpdir / exp_id)) == {run_id, 'meta.yaml'}
     assert 'epoch' in os.listdir(tmpdir / exp_id / run_id / 'metrics')
     assert set(os.listdir(tmpdir / exp_id / run_id / 'params')) == model.hparams.keys()
-    assert trainer.ckpt_path == trainer.weights_save_path == (tmpdir / exp_id / run_id / 'checkpoints')
-    assert set(os.listdir(trainer.ckpt_path)) == {'epoch=0.ckpt'}
+    assert trainer.checkpoint_callback.dirpath == (tmpdir / exp_id / run_id / 'checkpoints')
+    assert set(os.listdir(trainer.checkpoint_callback.dirpath)) == {'epoch=0.ckpt'}
diff --git a/tests/loggers/test_wandb.py b/tests/loggers/test_wandb.py
index 57b0aff311264..9907ad9d087a2 100644
--- a/tests/loggers/test_wandb.py
+++ b/tests/loggers/test_wandb.py
@@ -94,5 +94,5 @@ def test_wandb_logger_dirs_creation(wandb, tmpdir):
     trainer = Trainer(default_root_dir=tmpdir, logger=logger, max_epochs=1, limit_val_batches=3)
     trainer.fit(model)
 
-    assert trainer.ckpt_path == trainer.weights_save_path == str(tmpdir / 'project' / version / 'checkpoints')
-    assert set(os.listdir(trainer.ckpt_path)) == {'epoch=0.ckpt'}
+    assert trainer.checkpoint_callback.dirpath == str(tmpdir / 'project' / version / 'checkpoints')
+    assert set(os.listdir(trainer.checkpoint_callback.dirpath)) == {'epoch=0.ckpt'}
diff --git a/tests/models/data/horovod/train_default_model.py b/tests/models/data/horovod/train_default_model.py
index a71d7b576ca1f..7138021e8e7e9 100644
--- a/tests/models/data/horovod/train_default_model.py
+++ b/tests/models/data/horovod/train_default_model.py
@@ -63,7 +63,6 @@ def run_test_from_config(trainer_options):
     if trainer.global_rank > 0:
         # on higher ranks the checkpoint location is unknown
         # we want to test checkpointing on rank 0 only
-        assert not hasattr(trainer, 'ckpt_path')
         assert not trainer.checkpoint_callback.best_model_path
         return
 


Code review comments:
diff_hunk: @@ -894,6 +891,28 @@ def enable_validation(self) -> bool:
         val_loop_enabled = (self.is_overridden('validation_step') and self.limit_val_batches > 0)
         return val_loop_enabled or self.fast_dev_run
 
+    @property
+    def default_root_dir(self) -> str:
+        """
+        The default location to save artifacts of loggers, checkpoints etc.
+        It is used as a fallback if logger or checkpoint callback do not define specific save paths.
+        Defaults to ``os.getcwd()``.
+        """
+        path = self._default_root_dir or os.getcwd()
path: pytorch_lightning/trainer/trainer.py
text: @user1:
I would rather this have in init and write to log a warning that no part given so we use the actual folder

@author:
ok i moved it to init, but I don't like it as much because of documentation. I disagree with warning, because it is a default and it is documented and we don't print warnings for other trainer defaults.