Merge branch 'main' into ig/fix_zip_store

dcherian · web-flow · commit c453bdf10b1f · 2025-02-10T08:54:46.000-07:00
diff --git a/.github/workflows/hypothesis.yaml b/.github/workflows/hypothesis.yaml
@@ -69,6 +69,12 @@ jobs:
         path: .hypothesis/
         key: cache-hypothesis-${{ runner.os }}-${{ github.run_id }}
 
+    - name: Upload coverage
+      uses: codecov/codecov-action@v5
+      with:
+        token: ${{ secrets.CODECOV_TOKEN }}
+        verbose: true # optional (default = false)
+
     - name: Generate and publish the report
       if: |
         failure()
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
@@ -6,13 +6,13 @@ ci:
 default_stages: [pre-commit, pre-push]
 repos:
   - repo: https://github.com/astral-sh/ruff-pre-commit
-    rev: v0.9.1
+    rev: v0.9.4
     hooks:
     - id: ruff
       args: ["--fix", "--show-fixes"]
     - id: ruff-format
   - repo: https://github.com/codespell-project/codespell
-    rev: v2.3.0
+    rev: v2.4.1
     hooks:
       - id: codespell
         args: ["-L", "fo,ihs,kake,te", "-S", "fixture"]
@@ -37,7 +37,7 @@ repos:
           # Tests
           - pytest
   - repo: https://github.com/scientific-python/cookie
-    rev: 2024.08.19
+    rev: 2025.01.22
     hooks:
       - id: sp-repo-review
   - repo: https://github.com/pre-commit/pygrep-hooks
@@ -50,6 +50,6 @@ repos:
     hooks:
       - id: numpydoc-validation
   - repo: https://github.com/twisted/towncrier
-    rev: 23.11.0
+    rev: 24.8.0
     hooks:
       - id: towncrier-check
diff --git a/changes/2778.bugfix.rst b/changes/2778.bugfix.rst
@@ -0,0 +1 @@
+Use removeprefix rather than replace when removing filename prefixes in `FsspecStore.list`
diff --git a/changes/2796.chore.rst b/changes/2796.chore.rst
@@ -0,0 +1 @@
+The docs environment is now built with ``astroid`` pinned to a version less than 4. This allows the docs to build in CI.
diff --git a/changes/2801.bugfix.rst b/changes/2801.bugfix.rst
@@ -0,0 +1 @@
+Ensure utf8 compliant strings are used to construct numpy arrays in property-based tests
diff --git a/changes/2804.feature.rst b/changes/2804.feature.rst
@@ -0,0 +1 @@
+:py:class:`LocalStore` learned to ``delete_dir``. This makes array and group deletes more efficient.
diff --git a/changes/2807.bugfix.rst b/changes/2807.bugfix.rst
@@ -0,0 +1 @@
+Fix pickling for ZipStore
diff --git a/pyproject.toml b/pyproject.toml
@@ -103,6 +103,7 @@ docs = [
     'numcodecs[msgpack]',
     'rich',
     's3fs',
+    'astroid<4'
 ]
 
 
@@ -160,7 +161,7 @@ run = "run-coverage --no-cov"
 run-pytest = "run"
 run-verbose = "run-coverage --verbose"
 run-mypy = "mypy src"
-run-hypothesis = "pytest --hypothesis-profile ci tests/test_properties.py tests/test_store/test_stateful*"
+run-hypothesis = "run-coverage --hypothesis-profile ci --run-slow-hypothesis tests/test_properties.py tests/test_store/test_stateful*"
 list-env = "pip list"
 
 [tool.hatch.envs.doctest]
@@ -397,7 +398,8 @@ filterwarnings = [
     "ignore:.*is currently not part in the Zarr format 3 specification.*:UserWarning",
 ]
 markers = [
-    "gpu: mark a test as requiring CuPy and GPU"
+    "gpu: mark a test as requiring CuPy and GPU",
+    "slow_hypothesis: slow hypothesis tests",
 ]
 
 [tool.repo-review]
@@ -427,3 +429,6 @@ directory = 'changes'
 filename = "docs/release-notes.rst"
 underlines = ["-", "~", "^"]
 issue_format = ":issue:`{issue}`"
