From 5696d3a2cf95fa9327fc1a9be9fa95f4c3c38287 Mon Sep 17 00:00:00 2001 From: "Michael D. Smith" Date: Tue, 27 Aug 2024 18:40:04 -0400 Subject: [PATCH 1/9] first stab at caching metadata --- cloudpathlib/s3/s3client.py | 53 ++++++++++++++++++++++++++++++++++--- 1 file changed, 49 insertions(+), 4 deletions(-) diff --git a/cloudpathlib/s3/s3client.py b/cloudpathlib/s3/s3client.py index 9c1b5658..6766865e 100644 --- a/cloudpathlib/s3/s3client.py +++ b/cloudpathlib/s3/s3client.py @@ -1,7 +1,19 @@ +import dataclasses import mimetypes import os from pathlib import Path, PurePosixPath -from typing import Any, Callable, Dict, Iterable, Optional, Tuple, Union +from typing import ( + Any, + Callable, + Dict, + Iterable, + Optional, + TYPE_CHECKING, + Tuple, + Union, + MutableMapping, +) +from weakref import WeakKeyDictionary from ..client import Client, register_client_class from ..cloudpath import implementation_registry @@ -18,6 +30,10 @@ except ModuleNotFoundError: implementation_registry["s3"].dependencies_loaded = False +@dataclasses.dataclass +class PathMetadata: + is_file_or_dir: Optional[str] + @register_client_class("s3") class S3Client(Client): @@ -126,6 +142,8 @@ def __init__( for k in ["RequestPayer", "ExpectedBucketOwner"] if k in self._extra_args } + + self._metadata_cache: MutableMapping[S3Path, PathMetadata] = WeakKeyDictionary() super().__init__( local_cache_dir=local_cache_dir, @@ -138,7 +156,7 @@ def _get_metadata(self, cloud_path: S3Path) -> Dict[str, Any]: data = self.s3.ObjectSummary(cloud_path.bucket, cloud_path.key).get( **self.boto3_dl_extra_args ) - + self._set_metadata_cache(cloud_path, "file") return { "last_modified": data["LastModified"], "size": data["ContentLength"], @@ -252,8 +270,10 @@ def _list_dir(self, cloud_path: S3Path, recursive=False) -> Iterable[Tuple[S3Pat for parent in PurePosixPath(o_relative_path).parents: parent_canonical = prefix + str(parent).rstrip("/") if parent_canonical not in yielded_dirs and str(parent) != ".": + path = self.CloudPath(f"s3://{cloud_path.bucket}/{parent_canonical}") + self._set_metadata_cache(path, "dir") yield ( - self.CloudPath(f"s3://{cloud_path.bucket}/{parent_canonical}"), + path, True, ) yielded_dirs.add(parent_canonical) @@ -265,8 +285,10 @@ def _list_dir(self, cloud_path: S3Path, recursive=False) -> Iterable[Tuple[S3Pat # s3 fake directories have 0 size and end with "/" if result_key.get("Key").endswith("/") and result_key.get("Size") == 0: + path = self.CloudPath(f"s3://{cloud_path.bucket}/{canonical}") + self._set_metadata_cache(path, "file") yield ( - self.CloudPath(f"s3://{cloud_path.bucket}/{canonical}"), + path, True, ) yielded_dirs.add(canonical) @@ -298,12 +320,14 @@ def _move_file(self, src: S3Path, dst: S3Path, remove_src: bool = True) -> S3Pat ) if remove_src: + self._set_metadata_cache(src, None) self._remove(src) return dst def _remove(self, cloud_path: S3Path, missing_ok: bool = True) -> None: file_or_dir = self._is_file_or_dir(cloud_path=cloud_path) if file_or_dir == "file": + self._set_metadata_cache(cloud_path, None) resp = self.s3.Object(cloud_path.bucket, cloud_path.key).delete( **self.boto3_list_extra_args ) @@ -323,6 +347,13 @@ def _remove(self, cloud_path: S3Path, missing_ok: bool = True) -> None: resp = bucket.objects.filter(Prefix=prefix, **self.boto3_list_extra_args).delete( **self.boto3_list_extra_args ) + + files = [ + path for path, is_dir in self._list_dir(cloud_path, recursive=True) if not is_dir + ] + for path in files: + self._set_metadata_cache(path, None) + if resp[0].get("ResponseMetadata").get("HTTPStatusCode") not in (204, 200): raise CloudPathException( f"Delete operation failed for {cloud_path} with response: {resp}" @@ -348,6 +379,20 @@ def _upload_file(self, local_path: Union[str, os.PathLike], cloud_path: S3Path) obj.upload_file(str(local_path), Config=self.boto3_transfer_config, ExtraArgs=extra_args) return cloud_path + + def _set_metadata_cache(self, cloud_path: S3Path, is_file_or_dir: Optional[str]) -> None: + if is_file_or_dir is None: + self._metadata_cache[cloud_path] = PathMetadata(is_file_or_dir=is_file_or_dir) + # If a file/dir is now known to not exist, its parent directories may no longer exist + # either, since cloud directories only exist if they have a file in them. Since their + # state is no longer known we remove them from the cache. + for parent in cloud_path.parents: + if parent in self._metadata_cache: + del self._metadata_cache[parent] + else: + self._metadata_cache[cloud_path] = PathMetadata(is_file_or_dir=is_file_or_dir) + def clear_metadata_cache(self) -> None: + self._metadata_cache.clear() S3Client.S3Path = S3Client.CloudPath # type: ignore From 19a52c3468036988c8a1ec41f45988905b76dc1b Mon Sep 17 00:00:00 2001 From: "Michael D. Smith" Date: Thu, 29 Aug 2024 06:34:44 -0400 Subject: [PATCH 2/9] more work --- cloudpathlib/local/implementations/s3.py | 2 + cloudpathlib/s3/s3client.py | 47 +++++++++++++++++------- 2 files changed, 35 insertions(+), 14 deletions(-) diff --git a/cloudpathlib/local/implementations/s3.py b/cloudpathlib/local/implementations/s3.py index df9951bf..09f9a06b 100644 --- a/cloudpathlib/local/implementations/s3.py +++ b/cloudpathlib/local/implementations/s3.py @@ -14,6 +14,8 @@ class LocalS3Client(LocalClient): _cloud_meta = local_s3_implementation + def clear_metadata_cache(self): + pass LocalS3Client.S3Path = LocalS3Client.CloudPath # type: ignore diff --git a/cloudpathlib/s3/s3client.py b/cloudpathlib/s3/s3client.py index 6766865e..4ef3d750 100644 --- a/cloudpathlib/s3/s3client.py +++ b/cloudpathlib/s3/s3client.py @@ -33,6 +33,9 @@ @dataclasses.dataclass class PathMetadata: is_file_or_dir: Optional[str] + etag: Optional[str] + size: Optional[int] + last_modified: Optional[str] @register_client_class("s3") @@ -153,17 +156,29 @@ def __init__( def _get_metadata(self, cloud_path: S3Path) -> Dict[str, Any]: # get accepts all download extra args - data = self.s3.ObjectSummary(cloud_path.bucket, cloud_path.key).get( - **self.boto3_dl_extra_args - ) - self._set_metadata_cache(cloud_path, "file") - return { - "last_modified": data["LastModified"], - "size": data["ContentLength"], - "etag": data["ETag"], - "content_type": data.get("ContentType", None), - "extra": data["Metadata"], - } + results = None + path = f"s3://{cloud_path.bucket}/{cloud_path.key}" + size = self._metadata_cache[cloud_path].size + if size: + return { + "last_modified": self._metadata_cache[cloud_path].lastmodified, + "size": size, + "etag": self._metadata_cache[cloud_path].etag, + "content_type": None, + "extra": None, + } + else: + data = self.s3.ObjectSummary(cloud_path.bucket, cloud_path.key).get( + **self.boto3_dl_extra_args + ) + self._set_metadata_cache(path, "file", data["ETag"], data["ContentLength"], data["LastModified"]) + return { + "last_modified": data["LastModified"], + "size": data["ContentLength"], + "etag": data["ETag"], + "content_type": data.get("ContentType", None), + "extra": data["Metadata"], + } def _download_file(self, cloud_path: S3Path, local_path: Union[str, os.PathLike]) -> Path: local_path = Path(local_path) @@ -267,11 +282,14 @@ def _list_dir(self, cloud_path: S3Path, recursive=False) -> Iterable[Tuple[S3Pat for result_key in result.get("Contents", []): # yield all the parents of any key that have not been yielded already o_relative_path = result_key.get("Key")[len(prefix) :] + etag = result_key.get("ETag") + size = result_key.get("Size") + last_modified = result_key.get("LastModified") for parent in PurePosixPath(o_relative_path).parents: parent_canonical = prefix + str(parent).rstrip("/") if parent_canonical not in yielded_dirs and str(parent) != ".": path = self.CloudPath(f"s3://{cloud_path.bucket}/{parent_canonical}") - self._set_metadata_cache(path, "dir") + self._set_metadata_cache(path, "dir", etag, size, last_modified) yield ( path, True, @@ -286,7 +304,7 @@ def _list_dir(self, cloud_path: S3Path, recursive=False) -> Iterable[Tuple[S3Pat # s3 fake directories have 0 size and end with "/" if result_key.get("Key").endswith("/") and result_key.get("Size") == 0: path = self.CloudPath(f"s3://{cloud_path.bucket}/{canonical}") - self._set_metadata_cache(path, "file") + self._set_metadata_cache(path, "dir", etag, size, last_modified) yield ( path, True, @@ -295,6 +313,7 @@ def _list_dir(self, cloud_path: S3Path, recursive=False) -> Iterable[Tuple[S3Pat # yield object as file else: + self._set_metadata_cache(path, "file", etag, size, last_modified) yield ( self.CloudPath(f"s3://{cloud_path.bucket}/{result_key.get('Key')}"), False, @@ -391,7 +410,7 @@ def _set_metadata_cache(self, cloud_path: S3Path, is_file_or_dir: Optional[str]) del self._metadata_cache[parent] else: self._metadata_cache[cloud_path] = PathMetadata(is_file_or_dir=is_file_or_dir) - + def clear_metadata_cache(self) -> None: self._metadata_cache.clear() From 3de9f86332f032ac5681553fba5e0f423b2d7cf4 Mon Sep 17 00:00:00 2001 From: "Michael D. Smith" Date: Thu, 29 Aug 2024 06:41:05 -0400 Subject: [PATCH 3/9] update variable --- cloudpathlib/s3/s3client.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cloudpathlib/s3/s3client.py b/cloudpathlib/s3/s3client.py index 4ef3d750..2f13dd38 100644 --- a/cloudpathlib/s3/s3client.py +++ b/cloudpathlib/s3/s3client.py @@ -156,7 +156,7 @@ def __init__( def _get_metadata(self, cloud_path: S3Path) -> Dict[str, Any]: # get accepts all download extra args - results = None + size = None path = f"s3://{cloud_path.bucket}/{cloud_path.key}" size = self._metadata_cache[cloud_path].size if size: @@ -313,9 +313,10 @@ def _list_dir(self, cloud_path: S3Path, recursive=False) -> Iterable[Tuple[S3Pat # yield object as file else: + path = self.CloudPath(f"s3://{cloud_path.bucket}/{result_key.get('Key')}") self._set_metadata_cache(path, "file", etag, size, last_modified) yield ( - self.CloudPath(f"s3://{cloud_path.bucket}/{result_key.get('Key')}"), + path, False, ) From 03c5d34f1167c90462c2c285357d2322508f59a1 Mon Sep 17 00:00:00 2001 From: "Michael D. Smith" Date: Thu, 29 Aug 2024 06:48:05 -0400 Subject: [PATCH 4/9] fixes --- cloudpathlib/s3/s3client.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/cloudpathlib/s3/s3client.py b/cloudpathlib/s3/s3client.py index 2f13dd38..b867cc00 100644 --- a/cloudpathlib/s3/s3client.py +++ b/cloudpathlib/s3/s3client.py @@ -340,14 +340,14 @@ def _move_file(self, src: S3Path, dst: S3Path, remove_src: bool = True) -> S3Pat ) if remove_src: - self._set_metadata_cache(src, None) + self._set_metadata_cache(src, None, None, None, None) self._remove(src) return dst def _remove(self, cloud_path: S3Path, missing_ok: bool = True) -> None: file_or_dir = self._is_file_or_dir(cloud_path=cloud_path) if file_or_dir == "file": - self._set_metadata_cache(cloud_path, None) + self._set_metadata_cache(cloud_path, None, None, None, None) resp = self.s3.Object(cloud_path.bucket, cloud_path.key).delete( **self.boto3_list_extra_args ) @@ -372,7 +372,7 @@ def _remove(self, cloud_path: S3Path, missing_ok: bool = True) -> None: path for path, is_dir in self._list_dir(cloud_path, recursive=True) if not is_dir ] for path in files: - self._set_metadata_cache(path, None) + self._set_metadata_cache(path, None, None, None, None) if resp[0].get("ResponseMetadata").get("HTTPStatusCode") not in (204, 200): raise CloudPathException( @@ -400,9 +400,11 @@ def _upload_file(self, local_path: Union[str, os.PathLike], cloud_path: S3Path) obj.upload_file(str(local_path), Config=self.boto3_transfer_config, ExtraArgs=extra_args) return cloud_path - def _set_metadata_cache(self, cloud_path: S3Path, is_file_or_dir: Optional[str]) -> None: + def _set_metadata_cache(self, cloud_path: S3Path, is_file_or_dir: Optional[str], + etag: Optional[str], size: Optional[int], lastmodified: Optional[str]) -> None: if is_file_or_dir is None: - self._metadata_cache[cloud_path] = PathMetadata(is_file_or_dir=is_file_or_dir) + self._metadata_cache[cloud_path] = PathMetadata(is_file_or_dir=is_file_or_dir, + etag=etag, size=size, last_modified=lastmodified) # If a file/dir is now known to not exist, its parent directories may no longer exist # either, since cloud directories only exist if they have a file in them. Since their # state is no longer known we remove them from the cache. @@ -410,7 +412,8 @@ def _set_metadata_cache(self, cloud_path: S3Path, is_file_or_dir: Optional[str]) if parent in self._metadata_cache: del self._metadata_cache[parent] else: - self._metadata_cache[cloud_path] = PathMetadata(is_file_or_dir=is_file_or_dir) + self._metadata_cache[cloud_path] = PathMetadata(is_file_or_dir=is_file_or_dir, + etag=etag, size=size, last_modified=lastmodified) def clear_metadata_cache(self) -> None: self._metadata_cache.clear() From d020b0ef9724068eb2668f8f2ab98bee19d380c8 Mon Sep 17 00:00:00 2001 From: "Michael D. Smith" Date: Thu, 29 Aug 2024 07:10:34 -0400 Subject: [PATCH 5/9] fix cloud path references --- cloudpathlib/s3/s3client.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/cloudpathlib/s3/s3client.py b/cloudpathlib/s3/s3client.py index b867cc00..cc1a7426 100644 --- a/cloudpathlib/s3/s3client.py +++ b/cloudpathlib/s3/s3client.py @@ -157,7 +157,6 @@ def __init__( def _get_metadata(self, cloud_path: S3Path) -> Dict[str, Any]: # get accepts all download extra args size = None - path = f"s3://{cloud_path.bucket}/{cloud_path.key}" size = self._metadata_cache[cloud_path].size if size: return { @@ -171,7 +170,7 @@ def _get_metadata(self, cloud_path: S3Path) -> Dict[str, Any]: data = self.s3.ObjectSummary(cloud_path.bucket, cloud_path.key).get( **self.boto3_dl_extra_args ) - self._set_metadata_cache(path, "file", data["ETag"], data["ContentLength"], data["LastModified"]) + self._set_metadata_cache(cloud_path, "file", data["ETag"], data["ContentLength"], data["LastModified"]) return { "last_modified": data["LastModified"], "size": data["ContentLength"], @@ -289,7 +288,7 @@ def _list_dir(self, cloud_path: S3Path, recursive=False) -> Iterable[Tuple[S3Pat parent_canonical = prefix + str(parent).rstrip("/") if parent_canonical not in yielded_dirs and str(parent) != ".": path = self.CloudPath(f"s3://{cloud_path.bucket}/{parent_canonical}") - self._set_metadata_cache(path, "dir", etag, size, last_modified) + self._set_metadata_cache(cloud_path, "dir", etag, size, last_modified) yield ( path, True, @@ -304,7 +303,7 @@ def _list_dir(self, cloud_path: S3Path, recursive=False) -> Iterable[Tuple[S3Pat # s3 fake directories have 0 size and end with "/" if result_key.get("Key").endswith("/") and result_key.get("Size") == 0: path = self.CloudPath(f"s3://{cloud_path.bucket}/{canonical}") - self._set_metadata_cache(path, "dir", etag, size, last_modified) + self._set_metadata_cache(cloud_path, "dir", etag, size, last_modified) yield ( path, True, @@ -314,7 +313,7 @@ def _list_dir(self, cloud_path: S3Path, recursive=False) -> Iterable[Tuple[S3Pat # yield object as file else: path = self.CloudPath(f"s3://{cloud_path.bucket}/{result_key.get('Key')}") - self._set_metadata_cache(path, "file", etag, size, last_modified) + self._set_metadata_cache(cloud_path, "file", etag, size, last_modified) yield ( path, False, From 4214a5013a8c62a466459053dd9838f06f34e5b9 Mon Sep 17 00:00:00 2001 From: "Michael D. Smith" Date: Thu, 29 Aug 2024 10:17:44 -0400 Subject: [PATCH 6/9] need full path for key --- cloudpathlib/s3/s3client.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cloudpathlib/s3/s3client.py b/cloudpathlib/s3/s3client.py index cc1a7426..2b18507b 100644 --- a/cloudpathlib/s3/s3client.py +++ b/cloudpathlib/s3/s3client.py @@ -288,7 +288,7 @@ def _list_dir(self, cloud_path: S3Path, recursive=False) -> Iterable[Tuple[S3Pat parent_canonical = prefix + str(parent).rstrip("/") if parent_canonical not in yielded_dirs and str(parent) != ".": path = self.CloudPath(f"s3://{cloud_path.bucket}/{parent_canonical}") - self._set_metadata_cache(cloud_path, "dir", etag, size, last_modified) + self._set_metadata_cache(path, "dir", etag, size, last_modified) yield ( path, True, @@ -303,7 +303,7 @@ def _list_dir(self, cloud_path: S3Path, recursive=False) -> Iterable[Tuple[S3Pat # s3 fake directories have 0 size and end with "/" if result_key.get("Key").endswith("/") and result_key.get("Size") == 0: path = self.CloudPath(f"s3://{cloud_path.bucket}/{canonical}") - self._set_metadata_cache(cloud_path, "dir", etag, size, last_modified) + self._set_metadata_cache(path, "dir", etag, size, last_modified) yield ( path, True, @@ -313,7 +313,7 @@ def _list_dir(self, cloud_path: S3Path, recursive=False) -> Iterable[Tuple[S3Pat # yield object as file else: path = self.CloudPath(f"s3://{cloud_path.bucket}/{result_key.get('Key')}") - self._set_metadata_cache(cloud_path, "file", etag, size, last_modified) + self._set_metadata_cache(path, "file", etag, size, last_modified) yield ( path, False, From 25ef445a8a654997ecb5a93df3308231be5cfdaf Mon Sep 17 00:00:00 2001 From: marchowes Date: Thu, 29 Aug 2024 15:48:10 +0000 Subject: [PATCH 7/9] bugfixes --- cloudpathlib/s3/s3client.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/cloudpathlib/s3/s3client.py b/cloudpathlib/s3/s3client.py index 2b18507b..717cfcfb 100644 --- a/cloudpathlib/s3/s3client.py +++ b/cloudpathlib/s3/s3client.py @@ -156,11 +156,10 @@ def __init__( def _get_metadata(self, cloud_path: S3Path) -> Dict[str, Any]: # get accepts all download extra args - size = None - size = self._metadata_cache[cloud_path].size + size = None if not self._metadata_cache.get(cloud_path) else self._metadata_cache[cloud_path].size if size: return { - "last_modified": self._metadata_cache[cloud_path].lastmodified, + "last_modified": self._metadata_cache[cloud_path].last_modified, "size": size, "etag": self._metadata_cache[cloud_path].etag, "content_type": None, From a5590f6704fcf188cfe39e6d2b93b70a16952226 Mon Sep 17 00:00:00 2001 From: "Michael D. Smith" Date: Thu, 29 Aug 2024 17:43:11 -0400 Subject: [PATCH 8/9] move to normal dict --- cloudpathlib/s3/s3client.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cloudpathlib/s3/s3client.py b/cloudpathlib/s3/s3client.py index 717cfcfb..6e43a3b7 100644 --- a/cloudpathlib/s3/s3client.py +++ b/cloudpathlib/s3/s3client.py @@ -146,7 +146,7 @@ def __init__( if k in self._extra_args } - self._metadata_cache: MutableMapping[S3Path, PathMetadata] = WeakKeyDictionary() + self._metadata_cache: MutableMapping[S3Path, PathMetadata] = dict() super().__init__( local_cache_dir=local_cache_dir, @@ -399,10 +399,10 @@ def _upload_file(self, local_path: Union[str, os.PathLike], cloud_path: S3Path) return cloud_path def _set_metadata_cache(self, cloud_path: S3Path, is_file_or_dir: Optional[str], - etag: Optional[str], size: Optional[int], lastmodified: Optional[str]) -> None: + etag: Optional[str], size: Optional[int], last_modified: Optional[str]) -> None: if is_file_or_dir is None: self._metadata_cache[cloud_path] = PathMetadata(is_file_or_dir=is_file_or_dir, - etag=etag, size=size, last_modified=lastmodified) + etag=etag, size=size, last_modified=last_modified) # If a file/dir is now known to not exist, its parent directories may no longer exist # either, since cloud directories only exist if they have a file in them. Since their # state is no longer known we remove them from the cache. @@ -411,7 +411,7 @@ def _set_metadata_cache(self, cloud_path: S3Path, is_file_or_dir: Optional[str], del self._metadata_cache[parent] else: self._metadata_cache[cloud_path] = PathMetadata(is_file_or_dir=is_file_or_dir, - etag=etag, size=size, last_modified=lastmodified) + etag=etag, size=size, last_modified=last_modified) def clear_metadata_cache(self) -> None: self._metadata_cache.clear() From fea61856f1eacdbc8b6c7ee344d9e5d36f506ef1 Mon Sep 17 00:00:00 2001 From: "Michael D. Smith" Date: Fri, 30 Aug 2024 15:11:26 -0400 Subject: [PATCH 9/9] cleanup on closedown --- cloudpathlib/cloudpath.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cloudpathlib/cloudpath.py b/cloudpathlib/cloudpath.py index c7908714..358d4c8a 100644 --- a/cloudpathlib/cloudpath.py +++ b/cloudpathlib/cloudpath.py @@ -248,7 +248,7 @@ def __del__(self) -> None: # ensure file removed from cache when cloudpath object deleted if ( hasattr(self, "client") - and self.client.file_cache_mode == FileCacheMode.cloudpath_object + #and self.client.file_cache_mode == FileCacheMode.cloudpath_object ): self.clear_cache()