Repository navigation
ENH: add remfile backend with pluggable backend architecture - #131
yarikoptic wants to merge 10 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #131 +/- ##
==========================================
+ Coverage 92.84% 97.04% +4.20%
==========================================
Files 13 18 +5
Lines 1188 2100 +912
==========================================
+ Hits 1103 2038 +935
+ Misses 85 62 -23 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Refactor what used to live in `datalad_fuse/fsspec.py` into a small pluggable-backend layer so datalad-fuse can serve file content from backends other than fsspec — starting with *remfile*, which offers chunk-based HTTP access optimised for HDF5-structured files (.nwb, .h5, .hdf5, .hdf, .he5, .nc, .nc4). Module layout ------------- - datalad_fuse/backends.py — Backend ABC + DEFAULT_BACKENDS. - datalad_fuse/remfile.py — RemfileBackend + RemfileWrapper. Lazy remfile import, EXTENSIONS frozenset on the class. - datalad_fuse/adapter.py — RemoteFilesystemAdapter (renamed from FsspecAdapter), DatasetAdapter, FileState, resolve_backends, create_backends, is_http_url. - datalad_fuse/fsspec.py — slimmed to FsspecBackend + aiohttp helpers, with a module-level __getattr__ emitting DeprecationWarning for the moved names so downstream imports keep working. - datalad_fuse/fuse_.py, fsspec_head.py, fsspec_cache_clear.py, __init__.py — updated to use the new adapter and expose `--backends` (comma-separated priority list). Backend semantics ----------------- - Backend ABC: `name`, `can_handle(key, mode)`, `open_url(...)`, `clear()`. - DatasetAdapter.open walks the backend chain: tries each URL of the first backend that can_handle the key, falls through to the next backend on any exception, and finally raises IOError chained to the last cause if no backend succeeded. - resolve_backends returns (spec, explicit); explicit=True for user/config spec, False for the default. create_backends logs a warning (not debug) when an explicit backend is missing. Build / test infrastructure tie-ins ----------------------------------- - setup.cfg: add `remfile` and `full` extras_require. - tox.ini: add a `py314-full` env (via factor conditionals) that installs the `full` extra and passes `--libfuse --network` to pytest. Register `network` + `ai_generated` markers. Add the new source modules to the typing env mypy invocation. - .github/workflows/test-libfuse.yml: route Python 3.14 through `py314-full` so remfile + network tests exercise on one runner. Co-Authored-By: Claude Code 2.1.114 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- test_backends.py: unit tests for can_handle dispatch, backend-chain fallback in DatasetAdapter.open (first-wins, fall-through on failure, skip-on-cannot-handle, IOError with __cause__), RemoteFilesystemAdapter lifecycle (__exit__ closes datasets), resolve_backends precedence (argument > config > default), create_backends (unknown name, unavailable silent vs. warned, all-unavailable raises), FsspecBackend BlocksizeMismatchError retry, RemfileWrapper (read/seek/iter/info with and without Content-Length), is_http_url parametrised, ABC-compliance check across backends, and integration tests against dandiarchive S3 (marked `network`). Optional-dependency gating uses a shared `requires_remfile` pytest.mark.skipif. - test_deprecations.py: verifies the old `datalad_fuse.fsspec.*` import paths still resolve to the new locations and emit DeprecationWarning; verifies FsspecBackend (canonical) imports without warning; verifies unknown names raise AttributeError (direct access) / ImportError (from-import). - conftest.py: add `--network` CLI opt-in mirroring `--libfuse`. Co-Authored-By: Claude Code 2.1.114 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a pluggable remote-file backend architecture (defaulting to remfile,fsspec) and introduces a remfile backend optimized for HDF5-style access while preserving legacy datalad_fuse.fsspec import paths via DeprecationWarning.
Changes:
- Introduces backend ABC + backend resolution/creation logic and refactors the adapter layer into
adapter.py - Adds
RemfileBackend+ wrapper, and refactorsfsspec.pyinto a backend + compatibility shim - Updates CLI interfaces and tests to support
--backendsand validate backward compatibility
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tox.ini | Installs additional extras and expands mypy targets to cover new modules |
| setup.cfg | Adds new extras (remfile, full) for optional backend dependencies |
| datalad_fuse/tests/test_forgejo.py | Updates imports to the new module locations |
| datalad_fuse/tests/test_deprecations.py | Adds tests for legacy import-path deprecation behavior |
| datalad_fuse/tests/test_backends.py | Adds unit/integration tests for backend chaining, remfile, and fsspec behavior |
| datalad_fuse/remfile.py | Implements RemfileBackend and RemfileWrapper |
| datalad_fuse/fuse_.py | Switches FUSE operations layer to use RemoteFilesystemAdapter + adds backend option |
| datalad_fuse/fsspec_head.py | Adds --backends CLI option and switches to new adapter |
| datalad_fuse/fsspec_cache_clear.py | Updates imports to the new adapter module |
| datalad_fuse/fsspec.py | Refactors into FsspecBackend, keeps async HTTP helpers, adds compatibility __getattr__ |
| datalad_fuse/backends.py | Adds Backend ABC and DEFAULT_BACKENDS |
| datalad_fuse/adapter.py | Adds backend-agnostic dataset/remote filesystem adapter and backend chain logic |
| datalad_fuse/init.py | Adds --backends option to the main FuseFS interface |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- FsspecBackend.open_url: re-raise BlocksizeMismatchError when caching is
disabled instead of calling pop_from_cache (which would AttributeError on
a plain HTTPFileSystem). Update existing retry test to use caching=True
and add a propagation test for caching=False.
- create_backends: silently skip empty entries from spec.split(",") so
inputs like "remfile,,fsspec" or "fsspec," are tolerated rather than
raising "Unknown backend: ''". Add parametrized test.
Spotted by Copilot review on PR datalad#131.
Co-Authored-By: Claude Code 2.1.132 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- tox.ini: scope the `full` extra to envs with `full` or `libfuse` factor
rather than installing it in every environment. remfile is meant to be
optional, and the `requires_remfile` skipif on the tests already handles
the case when it is not installed, so the default
`py3{10..14}-nonetwork` rows can stay lean.
- remfile.py: cap RemfileWrapper.__next__ at _MAX_LINE_BYTES (1 MiB) so
iterating a binary HDF5/NWB file (no '\n') does not download the entire
remote object in a single iteration step. Add test_iteration_caps_no_newline
exercising the cap with a shrunk limit.
Spotted by Copilot review on PR datalad#131.
Co-Authored-By: Claude Code 2.1.132 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
RemfileBackend.open_url now raises NotImplementedError when mode != 'rb'. can_handle() already returns False for text modes, so the backend chain will never call open_url() with such a mode in practice — but if a caller bypasses can_handle() we want a loud failure rather than a silently-binary handle. Add parametrized test_open_url_rejects_non_binary_mode covering 'r', 'rt', 'w', 'wb'. Spotted by Copilot review on PR datalad#131. Co-Authored-By: Claude Code 2.1.132 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pulls in datalad#133 "ENH: add S3 exporttree URL fallback for legacy datasets" (790ff48) plus its CHANGELOG entry and dependency addition. Conflict resolution: - datalad_fuse/fsspec.py: kept this branch's slim refactored module (FsspecBackend + HTTP helpers + deprecation `__getattr__`). Master added 4 methods to a DatasetAdapter still living in fsspec.py; those methods are ported to datalad_fuse/adapter.py where DatasetAdapter now lives. - datalad_fuse/adapter.py: added * `subprocess`, `boto3`, `botocore.UNSIGNED`, `BotocoreConfig`, `itertools.chain` imports * `DatasetAdapter._get_exporttree_remotes` (cached, parses git-annex:remote.log) * `DatasetAdapter._list_s3_versions` (anonymous boto3 client) * `DatasetAdapter._match_s3_version` (size + ETag disambiguation) * `DatasetAdapter.get_exporttree_urls` (versioned S3 URLs with unversioned fallback) In `DatasetAdapter.open`, the per-backend URL loop now chains `get_urls(str(key))` with `get_exporttree_urls(relpath, key)` so the S3 fallback is tried after the primary URLs for every URL-capable backend (fsspec, remfile). Lazy: boto3 calls only happen when the primary URL set is exhausted. - datalad_fuse/tests/test_exporttree.py: updated import to `from datalad_fuse.adapter import DatasetAdapter` (canonical path); the deprecation shim still exposes it from `.fsspec` but the test shouldn't trigger DeprecationWarning on every run. - CHANGELOG.md, docs/designs/20260408-s3-exporttree-url-fallback.md, setup.cfg (boto3 added to install_requires): taken from master unchanged. Co-Authored-By: Claude Code 2.1.132 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
yarikoptic-gitmate
left a comment
There was a problem hiding this comment.
Review and test drive of 3c3bdd8
The backend chain and the module split look good to me. The fsspec.py shim works, CI is green, and in a real FUSE mount remfile reads HDF5 noticeably faster. I found two real bugs in RemfileWrapper that I'd fix before merging, and two behaviour changes in the new default (remfile,fsspec) that deserve a decision or documentation. Details are in the inline comments; summary below.
Should fix before merge
RemfileWrapper.info()makes a fresh HEAD request. FUSE reaches it viagetattr(path, fh)on open handles. Against a server that rejects HEAD (which presigned S3 GET URLs do),fstaton an open file fails withHTTPError 403; fsspec gets the size fine. The fix needs no network call:self._f.length.RemfileWrapper.read(): with no size it returnsb""and moves the position back by 1. It never clamps at EOF. For files whose size is a multiple of remfile's 100 KiB chunk, a read at EOF raises416 Range Not Satisfiable, which also breaks line iteration and sofsspec-head -n. The suggested clamp fixes all three. Also minor:seek()returnsNone.
Behaviour changes to decide on or document
- Dead URLs stall about 30 s each under remfile (its open probe retries 8× with backoff), versus about 1 s under fsspec. I measured 30.2 s vs 1.07 s on dandiset 000027. In
fusefsthis happens under the globalrwlock, so the whole mount stalls. --caching=ondiskno longer applies to HDF5 files once remfile is installed: no cache files were written, andfsspec-cache-clearhas nothing to clear for them.
Smaller points
- URL/VURL keys (
addurl --fast/--relaxed) have no suffix, so.nwbfiles with such keys go to fsspec. Falling back torelpath's extension would fix that. datalad.fusefs.backendsis read from the globalcfg, so dataset-local config is ignored. Same as the existingcache-clearoption, butds.configis right there.- When a backend falls through,
get_urls()and the exporttree lookup (whereissubprocess, boto3) run again. - The README isn't updated: nothing on
--backends,datalad.fusefs.backends, or the[remfile]/[full]extras, and the intro still says fsspec is used. The--cachinghelp says "fsspec'ed". - Nit: with
--backends remfileonREADME.md, the error isCould not open README.md within … (backends=remfile)with no cause attached. Saying that no configured backend handles.mdwould be clearer.
What I ran
Setup: Ubuntu 24.04, Python 3.13, datalad 1.7.1, git-annex 10.20260901, remfile 0.1.15, fsspec 2026.9.0, h5py 3.16, libfuse2.
- Unit and network tests:
test_backends.py test_deprecations.py test_exporttree.py test_util.py→ 134 passed, including the S3 integration tests. - Full suite with
--libfuse: 211 passed. The rest is environmental: 8test_forgejoerrors (no docker in my sandbox) and 1 failure oftest_fuse.py::test_parallel_access, because archive.org is blocked by the sandbox network policy. That test fails the same way onmaster. - flake8 and mypy (the tox
lint/typinginvocations): clean. - End to end, own dataset:
MD5E.nwb/.hdf5/.mdfiles registered to the pinneddandiarchive.s3objects, then dropped.fsspec-headdispatched correctly: remfile for.nwb/.hdf5, fsspec for.md.--backends fsspec|remfile|bogus|' , 'and-c datalad.fusefs.backends=fsspecall behaved as described.- Through
fusefs, thesha256sumof all three files matched the originals, and h5py walked both HDF5 files (42 and 5 objects).
- Real dandiset 000004, a 73 MB
ecephys+image.nwbread throughfusefs:- Full sequential read: 4.6 s with remfile vs 22.6 s with fsspec. The checksum matched the
SHA256Ekey in both cases. - h5py open, traverse and a small slice: comparable and noisy (≈5.2 s vs 3.1–6.9 s across runs).
- Peak RSS: similar (~146 vs ~141 MiB).
- Full sequential read: 4.6 s with remfile vs 22.6 s with fsspec. The checksum matched the
- Deprecation shim: old names warn, unknown names still raise
AttributeError/ImportError, andFsspecBackendimports without a warning.
Generated by Claude Code
| # caller still makes progress instead of OOM-ing on a binary file. | ||
| return b"".join(chunks) | ||
|
|
||
| def info(self) -> dict[str, Any]: |
There was a problem hiding this comment.
Bug: the HEAD request breaks fstat() on open FUSE handles when the server rejects HEAD.
This is not "rarely called". DataLadFUSE.getattr(path, fh) takes the "File already open" branch and calls file_getattr(f) → f.info() whenever the kernel asks for attributes of an open handle (fgetattr). I saw this in a real mount log while h5py was opening ros3test.nwb. It does not depend on the key carrying a size.
The HEAD is also the request remfile itself avoids, because presigned S3 GET URLs (e.g. what DANDI's api.dandiarchive.org/.../download/ redirects to) reject HEAD. When that happens, urllib raises HTTPError, which is not a FileNotFoundError, so file_getattr does not map it to ENOENT and fusepy gets an unhandled exception.
Reproduced deterministically: a dataset whose only URL is a local server that answers HEAD with 403 and serves ranged GETs. I called ops = DataLadFUSE(ds, caching=False, backends=...), then fh = ops.open(path, O_RDONLY), then ops.getattr(path, fh):
== remfile,fsspec
open -> 1000 RemfileWrapper
getattr(path, fh) FAILED: HTTPError HTTP Error 403: Forbidden
== fsspec
open -> 1000 HTTPFile
getattr(path, fh) size: 177728
fsspec survives because HTTPFile.info() returns the details it cached at open time. remfile also already knows the size, since RemFile.__init__ fetched Content-Length with an aborted GET. So no network call is needed here:
| def info(self) -> dict[str, Any]: | |
| def info(self) -> dict[str, Any]: | |
| """Minimal info dict matching the fsspec convention. | |
| remfile already determined the size (via an aborted GET, which unlike | |
| HEAD also works for presigned URLs) when the file was opened. | |
| """ | |
| return {"type": "file", "size": self._f.length} |
With this change, the urllib.request import and the two test_info_* tests that mock urlopen need adjusting.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in d7fb6b6 BF: address review of PR #131 — RemfileWrapper semantics, URL probing, caching. info() now returns {"type": "file", "size": self._f.length} and makes no request at all. The urllib.request import stays in the module, but only for the new size probe in open_url() (see the reply on the retry/stall thread). The two test_info_* tests that mocked urlopen are replaced by test_info_uses_remfile_length, which asserts urlopen is not called.
| def read(self, size: int = -1) -> bytes: | ||
| return self._f.read(size) # type: ignore[no-any-return] |
There was a problem hiding this comment.
Bug: read() with no size returns b"" and moves the position backwards, and reading at EOF can raise.
RemFile.read() does not support size=-1. With -1, its chunk range comes out empty, so it returns b"" and then does self._position += size. Against the real S3 object:
f.seek(10); f.read() -> 0 bytes; tell() == 9
fsspec: g.seek(10); g.read() -> 177718 bytes
RemoteFilesystemAdapter.open() returns this object typed as IO, so any API user calling .read() silently gets empty data.
RemFile.read(n) has two related problems:
- It always advances by
n, even past EOF.read(100)atlength-5leavestell() == length+95. - When the size is an exact multiple of its 100 KiB chunk, a read at EOF issues an unsatisfiable Range request instead of returning
b"". That breaks__next__'s EOF detection, sofsspec-head -nfails. On a 204800-byte.h5served locally:
iteration FAILED at EOF: _PermanentHTTPError Error fetching bytes 204800-204799: 416 Range Not Satisfiable
Clamping to the remaining length fixes all of these:
| def read(self, size: int = -1) -> bytes: | |
| return self._f.read(size) # type: ignore[no-any-return] | |
| def read(self, size: Optional[int] = -1) -> bytes: | |
| # remfile requires an explicit size, does not clamp at EOF, and may | |
| # issue an unsatisfiable Range request when reading at EOF | |
| remaining = max(self._f.length - self._f.tell(), 0) | |
| if size is None or size < 0 or size > remaining: | |
| size = remaining | |
| if not size: | |
| return b"" | |
| return self._f.read(size) # type: ignore[no-any-return] |
Generated by Claude Code
There was a problem hiding this comment.
Fixed in d7fb6b6 BF: address review of PR #131 — RemfileWrapper semantics, URL probing, caching, with the suggested clamp. __next__ now reads through self.read() rather than self._f.read(), so line iteration gets the clamp too — otherwise the 416 at EOF still broke fsspec-head -n.
The mock in _make_mock_remfile was made faithful to remfile.RemFile rather than well-behaved (requires an explicit non-negative size, advances past EOF, raises 416 at EOF, seek() returns None); with that, the existing test_iteration/test_iteration_empty went red before the fix. Checked against the real S3 object too: seek(size); read(10) → b"", over-long read returns the tail and leaves tell() at EOF, read() with no size returns all 4008 bytes.
| def seek(self, offset: int, whence: int = 0) -> int: | ||
| return self._f.seek(offset, whence) # type: ignore[no-any-return] |
There was a problem hiding this comment.
Minor: RemFile.seek() has no return statement, so this returns None despite -> int. mypy doesn't catch it because self._f is Any. In the test drive, f.seek(100) gave None, while io.BytesIO(...).seek(100) gives 100.
| def seek(self, offset: int, whence: int = 0) -> int: | |
| return self._f.seek(offset, whence) # type: ignore[no-any-return] | |
| def seek(self, offset: int, whence: int = 0) -> int: | |
| self._f.seek(offset, whence) | |
| return self.tell() |
Generated by Claude Code
There was a problem hiding this comment.
Fixed in d7fb6b6 BF: address review of PR #131 — RemfileWrapper semantics, URL probing, caching, as suggested. Covered by test_seek_returns_new_position.
| raise NotImplementedError( | ||
| f"RemfileBackend only supports mode='rb', got {mode!r}" | ||
| ) | ||
| return cast(IO, RemfileWrapper(self._remfile.File(url), url)) |
There was a problem hiding this comment.
Unreachable URLs stall about 30× longer than with fsspec, and in FUSE that blocks the whole mount.
remfile.File(url) probes Content-Length and retries transient failures (connection errors, 5xx) 8 times with exponential backoff, about 25 s plus connect time. Only 4xx fails fast. The adapter tries URLs one after another, so each dead URL costs that much before the next one is tried.
Test: dandiset 000027. Its first whereis URL is api.dandiarchive.org/..., which my sandbox's network policy blocks. Its second URL is a versioned dandiarchive.s3 one. Running datalad fsspec-head -c 8 sub-RAT123/sub-RAT123.nwb:
--backends |
wall time |
|---|---|
remfile,fsspec (default) |
30.2 s |
fsspec |
1.07 s |
In fusefs, DataLadFUSE.open() calls self._adapter.open() while holding the global rwlock, so every other read on the mount waits the whole time. Dead mirrors, firewalled hosts, offline special-remote URLs and the several candidate URLs that get_urls() builds per HTTP remote all hit this.
Possible fix: do a cheap probe here with no retries and a short timeout, for example a ranged GET bytes=0-0 that also reads Content-Range to get the size, so URL fall-through stays fast. Then pass the known size to remfile (key.size is almost always available) so it skips its own probe. remfile only exposes that as the underscore-private _size=, though. It may be worth asking remfile upstream for public size= and retry parameters, or pinning the remfile version.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in d7fb6b6 BF: address review of PR #131 — RemfileWrapper semantics, URL probing, caching, along the lines you suggested. RemfileBackend.open_url() now does the cheap probe itself — one GET bytes=0-0 with a 10 s timeout and no retries — reads the total from Content-Range (falling back to Content-Length when the server ignores Range, and to None when it says neither), and passes it to remfile.File(..., _size=...) so remfile skips its own retrying probe. For a live URL that is the same single round trip it was already paying.
Measured against a missing object in the same bucket: 0.20 s, down from ~30 s. The pinned-S3 integration tests still pass unchanged.
_size= is private API, so setup.cfg now pins remfile ~= 0.1.15. Asking upstream for a public size= (and retry parameters) still seems worth doing — happy to file that separately.
| def can_handle(self, key: Optional[AnnexKey], mode: str) -> bool: | ||
| if mode != "rb": | ||
| return False | ||
| if key is None or key.suffix is None: | ||
| return False | ||
| return key.suffix.lower() in self.EXTENSIONS |
There was a problem hiding this comment.
Dispatching only on key.suffix misses HDF5 files whose keys come from a non-*E backend. git annex addurl --fast/--relaxed (and datalad addurls --fast) produce URL/VURL keys, and AnnexKey.parse sets suffix=None for those:
fast.nwb VURL-s177728--https://dandiarchive.s3.amazonaws.com/ros3test.nwb
relaxed.hdf5 VURL--https://dandiarchive.s3.amazonaws.com/ros3test.hdf5
fast.nwb: backend remfile cannot handle (suffix=None, mode=rb)
fast.nwb: opening via backend fsspec
The dandisets/* datasets use SHA256E, so they are fine. DatasetAdapter.open() already has relpath, though. Passing it to can_handle() (e.g. can_handle(key, mode, relpath)) and falling back to PurePosixPath(relpath).suffix when the key has no suffix would make the choice follow what the user sees in the tree.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in d7fb6b6 BF: address review of PR #131 — RemfileWrapper semantics, URL probing, caching. Backend.can_handle() takes a third relpath argument (optional, so the signature stays usable standalone), and RemfileBackend falls back to PurePosixPath(relpath).suffix when the key carries no suffix. DatasetAdapter.open() passes the relpath it already has. A key suffix still wins when present.
| if name == "fsspec": | ||
| backends.append(FsspecBackend(path, caching)) | ||
| elif name == "remfile": | ||
| backends.append(RemfileBackend()) |
There was a problem hiding this comment.
--caching=ondisk is now silently a no-op for HDF5 files once remfile is installed. caching is passed only to FsspecBackend. With the new default remfile,fsspec, .nwb/.h5 files bypass the on-disk cache entirely:
datalad fsspec-head --caching ondisk --backends remfile,fsspec -c 8 ros3test.nwb -> 0 files in .git/datalad/cache
datalad fsspec-head --caching ondisk --backends fsspec -c 8 ros3test.nwb -> 2 files in .git/datalad/cache
So installing datalad-fuse[full] quietly changes what --caching ondisk does for exactly the large files where it matters most. Options:
- wire
remfile.DiskCacheinto e.g..git/datalad/cache/remfile, and clear it inRemfileBackend.clear()sofsspec-cache-clearanddatalad.fusefs.cache-clearkeep working; - have
RemfileBackenddecline whencaching=True; - at least document it, and update the
--cachinghelp, which still says "cache fsspec'ed files".
Generated by Claude Code
There was a problem hiding this comment.
Fixed in d7fb6b6 BF: address review of PR #131 — RemfileWrapper semantics, URL probing, caching — the first of your three options. RemfileBackend now takes (path, caching) like FsspecBackend, builds a remfile.DiskCache at .git/datalad/cache/remfile when caching is on, passes it to remfile.File(..., disk_cache=...), and removes the directory in clear(), so fsspec-cache-clear and datalad.fusefs.cache-clear cover HDF5 files again. Verified end to end against the real S3 object: a 100-byte read writes a chunk under that directory, and clear() removes it.
The --caching help text still says "fsspeced" — that and the README are in the follow-up noted on the PR.
| """ | ||
| if backends is not None: | ||
| return backends, True | ||
| from_cfg = cfg.get("datalad.fusefs.backends", None) |
There was a problem hiding this comment.
The global datalad.cfg doesn't read the dataset's own .git/config or .datalad/config. So git config datalad.fusefs.backends fsspec inside a dataset is ignored, whether the command runs from inside it or with -d. I checked both: remfile was still used. This matches how datalad.fusefs.cache-clear is read today, so it is not a regression. DatasetAdapter.__init__ already builds Dataset(path), though, so ds.config.get("datalad.fusefs.backends") would honor per-(sub)dataset settings at no extra cost.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in d7fb6b6 BF: address review of PR #131 — RemfileWrapper semantics, URL probing, caching. resolve_backends() takes an optional config, and DatasetAdapter.__init__ passes ds.config. That is a superset of the global one rather than a replacement: ConfigManager.__init__ does self.overrides = datalad.cfg.overrides.copy(), so -c datalad.fusefs.backends=... still wins. The global cfg remains the fallback when no config is supplied.
I left datalad.fusefs.cache-clear alone, since that is outside this PR.
| primary_urls = self.get_urls(str(key)) | ||
| fallback_urls: Iterator[str] = ( | ||
| self.get_exporttree_urls(relpath, key) | ||
| if key is not None | ||
| else iter([]) | ||
| ) | ||
| for url in chain(primary_urls, fallback_urls): |
There was a problem hiding this comment.
Minor (efficiency): when the first backend exhausts its URLs, the next backend calls get_urls() and get_exporttree_urls() again. That repeats the git annex whereis subprocess (batch=False), the two examinekey calls and, on the exporttree path, the boto3 list_object_versions call. Collecting the URLs lazily into a list the first time and replaying it for later backends would avoid that, and the lazy boto3 behaviour would stay the same.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in d7fb6b6 BF: address review of PR #131 — RemfileWrapper semantics, URL probing, caching. Added a small replayable() helper: the chained get_urls() + get_exporttree_urls() iterator is pulled at most once, and each backend replays what has already been seen before pulling anything new — so the laziness of the boto3 call is unchanged, but whereis/examinekey/list_object_versions run once per open() rather than once per backend. test_urls_enumerated_once_across_backends pins the call count; test_urls_not_enumerated_when_no_backend_handles pins the laziness.
…probing, caching Six points from the review of 3c3bdd8: * RemfileWrapper.info() issued a HEAD request. FUSE reaches it through DataLadFUSE.getattr(path, fh) on every fstat() of an open handle, so against a server that rejects HEAD -- which presigned S3 GET URLs do -- fstat() failed with HTTPError. remfile already knows the size, so report self._f.length and make no request at all. * RemfileWrapper.read() delegated straight to remfile, which requires an explicit non-negative size: read() with no size returned b"" and moved the position back by one, reads never clamped at EOF, and a read at EOF issued an unsatisfiable Range request (416), breaking line iteration and hence fsspec-head -n. Clamp to the remaining length, and route __next__ through the wrapper so it sees the clamp too. * RemfileWrapper.seek() returned None despite declaring -> int. * RemfileBackend.open_url() now probes the size itself with one ranged GET bytes=0-0 on a short timeout and hands it to remfile via _size=. remfile's own probe retries 8x with backoff, so a dead URL cost ~30 s before the chain moved on -- under the global rwlock in fusefs, stalling the whole mount. Measured against a missing S3 object: 0.20 s, down from ~30 s. setup.cfg pins remfile ~= 0.1.15, since _size= is private API. * --caching=ondisk was silently a no-op for .nwb/.h5 files once remfile was installed. RemfileBackend now takes (path, caching) like FsspecBackend and wires a remfile.DiskCache at .git/datalad/cache/remfile, cleared by clear(), so fsspec-cache-clear and datalad.fusefs.cache-clear keep working. * can_handle() takes the dataset-relative path and falls back to its suffix when the annex key has none, so .nwb files with URL/VURL keys (addurl --fast/--relaxed) reach remfile. * datalad.fusefs.backends is read from the dataset's own config, which inherits the global overrides, so per-(sub)dataset settings are honored. * DatasetAdapter.open() enumerates URLs once and replays them for later backends, instead of re-running whereis, examinekey and the boto3 list_object_versions call per backend. Still open, deliberately left for a follow-up: the README says nothing about --backends, datalad.fusefs.backends or the [remfile]/[full] extras, the --caching help still says "fsspec'ed", and the "Could not open" error does not say when no configured backend handles the file. Co-Authored-By: Claude Code 2.1.292 / Claude Opus 5 (1M context) <noreply@anthropic.com>
Conflict in datalad_fuse/fsspec.py: master added docstrings to FileState, DatasetAdapter and FsspecAdapter, all of which this branch moved to datalad_fuse/adapter.py (with FsspecAdapter renamed RemoteFilesystemAdapter). Resolved by keeping this branch's slim fsspec.py and porting the docstrings into adapter.py, extended for the backend chain: the `backends` parameter, the per-backend cache directories, and what open() now returns and raises. The DataLadFUSE docstring master added in fuse_.py referred to datalad_fuse.fsspec.FsspecAdapter / .DatasetAdapter; repointed at datalad_fuse.adapter and documented its `backends` parameter. The .rst documentation master added still describes the pre-split module layout and says nothing about backends; that is the next commit. Co-Authored-By: Claude Code 2.1.292 / Claude Opus 5 (1M context) <noreply@anthropic.com>
master's documentation rework landed while this branch split fsspec.py into adapter.py / backends.py / remfile.py and added the pluggable backends, so the .rst pages still described the pre-split module layout and did not mention backends at all. Module layout, everywhere it is referenced: - datalad_fuse.fsspec -> datalad_fuse.adapter, FsspecAdapter -> RemoteFilesystemAdapter, in README.md, index, concepts, python, tutorial, troubleshooting and conf.py's lru_cache unwrapping; - python.rst and api.rst note that the old names still work with a DeprecationWarning. New material: - concepts.rst: a "Backends" section (anchor concepts-backends) comparing remfile and fsspec -- what each handles, what it fetches, how it is installed -- how the chain is configured, and how a file is dispatched (key suffix, falling back to the path's); the caching section now covers the per-backend cache directories and remfile's lack of an expiry; the failure message is the current "Could not open <path> within <dataset> (backends=...)". - installation.rst: an "Optional backends" section for the [remfile] and [full] extras (anchor installation-backends). - cli.rst: --backends on fusefs and fsspec-head, and a section on the datalad.fusefs.backends configuration option. - python.rst: the adapters' `backends` argument, and what open() now returns per backend. - api.rst: datalad_fuse.backends / .remfile / FsspecBackend, plus resolve_backends and create_backends. - troubleshooting.rst: the backend-related warnings and errors, how to see which backend took a file, and remfile's chunk size under "Reading is slow". - tutorial.rst: installs datalad-fuse[remfile], since it is all NWB. - CONTRIBUTING.md: which tox envs have remfile. Also docstrings that the documentation renders: short ones for RemfileWrapper.read/seek/tell/close, a `#:` doc for DEFAULT_BACKENDS, and two cross-references that did not resolve because the role was split across lines. `make -C docs html` (warnings are errors) is clean, and a nitpicky build reports nothing new. Co-Authored-By: Claude Code 2.1.292 / Claude Opus 5 (1M context) <noreply@anthropic.com>
It is not only fsspec that fetches the data any more, and `ondisk` now also covers what the remfile backend caches. Co-Authored-By: Claude Code 2.1.292 / Claude Opus 5 (1M context) <noreply@anthropic.com>
Documentation build overview
23 files changed ·
|
Summary
--backendsCLI option (comma-separated, priority-ordered)remfilebackend (optionalpip install datalad-fuse[remfile]) optimized for HDF5 access patterns (.nwb, .h5, .hdf5, etc.)fsspec.pyinto focused modules:backends.py(ABC),remfile.py,adapter.py, slimmedfsspec.pydatalad_fuse.fsspecimport paths viaDeprecationWarningHow it works
The backend chain walks
--backends=remfile,fsspec(default) in order. Each backend'scan_handle(key, mode)decides if it should handle a file based on the annex key's extension. First match tries all URLs; on failure, falls through to the next backend. Configuration via--backendsflag,datalad.fusefs.backendsconfig variable, or programmatic API.New module layout
backends.pyBackendABC,DEFAULT_BACKENDSremfile.pyRemfileBackend,RemfileWrapperadapter.pyRemoteFilesystemAdapter(renamed fromFsspecAdapter),DatasetAdapter,FileStatefsspec.pyFsspecBackend, HTTP helpers, backward-compat__getattr__Commits
RF: address code review findingsTST: add backward compatibility deprecation testsENH: add remfile backend with pluggable backend architectureTest plan
pytest datalad_fuse/tests/test_backends.py datalad_fuse/tests/test_deprecations.py -k "not network"pytest datalad_fuse/tests/test_backends.py -k networkpytest datalad_fuse/tests/test_util.pyfrom datalad_fuse.fsspec import FsspecAdapterstill works with DeprecationWarning🤖 Generated with Claude Code
Overall it grew larger than initial quick "fix" was (first commit) since the whole setup had 'fsspec' naming throughout it.