Repository: Lightning-AI/pytorch-lightning
Pull request: Fix support for symlink save_dir in TensorBoardLogger
## 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.

If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

The following links the related issue to the PR (https://docs.github.com/en/free-pro-team@latest/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword)
-->
Fixes #6690

## Before submitting
- [x] Was this discussed/approved via a GitHub issue? (not for typos and docs)
- [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?
- [x] Did you make sure to update the documentation with your changes? (if necessary)
- [x] Did you write any new necessary tests? (not for typos and docs)
- [x] Did you verify new and existing tests pass locally with your changes?
- [x] Did you update the [CHANGELOG](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/CHANGELOG.md)? (not for typos, docs, test updates, or internal minor changes/refactorings)

<!-- For CHANGELOG separate each item in the unreleased section by a blank line to reduce collisions -->

## PR review
Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read [Review guidelines](https://github.com/PyTorchLightning/pytorch-lightning/wiki/Review-guidelines). In short, see the following bullet-list:

 - [x] Is this pull request ready for review? (if not, please submit in draft mode)
 - [x] Check that all items from **Before submitting** are resolved
 - [x] Make sure the title is self-explanatory and the description concisely explains the PR
 - [x] Add labels and milestones (and optionally projects) to the PR so it can be classified

## Did you have fun?
Make sure you had fun coding 🙃


Problem statement:
symlink not accepted as logger folder 
## 🐛 Bug

<!-- A clear and concise description of what the bug is. -->

Moving `lightning_logs` to another filesystem and symlinking it so I don't need to touch my code, I notice that the logger rejects symlinked logger folders:

`loggers/csv_logs.py        if not os.path.isdir(root_dir):`

Users will expect symlinks to folders to work the same way as folders.

### To Reproduce

```
mkdir /tmp/lightning_logs
ln -s /tmp/lightning_logs/
```
and try training as usual.

### Expected behavior

The symlinked folder is accepted and, in the above example, the logs start filling /tmp instead of $HOME.

### Workarounds

(Breaking existing paths)

Solution 1: Create a folder inside the symlinked folder and set `save_dir` to this subfolder (not tested)
Solution 2: Use the absolute path to the folder, e.g. `trainer = pl.Trainer(logger = pl.loggers.TensorBoardLogger(os.path.abspath('lightning_logs')))` (I was expecting the next run to go to `version_3` after I had versions 0, 1 and 2 but it went to `default/version_0` so there still seems to be a side effect)

### Environment

Private, local Jupyter notebook with manually setup virtualenv.

 - PyTorch Version (e.g., 1.0): 1.7.1+cu101
 - OS (e.g., Linux): Linux
 - How you installed PyTorch (`conda`, `pip`, source): pip
 - Build command you used (if compiling from source):
 - Python version: 3.6.12
 - CUDA/cuDNN version: 10.1
 - GPU models and configuration: GeForce GTX Titan-X
 - Any other relevant information: None

### Additional context

None

Hints from issue discussion:
Hi @jowagner, thanks for the issue! I had a go at CSV logging to a symlink and it is working fine at least for me. The [`os.path.isdir` docs](https://docs.python.org/3/library/os.path.html#os.path.isdir) seem to suggest that it follows symlinks, so shouldn't be an issue. Can you maybe reproduce the bug in colab using the [**BoringModel**](https://colab.research.google.com/drive/1HvWVVTK8j2Nj52qU4Q4YCyzOm0_aLQF3?usp=sharing)? You can use [`os.symlink`](https://docs.python.org/3/library/os.html#os.symlink) to create a symlink between some folders on the colab storage :smiley:

It may also be worth checking if the bug is still there if you update `pytorch-lightning` to master using the instructions here: https://github.com/PyTorchLightning/pytorch-lightning#install-bleeding-edge---future-12
Right. Commenting out my workaround `logger= ... abspath ...` lines, l cannot reproduce the error. This is in a new jupyter server instance as I had killed my server in the meantime. Maybe there are some strange interactions when moving the folder to another filesystem while the jupyter kernel is running. (The notebook was idle.) I'll need more time to try to reproduce the error. It was the "Missing logger folder" message. I now see there is a second occurrence of this message with a slightly different condition:

```
$ find -type f -name "*.py" | xargs -d'\n' fgrep -A 2 -B 1 "Missing logger folder" | tail -n 4
./loggers/tensorboard.py-        if not self._fs.isdir(root_dir):
./loggers/tensorboard.py:            log.warning('Missing logger folder: %s', root_dir)
./loggers/tensorboard.py-            return 0
./loggers/tensorboard.py-
```

I find `self._fs = get_filesystem(...)` and this leads to a call of fsspec.filesystem(), which may be https://pypi.org/project/fsspec/ and may have different semantics of `isdir()`. Though that cannot really be it given that the symlink now works in a new jupyter server instance. Maybe `fsspec` does some caching of metadata and doesn't behave well when a dir changes to a symlink. Will need more time to try to reproduce the error to find out more.
Dear @jowagner,

Please, keep us updated as `symlink` should definitely work !

Best,
T.C
I cleaned out the folder the symlink points to and restarted the notebook server. Training runs through OK but the "Missing logger folder: /home/retracted/notebooks/lightning_logs" message is still there. Repeat runs of the notebook write to the same "version_0" folder. This means it is not just a problem of a warning message but there is a change of behaviour.

The following suggests that the `isdir()` semantics for `fsspec` are indeed different:
```
$ ls -l
[...]
lrwxrwxrwx 1 retracted users         32 Mar 26 17:09 lightning_logs -> /scratch/retracted/lightning_logs/
[...]
$ python
Python 3.6.12 (default, Dec 02 2020, 09:44:23) [GCC] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import fsspec
>>> fs = fsspec.filesystem('file')
>>> fs.isdir('lightning_logs')
False
```
@jowagner, thanks for providing some more information. That definitely lools like a bug, I'll do a bit of digging and try to work on a fix :smiley:

Commit message: Simplify

Patch to review:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 780a8790b9fdd..7a168ffacdc45 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -209,6 +209,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 - Fixed a bug where gradients were disabled after calling `Trainer.predict` ([#6657](https://github.com/PyTorchLightning/pytorch-lightning/pull/6657))
 
 
+- Fixed a bug where `TensorBoardLogger` would give a warning and not log correctly to a symbolic link `save_dir` ([#6730](https://github.com/PyTorchLightning/pytorch-lightning/pull/6730))
+
+
 ## [1.2.4] - 2021-03-16
 
 ### Changed
diff --git a/pytorch_lightning/utilities/cloud_io.py b/pytorch_lightning/utilities/cloud_io.py
index e94934020107d..6af7bf887fb14 100644
--- a/pytorch_lightning/utilities/cloud_io.py
+++ b/pytorch_lightning/utilities/cloud_io.py
@@ -12,15 +12,27 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import os
 import io
 from distutils.version import LooseVersion
 from pathlib import Path
 from typing import IO, Union
 
 import fsspec
+from fsspec.implementations.local import LocalFileSystem
+
 import torch
 
 
+class LightningLocalFileSystem(LocalFileSystem):
+    """Extension of ``fsspec.implementations.local.LocalFileSystem`` where ``LightningLocalFileSystem.isdir`` behaves
+    the same as ``os.isdir``.
+    """
+
+    def isdir(self, path: str) -> bool:
+        return os.path.isdir(path)  # follows symlinks
+
+
 def load(path_or_url: Union[str, IO, Path], map_location=None):
     if not isinstance(path_or_url, (str, Path)):
         # any sort of BytesIO or similiar
@@ -39,7 +51,7 @@ def get_filesystem(path: Union[str, Path]):
         return fsspec.filesystem(path.split(":", 1)[0])
     else:
         # use local filesystem
-        return fsspec.filesystem("file")
+        return LightningLocalFileSystem()
 
 
 def atomic_save(checkpoint, filepath: str):
diff --git a/tests/loggers/test_tensorboard.py b/tests/loggers/test_tensorboard.py
index 1a85270c6dcbb..5d4d36db8aa4d 100644
--- a/tests/loggers/test_tensorboard.py
+++ b/tests/loggers/test_tensorboard.py
@@ -301,3 +301,22 @@ def test_tensorboard_save_hparams_to_yaml_once(tmpdir):
     hparams_file = "hparams.yaml"
     assert os.path.isfile(os.path.join(trainer.log_dir, hparams_file))
     assert not os.path.isfile(os.path.join(tmpdir, hparams_file))
+
+
+@mock.patch('pytorch_lightning.loggers.tensorboard.log')
+def test_tensorboard_with_symlink(log, tmpdir):
+    """
+    Tests a specific failure case when tensorboard logger is used with empty name, symbolic link ``save_dir``, and
+    relative paths.
+    """
+    os.chdir(tmpdir)  # need to use relative paths
+    source = os.path.join('.', 'lightning_logs')
+    dest = os.path.join('.', 'sym_lightning_logs')
+
+    os.makedirs(source, exist_ok=True)
+    os.symlink(source, dest)
+
+    logger = TensorBoardLogger(save_dir=dest, name='')
+    _ = logger.version
+
+    log.warning.assert_not_called()


Code review comments:
diff_hunk: @@ -12,15 +12,27 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import os
 import io
 from distutils.version import LooseVersion
 from pathlib import Path
 from typing import IO, Union
 
 import fsspec
+from fsspec.implementations.local import LocalFileSystem
+
 import torch
 
 
+class LightningLocalFileSystem(LocalFileSystem):
+    """Extension of ``fsspec.implementations.local.LocalFileSystem`` where ``LightningLocalFileSystem.isdir`` behaves
+    the same as ``os.isdir``.
+    """
path: pytorch_lightning/utilities/cloud_io.py
text: @user1:
```suggestion

    To be removed when https://github.com/intake/filesystem_spec/issues/591 is fixed.
    """
```

diff_hunk: @@ -12,15 +12,27 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import os
 import io
 from distutils.version import LooseVersion
 from pathlib import Path
 from typing import IO, Union
 
 import fsspec
+from fsspec.implementations.local import LocalFileSystem
+
 import torch
 
 
+class LightningLocalFileSystem(LocalFileSystem):
path: pytorch_lightning/utilities/cloud_io.py
text: @user1:
Can we make this protected in case this stays here for a while?

```suggestion
class _LightningLocalFileSystem(LocalFileSystem):
```