+
+[tool.codespell]
+ignore-words-list = "astroid"
diff --git a/src/zarr/storage/_fsspec.py b/src/zarr/storage/_fsspec.py
@@ -341,7 +341,7 @@ async def set_partial_values(
     async def list(self) -> AsyncIterator[str]:
         # docstring inherited
         allfiles = await self.fs._find(self.path, detail=False, withdirs=False)
-        for onefile in (a.replace(self.path + "/", "") for a in allfiles):
+        for onefile in (a.removeprefix(self.path + "/") for a in allfiles):
             yield onefile
 
     async def list_dir(self, prefix: str) -> AsyncIterator[str]:
diff --git a/src/zarr/storage/_local.py b/src/zarr/storage/_local.py
@@ -208,6 +208,19 @@ async def delete(self, key: str) -> None:
         else:
             await asyncio.to_thread(path.unlink, True)  # Q: we may want to raise if path is missing
 
+    async def delete_dir(self, prefix: str) -> None:
+        # docstring inherited
+        self._check_writable()
+        path = self.root / prefix
+        if path.is_dir():
+            shutil.rmtree(path)
+        elif path.is_file():
+            raise ValueError(f"delete_dir was passed a {prefix=!r} that is a file")
+        else:
+            # Non-existent directory
+            # This path is tested by test_group:test_create_creates_parents for one
+            pass
+
     async def exists(self, key: str) -> bool:
         # docstring inherited
         path = self.root / key
diff --git a/src/zarr/storage/_zip.py b/src/zarr/storage/_zip.py
@@ -108,7 +108,8 @@ async def _open(self) -> None:
         self._sync_open()
 
     def __getstate__(self) -> dict[str, Any]:
-        state = self.__dict__
+        # We need a copy to not modify the state of the original store
+        state = self.__dict__.copy()
         for attr in ["_zf", "_lock"]:
             state.pop(attr, None)
         return state
diff --git a/src/zarr/testing/stateful.py b/src/zarr/testing/stateful.py
@@ -68,6 +68,9 @@ def can_add(self, path: str) -> bool:
     # -------------------- store operations -----------------------
     @rule(name=node_names, data=st.data())
     def add_group(self, name: str, data: DataObject) -> None:
+        # Handle possible case-insensitive file systems (e.g. MacOS)
+        if isinstance(self.store, LocalStore):
+            name = name.lower()
         if self.all_groups:
             parent = data.draw(st.sampled_from(sorted(self.all_groups)), label="Group parent")
         else:
@@ -90,6 +93,9 @@ def add_array(
         name: str,
         array_and_chunks: tuple[np.ndarray[Any, Any], tuple[int, ...]],
     ) -> None:
+        # Handle possible case-insensitive file systems (e.g. MacOS)
+        if isinstance(self.store, LocalStore):
+            name = name.lower()
         array, chunks = array_and_chunks
         fill_value = data.draw(npst.from_dtype(array.dtype))
         if self.all_groups:
@@ -135,6 +141,7 @@ def add_array(
     #     self.model.rename(from_group, new_path)
     #     self.repo.store.rename(from_group, new_path)
 
+    @precondition(lambda self: self.store.supports_deletes)
     @precondition(lambda self: len(self.all_arrays) >= 1)
     @rule(data=st.data())
     def delete_array_using_del(self, data: DataObject) -> None:
@@ -149,6 +156,7 @@ def delete_array_using_del(self, data: DataObject) -> None:
             del group[array_name]
         self.all_arrays.remove(array_path)
 
+    @precondition(lambda self: self.store.supports_deletes)
     @precondition(lambda self: len(self.all_groups) >= 2)  # fixme don't delete root
     @rule(data=st.data())
     def delete_group_using_del(self, data: DataObject) -> None:
@@ -284,6 +292,10 @@ def supports_partial_writes(self) -> bool:
     def supports_writes(self) -> bool:
         return self.store.supports_writes
 
+    @property
+    def supports_deletes(self) -> bool:
+        return self.store.supports_deletes
+
 
 class ZarrStoreStateMachine(RuleBasedStateMachine):
     """ "
@@ -366,6 +378,7 @@ def get_partial_values(self, data: DataObject) -> None:
             model_vals_ls,
         )
 
+    @precondition(lambda self: self.store.supports_deletes)
     @precondition(lambda self: len(self.model.keys()) > 0)
     @rule(data=st.data())
     def delete(self, data: DataObject) -> None:
diff --git a/src/zarr/testing/store.py b/src/zarr/testing/store.py
@@ -99,10 +99,16 @@ def test_store_eq(self, store: S, store_kwargs: dict[str, Any]) -> None:
         store2 = self.store_cls(**store_kwargs)
         assert store == store2
 
-    def test_serializable_store(self, store: S) -> None:
+    async def test_serializable_store(self, store: S) -> None:
         new_store: S = pickle.loads(pickle.dumps(store))
         assert new_store == store
         assert new_store.read_only == store.read_only
+        # quickly roundtrip data to a key to test that new store works
+        data_buf = self.buffer_cls.from_bytes(b"\x01\x02\x03\x04")
+        key = "foo"
+        await store.set(key, data_buf)
+        observed = await store.get(key, prototype=default_buffer_prototype())
+        assert_bytes_equal(observed, data_buf)
 
     def test_store_read_only(self, store: S) -> None:
         assert not store.read_only
@@ -400,6 +406,37 @@ async def test_list_prefix(self, store: S) -> None:
             expected = tuple(sorted(expected))
             assert observed == expected
 
+    async def test_list_empty_path(self, store: S) -> None:
+        """
+        Verify that list and list_prefix work correctly when path is an empty string,
+        i.e. no unwanted replacement occurs.
+        """
+        data = self.buffer_cls.from_bytes(b"")
+        store_dict = {
+            "foo/bar/zarr.json": data,
+            "foo/bar/c/1": data,
+            "foo/baz/c/0": data,
+        }
+        await store._set_many(store_dict.items())
+
+        # Test list()
+        observed_list = await _collect_aiterator(store.list())
+        observed_list_sorted = sorted(observed_list)
+        expected_list_sorted = sorted(store_dict.keys())
+        assert observed_list_sorted == expected_list_sorted
+
+        # Test list_prefix() with an empty prefix
+        observed_prefix_empty = await _collect_aiterator(store.list_prefix(""))
+        observed_prefix_empty_sorted = sorted(observed_prefix_empty)
+        expected_prefix_empty_sorted = sorted(store_dict.keys())
+        assert observed_prefix_empty_sorted == expected_prefix_empty_sorted
+
+        # Test list_prefix() with a non-empty prefix
+        observed_prefix = await _collect_aiterator(store.list_prefix("foo/bar/"))
+        observed_prefix_sorted = sorted(observed_prefix)
+        expected_prefix_sorted = sorted(k for k in store_dict if k.startswith("foo/bar/"))
+        assert observed_prefix_sorted == expected_prefix_sorted
+
     async def test_list_dir(self, store: S) -> None:
         root = "foo"
         store_dict = {
diff --git a/src/zarr/testing/strategies.py b/src/zarr/testing/strategies.py
@@ -1,3 +1,4 @@
+import sys
 from typing import Any
 
 import hypothesis.extra.numpy as npst
@@ -51,6 +52,21 @@ def v2_dtypes() -> st.SearchStrategy[np.dtype]:
     )
 
 
+def safe_unicode_for_dtype(dtype: np.dtype[np.str_]) -> st.SearchStrategy[str]:
+    """Generate UTF-8-safe text constrained to max_len of dtype."""
+    # account for utf-32 encoding (i.e. 4 bytes/character)
+    max_len = max(1, dtype.itemsize // 4)
+
+    return st.text(
+        alphabet=st.characters(
+            blacklist_categories=["Cs"],  # Avoid *technically allowed* surrogates
+            min_codepoint=32,
+        ),
+        min_size=1,
+        max_size=max_len,
+    )
+
+
 # From https://zarr-specs.readthedocs.io/en/latest/v3/core/v3.0.html#node-names
 # 1. must not be the empty string ("")
 # 2. must not include the character "/"
@@ -86,7 +102,12 @@ def numpy_arrays(
     Generate numpy arrays that can be saved in the provided Zarr format.
     """
     zarr_format = draw(zarr_formats)
-    return draw(npst.arrays(dtype=v3_dtypes() if zarr_format == 3 else v2_dtypes(), shape=shapes))
+    dtype = draw(v3_dtypes() if zarr_format == 3 else v2_dtypes())
+    if np.issubdtype(dtype, np.str_):
+        safe_unicode_strings = safe_unicode_for_dtype(dtype)
+        return draw(npst.arrays(dtype=dtype, shape=shapes, elements=safe_unicode_strings))
+
+    return draw(npst.arrays(dtype=dtype, shape=shapes))
 
 
 @st.composite  # type: ignore[misc]
@@ -189,7 +210,7 @@ def basic_indices(draw: st.DrawFn, *, shape: tuple[int], **kwargs: Any) -> Any:
 
 
 def key_ranges(
-    keys: SearchStrategy = node_names, max_size: int | None = None
+    keys: SearchStrategy = node_names, max_size: int = sys.maxsize
 ) -> SearchStrategy[list[int]]:
     """
     Function to generate key_ranges strategy for get_partial_values()
@@ -198,10 +219,14 @@ def key_ranges(
         [(key, (range_start, range_end)),
          (key, (range_start, range_end)),...]
     """
+
+    def make_request(start: int, length: int) -> RangeByteRequest:
+        return RangeByteRequest(start, end=min(start + length, max_size))
+
     byte_ranges = st.builds(
-        RangeByteRequest,
+        make_request,
         start=st.integers(min_value=0, max_value=max_size),
-        end=st.integers(min_value=0, max_value=max_size),
+        length=st.integers(min_value=0, max_value=max_size),
     )
     key_tuple = st.tuples(keys, byte_ranges)
     return st.lists(key_tuple, min_size=1, max_size=10)
diff --git a/tests/conftest.py b/tests/conftest.py
@@ -147,6 +147,24 @@ def zarr_format(request: pytest.FixtureRequest) -> ZarrFormat:
     raise ValueError(msg)
 
 
+def pytest_addoption(parser: Any) -> None:
+    parser.addoption(
+        "--run-slow-hypothesis",
+        action="store_true",
+        default=False,
+        help="run slow hypothesis tests",
+    )
+
+
+def pytest_collection_modifyitems(config: Any, items: Any) -> None:
+    if config.getoption("--run-slow-hypothesis"):
+        return
+    skip_slow_hyp = pytest.mark.skip(reason="need --run-slow-hypothesis option to run")
+    for item in items:
+        if "slow_hypothesis" in item.keywords:
+            item.add_marker(skip_slow_hyp)
+
+
 settings.register_profile(
     "ci",
     max_examples=1000,
diff --git a/tests/test_store/test_stateful.py b/tests/test_store/test_stateful.py
@@ -1,25 +1,24 @@
 # Stateful tests for arbitrary Zarr stores.
 import pytest
 from hypothesis.stateful import (
-    Settings,
     run_state_machine_as_test,
 )
 
 from zarr.abc.store import Store
-from zarr.storage import LocalStore, MemoryStore, ZipStore
+from zarr.storage import LocalStore, ZipStore
 from zarr.testing.stateful import ZarrHierarchyStateMachine, ZarrStoreStateMachine
 
+pytestmark = pytest.mark.slow_hypothesis
+
 
 def test_zarr_hierarchy(sync_store: Store):
     def mk_test_instance_sync() -> ZarrHierarchyStateMachine:
         return ZarrHierarchyStateMachine(sync_store)
 
     if isinstance(sync_store, ZipStore):
         pytest.skip(reason="ZipStore does not support delete")
-    if isinstance(sync_store, MemoryStore):
-        run_state_machine_as_test(
-            mk_test_instance_sync, settings=Settings(report_multiple_bugs=False, max_examples=50)
-        )
+
+    run_state_machine_as_test(mk_test_instance_sync)
 
 
 def test_zarr_store(sync_store: Store) -> None:
@@ -28,11 +27,10 @@ def mk_test_instance_sync() -> None:
 
     if isinstance(sync_store, ZipStore):
         pytest.skip(reason="ZipStore does not support delete")
-    elif isinstance(sync_store, LocalStore):
-        pytest.skip(reason="This test has errors")
-    elif isinstance(sync_store, MemoryStore):
-        run_state_machine_as_test(mk_test_instance_sync, settings=Settings(max_examples=50))
-    else:
-        run_state_machine_as_test(
-            mk_test_instance_sync, settings=Settings(report_multiple_bugs=True)
-        )
+
+    if isinstance(sync_store, LocalStore):
+        # This test uses arbitrary keys, which are passed to `set` and `delete`.
+        # It assumes that `set` and `delete` are the only two operations that modify state.
+        # But LocalStore, directories can hang around even after a key is delete-d.
+        pytest.skip(reason="Test isn't suitable for LocalStore.")
+    run_state_machine_as_test(mk_test_instance_sync)

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	+Use removeprefix rather than replace when removing filename prefixes in `FsspecStore.list`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	+The docs environment is now built with ``astroid`` pinned to a version less than 4. This allows the docs to build in CI.
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+Ensure utf8 compliant strings are used to construct numpy arrays in property-based tests`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	+:py:class:`LocalStore` learned to ``delete_dir``. This makes array and group deletes more efficient.