Address the PR review: file:// URLs, cache identity, cache geometry

Three real defects, all confirmed by repro before fixing.

file:// was excluded from the fsspec branch so it could keep mmap and the
directory formats, but nothing downstream stripped the scheme, so it reached
os.path.exists() and the C layer as a literal filename and failed.  The
docstring promising otherwise was simply wrong.  Normalized to a native path
in open(), NDArray.save(), Storage, and the two constructor paths that
bypass Storage.

The directory cache manifest compared name, size and mtime, which is the
same mistake already fixed for the lazy chunk cache and missed here: on a
backend with no mtime -- memory://, and the tests only use memory:// -- a
same-size rewrite left the manifest unchanged and served stale files.  It
now hashes each entry with tokenize(), which is what fs.ukey() uses.

Reusing a proxy cache checked shape and dtype only.  Chunk numbers are the
currency between cache and source, so a same-shaped source chunked
differently fetched the wrong chunks and returned wrong data with no error
at all; chunks and blocks are compared now, and non-ND sources get the same
check on nbytes, chunksize and typesize instead of none.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: FrancescAlted

plans/fsspec-support.md

@@ -181,6 +181,10 @@ Notes on the details:
 
 - The `file://` exclusion lets fsspec-style local URLs keep working through
   the normal local path, which supports mmap and every container format.
