Support fsspec - #700
Conversation
Staged design notes for letting blosc2.open() accept fsspec URLs, so containers can live in S3, GCS, Azure, archives or memory without the caller downloading them first. Three phases, each shippable alone: a whole-object read via from_cframe, a local filecache layer that restores full format coverage and mmap, and byte-range access. The recommendation is to ship phase 1 and wait, since nearly every open design question belongs to the caching layer rather than to the feature itself. Records what was verified while scoping rather than assumed: a .b2nd file is a contiguous frame that from_cframe reconstructs; fsspec's filecache hands back a local path blosc2.open() already accepts; frame.c routes every read through blosc2_io_cb, so byte-range access needs no c-blosc2 change, though third-party backends re-open per lazy block and the callbacks run on the worker threads. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
blosc2.open(), save_array() and save_tensor() now accept any fsspec URL (s3://, gs://, zip://, memory://, and chained ones), behind a new optional [fsspec] extra. The container is transferred whole in one GET/PUT, which covers single-file containers in read mode; .b2d directories, offset != 0 and mode != 'r' raise NotImplementedError. The write branch lives in pack_tensor() so that save_array, save_tensor and pack_array2 all inherit it from one place. file:// and http(s):// keep their existing routes, the latter to C2Array. Protocol drivers (s3fs, gcsfs...) and their credentials stay the caller's concern: fsspec already names the missing package, so we do not mirror that table. Phases 2 (local cache) and 3 (byte-range chunk access) of the plan stay unstarted, awaiting a user who hits the in-memory ceiling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
blosc2.open(url, cache_storage=dir) downloads the container into dir and opens it as an ordinary local path, so directory containers (.b2d stores, sparse frames), offset and mmap_mode all work -- everything the whole-object in-memory read cannot do. Repeated opens then cost a staleness check instead of a transfer. Single files ride fsspec's filecache, with check_files=True: fsspec does not verify staleness by default and happily served a cached array whose remote bytes had changed. Directory containers have no such layer in fsspec, so the prefix is fetched whole and re-fetched whenever the remote listing stops matching a JSON manifest written at download time. Caching stays opt-in with no default location: an implicit cache filling a disk with multi-GB arrays is not a good surprise. Write-back is still out of scope, as is phase 3 of the plan. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
blosc2.open(url, lazy=True) leaves the container in the object store and returns a Proxy that fetches only the chunks a slice touches, one range read each. On a 36 KB frame over memory://, opening costs 276 bytes and a 50-element slice 2 KB. This is route 3a of the plan, which it had parked behind 3b's I/O-callback bridge on the grounds that we have no way to get chunk offsets out of a cframe. We do: the frame header is a msgpack array, so unpacking it yields header_len, the compressed size and the b2nd metalayer (shape, chunks, blocks, dtype) with exactly one field -- header_len itself -- located by hand. The offsets are a Blosc2 chunk at header_len + compressed_size, relative to the end of the header, and a negative offset is a run-length chunk that was never written and is rebuilt locally. FsspecNDSource is exported, so a Proxy over it can be given a persistent cache file. aget_chunk overlaps fetches on async backends, which is where the S3 win is; memory:// is not async, so only the blocking fallback is covered by tests. Contiguous frames holding an NDArray only: plain SChunks, sparse frames and .b2d stores raise and point at cache_storage=. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
test_http_still_goes_to_c2array asserted FileNotFoundError, which is not C2Array routing: a bare http(s) URL is not a C2Array at all, since that path is entered through blosc2.URLPath. The invariant being tested is that http(s) never reaches fsspec, so say that instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Proxy(src, urlpath=..., mode="a") always called blosc2.empty() on that path, so a cache only worked the first time: on the next run the file was there and the constructor died with "Could not build empty array". That made a persistent chunk cache -- the whole point of pointing a proxy at a file -- reachable only through the private _cache= escape hatch. Now mode="a" over an existing container adopts it, and chunks fetched by an earlier run are not fetched again. The reopen goes through blosc2_ext.open rather than blosc2.open, which would try to rebuild the source we already hold and raises outright for sources it cannot reconstruct from the cache metadata. Two guards, because silently serving the wrong bytes is worse than failing: the container must carry the proxy-source metalayer, and its shape and dtype must match the source. Anything else raises instead of being reused or overwritten. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The fsspec section had grown a 30-line API tutorial -- three usage modes, five code blocks, a note on cache lifetimes -- one commit at a time, on a page whose other extras get a table row and a pointer. All of it already lives in blosc2.open's docstring and the FsspecNDSource reference, so cut it to the table row, the pip recipe with the backend, and one sentence naming the three modes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Same accretion as the install page, in three more places. open()'s Notes had grown three fsspec bullets restating what the lazy= and cache_storage= kwargs entries already said, in a section whose other bullets are three lines; the release notes had four bullets for what a reader experiences as one feature plus one fix; and fsspecndsource.rst repeated the class docstring that autoclass renders right below it. Each fact now lives where a reader would look for it: the kwargs entries for what an option does, one Notes bullet for the install and read-only caveats, the class docstring for the persistent-cache recipe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
They were mutually exclusive, which made the persistent chunk cache reachable only by building the Proxy by hand -- open() cannot forward urlpath= to it, that being open()'s own parameter. But the two knobs answer different questions: cache_storage says where this container's local copy lives, lazy says whether that copy is the whole thing or just the chunks touched so far. So blosc2.open(url, lazy=True, cache_storage=dir) now keeps the fetched chunks in a container under dir, and a later run starts from them. The cache is stamped with the remote size and mtime and discarded when they change. A stale chunk cache is worse here than in the whole-object case: the chunks were fetched by offsets read from a frame that no longer exists, so they are not old data but wrong data. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The stamp was [size, mtime or LastModified or ""], which on memory:// -- the only backend the tests use -- collapses to size alone, since it exposes neither key. A frame replaced by one of identical size was therefore served from the stale cache: chunks fetched at offsets read from a file that no longer exists. The existing test missed it because compression made the two frames different sizes. fs.ukey() is fsspec's own token for "these exact bytes", the one filecache checks, so it works off whatever each backend actually exposes instead of the fields we guessed it would. The test now writes both frames uncompressed so their sizes match and only a real content check can pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The write side only covered save_array/save_tensor, so blosc2.save() and NDArray.save() -- the natural calls for a container that already exists -- died with "Error while copying the array": they route through copy(), and the C writer cannot target a URL. They now build the cframe and upload it as a single object, honouring cparams/chunks kwargs by copying in memory first. contiguous=False raises, a sparse frame being a directory. Constructors given a URL (zeros, asarray, copy, SChunk) cannot work at all: the C layer rewrites a frame's header and offsets as chunks land, and an object store has no partial write. They used to fail as "Could not build zeros array" from deep in C; Storage and SChunk.__init__ now reject the URL up front and name save() as the way to do it. Whole-object replace is the only shape a remote write takes here, so two writers to the same key silently lose one. Said in the docstring rather than left to be discovered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Covers the three read modes and the write, over memory:// so it runs with no network, no credentials and no protocol driver -- with the s3:// form in a comment, since swapping the URL is the only change needed. Ends on a LazyExpr over the lazy handle, which is the property worth showing: slicing an expression built on a remote array still fetches only the chunks that slice needs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
One line each on the install page and the FsspecNDSource reference, in the style random.rst already uses for random-constructor.py. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
get_chunk cost two reads: sixteen bytes for the chunk header, to learn the compressed size, then the chunk itself. Bounding the read by whatever is stored next -- the following chunk in file order, or the offsets chunk -- and truncating locally to the size in the header does it in one, halving the round trips a slice costs against an object store. The bound is capped at what a chunk can weigh, so the hole an updated chunk leaves behind cannot turn into an absurd read, and it comes from the sorted offsets rather than the next index, since a rewritten chunk is appended at the end and leaves the offsets non-ascending. Covered by a test that updates a chunk before uploading. This also drops the shared file handle: the source now keeps no file position, which is what makes it safe to call get_chunk from several threads at once. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Ordinary slicing goes through fetch(), which pulled chunks one at a time -- so a slice spanning 12 chunks was 12 serial round trips against an object store, whatever afetch() could do. fetch() now takes max_concurrency=, and reads it from the source when the source carries one, so blosc2.open(url, lazy=True, max_concurrency=8) applies to every slice without the caller ever naming fetch(). Threads, not asyncio: driving afetch() from __getitem__ would mean asyncio.run() inside a sync method, which raises inside any running event loop -- every notebook -- and would be the first sync-over-async in the library. A thread pool works the same in a script, a notebook, and someone else's event loop, and fsspec releases the GIL on socket I/O. Cache writes stay on the calling thread; only the fetches fan out. Default 1, i.e. exactly today's behaviour, since the gain is invisible against memory:// and needs a real endpoint to justify a different default. The test proves overlap rather than timing it: each fetch waits on a two-party barrier, which only clears if another fetch is in flight. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Serial-by-default was the inconsistent choice, not the conservative one: afetch() already used REMOTE_MAX_CONCURRENCY for remote sources, so the same source fetched eight at a time when awaited and one at a time when sliced. The speedup is still unmeasured here, but the cost of being wrong is not. Over memory://, where the pool can only lose, a 100-chunk read goes from 1.1 ms to 2.2 ms -- about 10 us per chunk, against the ~30 ms an S3 round trip costs. Pass max_concurrency=1 for a protocol with no latency to hide. Sources other than FsspecNDSource are unaffected: fetch() still reads the attribute from the source and falls back to serial, since concurrency is only safe for a thread-safe get_chunk. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Shows what max_concurrency= buys: 7x on a 100-chunk read, 5x on a 12-chunk slice, and free on a repeat since the proxy caches. Nothing available offline has latency to show it with -- memory://, zip:// and tar:// are all local reads, where the pool only costs ~10 us per chunk -- and http:// is reserved for Caterva2, so a local server is not an option either. The example therefore subclasses fsspec's in-memory filesystem with a fixed 5 ms delay and says so plainly, rather than implying a benchmark it cannot run. Against a real bucket the delay is real and the code is identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
One clause on the install page, one sentence on the FsspecNDSource reference, next to the max_concurrency parameter it demonstrates. The reference notes the round trip is simulated, so nobody reads the numbers as a benchmark of a real bucket. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Corrects the record on the cache stamp, which the plan still described as a size-and-mtime tuple after it became fs.ukey(). Adds the two findings worth keeping. First, the tier-3 trigger fired -- phase 3 exists -- and the answer is still no moto: of the three things memory:// cannot reach, moto buys two, and blockcache::memory:// covers one of those for free. Second, and less expected, memory:// misleads by being *poorer* in metadata than any real backend, which is what let a size-only stamp serve a stale chunk cache; moto would have hidden that bug rather than caught it. The concurrency default is now backed by numbers on both sides, and the open questions collapse to one: how any of this behaves against a real endpoint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tests/test_fsspec.py opens with importorskip("fsspec"), which is right --
the extra is optional -- but nothing in the test group pulls fsspec, so all
47 tests skipped in every CI job. The feature had zero coverage there while
looking green.
s3fs and friends stay out deliberately: the tests run on memory:// and a
local zip, and no backend installed is also the configuration most users of
the [fsspec] extra are in, so it is the one worth exercising. Excluded on
wasm32, following the other platform-gated test deps.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
max_concurrency=8 is the one number on this branch chosen by argument rather than measurement: the pool's cost was measured, the gain never was, because nothing offline has a round trip to hide. This sweeps 1..32 over a slice and a whole-array read and prints where the curve flattens. It also runs afetch(), whose async path has never executed at all -- memory:// is not an async backend, so the test suite only ever reaches its blocking fallback. Endpoint and anon options go through fsspec.config.conf, since blosc2.open() has no storage_options= passthrough; that is also the only way to point any of this at R2, B2 or MinIO today, and worth revisiting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
moto was dismissed earlier in this work, but that was about adding it as a CI test dependency. As a local endpoint for running this benchmark by hand it is the easiest of the options -- one pip install, no Docker, no binary -- and it is what s3fs's own test suite runs against. With the caveat that matters: it is a single-process Python mock with no latency, and it may serialize requests, so the sweep against it can show a flat or inverted curve while the client is behaving perfectly. It answers whether the async path runs, not how fast anything is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Pointed at a live S3 endpoint for the first time (moto server + s3fs), the
async path failed on every chunk:
HTTPClientError: ... got Future <...> attached to a different loop
fsspec drives an async filesystem's coroutines on a private event loop of
its own, in a background thread. Awaiting fs._cat_file() from the caller's
loop therefore uses a client built on one loop from another, which
aiobotocore rejects. Its blocking API is the supported way in, so
aget_chunk now hands get_chunk to a worker thread: that call dispatches to
fsspec's own loop, so the thread parks on a queue rather than on a socket,
and afetch() keeps overlapping fetches as before.
memory:// cannot catch this -- it is not an async backend, so aget_chunk
took the fallback branch there and everything passed. The test now asserts
the mechanism instead of only the result: afetch must reach get_chunk, which
fails if anyone awaits the filesystem coroutine again.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds fsspec-backed remote container access, including whole-object transfers, local caching, and lazy range reads.
Changes:
- Adds remote open/save support and cache invalidation.
- Introduces concurrent lazy chunk fetching through
FsspecNDSource. - Adds packaging, tests, examples, and documentation.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/blosc2/core.py |
Adds fsspec detection, caching, and tensor I/O. |
src/blosc2/schunk.py |
Dispatches remote opens and lazy proxies. |
src/blosc2/proxy.py |
Adds persistent caches and FsspecNDSource. |
src/blosc2/ndarray.py |
Supports remote NDArray.save(). |
src/blosc2/storage.py |
Rejects incrementally backed remote containers. |
src/blosc2/__init__.py |
Exports FsspecNDSource. |
tests/test_fsspec.py |
Tests remote I/O, caching, and lazy reads. |
tests/ndarray/test_proxy.py |
Tests persistent proxy cache reuse. |
pyproject.toml |
Adds fsspec dependencies. |
RELEASE_NOTES.md |
Documents the feature. |
plans/fsspec-support.md |
Records design and implementation details. |
examples/ndarray/rw-fsspec.py |
Demonstrates remote I/O modes. |
examples/ndarray/concurrent-fsspec.py |
Demonstrates concurrent fetching. |
doc/reference/fsspecndsource.rst |
Adds API reference documentation. |
doc/reference/classes.rst |
Adds the new reference page. |
doc/getting_started/installation.rst |
Documents installation and usage. |
Suppressed comments (2)
src/blosc2/core.py:685
- This manifest can serve stale directory data after a same-size overwrite. Backends such as
memory://expose neithermtimenorLastModified, so an unchanged filename and size produce the same manifest even when its bytes changed. Use each filesystem'sukey()(as the lazy cache already does) so content/version identity participates in invalidation.
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())
},
src/blosc2/proxy.py:810
- The actual async-driver branch is untested:
test_lazy_afetchusesMemoryFileSystem, whoseasync_implis false, and therefore only exercises the blocking fallback above. Add a fake async fsspec filesystem test that reaches_cat_fileand verifies range arguments, overlapping requests, and propagation of fetch errors before relying on this path for S3/GCS.
data = await self._fs._cat_file(self._path, start=offset, end=end)
return data[: struct.unpack("<i", data[12:16])[0]]
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
memory:// cannot see the class of bug that lives on a real backend, and has now hidden two: aget_chunk awaiting an async filesystem's coroutine (fine on memory://, which is not async, and broken on every chunk against s3fs), and a cache stamp that degraded to size-only because memory:// exposes no mtime. tests/test_fsspec_s3.py runs moto in-process (ThreadedMotoServer on a free port, so xdist workers do not collide) with s3fs in front of it: a real S3 protocol, real range requests, a real async backend, and still offline -- no credentials, no network, so no `network` marker. Eight tests in ~3 s. Reverting the aget_chunk fix fails exactly the two that cover it. moto[server] and s3fs go into the test group as well as dev, so this runs on push rather than only when someone remembers. That is a heavier dependency tree on every job; if it ever churns badly enough to break installs, moving both to dev-only and running them nightly is the fallback. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
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>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Suppressed comments (5)
src/blosc2/core.py:761
- An explicit
mode="r"is silently discarded here, after which the remote object is overwritten. Local saves reject writes in reading mode, so the fsspec path must validate the mode before removing it rather than bypassing that protection.
remote_urlpath = kwargs.get("urlpath") if is_fsspec_url(kwargs.get("urlpath")) else None
if remote_urlpath is not None:
del kwargs["urlpath"]
kwargs.pop("mode", None)
src/blosc2/proxy.py:779
- This opens a buffered fsspec file to read only the frame index. On s3fs the default block size is 50 MiB, so the initial 24-byte read can fetch a 50 MiB block and the seek to the index can fetch another, defeating the advertised small lazy-open transfer. Parse the header/index with exact
cat_file(start=..., end=...)range reads, or explicitly disable buffering.
with fs.open(path, "rb") as f:
raw, header, self._offsets = _read_frame_index(f)
src/blosc2/ndarray.py:7027
- Normalization happens before a mapping passed via
storage=is expanded. Consequentlyblosc2.zeros(..., storage={"urlpath": "memory://..."})bypasses bothStorage.__post_init__and this normalization, reaching the C layer instead of producing the new explicit fsspec error;file://paths in such mappings also remain unnormalized. Normalize/validate the merged storage URL (or construct aStoragefrom mappings) before returning kwargs.
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"])
src/blosc2/core.py:631
- A non-local file authority is concatenated without the leading
//. On Windows,file://server/share/a.b2ndtherefore becomes the relative pathserver\\share\\a.b2ndinstead of the UNC path\\\\server\\share\\a.b2nd(and is relative on POSIX too). Preserve//for host authorities while retaining the specialC:drive handling.
This issue also appears on line 758 of the same file.
# A Windows drive lands in netloc for the two-slash form, `file://C:/x`
prefix = parsed.netloc if parsed.netloc.lower() not in ("", "localhost") else ""
return urllib.request.url2pathname(prefix + parsed.path)
plans/fsspec-support.md:31
- These links use a developer's absolute
/Users/faltet/...paths, so they resolve to broken links when this plan is viewed from the repository. The same pattern recurs throughout the document; replace all of them with repository-relative links.
Today there is no S3 support at all. `s3fs` appears in the repo only in
[bench/ndarray/download_data.py](/Users/faltet/blosc/python-blosc2/bench/ndarray/download_data.py)
and in the `dev` dependency group of
[pyproject.toml](/Users/faltet/blosc/python-blosc2/pyproject.toml). Passing
`s3://...` to `blosc2.open()` falls through the store-probing branches in
[src/blosc2/schunk.py](/Users/faltet/blosc/python-blosc2/src/blosc2/schunk.py)
| else: | ||
| here = (schunk.nbytes, schunk.chunksize, schunk.typesize) | ||
| there = (self.src.nbytes, self.src.chunksize, self.src.typesize) | ||
| if here != there: |
There was a problem hiding this comment.
Good catch, and it was a real gap — fixed in bf87015.
blosc2.open(url, lazy=True, cache_storage=...) did compare identity: _lazy_fsspec_proxy() removed the cache when _cache_stamp(path) != src.stamp (fs.ukey(), which is fsspec's own token rather than metadata we guess a backend exposes). But the hand-built form the FsspecNDSource docstring recommends —
src = blosc2.FsspecNDSource("s3://bucket/big.b2nd")
a = blosc2.Proxy(src, urlpath="big-cache.b2nd", mode="a")— went straight to Proxy and skipped that check entirely, which is exactly the hole you describe.
So the check moved into Proxy itself: __init__ stamps the cache with src.stamp for any source that has one, and _reopen_cache() refuses a cache whose stamp disagrees. _lazy_fsspec_proxy() no longer stamps by hand; its remove_urlpath() is what turns that refusal into a refetch. Covered by test_handbuilt_proxy_rejects_a_stale_cache.
On the second half of the suggestion — rejecting adoption when the source cannot provide an identity — I deliberately did not do that. Geometry-based adoption for plain ProxySource/ProxyNDSource implementations is the documented behaviour of mode="a" (test_reuse_cache_across_runs), not an oversight; a user source has no identity to offer, and refusing it would remove the feature rather than harden it. Sources that can name themselves are now checked, and those are the ones where a replaced source is actually reachable.
| # An object store takes the whole thing at once, and always replaces | ||
| kwargs.pop("mode", None) | ||
| array = self.copy(**kwargs) if kwargs else self | ||
| with fsspec_open(urlpath, "wb") as f: |
There was a problem hiding this comment.
Correct — fixed in 5aec265.
NDArray.save() now runs the same guard local persistence does before touching the remote object:
# An object store takes the whole thing at once, and always replaces,
# but reading mode still forbids a write
blosc2_ext.check_access_mode(urlpath, kwargs.pop("mode", "w"))The identical bug in pack_tensor() (which you flagged as a suppressed comment on core.py:761) is fixed the same way. Both are covered by test_save_to_url_rejects_reading_mode.
Correctness: - A cached copy kept fsspec's plain hash as its name, so a `.b2e` store came back as a bare SChunk: `blosc2.open()` dispatches on the extension alone. - A lazy open of an empty array read the trailer as an offsets chunk and died with a decompression error. - A half-written cache left by an interrupted run was fatal to every later `lazy=True` open, rather than being discarded like a stale one. - `mode="r"` was dropped on the way to an fsspec URL in `NDArray.save()` and `pack_tensor()`, which then overwrote the object. - A `file://` URL naming a host built a relative path instead of a UNC one. - A `storage=` mapping never reached `Storage.__post_init__`, so it skipped both the `file://` normalization and the fsspec rejection. Silent no-ops, now rejected: - `max_concurrency=` outside `lazy=True`. - Constructor kwargs (`contiguous=`...) handed to a Proxy that reuses a cache. Also: read the frame index through exact ranges rather than a buffered handle, which on s3fs fetched a 50 MiB block per seek; make the cache-mismatch message name the fields it is actually comparing; and test slice membership against a set rather than scanning a numpy array per chunk. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Same shape, dtype and partitioning is not the same bytes: a replaced remote frame keeps its layout while every cached chunk, and every offset it was fetched by, goes stale. blosc2.open() already refetched in that case, but the hand-built form the FsspecNDSource docstring recommends went straight to the Proxy and skipped the check. So the Proxy stamps its cache with whatever identity the source can name itself by, and refuses one built against other bytes. Sources that have no identity to give are still adopted on geometry alone, as documented. This also takes over the stamping _lazy_fsspec_proxy() was doing by hand. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks — the two inline comments are answered in their threads. The five suppressed ones were all valid too, and all five are fixed; since they have no thread to reply to, here they are one by one.
if kwargs.get("urlpath") is not None:
# A Storage instance normalized (and vetted) its own on construction; a bare
# kwarg or a `storage=` mapping never reached __post_init__, so borrow it here
kwargs["urlpath"] = blosc2.Storage(urlpath=kwargs["urlpath"]).urlpathTest:
The review also surfaced nothing about the Windows CI failure, which was mine: A separate review pass found seven more, fixed in the same two commits: a cached All 35 checks are green, including Windows. Locally: 9399 passed / 29 skipped, plus the 8 moto-backed S3 tests in |
Whether a chunk is already cached could not be read off the cache itself: a fetched chunk that is a run of a single value is stored as a special chunk, exactly like the empty ones blosc2.empty() leaves behind. Every such chunk was therefore refetched on every access and on every run, which for a full() array behind an fsspec URL meant a range request per chunk, forever. Keep a bitmap of the chunks brought over instead, persisted in the cache's vlmeta so a later run picks up where this one stopped. It also replaces the per-call scan over the chunk infos in fetch() and afetch(). Two smaller fixes on the way: - Say in the Proxy docstring what adopting a cache actually checks: geometry only, unless the source can name the bytes it reads as FsspecNDSource does. - Shut the fetch pool down with cancel_futures: map() queues every chunk up front, so an error (or a Ctrl-C) waited for thousands of pending requests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
file://C:/x names the host C:, which only Windows can reach, so POSIX now raises instead of silently producing a relative path. And the suffix check that routes .b2d stores to cache_storage= saw the query string, not the name; strip it (and any fragment) before comparing.
One _mark_fetched helper for the three bitmap set-bits, the partial- failure rationale lives in _save_fetched's docstring instead of two comments, and get_slice_nchunks is already unique and ordered so the set-and-sort in _missing_chunks goes.
A weakref.finalize callback (_cleanup_in_memory_store) pops from the process-global _DATA_CACHE, _SIDECAR_HANDLE_CACHE and the hot cache whenever an in-memory indexed array dies, which can be at GC time in the middle of a comprehension over one of those dicts -- and then the next iteration step raises RuntimeError: dictionary changed size during iteration. Seen on Windows CI in test_indexed_matches_unindexed. tuple()/list() copies are atomic under the GIL, and are the idiom the rest of the file already uses.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
src/blosc2/proxy.py:358
- The fetched bitmap now overrides the cache's actual special state. After a chunk is fetched, calling
proxy.schunk.update_special(n, SpecialValue.UNINIT)leaves this bit set, so the next access serves the uninitialized cache chunk instead of refetching it. That regresses the documented cache-eviction behavior ofSChunk.update_special(src/blosc2/schunk.py:757-760). Keep explicit eviction synchronized with this bitmap and cover the fetch → evict → refetch path.
def _missing_chunks(self, item) -> list[int]:
"""The chunks *item* touches, minus those already in the cache."""
nchunks = self._schunk_cache.nchunks
# Full realization when item is (), else only the chunks it intersects
wanted = range(nchunks) if item == () else list(blosc2.get_slice_nchunks(self._cache, item))
return [int(n) for n in wanted if not self._fetched[n // 8] >> (n % 8) & 1]
src/blosc2/proxy.py:242
- This paragraph states that a same-geometry changed source is adopted immediately after citing
FsspecNDSource.stamp, but stamped sources are actually rejected by_reopen_cache. Clarify that geometry-only adoption applies only to sources without an identity; a stamp mismatch raises.
Geometry is all that is checked unless the source can name the exact
bytes it reads, as :ref:`FsspecNDSource` does with its ``stamp``: a
source whose contents changed underneath while its geometry did not
is adopted, and the cache keeps serving what the earlier run fetched.
Pass ``mode="w"`` when the source may have been rewritten.
| if vlmeta: | ||
| for key in vlmeta: | ||
| self._schunk_cache.vlmeta[key] = vlmeta[key] |
get_lazychunk() decided whether to cap the buffer at MAX_OVERHEAD by testing the special-value bits (0x70) of blosc2_flags, where the lazy flag is 0x08. So every ordinary chunk of a file-backed frame came back as 32 bytes, with its bstarts and trailer -- the block offsets and per-block compressed sizes that are the whole point of a lazy chunk -- thrown away. The 0x70 test was itself a workaround for testing 0x08 alone, which truncated the repeated value off special chunks; one test was deciding two cases. Cap only when the chunk is neither lazy nor special, so a whole chunk sitting in memory still does not get copied, which is what the cap is for. Nothing had noticed because every caller reads header fields only, and the sparse-gather path calls blosc2_schunk_get_lazychunk from C without going through this wrapper. Also guard the NULL chunk that an in-memory schunk returns for an empty data slot, which the flags read dereferenced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lazy=True fetches a whole compressed chunk per range request. Blocks are the smaller unit blosc2 already compresses independently, so a slice could fetch only the ones it touches. plans/fsspec-blocks.md works out what that would take and what it would buy. The format cooperates more than expected, and the analysis is grounded in probes rather than in the format docs: a block decodes standalone inside a synthetic one-block chunk; a chunk holding only some of its blocks, with csize == 0 zero streams standing in for the rest, is valid and update_chunk accepts it, so partial chunks can live in the cache with no compression on the fetch path; and bstarts is not monotonic (a multithreaded compressor writes blocks in completion order), so extents need the sorted-neighbour rule the chunk reader already uses. Measured against real S3 with the new benchmark, which computes exact touch ratios locally and then replays both request patterns over the network: 5-17x on arrays with multi-MB chunks, 2-5x on 1 MB chunks, and 0.5-0.7x -- one extra round trip -- on small ones or on slices that want most of their blocks anyway. Break-even is around 1 MB of compressed chunk, and it barely moves with the endpoint. Default block shapes are full in the trailing dimensions, so a column touches every block of every chunk it touches; the whole-chunk fallback is part of the design rather than a refinement of it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This gives access to the remote protocols that fsspec provides.