From ae576fac7d0e3b5e3ee0255dbf11cbfd88580cae Mon Sep 17 00:00:00 2001 From: Ethan Harris Date: Mon, 29 Mar 2021 22:03:23 +0100 Subject: [PATCH 1/6] Add test for symlink support and initial fix --- pytorch_lightning/utilities/cloud_io.py | 18 +++++++++++++++++- tests/loggers/test_tensorboard.py | 19 +++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/pytorch_lightning/utilities/cloud_io.py b/pytorch_lightning/utilities/cloud_io.py index e94934020107d..8b40ee316adbe 100644 --- a/pytorch_lightning/utilities/cloud_io.py +++ b/pytorch_lightning/utilities/cloud_io.py @@ -12,15 +12,31 @@ # 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): + def isdir(self, path): + try: + if self.info(path)["type"] == "directory": + return True + elif self.info(path)["type"] == "link": + return os.path.isdir(path) + else: + return False + except IOError: + return False + + 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 +55,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() From bab1213a878307b4a0218524de27f2178bc8b35a Mon Sep 17 00:00:00 2001 From: Ethan Harris Date: Tue, 30 Mar 2021 12:12:12 +0100 Subject: [PATCH 2/6] Respond to comment and add docstring --- pytorch_lightning/utilities/cloud_io.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/pytorch_lightning/utilities/cloud_io.py b/pytorch_lightning/utilities/cloud_io.py index 8b40ee316adbe..865a6f349172b 100644 --- a/pytorch_lightning/utilities/cloud_io.py +++ b/pytorch_lightning/utilities/cloud_io.py @@ -25,14 +25,16 @@ class LightningLocalFileSystem(LocalFileSystem): - def isdir(self, path): + """Extension of ``fsspec.implementations.local.LocalFileSystem`` which follows symbolic links so that + ``LightningLocalFileSystem.isdir`` behaves the same as ``os.isdir``. + """ + + def isdir(self, path: str) -> bool: try: - if self.info(path)["type"] == "directory": - return True - elif self.info(path)["type"] == "link": - return os.path.isdir(path) + if self.info(path)["type"] == "link": + return os.path.isdir(path) # follows symlinks else: - return False + return super().isdir(path) except IOError: return False From 747fe41a9d6eab12b5f2755ec8ff04268f16defb Mon Sep 17 00:00:00 2001 From: Ethan Harris Date: Tue, 30 Mar 2021 12:16:46 +0100 Subject: [PATCH 3/6] Update CHANGELOG.md --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) 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 From cf1d339dc7abf725dfbe493564e0e662bf437104 Mon Sep 17 00:00:00 2001 From: Ethan Harris Date: Tue, 30 Mar 2021 12:24:36 +0100 Subject: [PATCH 4/6] Simplify --- pytorch_lightning/utilities/cloud_io.py | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/pytorch_lightning/utilities/cloud_io.py b/pytorch_lightning/utilities/cloud_io.py index 865a6f349172b..6af7bf887fb14 100644 --- a/pytorch_lightning/utilities/cloud_io.py +++ b/pytorch_lightning/utilities/cloud_io.py @@ -25,18 +25,12 @@ class LightningLocalFileSystem(LocalFileSystem): - """Extension of ``fsspec.implementations.local.LocalFileSystem`` which follows symbolic links so that - ``LightningLocalFileSystem.isdir`` behaves the same as ``os.isdir``. + """Extension of ``fsspec.implementations.local.LocalFileSystem`` where ``LightningLocalFileSystem.isdir`` behaves + the same as ``os.isdir``. """ def isdir(self, path: str) -> bool: - try: - if self.info(path)["type"] == "link": - return os.path.isdir(path) # follows symlinks - else: - return super().isdir(path) - except IOError: - return False + return os.path.isdir(path) # follows symlinks def load(path_or_url: Union[str, IO, Path], map_location=None): From 252ef2642f13543c0b849378d1a34f6285fc488b Mon Sep 17 00:00:00 2001 From: Ethan Harris Date: Tue, 6 Apr 2021 07:49:57 +0100 Subject: [PATCH 5/6] Update pytorch_lightning/utilities/cloud_io.py MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Carlos Mocholí --- pytorch_lightning/utilities/cloud_io.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pytorch_lightning/utilities/cloud_io.py b/pytorch_lightning/utilities/cloud_io.py index 6af7bf887fb14..98041452fe92e 100644 --- a/pytorch_lightning/utilities/cloud_io.py +++ b/pytorch_lightning/utilities/cloud_io.py @@ -27,6 +27,8 @@ 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: From 401c34434e8232f739649a44f7ee0350d53c7593 Mon Sep 17 00:00:00 2001 From: Ethan Harris Date: Tue, 6 Apr 2021 07:52:33 +0100 Subject: [PATCH 6/6] Make `LightningLocalFileSystem` protected --- pytorch_lightning/utilities/cloud_io.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pytorch_lightning/utilities/cloud_io.py b/pytorch_lightning/utilities/cloud_io.py index 98041452fe92e..c179d0d0d0bf8 100644 --- a/pytorch_lightning/utilities/cloud_io.py +++ b/pytorch_lightning/utilities/cloud_io.py @@ -24,7 +24,7 @@ import torch -class LightningLocalFileSystem(LocalFileSystem): +class _LightningLocalFileSystem(LocalFileSystem): """Extension of ``fsspec.implementations.local.LocalFileSystem`` where ``LightningLocalFileSystem.isdir`` behaves the same as ``os.isdir``. @@ -53,7 +53,7 @@ def get_filesystem(path: Union[str, Path]): return fsspec.filesystem(path.split(":", 1)[0]) else: # use local filesystem - return LightningLocalFileSystem() + return _LightningLocalFileSystem() def atomic_save(checkpoint, filepath: str):