Skip to content

Commit ea09cb8

Browse files
FrancescAltedclaude
andcommitted
Fix six defects found reviewing the frame reader
All reproduced first; the first two make lazy=True unusable for whole classes of ordinary arrays. The frame header was unpacked with raw=False. Its flags field is a msgpack *string* holding four raw bytes, and clevel rides in the high nibble of one of them, so from clevel=8 up it is not valid UTF-8 and every lazy open died with UnicodeDecodeError. Unpacked raw now. _special_chunk rebuilt run-length chunks with compress2 and no blocksize, which makes blosc2 take the whole chunk. Whenever blocks != chunks -- the default for a large chunk -- the cache rejected the chunk with "Error while getting the buffer", and with cache_storage= the bad chunk was written to disk. It now passes the container's blocksize. Structured dtypes are stored as their repr, so np.dtype() on the metalayer string raised TypeError; added the ast.literal_eval fallback blosc2_ext already uses. _reopen_cache dereferenced cached.shape before it could report a kind mismatch, raising AttributeError instead of the intended ValueError. normalize_urlpath dropped the drive letter for file://C:/x, where urlparse puts it in netloc. And open()'s Notes listed .b2z among the formats a plain URL read handles: it is a zip archive, not a cframe, so it needs cache_storage like the directory formats. Tests now parametrise over clevel, over blocks != chunks and over a structured dtype, since every one of these hid behind default parameters. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent d2fe249 commit ea09cb8

6 files changed

Lines changed: 111 additions & 10 deletions

File tree

plans/fsspec-support.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,16 @@ The minimum that is genuinely useful.
103103
- `.b2d` raises `NotImplementedError`; sparse frames are not detected up front
104104
and fail on the `from_cframe` instead. Both messages now point at phase 2's
105105
`cache_storage=`, which is the actual fix.
106+
- What the frame parser got wrong, found in review rather than by tests, because
107+
every test had used default parameters: the header must be unpacked with
108+
`raw=True` (the flags field is a msgpack *string* of raw bytes, and `clevel`
109+
rides in the high nibble of one of them, so from `clevel=8` up it is not valid
110+
UTF-8 and `lazy=True` raised `UnicodeDecodeError`); structured dtypes are
111+
stored as their `repr` and need the same `ast.literal_eval` fallback
112+
`blosc2_ext` uses; and a rebuilt run-length chunk must carry the container's
113+
blocksize, since `compress2` left to itself takes the whole chunk and the
114+
cache then rejects the chunk. Parametrising the tests over `clevel`, over
115+
`blocks != chunks` and over a structured dtype is what pins these.
106116
- Tests: `tests/test_fsspec.py`, 12 tests over `memory://` plus one chained
107117
`zip://…::file://` URL, in the default suite behind `importorskip("fsspec")`.
108118
No tier-2 network test, per the open question below.

