From 30fa3fc41bd6457816cebf02c5e642b473f278f4 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Fri, 18 Feb 2022 14:26:36 +0200 Subject: [PATCH] fs: walk: get rid of onerror Makes dvc/repofs and other filesystems comply with fsspec. --- dvc/fs/base.py | 2 +- dvc/fs/dvc.py | 6 +----- dvc/fs/fsspec_wrapper.py | 14 ++------------ dvc/fs/local.py | 5 +++-- dvc/fs/repo.py | 20 ++++---------------- dvc/repo/ls.py | 21 +++++++-------------- tests/unit/fs/test_dvc.py | 21 +++++++-------------- tests/unit/fs/test_repo.py | 21 +++++++-------------- 8 files changed, 32 insertions(+), 78 deletions(-) diff --git a/dvc/fs/base.py b/dvc/fs/base.py index 3e7e6dfe0..7826cbcf7 100644 --- a/dvc/fs/base.py +++ b/dvc/fs/base.py @@ -154,7 +154,7 @@ class FileSystem: """Check if this file is an independent copy.""" return False # We can't be sure by default - def walk(self, top, topdown=True, onerror=None, **kwargs): + def walk(self, top, topdown=True, **kwargs): """Return a generator with (root, dirs, files).""" raise NotImplementedError diff --git a/dvc/fs/dvc.py b/dvc/fs/dvc.py index 1e85314c4..1a0b8d674 100644 --- a/dvc/fs/dvc.py +++ b/dvc/fs/dvc.py @@ -132,18 +132,14 @@ class DvcFileSystem(FileSystem): # pylint:disable=abstract-method for dname in dirs: yield from self._walk(self.path.join(root, dname)) - def walk(self, top, topdown=True, onerror=None, **kwargs): + def walk(self, top, topdown=True, **kwargs): assert topdown try: info = self.info(top) except FileNotFoundError: - if onerror is not None: - onerror(FileNotFoundError(top)) return if info["type"] != "directory": - if onerror is not None: - onerror(NotADirectoryError(top)) return yield from self._walk(top, topdown=topdown, **kwargs) diff --git a/dvc/fs/fsspec_wrapper.py b/dvc/fs/fsspec_wrapper.py index 554908e7a..85efea57d 100644 --- a/dvc/fs/fsspec_wrapper.py +++ b/dvc/fs/fsspec_wrapper.py @@ -1,15 +1,6 @@ import os import shutil -from typing import ( - IO, - TYPE_CHECKING, - Any, - Callable, - Dict, - Iterator, - Optional, - overload, -) +from typing import IO, TYPE_CHECKING, Any, Dict, Iterator, Optional, overload from funcy import cached_property from tqdm.utils import CallbackIOWrapper @@ -149,10 +140,9 @@ class FSSpecWrapper(FileSystem): self, top: AnyFSPath, topdown: bool = True, - onerror: Callable[[OSError], None] = None, **kwargs: Any, ): - return self.fs.walk(top, topdown=topdown, onerror=onerror, **kwargs) + return self.fs.walk(top, topdown=topdown, **kwargs) # pylint: disable=abstract-method diff --git a/dvc/fs/local.py b/dvc/fs/local.py index 7024aed2a..8bbfa2123 100644 --- a/dvc/fs/local.py +++ b/dvc/fs/local.py @@ -51,14 +51,15 @@ class LocalFileSystem(FileSystem): def iscopy(self, path): return not (System.is_symlink(path) or System.is_hardlink(path)) - def walk(self, top, topdown=True, onerror=None, **kwargs): + def walk(self, top, topdown=True, **kwargs): """Directory fs generator. See `os.walk` for the docs. Differences: - no support for symlinks """ for root, dirs, files in os.walk( - top, topdown=topdown, onerror=onerror + top, + topdown=topdown, ): yield os.path.normpath(root), dirs, files diff --git a/dvc/fs/repo.py b/dvc/fs/repo.py index 28d43adc6..ba4258a6d 100644 --- a/dvc/fs/repo.py +++ b/dvc/fs/repo.py @@ -404,14 +404,12 @@ class RepoFileSystem(FileSystem): # pylint:disable=abstract-method elif dirname in repo_set: yield from self._walk(repo_walk, None, dvcfiles=dvcfiles) - def walk(self, top, topdown=True, onerror=None, **kwargs): + def walk(self, top, topdown=True, **kwargs): """Walk and merge both DVC and repo fss. Args: top: path to walk from topdown: if True, fs will be walked from top down. - onerror: if set, onerror function will be called if an error - occurs (by default errors are ignored). dvcfiles: if True, dvcfiles will be included in the files list for walked directories. @@ -421,13 +419,9 @@ class RepoFileSystem(FileSystem): # pylint:disable=abstract-method assert topdown if not self.exists(top): - if onerror is not None: - onerror(FileNotFoundError(top)) return if not self.isdir(top): - if onerror is not None: - onerror(NotADirectoryError(top)) return repo = self._get_repo(os.path.abspath(top)) @@ -436,24 +430,18 @@ class RepoFileSystem(FileSystem): # pylint:disable=abstract-method fs, dvc_fs, dvc_path = self._get_fs_pair(top) repo_exists = fs.exists(top) - repo_walk = repo.dvcignore.walk( - fs, top, topdown=topdown, onerror=onerror, **kwargs - ) + repo_walk = repo.dvcignore.walk(fs, top, topdown=topdown, **kwargs) if not dvc_fs or (repo_exists and dvc_fs.isdvc(dvc_path)): yield from self._walk(repo_walk, None, dvcfiles=dvcfiles) return if not repo_exists: - yield from _wrap_walk( - dvc_fs, dvc_path, topdown=topdown, onerror=onerror, **kwargs - ) + yield from _wrap_walk(dvc_fs, dvc_path, topdown=topdown, **kwargs) dvc_walk = None if dvc_fs.exists(dvc_path): - dvc_walk = _wrap_walk( - dvc_fs, dvc_path, topdown=topdown, onerror=onerror, **kwargs - ) + dvc_walk = _wrap_walk(dvc_fs, dvc_path, topdown=topdown, **kwargs) yield from self._walk(repo_walk, dvc_walk, dvcfiles=dvcfiles) diff --git a/dvc/repo/ls.py b/dvc/repo/ls.py index 615d1a7f9..1f7672020 100644 --- a/dvc/repo/ls.py +++ b/dvc/repo/ls.py @@ -47,23 +47,16 @@ def ls(url, path=None, rev=None, recursive=None, dvc_only=False): def _ls(repo, fs_path, recursive=None, dvc_only=False): - def onerror(exc): - raise exc - fs = repo.repo_fs infos = [] - try: - for root, dirs, files in fs.walk( - fs_path, onerror=onerror, dvcfiles=True - ): - entries = chain(files, dirs) if not recursive else files - infos.extend(fs.path.join(root, entry) for entry in entries) - if not recursive: - break - except NotADirectoryError: + for root, dirs, files in fs.walk(fs_path, dvcfiles=True): + entries = chain(files, dirs) if not recursive else files + infos.extend(fs.path.join(root, entry) for entry in entries) + if not recursive: + break + + if not infos and fs.isfile(fs_path): infos.append(fs_path) - except FileNotFoundError: - return {} ret = {} for info in infos: diff --git a/tests/unit/fs/test_dvc.py b/tests/unit/fs/test_dvc.py index a90791e8c..a183eb0c1 100644 --- a/tests/unit/fs/test_dvc.py +++ b/tests/unit/fs/test_dvc.py @@ -181,26 +181,19 @@ def test_walk_dir(tmp_dir, dvc): assert len(actual) == len(expected) -def test_walk_onerror(tmp_dir, dvc): - def onerror(exc): - raise exc +def test_walk_missing(tmp_dir, dvc): + fs = DvcFileSystem(repo=dvc) + for _ in fs.walk("dir"): + pass + + +def test_walk_not_a_dir(tmp_dir, dvc): tmp_dir.dvc_gen("foo", "foo") fs = DvcFileSystem(repo=dvc) - # path does not exist - for _ in fs.walk("dir"): - pass - with pytest.raises(OSError): - for _ in fs.walk("dir", onerror=onerror): - pass - - # path is not a directory for _ in fs.walk("foo"): pass - with pytest.raises(OSError): - for _ in fs.walk("foo", onerror=onerror): - pass def test_isdvc(tmp_dir, dvc): diff --git a/tests/unit/fs/test_repo.py b/tests/unit/fs/test_repo.py index c7dbb1149..24b241fcf 100644 --- a/tests/unit/fs/test_repo.py +++ b/tests/unit/fs/test_repo.py @@ -291,26 +291,19 @@ def test_walk_mixed_dir(tmp_dir, scm, dvc): assert len(actual) == len(expected) -def test_walk_onerror(tmp_dir, dvc): - def onerror(exc): - raise exc +def test_walk_missing(tmp_dir, dvc): + fs = RepoFileSystem(repo=dvc) + for _ in fs.walk("dir"): + pass + + +def test_walk_not_a_dir(tmp_dir, dvc): tmp_dir.dvc_gen("foo", "foo") fs = RepoFileSystem(repo=dvc) - # path does not exist - for _ in fs.walk("dir"): - pass - with pytest.raises(OSError): - for _ in fs.walk("dir", onerror=onerror): - pass - - # path is not a directory for _ in fs.walk("foo"): pass - with pytest.raises(OSError): - for _ in fs.walk("foo", onerror=onerror): - pass def test_isdvc(tmp_dir, dvc):