+  *(This turned out to need more than the exclusion: nothing downstream stripped
+  the scheme, so a `file://` URL was taken as a literal filename and failed. It
+  is normalized to a native path now, in `open()`, `NDArray.save()`, `Storage`
+  and the two constructor paths that bypass `Storage`.)*
 - `offset != 0` should raise for now; the embedded-object case is a phase-3
   concern.
 - `copy=False` on `from_cframe` is tempting (it pins the read buffer instead of

src/blosc2/core.py

@@ -21,6 +21,8 @@
 import shutil
 import subprocess
 import sys
+import urllib.parse
+import urllib.request
 from dataclasses import asdict
 from functools import lru_cache
 from typing import TYPE_CHECKING, ClassVar
@@ -616,6 +618,17 @@ def load_array(urlpath: str, dparams: dict | None = None) -> np.ndarray:
     return load_tensor(urlpath, dparams=dparams)
 
 
+def normalize_urlpath(urlpath: object) -> object:
+    """Turn a `file://` URL into the native path it names, leaving anything else alone.
+
+    Local URLs are kept off the fsspec branch so they can use mmap and every
+    container format, which only works if the scheme is stripped first.
+    """
+    if isinstance(urlpath, str) and urlpath.startswith("file://"):
+        return urllib.request.url2pathname(urllib.parse.urlparse(urlpath).path)
+    return urlpath
+
+
 def is_fsspec_url(urlpath: object) -> bool:
     """Whether *urlpath* should be routed through fsspec.
 
@@ -665,6 +678,8 @@ def localize_fsspec_url(urlpath: str, cache_storage: str | pathlib.Path) -> str:
     matching the manifest written at download time.
     """
     fsspec = _import_fsspec(urlpath)
+    from fsspec.utils import tokenize
+
     cache_storage = str(cache_storage)
     fs, path = fsspec.url_to_fs(urlpath)
 
@@ -678,12 +693,11 @@ def localize_fsspec_url(urlpath: str, cache_storage: str | pathlib.Path) -> str:
 
     localdir = fsspec_cache_path(urlpath, cache_storage)
     manifest = pathlib.Path(localdir + ".json")
+    # tokenize(info) is what fs.ukey() hashes, so this asks each backend what
+    # identifies a file rather than guessing which fields it exposes -- size and
+    # mtime miss a same-size rewrite, and memory:// has no mtime at all
     listing = json.dumps(
-        {
-            name: (entry.get("size"), entry.get("mtime") or entry.get("LastModified"))
-            for name, entry in sorted(fs.find(path, detail=True).items())
-        },
-        default=str,
+        {name: tokenize(entry) for name, entry in sorted(fs.find(path, detail=True).items())}
     )
     if not manifest.exists() or manifest.read_text() != listing:
         shutil.rmtree(localdir, ignore_errors=True)

src/blosc2/ndarray.py

@@ -32,7 +32,7 @@
 
 import blosc2
 from blosc2 import SpecialValue, blosc2_ext, compute_chunks_blocks
-from blosc2.core import fsspec_open, is_fsspec_url
+from blosc2.core import fsspec_open, is_fsspec_url, normalize_urlpath
 from blosc2.info import InfoReporter, format_nbytes_info
 from blosc2.schunk import SChunk
 
@@ -5078,6 +5078,7 @@ def save(self, urlpath: str, contiguous=True, **kwargs: Any) -> None:
         >>> # Save the array to a file
         >>> a.save("array.b2frame")
         """
+        urlpath = normalize_urlpath(urlpath)
         if is_fsspec_url(urlpath):
             if not contiguous:
                 raise NotImplementedError(
@@ -7020,6 +7021,11 @@ def astype(
 
 
 def _check_ndarray_kwargs(**kwargs):  # noqa: C901
+    if kwargs.get("urlpath") is not None:
+        # A Storage instance normalizes its own; a bare kwarg has to be done here,
+        # since it takes precedence over the defaults built from it below
+        kwargs["urlpath"] = normalize_urlpath(kwargs["urlpath"])
+
     storage = kwargs.get("storage")
     if storage is not None:
         for key in kwargs:

src/blosc2/proxy.py

@@ -322,12 +322,24 @@ def _reopen_cache(self, urlpath: str):
             raise ValueError(
                 f"{urlpath} is not a proxy cache; pass mode='w' to overwrite it or choose another urlpath"
             )
-        if hasattr(self.src, "shape") and (
-            tuple(cached.shape) != tuple(self.src.shape) or cached.dtype != self.src.dtype
-        ):
+        # Chunk *numbers* are the currency between cache and source, so the
+        # partitioning has to match, not just the logical shape: fetch() would
+        # otherwise ask the source for chunk n meaning something else entirely
+        if hasattr(self.src, "shape"):
+            here = (tuple(cached.shape), cached.dtype, tuple(cached.chunks), tuple(cached.blocks))
+            there = (
+                tuple(self.src.shape),
+                np.dtype(self.src.dtype),
+                tuple(self.src.chunks),
+                tuple(self.src.blocks),
+            )
+        else:
+            here = (schunk.nbytes, schunk.chunksize, schunk.typesize)
+            there = (self.src.nbytes, self.src.chunksize, self.src.typesize)
+        if here != there:
             raise ValueError(
-                f"the cache at {urlpath} holds a {cached.shape} {cached.dtype} array, which "
-                f"does not fit the {self.src.shape} {self.src.dtype} source"
+                f"the cache at {urlpath} was built for a different source: it holds {here}, "
+                f"the source is {there} (shape, dtype, chunks, blocks)"
             )
         return cached
 

src/blosc2/schunk.py

@@ -22,7 +22,13 @@
 
 import blosc2
 from blosc2 import SpecialValue, blosc2_ext
-from blosc2.core import fsspec_cache_path, fsspec_open, is_fsspec_url, localize_fsspec_url
+from blosc2.core import (
+    fsspec_cache_path,
+    fsspec_open,
+    is_fsspec_url,
+    localize_fsspec_url,
+    normalize_urlpath,
+)
 from blosc2.info import InfoReporter, format_nbytes_info
 from blosc2.msgpack_utils import msgpack_packb, msgpack_unpackb
 
@@ -367,7 +373,9 @@ def __init__(  # noqa: C901
         if isinstance(kwargs.get("dparams"), blosc2.DParams):
             kwargs["dparams"] = asdict(kwargs.get("dparams"))
 
-        urlpath = kwargs.get("urlpath")
+        urlpath = normalize_urlpath(kwargs.get("urlpath"))
+        if urlpath is not None:
+            kwargs["urlpath"] = urlpath
         if is_fsspec_url(urlpath):
             raise ValueError(
                 f"{urlpath} is an fsspec URL, which cannot back a container as it is written; "
@@ -2181,6 +2189,7 @@ def open(
 
     if isinstance(urlpath, pathlib.PurePath):
         urlpath = str(urlpath)
+    urlpath = normalize_urlpath(urlpath)
 
     if is_fsspec_url(urlpath):
         return _open_fsspec_url(urlpath, mode, offset, kwargs)

src/blosc2/storage.py

@@ -10,7 +10,7 @@
 from dataclasses import asdict, dataclass, field, fields
 
 import blosc2
-from blosc2.core import is_fsspec_url
+from blosc2.core import is_fsspec_url, normalize_urlpath
 
 
 def default_nthreads():
@@ -249,6 +249,7 @@ class Storage:
     meta: dict = None
 
     def __post_init__(self):
+        self.urlpath = normalize_urlpath(self.urlpath)
         if is_fsspec_url(self.urlpath):
             # The C layer writes a container incrementally, rewriting its header
             # and offsets as chunks land; an object store has no partial writes

tests/ndarray/test_proxy.py

@@ -155,14 +155,25 @@ def test_reuse_cache_rejects_foreign_container(tmp_path):
         blosc2.Proxy(source, urlpath=path, mode="a")
 
 
-def test_reuse_cache_rejects_mismatched_source(tmp_path):
+@pytest.mark.parametrize(
+    "other",
+    [
+        lambda data: blosc2.asarray(np.arange(50, dtype=np.float64)),
+        # Same shape and dtype, different partitioning: chunk numbers are what
+        # the proxy passes to the source, so this would silently fetch the
+        # wrong chunk or run off the end
+        lambda data: blosc2.asarray(data, chunks=(2, 5), blocks=(1, 5)),
+    ],
+    ids=["shape", "chunks"],
+)
+def test_reuse_cache_rejects_mismatched_source(tmp_path, other):
     proxy_path = str(tmp_path / "proxy.b2nd")
     data = np.arange(120, dtype=np.int32).reshape(12, 10)
-    blosc2.Proxy(blosc2.asarray(data), urlpath=proxy_path, mode="a").fetch()
+    source = blosc2.asarray(data, chunks=(4, 5), blocks=(2, 5))
+    blosc2.Proxy(source, urlpath=proxy_path, mode="a").fetch()
 
-    other = blosc2.asarray(np.arange(50, dtype=np.float64))
-    with pytest.raises(ValueError, match="does not fit"):
-        blosc2.Proxy(other, urlpath=proxy_path, mode="a")
+    with pytest.raises(ValueError, match="different source"):
+        blosc2.Proxy(other(data), urlpath=proxy_path, mode="a")
 
 
 # Test the ProxyNDSources interface

tests/test_fsspec.py

@@ -480,6 +480,38 @@ def test_http_does_not_reach_fsspec():
         blosc2.open("http://localhost:1/foo.b2nd")
 
 
+def test_file_url_uses_the_local_path(tmp_path):
+    # file:// is kept off the fsspec branch so it can use mmap and every
+    # container format, which only works if the scheme is stripped first
+    a = blosc2.arange(10, dtype="i4")
+    url = (tmp_path / "f.b2nd").as_uri()
+
+    a.save(url)
+    assert (tmp_path / "f.b2nd").is_file()
+    assert np.array_equal(blosc2.open(url)[:], a[:])
+    assert np.array_equal(blosc2.open(url, mmap_mode="r")[:], a[:])
+
+
+def test_file_url_backs_a_container(tmp_path):
+    url = (tmp_path / "c.b2nd").as_uri()
+    a = blosc2.arange(10, dtype="i4", urlpath=url, mode="w")
+    a[0:5] = 7
+    assert np.array_equal(blosc2.open(url)[:], a[:])
+
+
+def test_cached_dir_refetches_on_same_size_change(tmp_path):
+    # Sizes and names alone cannot see this, and memory:// has no mtime to fall
+    # back on, so the manifest has to use each backend's own identity token
+    memfs = fsspec.filesystem("memory")
+    memfs.pipe_file("/samesize.b2d/a.bin", b"A" * 100)
+    localdir = blosc2.core.localize_fsspec_url("memory://samesize.b2d", tmp_path)
+    assert pathlib.Path(localdir, "a.bin").read_bytes() == b"A" * 100
+
+    memfs.pipe_file("/samesize.b2d/a.bin", b"B" * 100)
+    localdir = blosc2.core.localize_fsspec_url("memory://samesize.b2d", tmp_path)
+    assert pathlib.Path(localdir, "a.bin").read_bytes() == b"B" * 100
+
+
 def test_local_path_untouched(tmp_path):
     urlpath = str(tmp_path / "local.b2nd")
     a = blosc2.arange(10, dtype="i4", urlpath=urlpath, mode="w")