src/blosc2/core.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -625,7 +625,10 @@ def normalize_urlpath(urlpath: object) -> object:
625625
container format, which only works if the scheme is stripped first.
626626
"""
627627
if isinstance(urlpath, str) and urlpath.startswith("file://"):
628-
return urllib.request.url2pathname(urllib.parse.urlparse(urlpath).path)
628+
parsed = urllib.parse.urlparse(urlpath)
629+
# A Windows drive lands in netloc for the two-slash form, `file://C:/x`
630+
prefix = parsed.netloc if parsed.netloc.lower() not in ("", "localhost") else ""
631+
return urllib.request.url2pathname(prefix + parsed.path)
629632
return urlpath
630633

631634

src/blosc2/proxy.py

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -325,6 +325,13 @@ def _reopen_cache(self, urlpath: str):
325325
# Chunk *numbers* are the currency between cache and source, so the
326326
# partitioning has to match, not just the logical shape: fetch() would
327327
# otherwise ask the source for chunk n meaning something else entirely
328+
# A cache of the other kind is a mismatch in itself, and asking it for a
329+
# shape it does not have would raise AttributeError instead of saying so
330+
if hasattr(self.src, "shape") != hasattr(cached, "shape"):
331+
raise ValueError(
332+
f"the cache at {urlpath} is a {type(cached).__name__}, which does not fit a "
333+
f"{type(self.src).__name__} source"
334+
)
328335
if hasattr(self.src, "shape"):
329336
here = (tuple(cached.shape), cached.dtype, tuple(cached.chunks), tuple(cached.blocks))
330337
there = (
@@ -680,7 +687,10 @@ def _read_frame_index(f) -> tuple[bytes, list, np.ndarray]:
680687
header_len = struct.unpack(">i", prefix[11:15])[0]
681688
f.seek(0)
682689
raw = f.read(header_len)
683-
header = msgpack.unpackb(raw, raw=False, strict_map_key=False)
690+
# raw=True because the flags field is a msgpack *string* holding four raw
691+
# bytes, and codec_flags packs clevel into its high nibble: from clevel 8 up
692+
# that byte is not valid UTF-8 and decoding the header blows up
693+
header = msgpack.unpackb(raw, raw=True, strict_map_key=False)
684694

685695
# The offsets live in a Blosc2 chunk of their own, right after the data ones
686696
index_pos = header[1] + header[5]
@@ -694,7 +704,7 @@ def _read_frame_index(f) -> tuple[bytes, list, np.ndarray]:
694704

695705
def _frame_metalayer(raw: bytes, header: list, name: str):
696706
"""Decode the *name* metalayer out of an already-read frame header."""
697-
offset = header[13][1][name] # KeyError if the frame has no such metalayer
707+
offset = header[13][1][name.encode()] # KeyError if there is no such metalayer
698708
nbytes = struct.unpack(">I", raw[offset + 1 : offset + 5])[0] # msgpack bin32
699709
import msgpack
700710

@@ -779,7 +789,11 @@ def __init__(self, urlpath: str, max_concurrency: int = REMOTE_MAX_CONCURRENCY):
779789
if dtype_format != 0:
780790
raise NotImplementedError(f"unsupported dtype format {dtype_format} in {urlpath}")
781791
self._shape, self._chunks, self._blocks = tuple(shape), tuple(chunks), tuple(blocks)
782-
self._dtype = np.dtype(dtype)
792+
try:
793+
self._dtype = np.dtype(dtype)
794+
except TypeError:
795+
# Structured dtypes are stored as their repr, as blosc2_ext does too
796+
self._dtype = np.dtype(ast.literal_eval(dtype))
783797

784798
@property
785799
def shape(self) -> tuple:
@@ -835,7 +849,13 @@ def _special_chunk(self, offset: int) -> bytes:
835849
# A run of zeros (1); uninitialized chunks (4) have no defined
836850
# content, and zeros is what reading them locally hands back too
837851
data = np.zeros(nitems, dtype=self._dtype)
838-
return blosc2.compress2(data, typesize=self._dtype.itemsize)
852+
# The blocksize has to be the container's: left to choose, blosc2 takes
853+
# the whole chunk, and the cache then rejects the chunk we hand it
854+
return blosc2.compress2(
855+
data,
856+
typesize=self._dtype.itemsize,
857+
blocksize=int(np.prod(self._blocks)) * self._dtype.itemsize,
858+
)
839859

840860

841861
class ProxyNDField(blosc2.Operand):

src/blosc2/schunk.py

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2124,9 +2124,11 @@ def open(
21242124
the driver for the protocol (``s3fs``, ``gcsfs``...), which fsspec asks for
21252125
by name when it is missing; credentials are configured there, not here.
21262126
``mode != 'r'`` always raises, as object stores have no rename and no locks.
2127-
A plain URL read holds the whole object in memory, so it covers single-file
2128-
containers (``.b2nd``, ``.b2f``, ``.b2e``, ``.b2z``) only; ``cache_storage``
2129-
and ``lazy`` above lift that, each in its own way.
2127+
A plain URL read rebuilds the object from a cframe held in memory, so it
2128+
covers ``.b2nd``, ``.b2f`` and ``.b2e`` only -- a ``.b2z`` store is a zip
2129+
archive rather than a cframe, and needs ``cache_storage`` like the
2130+
directory formats do. ``cache_storage`` and ``lazy`` above lift that, each
2131+
in its own way.
21302132
21312133
* Persistent data handling follows a strict no-hidden-writes rule:
21322134

tests/ndarray/test_proxy.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,20 @@ def test_reuse_cache_across_runs(tmp_path):
146146
np.testing.assert_array_equal(proxy[:], data)
147147

148148

149+
def test_reuse_cache_rejects_other_kind(tmp_path):
150+
proxy_path = str(tmp_path / "proxy.b2f")
151+
152+
class Source(blosc2.ProxySource):
153+
nbytes, chunksize, typesize = 1000, 100, 1
154+
155+
def get_chunk(self, nchunk):
156+
raise NotImplementedError
157+
158+
blosc2.Proxy(Source(), urlpath=proxy_path, mode="a")
159+
with pytest.raises(ValueError, match="does not fit"):
160+
blosc2.Proxy(blosc2.asarray(np.arange(1000, dtype=np.int32)), urlpath=proxy_path, mode="a")
161+
162+
149163
def test_reuse_cache_rejects_foreign_container(tmp_path):
150164
path = str(tmp_path / "plain.b2nd")
151165
blosc2.arange(0, 120, dtype=np.int32, shape=(12, 10), urlpath=path, mode="w")

tests/test_fsspec.py

Lines changed: 54 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -247,13 +247,38 @@ def test_lazy_multidim():
247247
[
248248
blosc2.zeros((1000,), dtype="f8", chunks=(100,)),
249249
blosc2.full((1000,), np.nan, dtype="f8", chunks=(100,)),
250+
# blocks != chunks: a rebuilt run-length chunk must carry the container's
251+
# blocksize, not whatever blosc2 picks when left to choose
252+
blosc2.zeros((1000,), dtype="f8", chunks=(100,), blocks=(10,)),
253+
blosc2.zeros((4_000_000,), dtype="f8", chunks=(1_000_000,)),
254+
blosc2.uninit((1000,), dtype="i4", chunks=(100,), blocks=(10,)),
250255
],
251-
ids=["zeros", "nan"],
256+
ids=["zeros", "nan", "small-blocks", "auto-blocks", "uninit"],
252257
)
253258
def test_lazy_special_chunks(arr):
254259
# Run-length chunks live in the offset itself, with no bytes in the file
255260
p = blosc2.open(_put("special.b2nd", arr), lazy=True)
256-
assert np.allclose(p[:], arr[:], equal_nan=True)
261+
assert p[:].shape == arr.shape
262+
if arr.dtype.kind == "f":
263+
assert np.allclose(p[:], arr[:], equal_nan=True)
264+
265+
266+
@pytest.mark.parametrize("clevel", [1, 5, 8, 9])
267+
def test_lazy_any_clevel(clevel):
268+
# The frame header's flags are a msgpack *string* of raw bytes, and clevel
269+
# rides in the high nibble of one of them: from 8 up it is not valid UTF-8
270+
a = blosc2.arange(0, 10000, dtype="i4", chunks=(1000,), cparams={"clevel": clevel})
271+
p = blosc2.open(_put(f"clevel{clevel}.b2nd", a), lazy=True)
272+
assert np.array_equal(p[:], a[:])
273+
274+
275+
def test_lazy_structured_dtype():
276+
data = np.zeros(1000, dtype=[("a", "<i4"), ("b", "<f8")])
277+
data["a"] = np.arange(1000)
278+
a = blosc2.asarray(data, chunks=(100,), blocks=(10,))
279+
p = blosc2.open(_put("struct.b2nd", a), lazy=True)
280+
assert p.dtype == data.dtype
281+
assert np.array_equal(p[150:250], data[150:250])
257282

258283

259284
def test_lazy_one_request_per_chunk(monkeypatch):
@@ -480,6 +505,33 @@ def test_http_does_not_reach_fsspec():
480505
blosc2.open("http://localhost:1/foo.b2nd")
481506

482507

508+
def test_zip_store_needs_cache(tmp_path):
509+
# A .b2z store is a zip archive, not a cframe, so there is nothing for the
510+
# in-memory read to rebuild
511+
localpath = str(tmp_path / "t.b2z")
512+
with blosc2.TreeStore(localpath, mode="w") as tstore:
513+
tstore["/a"] = blosc2.arange(10, dtype="i4")
514+
fsspec.filesystem("memory").pipe_file("/t.b2z", pathlib.Path(localpath).read_bytes())
515+
516+
with pytest.raises(RuntimeError):
517+
blosc2.open("memory://t.b2z")
518+
with blosc2.open("memory://t.b2z", cache_storage=tmp_path / "cache") as tstore:
519+
assert np.array_equal(tstore["/a"][:], np.arange(10, dtype="i4"))
520+
521+
522+
@pytest.mark.parametrize(
523+
("url", "expected"),
524+
[
525+
("file:///tmp/a.b2nd", "/tmp/a.b2nd"),
526+
("file://localhost/tmp/a.b2nd", "/tmp/a.b2nd"),
527+
# A Windows drive lands in the netloc for the two-slash form
528+
("file://C:/data/a.b2nd", "C:"),
529+
],
530+
)
531+
def test_normalize_file_url(url, expected):
532+
assert expected in blosc2.core.normalize_urlpath(url)
533+
534+
483535
def test_file_url_uses_the_local_path(tmp_path):
484536
# file:// is kept off the fsspec branch so it can use mmap and every
485537
# container format, which only works if the scheme is stripped first

0 commit comments

Comments
 (0)