Skip to content

Commit

Permalink
Fix support for symlink save_dir in TensorBoardLogger (#6730)
Browse files Browse the repository at this point in the history
* Add test for symlink support and initial fix

* Respond to comment and add docstring

* Update CHANGELOG.md

* Simplify

* Update pytorch_lightning/utilities/cloud_io.py

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>

* Make `LightningLocalFileSystem` protected

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
  • Loading branch information
ethanwharris and carmocca authored Apr 6, 2021
1 parent cf8e828 commit 89b5326
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 1 deletion.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).

- Fixed resolve a bug with omegaconf and xm.save ([#6741](https://github.com/PyTorchLightning/pytorch-lightning/pull/6741))

- 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
Expand Down
16 changes: 15 additions & 1 deletion pytorch_lightning/utilities/cloud_io.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,29 @@
# 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``.
To be removed when https://github.com/intake/filesystem_spec/issues/591 is fixed.
"""

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
Expand All @@ -39,7 +53,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):
Expand Down
19 changes: 19 additions & 0 deletions tests/loggers/test_tensorboard.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()

0 comments on commit 89b5326

Please sign in to comment.