From 45729ff9596f0fb9c57637ac1b589a92e9d5e62c Mon Sep 17 00:00:00 2001 From: Martin Durant Date: Sun, 30 Oct 2022 16:06:45 -0400 Subject: [PATCH 1/8] Make sure fs exceptions are raised if not Missing --- zarr/storage.py | 7 ++++++- zarr/tests/test_storage.py | 15 +++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/zarr/storage.py b/zarr/storage.py index 1c3b39862a..0658697992 100644 --- a/zarr/storage.py +++ b/zarr/storage.py @@ -1414,9 +1414,14 @@ def getitems( self, keys: Sequence[str], *, contexts: Mapping[str, Context] ) -> Mapping[str, Any]: keys_transformed = [self._normalize_key(key) for key in keys] - results = self.map.getitems(keys_transformed, on_error="omit") + results = self.map.getitems(keys_transformed, on_error="return") # The function calling this method may not recognize the transformed keys # So we send the values returned by self.map.getitems back into the original key space. + for k, v in results.copy().items(): + if isinstance(v, self.exceptions): + results.pop(k) + elif isinstance(v, Exception): + raise v return {keys[keys_transformed.index(rk)]: rv for rk, rv in results.items()} def __getitem__(self, key): diff --git a/zarr/tests/test_storage.py b/zarr/tests/test_storage.py index 25863749d8..c17516ff1a 100644 --- a/zarr/tests/test_storage.py +++ b/zarr/tests/test_storage.py @@ -1337,6 +1337,21 @@ def test_s3_complex(self): ) assert (a[:] == -np.ones((8, 8, 8))).all() + def test_exceptions(self): + import fsspec + m = fsspec.filesystem("memory") + g = zarr.open_group("memory://test/out.zarr", mode='w') + arr = g.create_dataset("data", data=[1, 2, 3, 4], + dtype="i4", compression=None, chunks=[2]) + m.store["/test/out.zarr/data/0"] = None + del m.store["/test/out.zarr/data/1"] + assert g.store.getitems(["data/1"]) == {} # not found + with pytest.raises(Exception): + # None is bad daa, as opposed to missing + g.store.getitems(["data/0", "data/1"]) + with pytest.raises(Exception): + # None is bad daa, as opposed to missing + arr[:] @pytest.mark.skipif(have_fsspec is False, reason="needs fsspec") class TestFSStoreWithKeySeparator(StoreTests): From fa72689bb240e0dd91a18208821719cac2af8459 Mon Sep 17 00:00:00 2001 From: Martin Durant Date: Mon, 21 Nov 2022 15:23:20 -0500 Subject: [PATCH 2/8] lint --- zarr/tests/test_storage.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/zarr/tests/test_storage.py b/zarr/tests/test_storage.py index c17516ff1a..ad8f8411ed 100644 --- a/zarr/tests/test_storage.py +++ b/zarr/tests/test_storage.py @@ -1347,12 +1347,13 @@ def test_exceptions(self): del m.store["/test/out.zarr/data/1"] assert g.store.getitems(["data/1"]) == {} # not found with pytest.raises(Exception): - # None is bad daa, as opposed to missing + # None is bad data, as opposed to missing g.store.getitems(["data/0", "data/1"]) with pytest.raises(Exception): - # None is bad daa, as opposed to missing + # None is bad data, as opposed to missing arr[:] + @pytest.mark.skipif(have_fsspec is False, reason="needs fsspec") class TestFSStoreWithKeySeparator(StoreTests): def create_store(self, normalize_keys=False, key_separator=".", **kwargs): From 1b89bf3670340ebf3296e7152ea3fabb6d0decf8 Mon Sep 17 00:00:00 2001 From: Ian Carroll Date: Fri, 8 Dec 2023 11:30:43 -0500 Subject: [PATCH 3/8] add missing argument in tests, lint --- zarr/tests/test_storage.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/zarr/tests/test_storage.py b/zarr/tests/test_storage.py index ad8f8411ed..1f62dcc167 100644 --- a/zarr/tests/test_storage.py +++ b/zarr/tests/test_storage.py @@ -1341,14 +1341,13 @@ def test_exceptions(self): import fsspec m = fsspec.filesystem("memory") g = zarr.open_group("memory://test/out.zarr", mode='w') - arr = g.create_dataset("data", data=[1, 2, 3, 4], - dtype="i4", compression=None, chunks=[2]) + arr = g.create_dataset("data", data=[1, 2, 3, 4], dtype="i4", compression=None, chunks=[2]) m.store["/test/out.zarr/data/0"] = None del m.store["/test/out.zarr/data/1"] - assert g.store.getitems(["data/1"]) == {} # not found + assert g.store.getitems(["data/1"], contexts={}) == {} # not found with pytest.raises(Exception): # None is bad data, as opposed to missing - g.store.getitems(["data/0", "data/1"]) + g.store.getitems(["data/0", "data/1"], contexts={}) with pytest.raises(Exception): # None is bad data, as opposed to missing arr[:] From 923ffa328114e039869dae18170db8a2054ad525 Mon Sep 17 00:00:00 2001 From: Ian Carroll Date: Sun, 10 Dec 2023 16:44:47 -0500 Subject: [PATCH 4/8] clear memory filesystem during test --- zarr/tests/test_storage.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/zarr/tests/test_storage.py b/zarr/tests/test_storage.py index 1f62dcc167..f3b4ab2ba6 100644 --- a/zarr/tests/test_storage.py +++ b/zarr/tests/test_storage.py @@ -1339,8 +1339,9 @@ def test_s3_complex(self): def test_exceptions(self): import fsspec + m = fsspec.filesystem("memory") - g = zarr.open_group("memory://test/out.zarr", mode='w') + g = zarr.open_group("memory://test/out.zarr", mode="w") arr = g.create_dataset("data", data=[1, 2, 3, 4], dtype="i4", compression=None, chunks=[2]) m.store["/test/out.zarr/data/0"] = None del m.store["/test/out.zarr/data/1"] @@ -1351,6 +1352,8 @@ def test_exceptions(self): with pytest.raises(Exception): # None is bad data, as opposed to missing arr[:] + # clear the global memory filesystem's store for use in other tests + m.store.clear() @pytest.mark.skipif(have_fsspec is False, reason="needs fsspec") From fab317dfd01ad28ee88793ec473a54c16d319965 Mon Sep 17 00:00:00 2001 From: Ian Carroll Date: Sun, 10 Dec 2023 18:56:23 -0500 Subject: [PATCH 5/8] improve commenting --- zarr/storage.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/zarr/storage.py b/zarr/storage.py index 0658697992..81ca54c7f0 100644 --- a/zarr/storage.py +++ b/zarr/storage.py @@ -1415,13 +1415,14 @@ def getitems( ) -> Mapping[str, Any]: keys_transformed = [self._normalize_key(key) for key in keys] results = self.map.getitems(keys_transformed, on_error="return") - # The function calling this method may not recognize the transformed keys - # So we send the values returned by self.map.getitems back into the original key space. + # Only recognized errors will prompt KeyErrors in the function calling this method for k, v in results.copy().items(): if isinstance(v, self.exceptions): results.pop(k) elif isinstance(v, Exception): raise v + # The function calling this method may not recognize the transformed keys, so we + # send the values returned by self.map.getitems back into the original key space. return {keys[keys_transformed.index(rk)]: rv for rk, rv in results.items()} def __getitem__(self, key): From edab9ff3160c00626152fb64f51bf2a3f0262037 Mon Sep 17 00:00:00 2001 From: Ian Carroll Date: Tue, 12 Dec 2023 23:43:50 -0500 Subject: [PATCH 6/8] add memory_store fixture, getitems performance --- zarr/storage.py | 22 ++++++++++++++-------- zarr/tests/test_storage.py | 21 ++++++++++++--------- 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/zarr/storage.py b/zarr/storage.py index 81ca54c7f0..5b89870925 100644 --- a/zarr/storage.py +++ b/zarr/storage.py @@ -1413,17 +1413,23 @@ def _normalize_key(self, key): def getitems( self, keys: Sequence[str], *, contexts: Mapping[str, Context] ) -> Mapping[str, Any]: - keys_transformed = [self._normalize_key(key) for key in keys] - results = self.map.getitems(keys_transformed, on_error="return") - # Only recognized errors will prompt KeyErrors in the function calling this method - for k, v in results.copy().items(): + keys_transformed = {self._normalize_key(key): key for key in keys} + results_transformed = self.map.getitems(list(keys_transformed), on_error="return") + results = {} + for k, v in results_transformed.items(): if isinstance(v, self.exceptions): - results.pop(k) + # Cause recognized exceptions to prompt a KeyError in the + # function calling this method + continue elif isinstance(v, Exception): + # Raise any other exception raise v - # The function calling this method may not recognize the transformed keys, so we - # send the values returned by self.map.getitems back into the original key space. - return {keys[keys_transformed.index(rk)]: rv for rk, rv in results.items()} + else: + # The function calling this method may not recognize the transformed + # keys, so we send the values returned by self.map.getitems back into + # the original key space. + results[keys_transformed[k]] = v + return results def __getitem__(self, key): key = self._normalize_key(key) diff --git a/zarr/tests/test_storage.py b/zarr/tests/test_storage.py index f3b4ab2ba6..ff117253f3 100644 --- a/zarr/tests/test_storage.py +++ b/zarr/tests/test_storage.py @@ -1098,6 +1098,12 @@ def mock_walker_no_slash(_path): @pytest.mark.skipif(have_fsspec is False, reason="needs fsspec") class TestFSStore(StoreTests): + @pytest.fixture + def memory_store(self): + store = FSStore("memory://test/out.zarr") + yield store + store.map.fs.store.clear() + def create_store(self, normalize_keys=False, dimension_separator=".", path=None, **kwargs): if path is None: path = tempfile.mkdtemp() @@ -1337,14 +1343,13 @@ def test_s3_complex(self): ) assert (a[:] == -np.ones((8, 8, 8))).all() - def test_exceptions(self): - import fsspec - - m = fsspec.filesystem("memory") - g = zarr.open_group("memory://test/out.zarr", mode="w") + def test_exceptions(self, memory_store): + path = memory_store.path + m = memory_store.fs + g = zarr.open_group(memory_store, mode="w") arr = g.create_dataset("data", data=[1, 2, 3, 4], dtype="i4", compression=None, chunks=[2]) - m.store["/test/out.zarr/data/0"] = None - del m.store["/test/out.zarr/data/1"] + m.store[path + "/data/0"] = None + del m.store[path + "/data/1"] assert g.store.getitems(["data/1"], contexts={}) == {} # not found with pytest.raises(Exception): # None is bad data, as opposed to missing @@ -1352,8 +1357,6 @@ def test_exceptions(self): with pytest.raises(Exception): # None is bad data, as opposed to missing arr[:] - # clear the global memory filesystem's store for use in other tests - m.store.clear() @pytest.mark.skipif(have_fsspec is False, reason="needs fsspec") From 4c3f2bbbf33ae5989ee49a11fc2d5268fa540cb3 Mon Sep 17 00:00:00 2001 From: Ian Carroll Date: Tue, 19 Dec 2023 21:53:01 -0500 Subject: [PATCH 7/8] Update release.rst --- docs/release.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/release.rst b/docs/release.rst index c18e0b8c20..52d2c12f9c 100644 --- a/docs/release.rst +++ b/docs/release.rst @@ -49,6 +49,9 @@ Docs Maintenance ~~~~~~~~~~~ +* FSStore now raises rather than return bad data. + By :user:`Martin Durant ` and :user:`Ian Carroll ` :issue:`1604`. + * Cache result of ``FSStore._fsspec_installed()``. By :user:`Janick Martinez Esturo ` :issue:`1581`. From a0c0cbb166eb9b666c5109bcf6623e196d77b36c Mon Sep 17 00:00:00 2001 From: Ian Carroll Date: Sat, 2 Mar 2024 20:52:27 -0500 Subject: [PATCH 8/8] improve FSStore.test_exception coverage --- zarr/tests/test_storage.py | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/zarr/tests/test_storage.py b/zarr/tests/test_storage.py index 44a366b937..ae8a56fa61 100644 --- a/zarr/tests/test_storage.py +++ b/zarr/tests/test_storage.py @@ -1100,9 +1100,9 @@ def mock_walker_no_slash(_path): class TestFSStore(StoreTests): @pytest.fixture def memory_store(self): - store = FSStore("memory://test/out.zarr") + store = FSStore("memory://") yield store - store.map.fs.store.clear() + store.fs.store.clear() def create_store(self, normalize_keys=False, dimension_separator=".", path=None, **kwargs): if path is None: @@ -1344,19 +1344,23 @@ def test_s3_complex(self): assert (a[:] == -np.ones((8, 8, 8))).all() def test_exceptions(self, memory_store): - path = memory_store.path - m = memory_store.fs - g = zarr.open_group(memory_store, mode="w") - arr = g.create_dataset("data", data=[1, 2, 3, 4], dtype="i4", compression=None, chunks=[2]) - m.store[path + "/data/0"] = None - del m.store[path + "/data/1"] - assert g.store.getitems(["data/1"], contexts={}) == {} # not found + fs = memory_store.fs + group = zarr.open(memory_store, mode="w") + x = group.create_dataset("x", data=[1, 2, 3]) + y = group.create_dataset("y", data=1) + fs.store["/x/0"] = None + fs.store["/y/0"] = None + # no exception from FSStore.getitems getting KeyError + assert group.store.getitems(["foo"], contexts={}) == {} + # exception from FSStore.getitems getting AttributeError with pytest.raises(Exception): - # None is bad data, as opposed to missing - g.store.getitems(["data/0", "data/1"], contexts={}) + group.store.getitems(["x/0"], contexts={}) + # exception from FSStore.getitems getting AttributeError with pytest.raises(Exception): - # None is bad data, as opposed to missing - arr[:] + x[...] + # exception from FSStore.__getitem__ getting AttributeError + with pytest.raises(Exception): + y[...] @pytest.mark.skipif(have_fsspec is False, reason="needs fsspec")