From 11b0581536a0b2062bdac19fa8d399cc277dcc55 Mon Sep 17 00:00:00 2001 From: Batuhan Taskaya Date: Fri, 12 Mar 2021 19:11:08 +0300 Subject: [PATCH] import-url: support importing empty directories on object storage services (#5536) * import-url: support importing empty directories on object storage services Empty directories are basically files with a trailing slash at the end of their names, with empty contents on object service providers (such as azure, google storage). This patch implements the `is_empty()` method for the `FSSpecWrapper` to determining whether a path is empty (directory or a file) and also tweaks `.isdir()` in order to see empty directories. The base class is only used by azure and gcsfs for now, but if we go on the route of migrating normal providers (like proper filesystems, not just object storages), we might need to do these tweaks on a subclass (like `ObjectStorageFSSpecWrapper` etc), but I don't think it is neccesary as of now. The `is_empty` method is already there, and also used in the importing part to warn users that they are importing something empty (https://github.com/iterative/dvc/blob/e385e59a098cd661ea1ea4257c9a5735c5c32bc6/dvc/output/base.py#L252-L254) so this is not a new method. Just for the reference, as far as I can locate there is no variant of this method in the fsspec. * tests: revert custom mkdir for cloud storages * tests: allign assingments * add some comments --- dvc/fs/base.py | 3 +++ dvc/fs/fsspec_wrapper.py | 31 ++++++++++++++++++++++++++-- tests/func/test_import_url.py | 39 +++++++++++++++++++++++++++++++++++ tests/remotes/azure.py | 1 + tests/remotes/base.py | 3 +++ tests/remotes/gs.py | 3 +++ tests/remotes/oss.py | 3 +++ tests/remotes/s3.py | 3 +++ 8 files changed, 84 insertions(+), 2 deletions(-) diff --git a/dvc/fs/base.py b/dvc/fs/base.py index 9ffbe69a4..891207bdb 100644 --- a/dvc/fs/base.py +++ b/dvc/fs/base.py @@ -283,6 +283,9 @@ class BaseFileSystem: self, from_info, to_info, name, no_progress_bar, jobs, **kwargs, ): from_infos = list(self.walk_files(from_info, **kwargs)) + if not from_infos: + makedirs(to_info, exist_ok=True) + return None to_infos = ( to_info / info.relative_to(from_info) for info in from_infos ) diff --git a/dvc/fs/fsspec_wrapper.py b/dvc/fs/fsspec_wrapper.py index b4c730ce7..1550d69ec 100644 --- a/dvc/fs/fsspec_wrapper.py +++ b/dvc/fs/fsspec_wrapper.py @@ -1,5 +1,6 @@ import os import shutil +from functools import lru_cache from dvc.progress import Tqdm @@ -11,6 +12,7 @@ class FSSpecWrapper(BaseFileSystem): def fs(self): raise NotImplementedError + @lru_cache(512) def _with_bucket(self, path): if isinstance(path, self.PATH_CLS): return f"{path.bucket}/{path.path}" @@ -37,10 +39,32 @@ class FSSpecWrapper(BaseFileSystem): return entry def isdir(self, path_info): - return self.fs.isdir(self._with_bucket(path_info)) + if not self.exists(path_info): + return False + + # Directory in object storages are interpreted differently + # among different fsspec providers, so this logic is a temporary + # measure for us to adapt as of now. It checks whether it is a + # directory (as in a prefix with contents) or whether it is an empty + # file where it's name ends with a forward slash + entry = self.info(path_info) + return entry["type"] == "directory" or ( + entry["size"] == 0 + and entry["type"] == "file" + and entry["name"].endswith("/") + ) def isfile(self, path_info): - return self.fs.isfile(self._with_bucket(path_info)) + if not self.exists(path_info): + return False + + return not self.isdir(path_info) + + def is_empty(self, path_info): + entry = self.info(path_info) + if entry["type"] == "directory": + return not self.fs.ls(self._with_bucket(path_info)) + return entry["size"] == 0 def open( self, path_info, mode="r", **kwargs @@ -56,6 +80,9 @@ class FSSpecWrapper(BaseFileSystem): def ls( self, path_info, detail=False, recursive=False ): # pylint: disable=arguments-differ + if self.isdir(path_info) and self.is_empty(path_info): + return None + path = self._with_bucket(path_info) if recursive: for root, _, files in self.fs.walk(path, detail=detail): diff --git a/tests/func/test_import_url.py b/tests/func/test_import_url.py index 738b05521..28d03ab23 100644 --- a/tests/func/test_import_url.py +++ b/tests/func/test_import_url.py @@ -353,3 +353,42 @@ def test_import_url_to_remote_absolute( def test_import_url_to_remote_invalid_combinations(dvc): with pytest.raises(InvalidArgumentError, match="--no-exec"): dvc.imp_url("s3://bucket/foo", no_exec=True, to_remote=True) + + +empty_xfail = pytest.mark.xfail( + reason="https://github.com/iterative/dvc/issues/5521" +) + + +@pytest.mark.parametrize( + "workspace", + [ + pytest.lazy_fixture("s3"), + pytest.lazy_fixture("hdfs"), + pytest.param(pytest.lazy_fixture("gs"), marks=empty_xfail), + pytest.param(pytest.lazy_fixture("azure"), marks=empty_xfail), + pytest.param( + pytest.lazy_fixture("ssh"), + marks=pytest.mark.skipif( + os.name == "nt", reason="disabled on windows" + ), + ), + ], + indirect=True, +) +def test_import_url_empty_directory(tmp_dir, dvc, workspace): + # prefix based storage services (e.g s3) doesn't have the real concept + # of directories. So instead we create an empty file that ends with a + # trailing slash in order to actually support this operation + if workspace.IS_OBJECT_STORAGE: + contents = "" + else: + contents = {} + + workspace.gen({"empty_dir/": contents}) + + dvc.imp_url("remote://workspace/empty_dir/") + + empty_dir = tmp_dir / "empty_dir" + assert empty_dir.is_dir() + assert tuple(empty_dir.iterdir()) == () diff --git a/tests/remotes/azure.py b/tests/remotes/azure.py index 14306a472..c288302a5 100644 --- a/tests/remotes/azure.py +++ b/tests/remotes/azure.py @@ -22,6 +22,7 @@ TEST_AZURE_CONNECTION_STRING = ( class Azure(Base, CloudURLInfo): + IS_OBJECT_STORAGE = True CONNECTION_STRING = None @cached_property diff --git a/tests/remotes/base.py b/tests/remotes/base.py index 704503f3f..ea5fb79bb 100644 --- a/tests/remotes/base.py +++ b/tests/remotes/base.py @@ -7,6 +7,9 @@ from dvc.path_info import URLInfo class Base(URLInfo): + + IS_OBJECT_STORAGE = False + def is_file(self): raise NotImplementedError diff --git a/tests/remotes/gs.py b/tests/remotes/gs.py index 0099ccc84..0418fc94e 100644 --- a/tests/remotes/gs.py +++ b/tests/remotes/gs.py @@ -23,6 +23,9 @@ os.environ["GOOGLE_APPLICATION_CREDENTIALS"] = TEST_GCP_CREDS_FILE class GCP(Base, CloudURLInfo): + + IS_OBJECT_STORAGE = True + @staticmethod def should_test(): do_test = env2bool("DVC_TEST_GCP", undefined=None) diff --git a/tests/remotes/oss.py b/tests/remotes/oss.py index 43fbb4a77..e84f3948c 100644 --- a/tests/remotes/oss.py +++ b/tests/remotes/oss.py @@ -16,6 +16,9 @@ EMULATOR_OSS_ACCESS_KEY_SECRET = "AccessKeySecret" class OSS(Base, CloudURLInfo): + + IS_OBJECT_STORAGE = True + @staticmethod def get_url(): return f"oss://{TEST_OSS_REPO_BUCKET}/{uuid.uuid4()}" diff --git a/tests/remotes/s3.py b/tests/remotes/s3.py index 22f8c9a3d..b0517c221 100644 --- a/tests/remotes/s3.py +++ b/tests/remotes/s3.py @@ -15,6 +15,9 @@ TEST_AWS_REPO_BUCKET = os.environ.get("DVC_TEST_AWS_REPO_BUCKET", "dvc-temp") class S3(Base, CloudURLInfo): + + IS_OBJECT_STORAGE = True + @staticmethod def should_test(): do_test = env2bool("DVC_TEST_AWS", undefined=None)