From 2ca1c25ee7dfd120b1965f620955f39d1e49117e Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 20:46:50 +0000 Subject: [PATCH 01/25] Read missing annexed content with datalad-fuse, cached by annex key Add `dandi.support.datalad_fuse.AnnexedReadableFile`, a `Readable` for a locked annexed file whose content is not present locally, streamed with the adapter of datalad-fuse (no FUSE mount), which asks git-annex where the content is. It is available with the new `datalad` extra. Cache NWB metadata of locked annexed files, streamed or local, under their git-annex key paired with their path, with fscacher 0.5.0's `annex_key_fingerprint`. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- .github/workflows/run-tests.yml | 7 ++ dandi/metadata/nwb.py | 3 +- dandi/pynwb_utils.py | 25 +++- dandi/support/datalad_fuse.py | 147 +++++++++++++++++++++++ dandi/support/tests/test_datalad_fuse.py | 97 +++++++++++++++ dandi/tests/fixtures.py | 123 +++++++++++++++++++ dandi/tests/skip.py | 5 + pyproject.toml | 8 +- 8 files changed, 410 insertions(+), 5 deletions(-) create mode 100644 dandi/support/datalad_fuse.py create mode 100644 dandi/support/tests/test_datalad_fuse.py diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml index fec7c9104..d33f99a4f 100644 --- a/.github/workflows/run-tests.yml +++ b/.github/workflows/run-tests.yml @@ -73,6 +73,9 @@ jobs: - os: ubuntu-latest python: '3.11' mode: lowest-deps + - os: ubuntu-latest + python: '3.12' + mode: datalad steps: - name: Set up environment @@ -123,6 +126,10 @@ jobs: git+https://github.com/hdmf-dev/hdmf \ git+https://github.com/hdmf-dev/hdmf-zarr + - name: Install git-annex and datalad-fuse (for streaming annexed content) + if: matrix.mode == 'datalad' + run: pip install git-annex ".[datalad]" + - name: Create NFS filesystem if: matrix.mode == 'nfs' run: | diff --git a/dandi/metadata/nwb.py b/dandi/metadata/nwb.py index bf43dbcb2..140f2e1f7 100644 --- a/dandi/metadata/nwb.py +++ b/dandi/metadata/nwb.py @@ -16,6 +16,7 @@ from ..misctypes import DUMMY_DANDI_ETAG, Digest, LocalReadableFile, Readable from ..pynwb_utils import ( _get_pynwb_metadata, + annex_fingerprint, get_neurodata_types, get_nwb_version, ignore_benign_pynwb_warnings, @@ -28,7 +29,7 @@ # Disable this for clean hacking -@metadata_cache.memoize_path +@metadata_cache.memoize_path(custom_fingerprint=annex_fingerprint) def get_metadata( path: str | Path | Readable, digest: Digest | None = None ) -> dict[str, Any]: diff --git a/dandi/pynwb_utils.py b/dandi/pynwb_utils.py index 297df05f3..da6e4d835 100644 --- a/dandi/pynwb_utils.py +++ b/dandi/pynwb_utils.py @@ -23,7 +23,7 @@ import warnings import dandischema -from fscacher import PersistentCache +from fscacher import PersistentCache, annex_key_fingerprint import h5py import hdmf import numpy as np @@ -73,6 +73,25 @@ ) +def annex_fingerprint(source: Any) -> tuple[str, str] | None: + """ + Fingerprint of ``source`` for ``PersistentCache.memoize_path`` + + Pass it as ``custom_fingerprint`` to cache the results of a function of a + local path or a `Readable` under the git-annex key of a locked annexed + file, paired with its path (see `fscacher.annex_key_fingerprint`), rather + than under ``stat()``: the content of an `AnnexedReadableFile`, which is + not present locally, cannot be ``stat()``-ed. Anything else is handled as + without this. + """ + # Avoid circular import: + from .support.datalad_fuse import AnnexedReadableFile + + if isinstance(source, AnnexedReadableFile): + source = source.filepath + return cast("tuple[str, str] | None", annex_key_fingerprint(source)) + + def _sanitize_nwb_version( v: Any, filename: str | Path | None = None, @@ -194,7 +213,7 @@ def get_neurodata_types_to_modalities_map() -> dict[str, str]: return ndtypes -@metadata_cache.memoize_path +@metadata_cache.memoize_path(custom_fingerprint=annex_fingerprint) def get_neurodata_types(filepath: str | Path | Readable) -> list[str]: with open_readable(filepath) as fp, h5py.File(fp, "r") as h5file: all_pairs = _scan_neurodata_types(h5file) @@ -808,7 +827,7 @@ def copy_nwb_file(src: str | Path, dest: str | Path) -> str: return str(dest) -@metadata_cache.memoize_path +@metadata_cache.memoize_path(custom_fingerprint=annex_fingerprint) def nwb_has_external_links(filepath: str | Path | Readable) -> bool: with open_readable(filepath) as f, h5py.File(f, "r") as fp: visited = set() diff --git a/dandi/support/datalad_fuse.py b/dandi/support/datalad_fuse.py new file mode 100644 index 000000000..8460039f3 --- /dev/null +++ b/dandi/support/datalad_fuse.py @@ -0,0 +1,147 @@ +""" +Reading the content of annexed files of git-annex_ repositories (such as +DataLad_ datasets) that is not present locally, with datalad-fuse_. + +In a git-annex repository, a (locked) annexed file is a symbolic link into +:file:`.git/annex/objects/`, which is broken until the content is fetched. +datalad-fuse can mount such a repository with FUSE so that the content is +streamed on demand, but its adapter, which the mount is built on, also opens +the files directly with no mount. It asks git-annex where the content is +(``git annex whereis``) and streams it over HTTP(S) with fsspec_. That adapter +is used here, so that the files of a DataLad Dandiset can be read (e.g., for +their metadata, or to be validated) without downloading the (possibly +terabytes of) data. + +This requires datalad-fuse (``pip install "dandi[datalad]"``), which requires +DataLad and git-annex. + +.. _git-annex: https://git-annex.branchable.com +.. _DataLad: https://www.datalad.org +.. _datalad-fuse: https://github.com/datalad/datalad-fuse +.. _fsspec: https://github.com/fsspec/filesystem_spec +""" + +from __future__ import annotations + +import atexit +from dataclasses import dataclass, field +from datetime import datetime +from functools import cache +import logging +import os +from pathlib import Path +import subprocess +from typing import IO, Any, cast + +from ..misctypes import Readable + +lgr = logging.getLogger("dandi.support.datalad_fuse") + + +@cache +def get_adapter() -> Any: + """ + The datalad-fuse adapter shared by all files, rooted at the filesystem + root so that it serves files of any dataset, and closed at exit + """ + # Optional dependency: + from datalad_fuse.fsspec import FsspecAdapter + + # caching=False: do not keep the streamed blocks on disk, in the dataset + adapter = FsspecAdapter(Path(os.path.abspath(os.sep)), caching=False) + atexit.register(adapter.__exit__, None, None, None) + return adapter + + +@cache +def annex_initialized(directory: Path) -> bool: + """ + Whether ``directory`` is in a Git repository in which git-annex is + initialized. (DataLad would otherwise initialize it, modifying the + repository.) + """ + try: + r = subprocess.run( + ["git", "-C", str(directory), "config", "--get", "annex.uuid"], + capture_output=True, + text=True, + ) + except FileNotFoundError: + return False + return r.returncode == 0 and bool(r.stdout.strip()) + + +@dataclass +class AnnexedReadableFile(Readable): + """ + A `Readable` for a (locked) annexed file whose content is not present + locally, which is streamed with datalad-fuse (see the module docstring). + + Instances are obtained by calling `get_annexed_readable()`. + """ + + #: The absolute path of the (broken) symbolic link to the file's content + filepath: Path + + #: The git-annex key of the file + key: str + + #: The size of the content, as recorded in the key + size: int + + #: The first URL that the content may be streamed from (for reporting; + #: the adapter tries the others if it cannot be read from that one) + url: str + + adapter: Any = field(repr=False, compare=False) + + def open(self) -> IO[bytes]: + return cast("IO[bytes]", self.adapter.open(self.filepath, "rb")) + + def get_size(self) -> int: + return self.size + + def get_mtime(self) -> datetime | None: + return None + + def get_filename(self) -> str: + return self.filepath.name + + def __str__(self) -> str: + return str(self.filepath) + + +def get_annexed_readable(path: str | Path) -> AnnexedReadableFile | None: + """ + Return an `AnnexedReadableFile` for streaming the content of the annexed + file at ``path``, or `None` if datalad-fuse is not installed, ``path`` is + not a (locked) annexed file whose content is missing in a repository in + which git-annex is initialized, its key does not record the size of the + content, or git-annex knows no URL to stream it from + """ + filepath = Path(os.path.abspath(path)) + if not filepath.is_symlink() or filepath.exists(): + # Not a broken symbolic link + return None + try: + from datalad_fuse.fsspec import FileState + except ImportError: + return None + if not annex_initialized(filepath.parent): + return None + try: + dsap, relpath = get_adapter().resolve_dataset(filepath) + state, key = dsap.get_file_state(relpath) + except Exception as e: + # E.g., a repository without any commit + lgr.debug("%s: Cannot be read with datalad-fuse: %s", filepath, e) + return None + if state is not FileState.NO_CONTENT or key is None or key.size is None: + return None + url = next(dsap.get_urls(str(key)), None) + if url is None: + lgr.debug("%s: git-annex knows no URL for key %s", filepath, key) + return None + return AnnexedReadableFile( + filepath=filepath, key=str(key), size=key.size, url=url, adapter=get_adapter() + ) diff --git a/dandi/support/tests/test_datalad_fuse.py b/dandi/support/tests/test_datalad_fuse.py new file mode 100644 index 000000000..6dc31495a --- /dev/null +++ b/dandi/support/tests/test_datalad_fuse.py @@ -0,0 +1,97 @@ +from __future__ import annotations + +from pathlib import Path +import shutil + +from fscacher import PersistentCache +import h5py +import pytest + +from ..datalad_fuse import AnnexedReadableFile, get_annexed_readable +from ...pynwb_utils import annex_fingerprint, get_neurodata_types +from ...tests.fixtures import RangeHTTPServer, make_git_annex_dandiset +from ...tests.skip import mark + +pytestmark = mark.skipif_no_git_annex + + +@pytest.fixture +def streamed_nwb( + range_http_server: tuple[RangeHTTPServer, str], simple2_nwb: Path, tmp_path: Path +) -> tuple[Path, RangeHTTPServer]: + """ + An annexed copy of ``simple2_nwb`` in a git-annex repository, whose content + is not present but registered at a URL of the HTTP server + """ + pytest.importorskip("datalad_fuse.fsspec") + server, base_url = range_http_server + shutil.copy(simple2_nwb, tmp_path / "served" / "content.nwb") + make_git_annex_dandiset( + tmp_path / "ds", + {"sub-01/sub-01.nwb": (simple2_nwb, f"{base_url}/content.nwb")}, + ) + nwb = tmp_path / "ds" / "sub-01" / "sub-01.nwb" + assert not nwb.exists() + return nwb, server + + +@pytest.mark.ai_generated +def test_annexed_readable( + streamed_nwb: tuple[Path, RangeHTTPServer], simple2_nwb: Path +) -> None: + nwb, server = streamed_nwb + r = get_annexed_readable(nwb) + assert isinstance(r, AnnexedReadableFile) + assert r.key.startswith(f"SHA256E-s{simple2_nwb.stat().st_size}--") + assert r.url.endswith("/content.nwb") + assert r.get_size() == simple2_nwb.stat().st_size + assert r.get_filename() == "sub-01.nwb" + fp = r.open() + try: + # Random access over HTTP is enough for h5py to read the file + with h5py.File(fp, "r") as h5: + assert h5.attrs["nwb_version"] + finally: + fp.close() + assert server.ranged_requests > 0 + + +@pytest.mark.ai_generated +def test_annexed_readable_none( + streamed_nwb: tuple[Path, RangeHTTPServer], simple2_nwb: Path, tmp_path: Path +) -> None: + nwb, _ = streamed_nwb + # The content is present + (nwb.parent / "regular.nwb").write_bytes(b"content") + assert get_annexed_readable(nwb.parent / "regular.nwb") is None + # Not in a Git repository + assert get_annexed_readable(simple2_nwb) is None + # A broken symbolic link in a repository without git-annex initialized + plain = tmp_path / "plain" + plain.mkdir() + shutil.copytree(nwb.parent, plain / "sub-01", symlinks=True) + shutil.copytree(nwb.parent.parent / ".git", plain / ".git") + (plain / ".git" / "config").write_text("[core]\n\tbare = false\n") + assert get_annexed_readable(plain / "sub-01" / "sub-01.nwb") is None + + +@pytest.mark.ai_generated +def test_annexed_readable_cached_by_key( + streamed_nwb: tuple[Path, RangeHTTPServer], simple2_nwb: Path, tmp_path: Path +) -> None: + nwb, server = streamed_nwb + cache = PersistentCache(path=tmp_path / "cache") + neurodata_types = cache.memoize_path(custom_fingerprint=annex_fingerprint)( + get_neurodata_types.__wrapped__ + ) + r = get_annexed_readable(nwb) + assert r is not None + expected = get_neurodata_types.__wrapped__(simple2_nwb) + assert neurodata_types(r) == expected + requests = server.ranged_requests + assert requests > 0 + # Served from the cache, without the content being read again: by the + # readable, and by the path of the (broken) link itself + assert neurodata_types(get_annexed_readable(nwb)) == expected + assert neurodata_types(nwb) == expected + assert server.ranged_requests == requests diff --git a/dandi/tests/fixtures.py b/dandi/tests/fixtures.py index 9b128a23d..438219c99 100644 --- a/dandi/tests/fixtures.py +++ b/dandi/tests/fixtures.py @@ -3,12 +3,16 @@ from collections.abc import Callable, Iterator from dataclasses import dataclass, field, replace from datetime import datetime, timezone +from functools import partial +from http import HTTPStatus +from http.server import SimpleHTTPRequestHandler, ThreadingHTTPServer import logging import os from pathlib import Path import re import shutil from subprocess import DEVNULL, check_output, run +import threading from time import sleep from typing import Any, Literal from uuid import uuid4 @@ -352,6 +356,125 @@ def fixture( return fixture +def make_git_annex_dandiset(dandiset: Path, files: dict[str, tuple[Path, str]]) -> None: + """ + Make the directory ``dandiset`` (created if needed, with a minimal + :file:`dandiset.yaml`) a git-annex repository looking like a clone of a + DataLad Dandiset whose annexed content has not been fetched: for each + ``relpath: (source, url)`` item of ``files``, a copy of ``source`` is + annexed at ``relpath`` with ``url`` registered for it, and then dropped. + This needs git-annex. + """ + env = { + **os.environ, + "GIT_AUTHOR_NAME": "DANDI tests", + "GIT_AUTHOR_EMAIL": "tests@dandiarchive.org", + "GIT_COMMITTER_NAME": "DANDI tests", + "GIT_COMMITTER_EMAIL": "tests@dandiarchive.org", + } + + def git(*args: str) -> str: + return check_output( + ["git", "-C", str(dandiset), *args], env=env, text=True + ).strip() + + dandiset.mkdir(parents=True, exist_ok=True) + (dandiset / dandiset_metadata_file).write_text( + "identifier: '000027'\nname: Test\ndescription: Test dandiset\n" + ) + git("init", "-q") + git("annex", "init", "-q") + git("config", "annex.backend", "SHA256E") + git("add", dandiset_metadata_file) + for relpath, (source, _) in files.items(): + (dandiset / relpath).parent.mkdir(parents=True, exist_ok=True) + shutil.copy(source, dandiset / relpath) + git("annex", "add", "-q", *files) + for relpath, (_, url) in files.items(): + key = git("annex", "lookupkey", relpath) + git("annex", "registerurl", key, url) + git("commit", "-q", "-m", "Add files") + git("annex", "drop", "-q", "--force", *files) + + +class RangeHTTPServer(ThreadingHTTPServer): + """An HTTP server counting the range requests it served""" + + ranged_requests: int = 0 + + +class RangeHTTPRequestHandler(SimpleHTTPRequestHandler): + """ + A handler serving files with support for ``HEAD`` and single-range ``GET`` + requests, like S3 does + """ + + def log_message(self, format: str, *args: Any) -> None: + pass + + def _send_headers( + self, status: HTTPStatus, length: int, content_range: str | None = None + ) -> None: + self.send_response(status) + self.send_header("Content-Type", "application/octet-stream") + self.send_header("Content-Length", str(length)) + self.send_header("Accept-Ranges", "bytes") + if content_range is not None: + self.send_header("Content-Range", content_range) + self.end_headers() + + def do_HEAD(self) -> None: + path = self.translate_path(self.path) + if not os.path.isfile(path): + self.send_error(HTTPStatus.NOT_FOUND) + return + self._send_headers(HTTPStatus.OK, os.path.getsize(path)) + + def do_GET(self) -> None: + path = self.translate_path(self.path) + if not os.path.isfile(path): + self.send_error(HTTPStatus.NOT_FOUND) + return + size = os.path.getsize(path) + start, end = 0, size - 1 + if m := re.fullmatch(r"bytes=(\d+)-(\d*)", self.headers.get("Range", "")): + start = int(m[1]) + end = min(int(m[2]) if m[2] else size - 1, size - 1) + assert isinstance(self.server, RangeHTTPServer) + self.server.ranged_requests += 1 + self._send_headers( + HTTPStatus.PARTIAL_CONTENT, + end - start + 1, + f"bytes {start}-{end}/{size}", + ) + else: + self._send_headers(HTTPStatus.OK, size) + with open(path, "rb") as fp: + fp.seek(start) + self.wfile.write(fp.read(end - start + 1)) + + +@pytest.fixture() +def range_http_server(tmp_path: Path) -> Iterator[tuple[RangeHTTPServer, str]]: + """ + Serve the files in ``tmp_path / "served"`` over HTTP with support for range + requests; yields the server and its base URL + """ + served = tmp_path / "served" + served.mkdir() + server = RangeHTTPServer( + ("127.0.0.1", 0), partial(RangeHTTPRequestHandler, directory=str(served)) + ) + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + try: + yield server, f"http://127.0.0.1:{server.server_address[1]}" + finally: + server.shutdown() + server.server_close() + thread.join() + + def _make_subdirs_dandisets(path: Path) -> None: for bids_dataset_path in path.iterdir(): if bids_dataset_path.is_dir(): diff --git a/dandi/tests/skip.py b/dandi/tests/skip.py index 54d864b76..9e30b195d 100644 --- a/dandi/tests/skip.py +++ b/dandi/tests/skip.py @@ -125,6 +125,10 @@ def no_git(): return "Git not installed", shutil.which("git") is None +def no_git_annex(): + return "git-annex not installed", shutil.which("git-annex") is None + + # ### END MODIFIED CODE @@ -162,6 +166,7 @@ def on_windows(): no_docker_commands, no_docker_engine, no_git, + no_git_annex, no_network, # no_singularity, no_ssh, diff --git a/pyproject.toml b/pyproject.toml index b98d4f7a0..e7d3637fb 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -53,7 +53,7 @@ dependencies = [ "dandischema >= 0.12.0, != 0.13.0, < 0.15.0", "etelemetry >= 0.2.2", "fasteners >= 0.19", - "fscacher >= 0.3.0", + "fscacher >= 0.5.0", # Floor raised to what `py3-lowest` actually resolves: modern pynwb pins # hdmf >= 4, so declared floor is now the tested floor. "hdmf >= 4.1.0", @@ -119,6 +119,11 @@ test = [ # 5.0 switched to urllib3.connection.VerifiedHTTPSConnection (urllib3 2.x compat) "vcrpy >= 5.0.0", ] +# For reading the content of annexed files of DataLad datasets without +# fetching it (requires git-annex) +datalad = [ + "datalad-fuse >= 0.5.1", +] tools = [ "boto3", ] @@ -214,6 +219,7 @@ follow_imports = "skip" module = [ "bidsschematools.*", "click_didyoumean.*", + "datalad_fuse.*", "etelemetry.*", "fasteners.*", "fscacher.*", From 36552870083e9ba98816910128c30065c41aee67 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 20:51:13 +0000 Subject: [PATCH 02/25] validate: stream missing annexed content with datalad-fuse Add a `stream` policy to `--missing-file-content`: the content of annexed files that is not present locally (as in a DataLad clone of a Dandiset) is streamed with datalad-fuse's adapter, so that pynwb and nwbinspector run on it without it being downloaded. pynwb validation results are cached under the files' git-annex keys. This is the validation integration of #1933, on top of the datalad-fuse readable rather than a git-only reader of the git-annex branch. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- dandi/cli/cmd_validate.py | 9 +- dandi/cli/tests/test_cmd_validate.py | 43 +++++++- dandi/files/bases.py | 82 ++++++++++++-- dandi/files/bids.py | 2 +- dandi/pynwb_utils.py | 51 +++++++-- dandi/validate/_core.py | 135 +++++++++++++++++------ dandi/validate/_types.py | 6 + dandi/validate/tests/test_core.py | 158 ++++++++++++++++++++++++++- docs/source/cmdline/validate.rst | 62 ++++++++++- 9 files changed, 495 insertions(+), 53 deletions(-) diff --git a/dandi/cli/cmd_validate.py b/dandi/cli/cmd_validate.py index 5241c5eb4..ea18707c0 100644 --- a/dandi/cli/cmd_validate.py +++ b/dandi/cli/cmd_validate.py @@ -170,8 +170,13 @@ def _filter_results( "in a datalad dataset without fetched data). 'error' (default) emits a " "concise error per file, 'skip' skips each such file with a warning, " "'only-non-data' skips content-dependent validators but still validates " - "path layout.", - type=click.Choice(["error", "only-non-data", "skip"], case_sensitive=True), + "path layout, 'stream' streams the content of annexed files with " + "datalad-fuse so that content-dependent validators run without the files " + "having to be downloaded (requires git-annex and `pip install " + "'dandi[datalad]'`).", + type=click.Choice( + ["error", "only-non-data", "skip", "stream"], case_sensitive=True + ), default="error", ) @click.option( diff --git a/dandi/cli/tests/test_cmd_validate.py b/dandi/cli/tests/test_cmd_validate.py index 1da5c16ca..aef651740 100644 --- a/dandi/cli/tests/test_cmd_validate.py +++ b/dandi/cli/tests/test_cmd_validate.py @@ -1,6 +1,7 @@ import json from pathlib import Path -from typing import cast +import shutil +from typing import Any, cast from click.testing import CliRunner import pytest @@ -998,6 +999,46 @@ def test_validate_missing_file_content_no_broken_symlinks(tmp_path: Path) -> Non assert "FILE_CONTENT_MISSING" not in r_skip.output +@pytest.mark.ai_generated +def test_validate_missing_file_content_stream( + tmp_path: Path, simple3_nwb: Path, range_http_server: tuple[Any, str] +) -> None: + """--missing-file-content=stream validates annexed content with datalad-fuse.""" + from ...tests.fixtures import make_git_annex_dandiset + from ...tests.skip import skipif + + skipif.no_git_annex() + pytest.importorskip("datalad_fuse.fsspec") + _, base_url = range_http_server + shutil.copy(simple3_nwb, tmp_path / "served" / "content.nwb") + ds = tmp_path / "ds" + make_git_annex_dandiset( + ds, {"sub-001/sub-001.nwb": (simple3_nwb, f"{base_url}/content.nwb")} + ) + out = tmp_path / "out.jsonl" + r = CliRunner().invoke( + validate, + [ + "--missing-file-content", + "stream", + "--min-severity", + "INFO", + "-f", + "json_lines", + "-o", + str(out), + str(ds), + ], + ) + assert "Traceback" not in r.output + ids = [rec.id for rec in load_validation_jsonl([str(out)])] + assert "DANDI.FILE_CONTENT_STREAMED" in ids + assert "DANDI.FILE_CONTENT_MISSING" not in ids + # simple3_nwb lacks a subject_id, which only content-based checks notice + assert "NWBI.check_subject_id_exists" in ids + assert r.exit_code == 1 + + @pytest.mark.ai_generated def test_validate_broken_symlink_path_argument(tmp_path: Path) -> None: """A broken symlink can be given directly as a path to validate. diff --git a/dandi/files/bases.py b/dandi/files/bases.py index db502dcc3..086298e88 100644 --- a/dandi/files/bases.py +++ b/dandi/files/bases.py @@ -10,6 +10,7 @@ from pathlib import Path import re from threading import Lock +import traceback from typing import IO, Any, Generic from xml.etree.ElementTree import fromstring @@ -32,7 +33,7 @@ set_asset_schema_key, ) from dandi.metadata.core import get_default_metadata -from dandi.misctypes import DUMMY_DANDI_ETAG, Digest, LocalReadableFile, P +from dandi.misctypes import DUMMY_DANDI_ETAG, Digest, LocalReadableFile, P, Readable from dandi.utils import post_upload_size_check, pre_upload_size_check, yaml_load from dandi.validate._types import ( ORIGIN_INTERNAL_DANDI, @@ -320,12 +321,22 @@ class LocalFileAsset(LocalAsset): an asset of a Dandiset """ + #: An alternative source for the asset's content, used instead of the file + #: at `filepath` when set. This is used to stream the content of an + #: annexed file (e.g., in a DataLad dataset) that is not present locally; + #: see `dandi.support.datalad_fuse`. + content_source: Readable | None = None + + def _content(self) -> Path | Readable: + """The source to read the asset's content from""" + return self.content_source if self.content_source is not None else self.filepath + def get_metadata( self, digest: Digest | None = None, ignore_errors: bool = True, ) -> BareAsset: - metadata = get_default_metadata(self.filepath, digest=digest) + metadata = get_default_metadata(self._content(), digest=digest) metadata.path = self.path return metadata @@ -515,7 +526,7 @@ def get_metadata( from dandi.metadata.nwb import nwb2asset try: - metadata = nwb2asset(self.filepath, digest=digest) + metadata = nwb2asset(self._content(), digest=digest) except Exception as e: lgr.warning( "Failed to extract NWB metadata from %s: %s: %s", @@ -524,7 +535,7 @@ def get_metadata( str(e), ) if ignore_errors: - metadata = get_default_metadata(self.filepath, digest=digest) + metadata = get_default_metadata(self._content(), digest=digest) else: raise metadata.path = self.path @@ -555,12 +566,16 @@ def get_validation_errors( pass else: # Avoid heavy import by importing within function: - from nwbinspector import Importance, inspect_nwbfile, load_config + from nwbinspector import Importance, load_config # Avoid heavy import by importing within function: from dandi.pynwb_utils import validate as pynwb_validate - errors.extend(pynwb_validate(self.filepath, devel_debug=devel_debug)) + errors.extend( + pynwb_validate( + self.filepath, devel_debug=devel_debug, readable=self.content_source + ) + ) if schema_version is not None: errors.extend( super().get_validation_errors( @@ -576,9 +591,7 @@ def get_validation_errors( validator_version=str(_get_nwb_inspector_version()), ) - for error in inspect_nwbfile( - nwbfile_path=self.filepath, - skip_validate=True, + for error in self._inspect_nwbfile( config=load_config(filepath_or_keyword="dandi"), importance_threshold=Importance.BEST_PRACTICE_VIOLATION, ): @@ -623,6 +636,57 @@ def get_validation_errors( ) return errors + def _inspect_nwbfile(self, **kwargs: Any) -> Iterator[Any]: + """ + Yield the messages from inspecting the file with nwbinspector, reading + the content from `content_source` if it is set (in which case what + `nwbinspector.inspect_nwbfile()` does for a local file is replicated + here for the streamed content) + """ + # Avoid heavy import by importing within function: + from nwbinspector import ( + Importance, + InspectorMessage, + inspect_nwbfile, + inspect_nwbfile_object, + ) + + if self.content_source is None: + yield from inspect_nwbfile( + nwbfile_path=self.filepath, skip_validate=True, **kwargs + ) + return + + # Avoid heavy import by importing within function: + import h5py + from pynwb import NWBHDF5IO + + from dandi.pynwb_utils import open_readable + + try: + with open_readable(self.content_source) as fp, h5py.File( + fp, "r" + ) as h5, NWBHDF5IO(file=h5, mode="r", load_namespaces=True) as io: + nwbfile = io.read() + for message in inspect_nwbfile_object(nwbfile_object=nwbfile, **kwargs): + if message is not None: + message.file_path = str(self.filepath) + yield message + except Exception as e: + # Report the failure the same way inspect_nwbfile() does for a + # local file that PyNWB cannot read + yield InspectorMessage( + message=traceback.format_exc(), + importance=Importance.ERROR, + check_function_name=( + f"During io.read(), an error occurred: " + f"{type(e).__module__}.{type(e).__name__}. " + "This indicates that PyNWB was unable to read the file. " + "See the traceback message for more details." + ), + file_path=str(self.filepath), + ) + class VideoAsset(LocalFileAsset): pass diff --git a/dandi/files/bids.py b/dandi/files/bids.py index 008b085dd..7624c40a3 100644 --- a/dandi/files/bids.py +++ b/dandi/files/bids.py @@ -250,7 +250,7 @@ def get_metadata( ) -> BareAsset: metadata = self.bids_dataset_description.get_asset_metadata(self) start_time = end_time = datetime.now().astimezone() - add_common_metadata(metadata, self.filepath, start_time, end_time, digest) + add_common_metadata(metadata, self._content(), start_time, end_time, digest) metadata.path = self.path return metadata diff --git a/dandi/pynwb_utils.py b/dandi/pynwb_utils.py index da6e4d835..34c454bd0 100644 --- a/dandi/pynwb_utils.py +++ b/dandi/pynwb_utils.py @@ -629,8 +629,9 @@ def rename_if_matched(_name: str, obj: Any) -> None: f.visititems(rename_if_matched) -@validate_cache.memoize_path -def validate(path: str | Path, devel_debug: bool = False) -> list[ValidationResult]: +def validate( + path: str | Path, devel_debug: bool = False, *, readable: Readable | None = None +) -> list[ValidationResult]: """Run validation on a file and return errors In case of an exception being thrown, an error message added to the @@ -639,8 +640,38 @@ def validate(path: str | Path, devel_debug: bool = False) -> list[ValidationResu Parameters ---------- path: str or Path + Path of the file, as reported in the returned results + devel_debug: bool, optional + Whether to re-raise exceptions instead of reporting them as errors + readable: Readable, optional + If given, the file's content is read from this `Readable` instead of from + ``path`` (e.g., to stream the content of an annexed file which is not + present locally); the results are then cached only if it is an + `AnnexedReadableFile` (see `annex_fingerprint`). + """ + source: str | Readable = readable if readable is not None else str(path) + # fscacher is untyped, so the memoized _validate returns Any for mypy + return cast( + "list[ValidationResult]", + _validate(source, str(path), devel_debug=devel_debug), + ) + + +@validate_cache.memoize_path(custom_fingerprint=annex_fingerprint) +def _validate( + source: str | Readable, path: str, devel_debug: bool = False +) -> list[ValidationResult]: + """`validate` proper, with the content source as first argument for caching + + Parameters + ---------- + source: str or Readable + What to read the file's content from: its path or a `Readable` + path: str + Path of the file, as reported in the returned results + devel_debug: bool + Whether to re-raise exceptions instead of reporting them as errors """ - path = str(path) # Might come in as pathlib's PATH errors: list[ValidationResult] = [] # To overcome @@ -652,7 +683,7 @@ def validate(path: str | Path, devel_debug: bool = False) -> list[ValidationResu ) version = None try: - version = get_nwb_version(path, sanitize=False) + version = get_nwb_version(source, sanitize=False) except Exception: # we just will not remove any errors, it is required so should be some pass @@ -698,9 +729,15 @@ def validate(path: str | Path, devel_debug: bool = False) -> list[ValidationResu ) try: - # Validates against the namespaces cached in the file; pynwb falls back - # to its own namespaces if the file has none cached - error_outputs = pynwb.validate(path=path) + # Either way, validates against the namespaces cached in the file; pynwb + # falls back to its own namespaces if the file has none cached + if isinstance(source, Readable): + with open_readable(source) as fp, h5py.File(fp, "r") as h5, NWBHDF5IO( + file=h5, mode="r", load_namespaces=True + ) as reader: + error_outputs = pynwb.validate(io=reader) + else: + error_outputs = pynwb.validate(path=source) except Exception as exc: if devel_debug: raise diff --git a/dandi/validate/_core.py b/dandi/validate/_core.py index 371ce5a30..e3c0e4f33 100644 --- a/dandi/validate/_core.py +++ b/dandi/validate/_core.py @@ -10,9 +10,10 @@ from __future__ import annotations from collections.abc import Iterator +from importlib.util import find_spec import os from pathlib import Path -from typing import Any +import shutil from ._types import ( ORIGIN_VALIDATION_DANDI_LAYOUT, @@ -26,7 +27,8 @@ Validator, ) from ..consts import dandiset_metadata_file -from ..files import find_dandi_files +from ..files import DandiFile, LocalFileAsset, find_dandi_files +from ..support.datalad_fuse import AnnexedReadableFile, get_annexed_readable from ..utils import find_parent_directory_containing BIDS_TO_DANDI = { @@ -155,7 +157,8 @@ def _is_broken_symlink(filepath: Path) -> bool: # BIDS error codes that require reading file content (headers, pixel data). -# When ``only-non-data`` is active these are suppressed for broken-symlink files. +# When ``only-non-data`` or ``stream`` is active these are suppressed for +# broken-symlink files (the BIDS validator cannot stream content). _BIDS_CONTENT_DEPENDENT_CODES = frozenset( { "BIDS.NIFTI_HEADER_UNREADABLE", @@ -181,13 +184,20 @@ def validate( Policy for files whose content is unavailable (e.g. broken symlinks in a datalad dataset without fetched data). ``error`` emits a concise error, ``skip`` skips the file with a warning, ``only-non-data`` skips - content-dependent validators but still validates path layout. + content-dependent validators but still validates path layout, and + ``stream`` streams the content of annexed files with datalad-fuse (see + `dandi.support.datalad_fuse`) so that + content-dependent validators run without the content being present + locally. Yields ------ path, errors errors for a path """ + if missing_file_content == MissingFileContent.stream: + _check_streaming_requirements() + # Archive of unique `ValidationResult` objects obtained through # `DandiFile.get_validation_errors()` # Note: This is needed to hold on to the unique `ValidationResult` objects @@ -214,34 +224,43 @@ def validate( p, dandiset_path=dandiset_path, allow_all=allow_any_path ): # Handle broken symlinks (missing file content) - if _is_broken_symlink(df.filepath): - r = _handle_missing_content(df, missing_file_content) - if r is not None: - r_id = id(r) - if r_id not in df_result_ids: - df_results.append(r) - df_result_ids.add(r_id) - yield r - if missing_file_content in ( - MissingFileContent.skip, - MissingFileContent.error, - ): + is_broken = _is_broken_symlink(df.filepath) + if is_broken: + if missing_file_content == MissingFileContent.stream: + readable = _prepare_streaming(df) + # A file whose content cannot be streamed gets an error + # and is otherwise skipped + skip_file = readable is None + else: + readable = None + skip_file = missing_file_content in ( + MissingFileContent.skip, + MissingFileContent.error, + ) + r = _handle_missing_content(df, missing_file_content, readable) + r_id = id(r) + if r_id not in df_result_ids: + df_results.append(r) + df_result_ids.add(r_id) + yield r + if skip_file: continue - # only-non-data: fall through but pass the flag to validators + # only-non-data & stream: fall through but pass the policy to + # the validators - is_broken = _is_broken_symlink(df.filepath) for r in df.get_validation_errors( schema_version=schema_version, devel_debug=devel_debug, missing_file_content=(missing_file_content if is_broken else None), ): - # For broken-symlink files under only-non-data, suppress - # BIDS errors that require reading file content (e.g. + # For broken-symlink files under only-non-data and stream, + # suppress BIDS errors that require reading file content (e.g. # NIFTI_HEADER_UNREADABLE). The validator ran in full so # real files still get those checks. if ( is_broken - and missing_file_content == MissingFileContent.only_non_data + and missing_file_content + in (MissingFileContent.only_non_data, MissingFileContent.stream) and r.id in _BIDS_CONTENT_DEPENDENT_CODES ): continue @@ -252,21 +271,74 @@ def validate( yield r +def _check_streaming_requirements() -> None: + """Raise an informative error if what is needed for streaming file content + is not installed""" + if find_spec("datalad_fuse") is None: + raise RuntimeError( + "Streaming file content requires datalad-fuse; install it with " + "`pip install 'dandi[datalad]'`" + ) + if shutil.which("git-annex") is None: + raise RuntimeError("Streaming file content requires git-annex") + + +def _prepare_streaming(df: DandiFile) -> AnnexedReadableFile | None: + """ + Set up streaming of the content of the annexed file represented by ``df``, + and return the `Readable` that will be used for it, or `None` if the + content cannot be streamed (see `get_annexed_readable()`) + """ + if not isinstance(df, LocalFileAsset): + return None + readable = get_annexed_readable(df.filepath) + if readable is not None: + df.content_source = readable + return readable + + def _handle_missing_content( - df: Any, + df: DandiFile, policy: MissingFileContent, -) -> ValidationResult | None: + readable: AnnexedReadableFile | None = None, +) -> ValidationResult: """Produce a single :class:`ValidationResult` for a file with missing content. - Returns ``None`` when *policy* is ``only-non-data`` (a warning is not - needed because validation still proceeds on the non-data aspects). + For the ``stream`` policy, ``readable`` is the `Readable` streaming the + file's content, or `None` if the content cannot be streamed. """ - from ..files import DandiFile - - assert isinstance(df, DandiFile) filepath = df.filepath - if policy == MissingFileContent.error: + if policy == MissingFileContent.stream: + if readable is not None: + return ValidationResult( + id="DANDI.FILE_CONTENT_STREAMED", + origin=ORIGIN_VALIDATION_DANDI_LAYOUT, + severity=Severity.INFO, + scope=Scope.FILE, + path=filepath, + dandiset_path=df.dandiset_path, + message=( + f"File content is not present locally (git-annex key " + f"{readable.key}); content-dependent validation streams " + f"it from {readable.url}" + ), + ) + return ValidationResult( + id="DANDI.FILE_CONTENT_MISSING", + origin=ORIGIN_VALIDATION_DANDI_LAYOUT, + severity=Severity.ERROR, + scope=Scope.FILE, + path=filepath, + dandiset_path=df.dandiset_path, + message=( + f"File content is not available (broken symlink: " + f"{filepath} -> {os.readlink(filepath)}) and cannot be " + f"streamed: not an annexed file in a git-annex repository " + f"with a URL known to git-annex." + ), + ) + elif policy == MissingFileContent.error: return ValidationResult( id="DANDI.FILE_CONTENT_MISSING", origin=ORIGIN_VALIDATION_DANDI_LAYOUT, @@ -277,8 +349,9 @@ def _handle_missing_content( message=( f"File content is not available (broken symlink: " f"{filepath} -> {os.readlink(filepath)}). " - f"Use --missing-file-content=skip or " - f"--missing-file-content=only-non-data to handle gracefully." + f"Use --missing-file-content=skip, " + f"--missing-file-content=only-non-data, or " + f"--missing-file-content=stream to handle gracefully." ), ) elif policy == MissingFileContent.skip: diff --git a/dandi/validate/_types.py b/dandi/validate/_types.py index f41c8402e..48b18b45a 100644 --- a/dandi/validate/_types.py +++ b/dandi/validate/_types.py @@ -26,6 +26,12 @@ class MissingFileContent(StrEnum): skip = auto() """Skip the file entirely; emit a WARNING noting that validation was skipped.""" + stream = auto() + """Stream the content of annexed files with datalad-fuse (see + `dandi.support.datalad_fuse`) so that content-dependent validators run + without the content being present locally. An INFO result is emitted + for each streamed file, and an ERROR for each file that cannot be streamed.""" + lgr = logging.getLogger(__name__) diff --git a/dandi/validate/tests/test_core.py b/dandi/validate/tests/test_core.py index 7d331bb61..ab3759542 100644 --- a/dandi/validate/tests/test_core.py +++ b/dandi/validate/tests/test_core.py @@ -1,5 +1,7 @@ import json +import os from pathlib import Path +import shutil from typing import Any import pytest @@ -17,7 +19,14 @@ ) from ... import __version__ from ...consts import dandiset_metadata_file -from ...tests.fixtures import BIDS_TESTDATA_SELECTION +from ...pynwb_utils import validate as pynwb_validate +from ...support.datalad_fuse import get_annexed_readable +from ...tests.fixtures import ( + BIDS_TESTDATA_SELECTION, + RangeHTTPServer, + make_git_annex_dandiset, +) +from ...tests.skip import skipif def test_validate_nwb_error(simple3_nwb: Path) -> None: @@ -310,3 +319,150 @@ def test_validate_broken_symlink_real_file_still_validated(tmp_path: Path) -> No assert ( len(broken_pynwb) == 0 ), f"policy={policy.value}: pynwb should not run on the broken symlink" + + +# ---- Tests for streaming the content of annexed files (missing_file_content=stream) ---- + + +@pytest.fixture +def streamable_dandiset( + range_http_server: tuple[RangeHTTPServer, str], simple3_nwb: Path, tmp_path: Path +) -> tuple[Path, RangeHTTPServer]: + """A git-annex dandiset looking like a DataLad clone without fetched content. + + ``sub-001/sub-001.nwb`` is an annexed copy of *simple3_nwb* whose content + is served by the HTTP server, and ``sub-001/sub-001_video.mp4`` an annexed + video registered at a URL that is never read (only the size recorded in + its key is needed). + """ + skipif.no_git_annex() + pytest.importorskip("datalad_fuse.fsspec") + server, base_url = range_http_server + shutil.copy(simple3_nwb, tmp_path / "served" / "content.nwb") + video = tmp_path / "video.mp4" + video.write_bytes(b"not really a video") + ds = tmp_path / "ds" + make_git_annex_dandiset( + ds, + { + "sub-001/sub-001.nwb": (simple3_nwb, f"{base_url}/content.nwb"), + "sub-001/sub-001_video.mp4": (video, f"{base_url}/never-read.mp4"), + }, + ) + return ds, server + + +def _content_results(results: list[ValidationResult], name: str) -> list[tuple]: + """Path-independent summary of the results for the file called *name*.""" + return sorted( + (r.id, r.severity, r.origin.validator, r.message) + for r in results + if r.path is not None + and r.path.name == name + and r.id != "DANDI.FILE_CONTENT_STREAMED" + ) + + +@pytest.mark.ai_generated +def test_validate_stream( + streamable_dandiset: tuple[Path, RangeHTTPServer], + simple3_nwb: Path, + tmp_path: Path, +) -> None: + """stream policy validates the content of annexed files with datalad-fuse.""" + ds, server = streamable_dandiset + results = list(validate(ds, missing_file_content=MissingFileContent.stream)) + + streamed = [r for r in results if r.id == "DANDI.FILE_CONTENT_STREAMED"] + assert sorted(r.path.name for r in streamed if r.path is not None) == [ + "sub-001.nwb", + "sub-001_video.mp4", + ] + assert all(r.severity == Severity.INFO for r in streamed) + assert not [r for r in results if r.id.startswith("DANDI.FILE_CONTENT_MISSING")] + assert server.ranged_requests > 0 + + # Validating the video only needs the size recorded in its key + assert _content_results(results, "sub-001_video.mp4") == [] + + # Content-dependent validation of the NWB file gives the same results as + # for a regular dandiset containing the file itself + local = tmp_path / "local" + (local / "sub-001").mkdir(parents=True) + shutil.copy(ds / dandiset_metadata_file, local / dandiset_metadata_file) + shutil.copy(simple3_nwb, local / "sub-001" / "sub-001.nwb") + expected = list(validate(local)) + # simple3_nwb lacks a subject_id, which only content-based checks notice + assert any(r.origin.validator == Validator.nwbinspector for r in expected) + assert _content_results(results, "sub-001.nwb") == _content_results( + expected, "sub-001.nwb" + ) + + +@pytest.mark.ai_generated +@pytest.mark.skipif( + os.environ.get("DANDI_CACHE") == "ignore", reason="the validation cache is disabled" +) +def test_validate_stream_cached_by_key( + streamable_dandiset: tuple[Path, RangeHTTPServer], +) -> None: + """pynwb validation results of a streamed file are cached under its annex key.""" + ds, server = streamable_dandiset + nwb = ds / "sub-001" / "sub-001.nwb" + readable = get_annexed_readable(nwb) + assert readable is not None + results = pynwb_validate(nwb, readable=readable) + assert not [r for r in results if r.id == "pynwb.GENERIC"] + requests = server.ranged_requests + assert requests > 0 + # Served from the cache, without the content being read again + assert pynwb_validate(nwb, readable=get_annexed_readable(nwb)) == results + assert server.ranged_requests == requests + + +@pytest.mark.ai_generated +def test_validate_stream_not_streamable(tmp_path: Path) -> None: + """stream policy emits an error for a broken symlink that cannot be streamed.""" + pytest.importorskip("datalad_fuse.fsspec") + skipif.no_git_annex() + ds = _make_dandiset_with_broken_symlink(tmp_path) + results = list(validate(ds, missing_file_content=MissingFileContent.stream)) + errs = [r for r in results if r.id == "DANDI.FILE_CONTENT_MISSING"] + assert len(errs) == 1 + assert errs[0].severity == Severity.ERROR + assert errs[0].message is not None + assert "cannot be streamed" in errs[0].message + assert not [r for r in results if r.id == "DANDI.FILE_CONTENT_STREAMED"] + assert not [ + r + for r in results + if r.origin.validator in (Validator.pynwb, Validator.nwbinspector) + ] + + +@pytest.mark.ai_generated +def test_validate_stream_unreadable_url( + streamable_dandiset: tuple[Path, RangeHTTPServer], tmp_path: Path +) -> None: + """A registered URL that cannot be read yields errors, not an exception.""" + ds, _ = streamable_dandiset + (tmp_path / "served" / "content.nwb").unlink() + results = list(validate(ds, missing_file_content=MissingFileContent.stream)) + nwb = ds / "sub-001" / "sub-001.nwb" + assert [ + r for r in results if r.path == nwb and r.id == "DANDI.FILE_CONTENT_STREAMED" + ] + errs = [r for r in results if r.path == nwb and r.severity == Severity.ERROR] + assert {r.origin.validator for r in errs} == { + Validator.pynwb, + Validator.nwbinspector, + } + + +@pytest.mark.ai_generated +def test_validate_stream_requires_datalad_fuse( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setattr("dandi.validate._core.find_spec", lambda name: None) + with pytest.raises(RuntimeError, match=r"dandi\[datalad\]"): + list(validate(tmp_path, missing_file_content=MissingFileContent.stream)) diff --git a/docs/source/cmdline/validate.rst b/docs/source/cmdline/validate.rst index 3a07d7e7a..a789f6203 100644 --- a/docs/source/cmdline/validate.rst +++ b/docs/source/cmdline/validate.rst @@ -56,7 +56,7 @@ Options Limit the number of results shown per group (or in total when not grouping); the excess is replaced by a count of omitted results -.. option:: --missing-file-content [error|only-non-data|skip] +.. option:: --missing-file-content [error|only-non-data|skip|stream] How to handle files whose content is unavailable, such as the broken symbolic links of a DataLad_ dataset (a git-annex_ repository) whose @@ -73,6 +73,12 @@ Options Skip content-dependent validators (pynwb, nwbinspector, ...) for each such file but still validate its path layout + ``stream`` + Stream the content of each such file with datalad-fuse_ so that + content-dependent validators run without the file having to be + downloaded; see `Validating DataLad Dandisets Remotely`_ + below. + .. option:: --load Instead of running validation, load previously saved results from the @@ -84,6 +90,60 @@ Options .. _git-annex: https://git-annex.branchable.com +Validating DataLad Dandisets Remotely +------------------------------------- + +Every Dandiset on the DANDI Archive is mirrored as a DataLad dataset at +https://github.com/dandisets (with https://github.com/dandisets/dandisets as +the superdataset containing all of them). In such a dataset, the assets are +annexed files: symbolic links that remain broken until the content is fetched, +which for some Dandisets would mean downloading terabytes of data. The +``stream`` policy of :option:`--missing-file-content` lets ``dandi validate`` +run the content-dependent validators (pynwb and nwbinspector for NWB files) on +such files by streaming their content on demand with datalad-fuse_ (without +mounting anything), which asks git-annex where the content is, reading only +the parts of each file that the validators need. This requires git-annex and +``pip install "dandi[datalad]"``. + +For example, to produce a JSON Lines record of *all* validation results for +a Dandiset:: + + datalad clone https://github.com/dandisets/000003 + dandi validate --missing-file-content=stream --min-severity=INFO \ + --format=json_lines --output=000003.jsonl 000003 + +Each streamed file yields an ``INFO``-level ``DANDI.FILE_CONTENT_STREAMED`` +result naming the URL its content was read from; a file that cannot be streamed +(not an annexed file, or no URL known to git-annex) yields a +``DANDI.FILE_CONTENT_MISSING`` error instead. + +Notes: + +- git-annex must be initialized in the clone, which ``datalad clone`` does + (after ``git clone``, run ``git annex init``). +- Some nwbinspector checks read data arrays (e.g., timestamps), so the amount + of data streamed for a file depends on its content; it is nevertheless + usually a small fraction of the file. +- The pynwb validation results and the metadata of a streamed file are cached + under its git-annex key (a digest of its content), just as those of a local + file are cached under its modification time and size, so re-running the + command skips that work for files whose key has not changed since. As for + local files, set the :envvar:`DANDI_CACHE` environment variable to ``clear`` + to start afresh or to ``ignore`` to bypass the cache. +- Zarr assets are stored as separate subdatasets (https://github.com/dandizarrs) + and are not streamed yet: an uninstalled Zarr subdataset is an empty directory + that is not validated at all. Streaming them is a follow-up for when NWB Zarr + support has matured across the ecosystem. +- The BIDS validator cannot stream content, so BIDS errors that require reading + a file (e.g., unreadable NIfTI headers) are suppressed for annexed files under + the ``stream`` and ``only-non-data`` policies. For NWB datasets, the primary + use case, nothing is lost: everything the BIDS validator needs is either + present (the non-annexed sidecar files, which are kept in git) or encoded in + the file and folder names of the annexed files themselves. + +.. _datalad-fuse: https://github.com/datalad/datalad-fuse + + Development Options ------------------- From 52b675ee05b2eee9b2c1a44abafe8e8183338510 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 21:17:17 +0000 Subject: [PATCH 03/25] Move annex_fingerprint next to AnnexedReadableFile; trim field comments annex_fingerprint is not specific to NWB files, and living in dandi.support.datalad_fuse it needs no deferred import. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- dandi/metadata/nwb.py | 2 +- dandi/pynwb_utils.py | 22 ++------------- dandi/support/datalad_fuse.py | 34 ++++++++++++++++-------- dandi/support/tests/test_datalad_fuse.py | 4 +-- 4 files changed, 28 insertions(+), 34 deletions(-) diff --git a/dandi/metadata/nwb.py b/dandi/metadata/nwb.py index 140f2e1f7..d0468d607 100644 --- a/dandi/metadata/nwb.py +++ b/dandi/metadata/nwb.py @@ -16,13 +16,13 @@ from ..misctypes import DUMMY_DANDI_ETAG, Digest, LocalReadableFile, Readable from ..pynwb_utils import ( _get_pynwb_metadata, - annex_fingerprint, get_neurodata_types, get_nwb_version, ignore_benign_pynwb_warnings, metadata_cache, nwb_has_external_links, ) +from ..support.datalad_fuse import annex_fingerprint from ..utils import find_parent_directory_containing lgr = get_logger() diff --git a/dandi/pynwb_utils.py b/dandi/pynwb_utils.py index da6e4d835..c49293f57 100644 --- a/dandi/pynwb_utils.py +++ b/dandi/pynwb_utils.py @@ -23,7 +23,7 @@ import warnings import dandischema -from fscacher import PersistentCache, annex_key_fingerprint +from fscacher import PersistentCache import h5py import hdmf import numpy as np @@ -42,6 +42,7 @@ metadata_nwb_subject_fields, ) from .misctypes import Readable +from .support.datalad_fuse import annex_fingerprint from .utils import get_module_version, is_url from .validate._types import ( Origin, @@ -73,25 +74,6 @@ ) -def annex_fingerprint(source: Any) -> tuple[str, str] | None: - """ - Fingerprint of ``source`` for ``PersistentCache.memoize_path`` - - Pass it as ``custom_fingerprint`` to cache the results of a function of a - local path or a `Readable` under the git-annex key of a locked annexed - file, paired with its path (see `fscacher.annex_key_fingerprint`), rather - than under ``stat()``: the content of an `AnnexedReadableFile`, which is - not present locally, cannot be ``stat()``-ed. Anything else is handled as - without this. - """ - # Avoid circular import: - from .support.datalad_fuse import AnnexedReadableFile - - if isinstance(source, AnnexedReadableFile): - source = source.filepath - return cast("tuple[str, str] | None", annex_key_fingerprint(source)) - - def _sanitize_nwb_version( v: Any, filename: str | Path | None = None, diff --git a/dandi/support/datalad_fuse.py b/dandi/support/datalad_fuse.py index 8460039f3..a33c84fe9 100644 --- a/dandi/support/datalad_fuse.py +++ b/dandi/support/datalad_fuse.py @@ -33,6 +33,8 @@ import subprocess from typing import IO, Any, cast +from fscacher import annex_key_fingerprint + from ..misctypes import Readable lgr = logging.getLogger("dandi.support.datalad_fuse") @@ -74,25 +76,19 @@ def annex_initialized(directory: Path) -> bool: @dataclass class AnnexedReadableFile(Readable): """ - A `Readable` for a (locked) annexed file whose content is not present - locally, which is streamed with datalad-fuse (see the module docstring). + A `Readable` for a (locked) annexed file at ``filepath`` (a broken symbolic + link) whose content is not present locally, which is streamed with + datalad-fuse (see the module docstring). ``url`` is only the first URL + known for the content, for reporting: if it cannot be read, the adapter + tries the others. Instances are obtained by calling `get_annexed_readable()`. """ - #: The absolute path of the (broken) symbolic link to the file's content filepath: Path - - #: The git-annex key of the file key: str - - #: The size of the content, as recorded in the key size: int - - #: The first URL that the content may be streamed from (for reporting; - #: the adapter tries the others if it cannot be read from that one) url: str - adapter: Any = field(repr=False, compare=False) def open(self) -> IO[bytes]: @@ -111,6 +107,22 @@ def __str__(self) -> str: return str(self.filepath) +def annex_fingerprint(source: Any) -> tuple[str, str] | None: + """ + Fingerprint of ``source`` for ``PersistentCache.memoize_path`` + + Pass it as ``custom_fingerprint`` to cache the results of a function of a + local path or a `Readable` under the git-annex key of a locked annexed + file, paired with its path (see `fscacher.annex_key_fingerprint`), rather + than under ``stat()``: the content of an `AnnexedReadableFile`, which is + not present locally, cannot be ``stat()``-ed. Anything else is handled as + without this. + """ + if isinstance(source, AnnexedReadableFile): + source = source.filepath + return cast("tuple[str, str] | None", annex_key_fingerprint(source)) + + def get_annexed_readable(path: str | Path) -> AnnexedReadableFile | None: """ Return an `AnnexedReadableFile` for streaming the content of the annexed diff --git a/dandi/support/tests/test_datalad_fuse.py b/dandi/support/tests/test_datalad_fuse.py index 6dc31495a..6f594eb07 100644 --- a/dandi/support/tests/test_datalad_fuse.py +++ b/dandi/support/tests/test_datalad_fuse.py @@ -7,8 +7,8 @@ import h5py import pytest -from ..datalad_fuse import AnnexedReadableFile, get_annexed_readable -from ...pynwb_utils import annex_fingerprint, get_neurodata_types +from ..datalad_fuse import AnnexedReadableFile, annex_fingerprint, get_annexed_readable +from ...pynwb_utils import get_neurodata_types from ...tests.fixtures import RangeHTTPServer, make_git_annex_dandiset from ...tests.skip import mark From 22a54594e2dbee28fce0d0867d25fdfac593492f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 21:18:07 +0000 Subject: [PATCH 04/25] Note URL order and proxy behavior of datalad-fuse As documented in datalad/datalad-fuse#138. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- dandi/support/datalad_fuse.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/dandi/support/datalad_fuse.py b/dandi/support/datalad_fuse.py index a33c84fe9..cfe71d895 100644 --- a/dandi/support/datalad_fuse.py +++ b/dandi/support/datalad_fuse.py @@ -12,6 +12,10 @@ their metadata, or to be validated) without downloading the (possibly terabytes of) data. +The URLs are tried in the order git-annex lists them; for a DataLad Dandiset, +that is the DANDI Archive's API download URL (which redirects to S3) before the +direct S3 URL. datalad-fuse does not use HTTP proxy environment variables. + This requires datalad-fuse (``pip install "dandi[datalad]"``), which requires DataLad and git-annex. @@ -49,7 +53,8 @@ def get_adapter() -> Any: # Optional dependency: from datalad_fuse.fsspec import FsspecAdapter - # caching=False: do not keep the streamed blocks on disk, in the dataset + # caching=False: do not keep the streamed blocks on disk, in the dataset. + # The root and all paths passed to the adapter must be absolute. adapter = FsspecAdapter(Path(os.path.abspath(os.sep)), caching=False) atexit.register(adapter.__exit__, None, None, None) return adapter From 3361ae3a68cde55b7fc676f03077bc13964ec109 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 21:18:18 +0000 Subject: [PATCH 05/25] Document URL order and proxy behavior of stream policy Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- docs/source/cmdline/validate.rst | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/docs/source/cmdline/validate.rst b/docs/source/cmdline/validate.rst index a789f6203..00b76eb97 100644 --- a/docs/source/cmdline/validate.rst +++ b/docs/source/cmdline/validate.rst @@ -113,7 +113,8 @@ a Dandiset:: --format=json_lines --output=000003.jsonl 000003 Each streamed file yields an ``INFO``-level ``DANDI.FILE_CONTENT_STREAMED`` -result naming the URL its content was read from; a file that cannot be streamed +result naming the first URL known for its content (for a Dandiset, the DANDI +Archive's API download URL, which redirects to S3); a file that cannot be streamed (not an annexed file, or no URL known to git-annex) yields a ``DANDI.FILE_CONTENT_MISSING`` error instead. @@ -121,6 +122,8 @@ Notes: - git-annex must be initialized in the clone, which ``datalad clone`` does (after ``git clone``, run ``git annex init``). +- datalad-fuse does not use the HTTP proxy environment variables + (``HTTPS_PROXY`` etc.), so the URLs must be reachable directly. - Some nwbinspector checks read data arrays (e.g., timestamps), so the amount of data streamed for a file depends on its content; it is nevertheless usually a small fraction of the file. From 228026adc1a1e96f5268fd416a1638b6061ca15c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 21:21:00 +0000 Subject: [PATCH 06/25] Use datalad-fuse's pluggable backends (remfile) when available With the development version of datalad-fuse from datalad/datalad-fuse#131, use its RemoteFilesystemAdapter, which reads HDF5-based files such as NWB with remfile if installed (fsspec otherwise), and look up S3 exports for content without a registered URL. The released FsspecAdapter is still used with datalad-fuse 0.5.1. Test it in a new datalad-dev CI job. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- .github/workflows/run-tests.yml | 10 ++++++++++ dandi/support/datalad_fuse.py | 25 +++++++++++++++++++----- dandi/support/tests/test_datalad_fuse.py | 25 ++++++++++++++++++++++++ docs/source/cmdline/validate.rst | 6 ++++++ 4 files changed, 61 insertions(+), 5 deletions(-) diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml index d33f99a4f..c1129c7f9 100644 --- a/.github/workflows/run-tests.yml +++ b/.github/workflows/run-tests.yml @@ -76,6 +76,9 @@ jobs: - os: ubuntu-latest python: '3.12' mode: datalad + - os: ubuntu-latest + python: '3.12' + mode: datalad-dev steps: - name: Set up environment @@ -130,6 +133,13 @@ jobs: if: matrix.mode == 'datalad' run: pip install git-annex ".[datalad]" + # TODO: drop once datalad/datalad-fuse#131 is released + - name: Install git-annex, remfile and development datalad-fuse + if: matrix.mode == 'datalad-dev' + run: | + pip install git-annex remfile ".[datalad]" \ + "datalad-fuse @ git+https://github.com/datalad/datalad-fuse@refs/pull/131/head" + - name: Create NFS filesystem if: matrix.mode == 'nfs' run: | diff --git a/dandi/support/datalad_fuse.py b/dandi/support/datalad_fuse.py index cfe71d895..a6cea1f49 100644 --- a/dandi/support/datalad_fuse.py +++ b/dandi/support/datalad_fuse.py @@ -28,9 +28,11 @@ from __future__ import annotations import atexit +from collections.abc import Iterator from dataclasses import dataclass, field from datetime import datetime from functools import cache +from itertools import chain import logging import os from pathlib import Path @@ -51,11 +53,17 @@ def get_adapter() -> Any: root so that it serves files of any dataset, and closed at exit """ # Optional dependency: - from datalad_fuse.fsspec import FsspecAdapter + try: + # datalad-fuse with pluggable backends (datalad/datalad-fuse#131): + # remfile for HDF5-based files (such as NWB) if installed, fsspec + # otherwise, as configured with datalad.fusefs.backends + from datalad_fuse.adapter import RemoteFilesystemAdapter as Adapter + except ImportError: + from datalad_fuse.fsspec import FsspecAdapter as Adapter # caching=False: do not keep the streamed blocks on disk, in the dataset. # The root and all paths passed to the adapter must be absolute. - adapter = FsspecAdapter(Path(os.path.abspath(os.sep)), caching=False) + adapter = Adapter(Path(os.path.abspath(os.sep)), caching=False) atexit.register(adapter.__exit__, None, None, None) return adapter @@ -141,9 +149,12 @@ def get_annexed_readable(path: str | Path) -> AnnexedReadableFile | None: # Not a broken symbolic link return None try: - from datalad_fuse.fsspec import FileState + from datalad_fuse.adapter import FileState except ImportError: - return None + try: + from datalad_fuse.fsspec import FileState + except ImportError: + return None if not annex_initialized(filepath.parent): return None try: @@ -155,7 +166,11 @@ def get_annexed_readable(path: str | Path) -> AnnexedReadableFile | None: return None if state is not FileState.NO_CONTENT or key is None or key.size is None: return None - url = next(dsap.get_urls(str(key)), None) + urls: Iterator[str] = dsap.get_urls(str(key)) + if hasattr(dsap, "get_exporttree_urls"): + # Fallback to S3 exports of datalad/datalad-fuse#131 + urls = chain(urls, dsap.get_exporttree_urls(relpath, key)) + url = next(urls, None) if url is None: lgr.debug("%s: git-annex knows no URL for key %s", filepath, key) return None diff --git a/dandi/support/tests/test_datalad_fuse.py b/dandi/support/tests/test_datalad_fuse.py index 6f594eb07..b27fcfb36 100644 --- a/dandi/support/tests/test_datalad_fuse.py +++ b/dandi/support/tests/test_datalad_fuse.py @@ -95,3 +95,28 @@ def test_annexed_readable_cached_by_key( assert neurodata_types(get_annexed_readable(nwb)) == expected assert neurodata_types(nwb) == expected assert server.ranged_requests == requests + + +@pytest.mark.ai_generated +def test_annexed_readable_remfile( + streamed_nwb: tuple[Path, RangeHTTPServer], simple2_nwb: Path +) -> None: + """With datalad/datalad-fuse#131 and remfile, NWB files are read with remfile""" + pytest.importorskip("datalad_fuse.adapter") + pytest.importorskip("remfile") + from datalad_fuse.remfile import RemfileWrapper + + nwb, server = streamed_nwb + r = get_annexed_readable(nwb) + assert r is not None + fp = r.open() + try: + assert isinstance(fp, RemfileWrapper) + with h5py.File(fp, "r") as h5: + assert h5.attrs["nwb_version"] + finally: + fp.close() + assert server.ranged_requests > 0 + assert get_neurodata_types.__wrapped__(r) == get_neurodata_types.__wrapped__( + simple2_nwb + ) diff --git a/docs/source/cmdline/validate.rst b/docs/source/cmdline/validate.rst index 00b76eb97..ed0d8ad26 100644 --- a/docs/source/cmdline/validate.rst +++ b/docs/source/cmdline/validate.rst @@ -122,6 +122,11 @@ Notes: - git-annex must be initialized in the clone, which ``datalad clone`` does (after ``git clone``, run ``git annex init``). +- With the development version of datalad-fuse from + https://github.com/datalad/datalad-fuse/pull/131 and remfile_ installed + (``pip install remfile``), NWB and other HDF5-based files are read with + remfile, which is optimized for HDF5's access pattern, and content exported + to S3 without a URL registered in git-annex can be found too. - datalad-fuse does not use the HTTP proxy environment variables (``HTTPS_PROXY`` etc.), so the URLs must be reachable directly. - Some nwbinspector checks read data arrays (e.g., timestamps), so the amount @@ -145,6 +150,7 @@ Notes: the file and folder names of the annexed files themselves. .. _datalad-fuse: https://github.com/datalad/datalad-fuse +.. _remfile: https://github.com/flatironinstitute/remfile Development Options From 1ad6495fccfd121ddc6f0070a5a1ac0b3d063f0c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 21:26:15 +0000 Subject: [PATCH 07/25] Import fscacher lazily in dandi.support.datalad_fuse fscacher imports joblib, which imports numpy, so importing it with the module made the CLI (via dandi.validate in the stacked PR) import numpy, failing test_no_heavy_imports. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- dandi/support/datalad_fuse.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/dandi/support/datalad_fuse.py b/dandi/support/datalad_fuse.py index cfe71d895..54c27a18c 100644 --- a/dandi/support/datalad_fuse.py +++ b/dandi/support/datalad_fuse.py @@ -37,8 +37,6 @@ import subprocess from typing import IO, Any, cast -from fscacher import annex_key_fingerprint - from ..misctypes import Readable lgr = logging.getLogger("dandi.support.datalad_fuse") @@ -123,6 +121,10 @@ def annex_fingerprint(source: Any) -> tuple[str, str] | None: not present locally, cannot be ``stat()``-ed. Anything else is handled as without this. """ + # Avoid heavy import (fscacher imports joblib, which imports numpy) when + # this module is imported, e.g., by the CLI: + from fscacher import annex_key_fingerprint + if isinstance(source, AnnexedReadableFile): source = source.filepath return cast("tuple[str, str] | None", annex_key_fingerprint(source)) From d18843d73c449559c5c79b66c3bb7b770b98a584 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 01:40:01 +0000 Subject: [PATCH 08/25] TEMP: CI diagnostics for datalad job hang (to be reverted) Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- .github/workflows/run-tests.yml | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml index 97c9ab948..6e259c273 100644 --- a/.github/workflows/run-tests.yml +++ b/.github/workflows/run-tests.yml @@ -152,7 +152,17 @@ jobs: # Start coverage before pytest, which loads dandi's pytest plugin (and # thus imports most of dandi) before pytest-cov would start it run: | - python -m coverage run -m pytest -s -v -m "not obolibrary" dandi + set +e + python -X faulthandler -m coverage run -m pytest -s -v -m "not obolibrary" dandi 2>&1 | tee "$RUNNER_TEMP/pytest.log" + status=${PIPESTATUS[0]} + if [ "$status" -ne 0 ]; then + # TEMPORARY diagnostics: surface the hang/crash tracebacks as an annotation + grep -n -B3 -A80 -E "Timeout|Fatal Python|Current thread|Thread 0x" "$RUNNER_TEMP/pytest.log" | head -400 > "$RUNNER_TEMP/diag.txt" + grep -E "^dandi/.*::" "$RUNNER_TEMP/pytest.log" | tail -5 >> "$RUNNER_TEMP/diag.txt" + echo "::error title=pytest diagnostics::$(sed -e 's/%/%25/g' -e 's/\r//g' "$RUNNER_TEMP/diag.txt" | sed ':a;N;$!ba;s/\n/%0A/g')" + exit "$status" + fi + set -e python -m coverage combine python -m coverage xml From 796463300563e9ee347cdf38540ffdbd30552a0c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 02:40:19 +0000 Subject: [PATCH 09/25] Disable automatic GC while a streamed annexed file is open h5py holds its global lock while reading from a Python file object, and the read waits for another thread to fetch the data over HTTP (fsspec's I/O thread, or an HTTP server in the same process as in the tests). A garbage collection in that thread could then run the finalizer of an unreferenced h5py-backed object (e.g., hdmf's HDF5IO.__del__), which waits for the same lock: a deadlock, as seen in CI in test_validate_stream, ended by pytest-timeout with a segfault. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- dandi/support/datalad_fuse.py | 60 +++++++++++++++++++++++- dandi/support/tests/test_datalad_fuse.py | 21 +++++++++ 2 files changed, 80 insertions(+), 1 deletion(-) diff --git a/dandi/support/datalad_fuse.py b/dandi/support/datalad_fuse.py index 54c27a18c..1dfaf117c 100644 --- a/dandi/support/datalad_fuse.py +++ b/dandi/support/datalad_fuse.py @@ -31,10 +31,12 @@ from dataclasses import dataclass, field from datetime import datetime from functools import cache +import gc import logging import os from pathlib import Path import subprocess +import threading from typing import IO, Any, cast from ..misctypes import Readable @@ -76,6 +78,62 @@ def annex_initialized(directory: Path) -> bool: return r.returncode == 0 and bool(r.stdout.strip()) +_gc_lock = threading.Lock() +_gc_pauses = 0 +_gc_was_enabled = False + + +class _GCPausedFile: + """ + A file object streamed by datalad-fuse, during whose lifetime automatic + garbage collection is disabled + + h5py holds a global lock while it reads from a Python file object, and the + read waits for another thread (fsspec's I/O thread, or an HTTP server in + the same process) to fetch the data. If a garbage collection ran in that + thread meanwhile, finalizers of h5py objects (e.g., of an unreferenced + ``NWBHDF5IO``) would wait for the same lock, deadlocking the process. + """ + + def __init__(self, fp: Any) -> None: + global _gc_pauses, _gc_was_enabled + with _gc_lock: + if _gc_pauses == 0: + # Collect pending garbage now, in this thread, which does not + # hold h5py's lock yet + gc.collect() + _gc_was_enabled = gc.isenabled() + gc.disable() + _gc_pauses += 1 + self._fp = fp + self._paused = True + + def __getattr__(self, name: str) -> Any: + return getattr(self._fp, name) + + def __enter__(self) -> _GCPausedFile: + return self + + def __exit__(self, *_exc: Any) -> None: + self.close() + + def __del__(self) -> None: + if self._paused: + self.close() + + def close(self) -> None: + global _gc_pauses + try: + self._fp.close() + finally: + with _gc_lock: + if self._paused: + self._paused = False + _gc_pauses -= 1 + if _gc_pauses == 0 and _gc_was_enabled: + gc.enable() + + @dataclass class AnnexedReadableFile(Readable): """ @@ -95,7 +153,7 @@ class AnnexedReadableFile(Readable): adapter: Any = field(repr=False, compare=False) def open(self) -> IO[bytes]: - return cast("IO[bytes]", self.adapter.open(self.filepath, "rb")) + return cast("IO[bytes]", _GCPausedFile(self.adapter.open(self.filepath, "rb"))) def get_size(self) -> int: return self.size diff --git a/dandi/support/tests/test_datalad_fuse.py b/dandi/support/tests/test_datalad_fuse.py index 6f594eb07..d75b20c32 100644 --- a/dandi/support/tests/test_datalad_fuse.py +++ b/dandi/support/tests/test_datalad_fuse.py @@ -1,5 +1,6 @@ from __future__ import annotations +import gc from pathlib import Path import shutil @@ -56,6 +57,26 @@ def test_annexed_readable( assert server.ranged_requests > 0 +@pytest.mark.ai_generated +def test_annexed_readable_pauses_gc( + streamed_nwb: tuple[Path, RangeHTTPServer], +) -> None: + # Automatic garbage collection is disabled while any streamed file is open + # (to avoid deadlocks with h5py's lock; see `_GCPausedFile`) + nwb, _ = streamed_nwb + r = get_annexed_readable(nwb) + assert r is not None + assert gc.isenabled() + with r.open() as fp: + assert not gc.isenabled() + with r.open() as fp2: + assert fp2.read(8) == b"\x89HDF\r\n\x1a\n" + assert not gc.isenabled() + fp.seek(1) + assert fp.read(3) == b"HDF" + assert gc.isenabled() + + @pytest.mark.ai_generated def test_annexed_readable_none( streamed_nwb: tuple[Path, RangeHTTPServer], simple2_nwb: Path, tmp_path: Path From de6df649448e7f8aec91b1a0013e9d1437255d1b Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 02:40:26 +0000 Subject: [PATCH 10/25] Revert "TEMP: CI diagnostics for datalad job hang (to be reverted)" This reverts commit d18843d73c449559c5c79b66c3bb7b770b98a584. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- .github/workflows/run-tests.yml | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml index 6e259c273..97c9ab948 100644 --- a/.github/workflows/run-tests.yml +++ b/.github/workflows/run-tests.yml @@ -152,17 +152,7 @@ jobs: # Start coverage before pytest, which loads dandi's pytest plugin (and # thus imports most of dandi) before pytest-cov would start it run: | - set +e - python -X faulthandler -m coverage run -m pytest -s -v -m "not obolibrary" dandi 2>&1 | tee "$RUNNER_TEMP/pytest.log" - status=${PIPESTATUS[0]} - if [ "$status" -ne 0 ]; then - # TEMPORARY diagnostics: surface the hang/crash tracebacks as an annotation - grep -n -B3 -A80 -E "Timeout|Fatal Python|Current thread|Thread 0x" "$RUNNER_TEMP/pytest.log" | head -400 > "$RUNNER_TEMP/diag.txt" - grep -E "^dandi/.*::" "$RUNNER_TEMP/pytest.log" | tail -5 >> "$RUNNER_TEMP/diag.txt" - echo "::error title=pytest diagnostics::$(sed -e 's/%/%25/g' -e 's/\r//g' "$RUNNER_TEMP/diag.txt" | sed ':a;N;$!ba;s/\n/%0A/g')" - exit "$status" - fi - set -e + python -m coverage run -m pytest -s -v -m "not obolibrary" dandi python -m coverage combine python -m coverage xml From 5383c68f33583386d987663f2a7fa3d624d5eb2d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 20:46:50 +0000 Subject: [PATCH 11/25] Read missing annexed content with datalad-fuse, cached by annex key Add `dandi.support.datalad_fuse.AnnexedReadableFile`, a `Readable` for a locked annexed file whose content is not present locally, streamed with the adapter of datalad-fuse (no FUSE mount), which asks git-annex where the content is. It is available with the new `datalad` extra. Cache NWB metadata of locked annexed files, streamed or local, under their git-annex key paired with their path, with fscacher 0.5.0's `annex_key_fingerprint`. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- .github/workflows/run-tests.yml | 7 ++ dandi/metadata/nwb.py | 3 +- dandi/pynwb_utils.py | 25 +++- dandi/support/datalad_fuse.py | 147 +++++++++++++++++++++++ dandi/support/tests/test_datalad_fuse.py | 97 +++++++++++++++ dandi/tests/fixtures.py | 123 +++++++++++++++++++ dandi/tests/skip.py | 5 + pyproject.toml | 8 +- 8 files changed, 410 insertions(+), 5 deletions(-) create mode 100644 dandi/support/datalad_fuse.py create mode 100644 dandi/support/tests/test_datalad_fuse.py diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml index 948b34353..97c9ab948 100644 --- a/.github/workflows/run-tests.yml +++ b/.github/workflows/run-tests.yml @@ -73,6 +73,9 @@ jobs: - os: ubuntu-latest python: '3.11' mode: lowest-deps + - os: ubuntu-latest + python: '3.12' + mode: datalad steps: - name: Set up environment @@ -123,6 +126,10 @@ jobs: git+https://github.com/hdmf-dev/hdmf \ git+https://github.com/hdmf-dev/hdmf-zarr + - name: Install git-annex and datalad-fuse (for streaming annexed content) + if: matrix.mode == 'datalad' + run: pip install git-annex ".[datalad]" + - name: Create NFS filesystem if: matrix.mode == 'nfs' run: | diff --git a/dandi/metadata/nwb.py b/dandi/metadata/nwb.py index bf43dbcb2..140f2e1f7 100644 --- a/dandi/metadata/nwb.py +++ b/dandi/metadata/nwb.py @@ -16,6 +16,7 @@ from ..misctypes import DUMMY_DANDI_ETAG, Digest, LocalReadableFile, Readable from ..pynwb_utils import ( _get_pynwb_metadata, + annex_fingerprint, get_neurodata_types, get_nwb_version, ignore_benign_pynwb_warnings, @@ -28,7 +29,7 @@ # Disable this for clean hacking -@metadata_cache.memoize_path +@metadata_cache.memoize_path(custom_fingerprint=annex_fingerprint) def get_metadata( path: str | Path | Readable, digest: Digest | None = None ) -> dict[str, Any]: diff --git a/dandi/pynwb_utils.py b/dandi/pynwb_utils.py index 297df05f3..da6e4d835 100644 --- a/dandi/pynwb_utils.py +++ b/dandi/pynwb_utils.py @@ -23,7 +23,7 @@ import warnings import dandischema -from fscacher import PersistentCache +from fscacher import PersistentCache, annex_key_fingerprint import h5py import hdmf import numpy as np @@ -73,6 +73,25 @@ ) +def annex_fingerprint(source: Any) -> tuple[str, str] | None: + """ + Fingerprint of ``source`` for ``PersistentCache.memoize_path`` + + Pass it as ``custom_fingerprint`` to cache the results of a function of a + local path or a `Readable` under the git-annex key of a locked annexed + file, paired with its path (see `fscacher.annex_key_fingerprint`), rather + than under ``stat()``: the content of an `AnnexedReadableFile`, which is + not present locally, cannot be ``stat()``-ed. Anything else is handled as + without this. + """ + # Avoid circular import: + from .support.datalad_fuse import AnnexedReadableFile + + if isinstance(source, AnnexedReadableFile): + source = source.filepath + return cast("tuple[str, str] | None", annex_key_fingerprint(source)) + + def _sanitize_nwb_version( v: Any, filename: str | Path | None = None, @@ -194,7 +213,7 @@ def get_neurodata_types_to_modalities_map() -> dict[str, str]: return ndtypes -@metadata_cache.memoize_path +@metadata_cache.memoize_path(custom_fingerprint=annex_fingerprint) def get_neurodata_types(filepath: str | Path | Readable) -> list[str]: with open_readable(filepath) as fp, h5py.File(fp, "r") as h5file: all_pairs = _scan_neurodata_types(h5file) @@ -808,7 +827,7 @@ def copy_nwb_file(src: str | Path, dest: str | Path) -> str: return str(dest) -@metadata_cache.memoize_path +@metadata_cache.memoize_path(custom_fingerprint=annex_fingerprint) def nwb_has_external_links(filepath: str | Path | Readable) -> bool: with open_readable(filepath) as f, h5py.File(f, "r") as fp: visited = set() diff --git a/dandi/support/datalad_fuse.py b/dandi/support/datalad_fuse.py new file mode 100644 index 000000000..8460039f3 --- /dev/null +++ b/dandi/support/datalad_fuse.py @@ -0,0 +1,147 @@ +""" +Reading the content of annexed files of git-annex_ repositories (such as +DataLad_ datasets) that is not present locally, with datalad-fuse_. + +In a git-annex repository, a (locked) annexed file is a symbolic link into +:file:`.git/annex/objects/`, which is broken until the content is fetched. +datalad-fuse can mount such a repository with FUSE so that the content is +streamed on demand, but its adapter, which the mount is built on, also opens +the files directly with no mount. It asks git-annex where the content is +(``git annex whereis``) and streams it over HTTP(S) with fsspec_. That adapter +is used here, so that the files of a DataLad Dandiset can be read (e.g., for +their metadata, or to be validated) without downloading the (possibly +terabytes of) data. + +This requires datalad-fuse (``pip install "dandi[datalad]"``), which requires +DataLad and git-annex. + +.. _git-annex: https://git-annex.branchable.com +.. _DataLad: https://www.datalad.org +.. _datalad-fuse: https://github.com/datalad/datalad-fuse +.. _fsspec: https://github.com/fsspec/filesystem_spec +""" + +from __future__ import annotations + +import atexit +from dataclasses import dataclass, field +from datetime import datetime +from functools import cache +import logging +import os +from pathlib import Path +import subprocess +from typing import IO, Any, cast + +from ..misctypes import Readable + +lgr = logging.getLogger("dandi.support.datalad_fuse") + + +@cache +def get_adapter() -> Any: + """ + The datalad-fuse adapter shared by all files, rooted at the filesystem + root so that it serves files of any dataset, and closed at exit + """ + # Optional dependency: + from datalad_fuse.fsspec import FsspecAdapter + + # caching=False: do not keep the streamed blocks on disk, in the dataset + adapter = FsspecAdapter(Path(os.path.abspath(os.sep)), caching=False) + atexit.register(adapter.__exit__, None, None, None) + return adapter + + +@cache +def annex_initialized(directory: Path) -> bool: + """ + Whether ``directory`` is in a Git repository in which git-annex is + initialized. (DataLad would otherwise initialize it, modifying the + repository.) + """ + try: + r = subprocess.run( + ["git", "-C", str(directory), "config", "--get", "annex.uuid"], + capture_output=True, + text=True, + ) + except FileNotFoundError: + return False + return r.returncode == 0 and bool(r.stdout.strip()) + + +@dataclass +class AnnexedReadableFile(Readable): + """ + A `Readable` for a (locked) annexed file whose content is not present + locally, which is streamed with datalad-fuse (see the module docstring). + + Instances are obtained by calling `get_annexed_readable()`. + """ + + #: The absolute path of the (broken) symbolic link to the file's content + filepath: Path + + #: The git-annex key of the file + key: str + + #: The size of the content, as recorded in the key + size: int + + #: The first URL that the content may be streamed from (for reporting; + #: the adapter tries the others if it cannot be read from that one) + url: str + + adapter: Any = field(repr=False, compare=False) + + def open(self) -> IO[bytes]: + return cast("IO[bytes]", self.adapter.open(self.filepath, "rb")) + + def get_size(self) -> int: + return self.size + + def get_mtime(self) -> datetime | None: + return None + + def get_filename(self) -> str: + return self.filepath.name + + def __str__(self) -> str: + return str(self.filepath) + + +def get_annexed_readable(path: str | Path) -> AnnexedReadableFile | None: + """ + Return an `AnnexedReadableFile` for streaming the content of the annexed + file at ``path``, or `None` if datalad-fuse is not installed, ``path`` is + not a (locked) annexed file whose content is missing in a repository in + which git-annex is initialized, its key does not record the size of the + content, or git-annex knows no URL to stream it from + """ + filepath = Path(os.path.abspath(path)) + if not filepath.is_symlink() or filepath.exists(): + # Not a broken symbolic link + return None + try: + from datalad_fuse.fsspec import FileState + except ImportError: + return None + if not annex_initialized(filepath.parent): + return None + try: + dsap, relpath = get_adapter().resolve_dataset(filepath) + state, key = dsap.get_file_state(relpath) + except Exception as e: + # E.g., a repository without any commit + lgr.debug("%s: Cannot be read with datalad-fuse: %s", filepath, e) + return None + if state is not FileState.NO_CONTENT or key is None or key.size is None: + return None + url = next(dsap.get_urls(str(key)), None) + if url is None: + lgr.debug("%s: git-annex knows no URL for key %s", filepath, key) + return None + return AnnexedReadableFile( + filepath=filepath, key=str(key), size=key.size, url=url, adapter=get_adapter() + ) diff --git a/dandi/support/tests/test_datalad_fuse.py b/dandi/support/tests/test_datalad_fuse.py new file mode 100644 index 000000000..6dc31495a --- /dev/null +++ b/dandi/support/tests/test_datalad_fuse.py @@ -0,0 +1,97 @@ +from __future__ import annotations + +from pathlib import Path +import shutil + +from fscacher import PersistentCache +import h5py +import pytest + +from ..datalad_fuse import AnnexedReadableFile, get_annexed_readable +from ...pynwb_utils import annex_fingerprint, get_neurodata_types +from ...tests.fixtures import RangeHTTPServer, make_git_annex_dandiset +from ...tests.skip import mark + +pytestmark = mark.skipif_no_git_annex + + +@pytest.fixture +def streamed_nwb( + range_http_server: tuple[RangeHTTPServer, str], simple2_nwb: Path, tmp_path: Path +) -> tuple[Path, RangeHTTPServer]: + """ + An annexed copy of ``simple2_nwb`` in a git-annex repository, whose content + is not present but registered at a URL of the HTTP server + """ + pytest.importorskip("datalad_fuse.fsspec") + server, base_url = range_http_server + shutil.copy(simple2_nwb, tmp_path / "served" / "content.nwb") + make_git_annex_dandiset( + tmp_path / "ds", + {"sub-01/sub-01.nwb": (simple2_nwb, f"{base_url}/content.nwb")}, + ) + nwb = tmp_path / "ds" / "sub-01" / "sub-01.nwb" + assert not nwb.exists() + return nwb, server + + +@pytest.mark.ai_generated +def test_annexed_readable( + streamed_nwb: tuple[Path, RangeHTTPServer], simple2_nwb: Path +) -> None: + nwb, server = streamed_nwb + r = get_annexed_readable(nwb) + assert isinstance(r, AnnexedReadableFile) + assert r.key.startswith(f"SHA256E-s{simple2_nwb.stat().st_size}--") + assert r.url.endswith("/content.nwb") + assert r.get_size() == simple2_nwb.stat().st_size + assert r.get_filename() == "sub-01.nwb" + fp = r.open() + try: + # Random access over HTTP is enough for h5py to read the file + with h5py.File(fp, "r") as h5: + assert h5.attrs["nwb_version"] + finally: + fp.close() + assert server.ranged_requests > 0 + + +@pytest.mark.ai_generated +def test_annexed_readable_none( + streamed_nwb: tuple[Path, RangeHTTPServer], simple2_nwb: Path, tmp_path: Path +) -> None: + nwb, _ = streamed_nwb + # The content is present + (nwb.parent / "regular.nwb").write_bytes(b"content") + assert get_annexed_readable(nwb.parent / "regular.nwb") is None + # Not in a Git repository + assert get_annexed_readable(simple2_nwb) is None + # A broken symbolic link in a repository without git-annex initialized + plain = tmp_path / "plain" + plain.mkdir() + shutil.copytree(nwb.parent, plain / "sub-01", symlinks=True) + shutil.copytree(nwb.parent.parent / ".git", plain / ".git") + (plain / ".git" / "config").write_text("[core]\n\tbare = false\n") + assert get_annexed_readable(plain / "sub-01" / "sub-01.nwb") is None + + +@pytest.mark.ai_generated +def test_annexed_readable_cached_by_key( + streamed_nwb: tuple[Path, RangeHTTPServer], simple2_nwb: Path, tmp_path: Path +) -> None: + nwb, server = streamed_nwb + cache = PersistentCache(path=tmp_path / "cache") + neurodata_types = cache.memoize_path(custom_fingerprint=annex_fingerprint)( + get_neurodata_types.__wrapped__ + ) + r = get_annexed_readable(nwb) + assert r is not None + expected = get_neurodata_types.__wrapped__(simple2_nwb) + assert neurodata_types(r) == expected + requests = server.ranged_requests + assert requests > 0 + # Served from the cache, without the content being read again: by the + # readable, and by the path of the (broken) link itself + assert neurodata_types(get_annexed_readable(nwb)) == expected + assert neurodata_types(nwb) == expected + assert server.ranged_requests == requests diff --git a/dandi/tests/fixtures.py b/dandi/tests/fixtures.py index 9b128a23d..438219c99 100644 --- a/dandi/tests/fixtures.py +++ b/dandi/tests/fixtures.py @@ -3,12 +3,16 @@ from collections.abc import Callable, Iterator from dataclasses import dataclass, field, replace from datetime import datetime, timezone +from functools import partial +from http import HTTPStatus +from http.server import SimpleHTTPRequestHandler, ThreadingHTTPServer import logging import os from pathlib import Path import re import shutil from subprocess import DEVNULL, check_output, run +import threading from time import sleep from typing import Any, Literal from uuid import uuid4 @@ -352,6 +356,125 @@ def fixture( return fixture +def make_git_annex_dandiset(dandiset: Path, files: dict[str, tuple[Path, str]]) -> None: + """ + Make the directory ``dandiset`` (created if needed, with a minimal + :file:`dandiset.yaml`) a git-annex repository looking like a clone of a + DataLad Dandiset whose annexed content has not been fetched: for each + ``relpath: (source, url)`` item of ``files``, a copy of ``source`` is + annexed at ``relpath`` with ``url`` registered for it, and then dropped. + This needs git-annex. + """ + env = { + **os.environ, + "GIT_AUTHOR_NAME": "DANDI tests", + "GIT_AUTHOR_EMAIL": "tests@dandiarchive.org", + "GIT_COMMITTER_NAME": "DANDI tests", + "GIT_COMMITTER_EMAIL": "tests@dandiarchive.org", + } + + def git(*args: str) -> str: + return check_output( + ["git", "-C", str(dandiset), *args], env=env, text=True + ).strip() + + dandiset.mkdir(parents=True, exist_ok=True) + (dandiset / dandiset_metadata_file).write_text( + "identifier: '000027'\nname: Test\ndescription: Test dandiset\n" + ) + git("init", "-q") + git("annex", "init", "-q") + git("config", "annex.backend", "SHA256E") + git("add", dandiset_metadata_file) + for relpath, (source, _) in files.items(): + (dandiset / relpath).parent.mkdir(parents=True, exist_ok=True) + shutil.copy(source, dandiset / relpath) + git("annex", "add", "-q", *files) + for relpath, (_, url) in files.items(): + key = git("annex", "lookupkey", relpath) + git("annex", "registerurl", key, url) + git("commit", "-q", "-m", "Add files") + git("annex", "drop", "-q", "--force", *files) + + +class RangeHTTPServer(ThreadingHTTPServer): + """An HTTP server counting the range requests it served""" + + ranged_requests: int = 0 + + +class RangeHTTPRequestHandler(SimpleHTTPRequestHandler): + """ + A handler serving files with support for ``HEAD`` and single-range ``GET`` + requests, like S3 does + """ + + def log_message(self, format: str, *args: Any) -> None: + pass + + def _send_headers( + self, status: HTTPStatus, length: int, content_range: str | None = None + ) -> None: + self.send_response(status) + self.send_header("Content-Type", "application/octet-stream") + self.send_header("Content-Length", str(length)) + self.send_header("Accept-Ranges", "bytes") + if content_range is not None: + self.send_header("Content-Range", content_range) + self.end_headers() + + def do_HEAD(self) -> None: + path = self.translate_path(self.path) + if not os.path.isfile(path): + self.send_error(HTTPStatus.NOT_FOUND) + return + self._send_headers(HTTPStatus.OK, os.path.getsize(path)) + + def do_GET(self) -> None: + path = self.translate_path(self.path) + if not os.path.isfile(path): + self.send_error(HTTPStatus.NOT_FOUND) + return + size = os.path.getsize(path) + start, end = 0, size - 1 + if m := re.fullmatch(r"bytes=(\d+)-(\d*)", self.headers.get("Range", "")): + start = int(m[1]) + end = min(int(m[2]) if m[2] else size - 1, size - 1) + assert isinstance(self.server, RangeHTTPServer) + self.server.ranged_requests += 1 + self._send_headers( + HTTPStatus.PARTIAL_CONTENT, + end - start + 1, + f"bytes {start}-{end}/{size}", + ) + else: + self._send_headers(HTTPStatus.OK, size) + with open(path, "rb") as fp: + fp.seek(start) + self.wfile.write(fp.read(end - start + 1)) + + +@pytest.fixture() +def range_http_server(tmp_path: Path) -> Iterator[tuple[RangeHTTPServer, str]]: + """ + Serve the files in ``tmp_path / "served"`` over HTTP with support for range + requests; yields the server and its base URL + """ + served = tmp_path / "served" + served.mkdir() + server = RangeHTTPServer( + ("127.0.0.1", 0), partial(RangeHTTPRequestHandler, directory=str(served)) + ) + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + try: + yield server, f"http://127.0.0.1:{server.server_address[1]}" + finally: + server.shutdown() + server.server_close() + thread.join() + + def _make_subdirs_dandisets(path: Path) -> None: for bids_dataset_path in path.iterdir(): if bids_dataset_path.is_dir(): diff --git a/dandi/tests/skip.py b/dandi/tests/skip.py index 54d864b76..9e30b195d 100644 --- a/dandi/tests/skip.py +++ b/dandi/tests/skip.py @@ -125,6 +125,10 @@ def no_git(): return "Git not installed", shutil.which("git") is None +def no_git_annex(): + return "git-annex not installed", shutil.which("git-annex") is None + + # ### END MODIFIED CODE @@ -162,6 +166,7 @@ def on_windows(): no_docker_commands, no_docker_engine, no_git, + no_git_annex, no_network, # no_singularity, no_ssh, diff --git a/pyproject.toml b/pyproject.toml index 7a6ba7b9b..3fcdedd51 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -53,7 +53,7 @@ dependencies = [ "dandischema >= 0.12.0, != 0.13.0, < 0.15.0", "etelemetry >= 0.2.2", "fasteners >= 0.19", - "fscacher >= 0.3.0", + "fscacher >= 0.5.0", # Floor raised to what `py3-lowest` actually resolves: modern pynwb pins # hdmf >= 4, so declared floor is now the tested floor. "hdmf >= 4.1.0", @@ -120,6 +120,11 @@ test = [ # 5.0 switched to urllib3.connection.VerifiedHTTPSConnection (urllib3 2.x compat) "vcrpy >= 5.0.0", ] +# For reading the content of annexed files of DataLad datasets without +# fetching it (requires git-annex) +datalad = [ + "datalad-fuse >= 0.5.1", +] tools = [ "boto3", ] @@ -215,6 +220,7 @@ follow_imports = "skip" module = [ "bidsschematools.*", "click_didyoumean.*", + "datalad_fuse.*", "etelemetry.*", "fasteners.*", "fscacher.*", From 359e4af40322d78469687be32021d6931dabe65c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 21:17:17 +0000 Subject: [PATCH 12/25] Move annex_fingerprint next to AnnexedReadableFile; trim field comments annex_fingerprint is not specific to NWB files, and living in dandi.support.datalad_fuse it needs no deferred import. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- dandi/metadata/nwb.py | 2 +- dandi/pynwb_utils.py | 22 ++------------- dandi/support/datalad_fuse.py | 34 ++++++++++++++++-------- dandi/support/tests/test_datalad_fuse.py | 4 +-- 4 files changed, 28 insertions(+), 34 deletions(-) diff --git a/dandi/metadata/nwb.py b/dandi/metadata/nwb.py index 140f2e1f7..d0468d607 100644 --- a/dandi/metadata/nwb.py +++ b/dandi/metadata/nwb.py @@ -16,13 +16,13 @@ from ..misctypes import DUMMY_DANDI_ETAG, Digest, LocalReadableFile, Readable from ..pynwb_utils import ( _get_pynwb_metadata, - annex_fingerprint, get_neurodata_types, get_nwb_version, ignore_benign_pynwb_warnings, metadata_cache, nwb_has_external_links, ) +from ..support.datalad_fuse import annex_fingerprint from ..utils import find_parent_directory_containing lgr = get_logger() diff --git a/dandi/pynwb_utils.py b/dandi/pynwb_utils.py index da6e4d835..c49293f57 100644 --- a/dandi/pynwb_utils.py +++ b/dandi/pynwb_utils.py @@ -23,7 +23,7 @@ import warnings import dandischema -from fscacher import PersistentCache, annex_key_fingerprint +from fscacher import PersistentCache import h5py import hdmf import numpy as np @@ -42,6 +42,7 @@ metadata_nwb_subject_fields, ) from .misctypes import Readable +from .support.datalad_fuse import annex_fingerprint from .utils import get_module_version, is_url from .validate._types import ( Origin, @@ -73,25 +74,6 @@ ) -def annex_fingerprint(source: Any) -> tuple[str, str] | None: - """ - Fingerprint of ``source`` for ``PersistentCache.memoize_path`` - - Pass it as ``custom_fingerprint`` to cache the results of a function of a - local path or a `Readable` under the git-annex key of a locked annexed - file, paired with its path (see `fscacher.annex_key_fingerprint`), rather - than under ``stat()``: the content of an `AnnexedReadableFile`, which is - not present locally, cannot be ``stat()``-ed. Anything else is handled as - without this. - """ - # Avoid circular import: - from .support.datalad_fuse import AnnexedReadableFile - - if isinstance(source, AnnexedReadableFile): - source = source.filepath - return cast("tuple[str, str] | None", annex_key_fingerprint(source)) - - def _sanitize_nwb_version( v: Any, filename: str | Path | None = None, diff --git a/dandi/support/datalad_fuse.py b/dandi/support/datalad_fuse.py index 8460039f3..a33c84fe9 100644 --- a/dandi/support/datalad_fuse.py +++ b/dandi/support/datalad_fuse.py @@ -33,6 +33,8 @@ import subprocess from typing import IO, Any, cast +from fscacher import annex_key_fingerprint + from ..misctypes import Readable lgr = logging.getLogger("dandi.support.datalad_fuse") @@ -74,25 +76,19 @@ def annex_initialized(directory: Path) -> bool: @dataclass class AnnexedReadableFile(Readable): """ - A `Readable` for a (locked) annexed file whose content is not present - locally, which is streamed with datalad-fuse (see the module docstring). + A `Readable` for a (locked) annexed file at ``filepath`` (a broken symbolic + link) whose content is not present locally, which is streamed with + datalad-fuse (see the module docstring). ``url`` is only the first URL + known for the content, for reporting: if it cannot be read, the adapter + tries the others. Instances are obtained by calling `get_annexed_readable()`. """ - #: The absolute path of the (broken) symbolic link to the file's content filepath: Path - - #: The git-annex key of the file key: str - - #: The size of the content, as recorded in the key size: int - - #: The first URL that the content may be streamed from (for reporting; - #: the adapter tries the others if it cannot be read from that one) url: str - adapter: Any = field(repr=False, compare=False) def open(self) -> IO[bytes]: @@ -111,6 +107,22 @@ def __str__(self) -> str: return str(self.filepath) +def annex_fingerprint(source: Any) -> tuple[str, str] | None: + """ + Fingerprint of ``source`` for ``PersistentCache.memoize_path`` + + Pass it as ``custom_fingerprint`` to cache the results of a function of a + local path or a `Readable` under the git-annex key of a locked annexed + file, paired with its path (see `fscacher.annex_key_fingerprint`), rather + than under ``stat()``: the content of an `AnnexedReadableFile`, which is + not present locally, cannot be ``stat()``-ed. Anything else is handled as + without this. + """ + if isinstance(source, AnnexedReadableFile): + source = source.filepath + return cast("tuple[str, str] | None", annex_key_fingerprint(source)) + + def get_annexed_readable(path: str | Path) -> AnnexedReadableFile | None: """ Return an `AnnexedReadableFile` for streaming the content of the annexed diff --git a/dandi/support/tests/test_datalad_fuse.py b/dandi/support/tests/test_datalad_fuse.py index 6dc31495a..6f594eb07 100644 --- a/dandi/support/tests/test_datalad_fuse.py +++ b/dandi/support/tests/test_datalad_fuse.py @@ -7,8 +7,8 @@ import h5py import pytest -from ..datalad_fuse import AnnexedReadableFile, get_annexed_readable -from ...pynwb_utils import annex_fingerprint, get_neurodata_types +from ..datalad_fuse import AnnexedReadableFile, annex_fingerprint, get_annexed_readable +from ...pynwb_utils import get_neurodata_types from ...tests.fixtures import RangeHTTPServer, make_git_annex_dandiset from ...tests.skip import mark From 380929276a6c2ed3e66e6b876c5c4b7e89c51576 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 21:18:07 +0000 Subject: [PATCH 13/25] Note URL order and proxy behavior of datalad-fuse As documented in datalad/datalad-fuse#138. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- dandi/support/datalad_fuse.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/dandi/support/datalad_fuse.py b/dandi/support/datalad_fuse.py index a33c84fe9..cfe71d895 100644 --- a/dandi/support/datalad_fuse.py +++ b/dandi/support/datalad_fuse.py @@ -12,6 +12,10 @@ their metadata, or to be validated) without downloading the (possibly terabytes of) data. +The URLs are tried in the order git-annex lists them; for a DataLad Dandiset, +that is the DANDI Archive's API download URL (which redirects to S3) before the +direct S3 URL. datalad-fuse does not use HTTP proxy environment variables. + This requires datalad-fuse (``pip install "dandi[datalad]"``), which requires DataLad and git-annex. @@ -49,7 +53,8 @@ def get_adapter() -> Any: # Optional dependency: from datalad_fuse.fsspec import FsspecAdapter - # caching=False: do not keep the streamed blocks on disk, in the dataset + # caching=False: do not keep the streamed blocks on disk, in the dataset. + # The root and all paths passed to the adapter must be absolute. adapter = FsspecAdapter(Path(os.path.abspath(os.sep)), caching=False) atexit.register(adapter.__exit__, None, None, None) return adapter From 83a1b9390389035627c47d444891e5bbd619502a Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 21:26:15 +0000 Subject: [PATCH 14/25] Import fscacher lazily in dandi.support.datalad_fuse fscacher imports joblib, which imports numpy, so importing it with the module made the CLI (via dandi.validate in the stacked PR) import numpy, failing test_no_heavy_imports. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- dandi/support/datalad_fuse.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/dandi/support/datalad_fuse.py b/dandi/support/datalad_fuse.py index cfe71d895..54c27a18c 100644 --- a/dandi/support/datalad_fuse.py +++ b/dandi/support/datalad_fuse.py @@ -37,8 +37,6 @@ import subprocess from typing import IO, Any, cast -from fscacher import annex_key_fingerprint - from ..misctypes import Readable lgr = logging.getLogger("dandi.support.datalad_fuse") @@ -123,6 +121,10 @@ def annex_fingerprint(source: Any) -> tuple[str, str] | None: not present locally, cannot be ``stat()``-ed. Anything else is handled as without this. """ + # Avoid heavy import (fscacher imports joblib, which imports numpy) when + # this module is imported, e.g., by the CLI: + from fscacher import annex_key_fingerprint + if isinstance(source, AnnexedReadableFile): source = source.filepath return cast("tuple[str, str] | None", annex_key_fingerprint(source)) From 716d0d7211bc2ef8f148ad6da2f436c863a30de9 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 02:40:19 +0000 Subject: [PATCH 15/25] Disable automatic GC while a streamed annexed file is open h5py holds its global lock while reading from a Python file object, and the read waits for another thread to fetch the data over HTTP (fsspec's I/O thread, or an HTTP server in the same process as in the tests). A garbage collection in that thread could then run the finalizer of an unreferenced h5py-backed object (e.g., hdmf's HDF5IO.__del__), which waits for the same lock: a deadlock, as seen in CI in test_validate_stream, ended by pytest-timeout with a segfault. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- dandi/support/datalad_fuse.py | 60 +++++++++++++++++++++++- dandi/support/tests/test_datalad_fuse.py | 21 +++++++++ 2 files changed, 80 insertions(+), 1 deletion(-) diff --git a/dandi/support/datalad_fuse.py b/dandi/support/datalad_fuse.py index 54c27a18c..1dfaf117c 100644 --- a/dandi/support/datalad_fuse.py +++ b/dandi/support/datalad_fuse.py @@ -31,10 +31,12 @@ from dataclasses import dataclass, field from datetime import datetime from functools import cache +import gc import logging import os from pathlib import Path import subprocess +import threading from typing import IO, Any, cast from ..misctypes import Readable @@ -76,6 +78,62 @@ def annex_initialized(directory: Path) -> bool: return r.returncode == 0 and bool(r.stdout.strip()) +_gc_lock = threading.Lock() +_gc_pauses = 0 +_gc_was_enabled = False + + +class _GCPausedFile: + """ + A file object streamed by datalad-fuse, during whose lifetime automatic + garbage collection is disabled + + h5py holds a global lock while it reads from a Python file object, and the + read waits for another thread (fsspec's I/O thread, or an HTTP server in + the same process) to fetch the data. If a garbage collection ran in that + thread meanwhile, finalizers of h5py objects (e.g., of an unreferenced + ``NWBHDF5IO``) would wait for the same lock, deadlocking the process. + """ + + def __init__(self, fp: Any) -> None: + global _gc_pauses, _gc_was_enabled + with _gc_lock: + if _gc_pauses == 0: + # Collect pending garbage now, in this thread, which does not + # hold h5py's lock yet + gc.collect() + _gc_was_enabled = gc.isenabled() + gc.disable() + _gc_pauses += 1 + self._fp = fp + self._paused = True + + def __getattr__(self, name: str) -> Any: + return getattr(self._fp, name) + + def __enter__(self) -> _GCPausedFile: + return self + + def __exit__(self, *_exc: Any) -> None: + self.close() + + def __del__(self) -> None: + if self._paused: + self.close() + + def close(self) -> None: + global _gc_pauses + try: + self._fp.close() + finally: + with _gc_lock: + if self._paused: + self._paused = False + _gc_pauses -= 1 + if _gc_pauses == 0 and _gc_was_enabled: + gc.enable() + + @dataclass class AnnexedReadableFile(Readable): """ @@ -95,7 +153,7 @@ class AnnexedReadableFile(Readable): adapter: Any = field(repr=False, compare=False) def open(self) -> IO[bytes]: - return cast("IO[bytes]", self.adapter.open(self.filepath, "rb")) + return cast("IO[bytes]", _GCPausedFile(self.adapter.open(self.filepath, "rb"))) def get_size(self) -> int: return self.size diff --git a/dandi/support/tests/test_datalad_fuse.py b/dandi/support/tests/test_datalad_fuse.py index 6f594eb07..d75b20c32 100644 --- a/dandi/support/tests/test_datalad_fuse.py +++ b/dandi/support/tests/test_datalad_fuse.py @@ -1,5 +1,6 @@ from __future__ import annotations +import gc from pathlib import Path import shutil @@ -56,6 +57,26 @@ def test_annexed_readable( assert server.ranged_requests > 0 +@pytest.mark.ai_generated +def test_annexed_readable_pauses_gc( + streamed_nwb: tuple[Path, RangeHTTPServer], +) -> None: + # Automatic garbage collection is disabled while any streamed file is open + # (to avoid deadlocks with h5py's lock; see `_GCPausedFile`) + nwb, _ = streamed_nwb + r = get_annexed_readable(nwb) + assert r is not None + assert gc.isenabled() + with r.open() as fp: + assert not gc.isenabled() + with r.open() as fp2: + assert fp2.read(8) == b"\x89HDF\r\n\x1a\n" + assert not gc.isenabled() + fp.seek(1) + assert fp.read(3) == b"HDF" + assert gc.isenabled() + + @pytest.mark.ai_generated def test_annexed_readable_none( streamed_nwb: tuple[Path, RangeHTTPServer], simple2_nwb: Path, tmp_path: Path From c493d6c2e7be9e06a9bdfbdedf8380ebd2782a88 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 20:51:13 +0000 Subject: [PATCH 16/25] validate: stream missing annexed content with datalad-fuse Add a `stream` policy to `--missing-file-content`: the content of annexed files that is not present locally (as in a DataLad clone of a Dandiset) is streamed with datalad-fuse's adapter, so that pynwb and nwbinspector run on it without it being downloaded. pynwb validation results are cached under the files' git-annex keys. This is the validation integration of #1933, on top of the datalad-fuse readable rather than a git-only reader of the git-annex branch. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- dandi/cli/cmd_validate.py | 9 +- dandi/cli/tests/test_cmd_validate.py | 43 +++++++- dandi/files/bases.py | 82 ++++++++++++-- dandi/files/bids.py | 2 +- dandi/pynwb_utils.py | 51 +++++++-- dandi/validate/_core.py | 135 +++++++++++++++++------ dandi/validate/_types.py | 6 + dandi/validate/tests/test_core.py | 158 ++++++++++++++++++++++++++- docs/source/cmdline/validate.rst | 62 ++++++++++- 9 files changed, 495 insertions(+), 53 deletions(-) diff --git a/dandi/cli/cmd_validate.py b/dandi/cli/cmd_validate.py index 5241c5eb4..ea18707c0 100644 --- a/dandi/cli/cmd_validate.py +++ b/dandi/cli/cmd_validate.py @@ -170,8 +170,13 @@ def _filter_results( "in a datalad dataset without fetched data). 'error' (default) emits a " "concise error per file, 'skip' skips each such file with a warning, " "'only-non-data' skips content-dependent validators but still validates " - "path layout.", - type=click.Choice(["error", "only-non-data", "skip"], case_sensitive=True), + "path layout, 'stream' streams the content of annexed files with " + "datalad-fuse so that content-dependent validators run without the files " + "having to be downloaded (requires git-annex and `pip install " + "'dandi[datalad]'`).", + type=click.Choice( + ["error", "only-non-data", "skip", "stream"], case_sensitive=True + ), default="error", ) @click.option( diff --git a/dandi/cli/tests/test_cmd_validate.py b/dandi/cli/tests/test_cmd_validate.py index 1da5c16ca..aef651740 100644 --- a/dandi/cli/tests/test_cmd_validate.py +++ b/dandi/cli/tests/test_cmd_validate.py @@ -1,6 +1,7 @@ import json from pathlib import Path -from typing import cast +import shutil +from typing import Any, cast from click.testing import CliRunner import pytest @@ -998,6 +999,46 @@ def test_validate_missing_file_content_no_broken_symlinks(tmp_path: Path) -> Non assert "FILE_CONTENT_MISSING" not in r_skip.output +@pytest.mark.ai_generated +def test_validate_missing_file_content_stream( + tmp_path: Path, simple3_nwb: Path, range_http_server: tuple[Any, str] +) -> None: + """--missing-file-content=stream validates annexed content with datalad-fuse.""" + from ...tests.fixtures import make_git_annex_dandiset + from ...tests.skip import skipif + + skipif.no_git_annex() + pytest.importorskip("datalad_fuse.fsspec") + _, base_url = range_http_server + shutil.copy(simple3_nwb, tmp_path / "served" / "content.nwb") + ds = tmp_path / "ds" + make_git_annex_dandiset( + ds, {"sub-001/sub-001.nwb": (simple3_nwb, f"{base_url}/content.nwb")} + ) + out = tmp_path / "out.jsonl" + r = CliRunner().invoke( + validate, + [ + "--missing-file-content", + "stream", + "--min-severity", + "INFO", + "-f", + "json_lines", + "-o", + str(out), + str(ds), + ], + ) + assert "Traceback" not in r.output + ids = [rec.id for rec in load_validation_jsonl([str(out)])] + assert "DANDI.FILE_CONTENT_STREAMED" in ids + assert "DANDI.FILE_CONTENT_MISSING" not in ids + # simple3_nwb lacks a subject_id, which only content-based checks notice + assert "NWBI.check_subject_id_exists" in ids + assert r.exit_code == 1 + + @pytest.mark.ai_generated def test_validate_broken_symlink_path_argument(tmp_path: Path) -> None: """A broken symlink can be given directly as a path to validate. diff --git a/dandi/files/bases.py b/dandi/files/bases.py index db502dcc3..086298e88 100644 --- a/dandi/files/bases.py +++ b/dandi/files/bases.py @@ -10,6 +10,7 @@ from pathlib import Path import re from threading import Lock +import traceback from typing import IO, Any, Generic from xml.etree.ElementTree import fromstring @@ -32,7 +33,7 @@ set_asset_schema_key, ) from dandi.metadata.core import get_default_metadata -from dandi.misctypes import DUMMY_DANDI_ETAG, Digest, LocalReadableFile, P +from dandi.misctypes import DUMMY_DANDI_ETAG, Digest, LocalReadableFile, P, Readable from dandi.utils import post_upload_size_check, pre_upload_size_check, yaml_load from dandi.validate._types import ( ORIGIN_INTERNAL_DANDI, @@ -320,12 +321,22 @@ class LocalFileAsset(LocalAsset): an asset of a Dandiset """ + #: An alternative source for the asset's content, used instead of the file + #: at `filepath` when set. This is used to stream the content of an + #: annexed file (e.g., in a DataLad dataset) that is not present locally; + #: see `dandi.support.datalad_fuse`. + content_source: Readable | None = None + + def _content(self) -> Path | Readable: + """The source to read the asset's content from""" + return self.content_source if self.content_source is not None else self.filepath + def get_metadata( self, digest: Digest | None = None, ignore_errors: bool = True, ) -> BareAsset: - metadata = get_default_metadata(self.filepath, digest=digest) + metadata = get_default_metadata(self._content(), digest=digest) metadata.path = self.path return metadata @@ -515,7 +526,7 @@ def get_metadata( from dandi.metadata.nwb import nwb2asset try: - metadata = nwb2asset(self.filepath, digest=digest) + metadata = nwb2asset(self._content(), digest=digest) except Exception as e: lgr.warning( "Failed to extract NWB metadata from %s: %s: %s", @@ -524,7 +535,7 @@ def get_metadata( str(e), ) if ignore_errors: - metadata = get_default_metadata(self.filepath, digest=digest) + metadata = get_default_metadata(self._content(), digest=digest) else: raise metadata.path = self.path @@ -555,12 +566,16 @@ def get_validation_errors( pass else: # Avoid heavy import by importing within function: - from nwbinspector import Importance, inspect_nwbfile, load_config + from nwbinspector import Importance, load_config # Avoid heavy import by importing within function: from dandi.pynwb_utils import validate as pynwb_validate - errors.extend(pynwb_validate(self.filepath, devel_debug=devel_debug)) + errors.extend( + pynwb_validate( + self.filepath, devel_debug=devel_debug, readable=self.content_source + ) + ) if schema_version is not None: errors.extend( super().get_validation_errors( @@ -576,9 +591,7 @@ def get_validation_errors( validator_version=str(_get_nwb_inspector_version()), ) - for error in inspect_nwbfile( - nwbfile_path=self.filepath, - skip_validate=True, + for error in self._inspect_nwbfile( config=load_config(filepath_or_keyword="dandi"), importance_threshold=Importance.BEST_PRACTICE_VIOLATION, ): @@ -623,6 +636,57 @@ def get_validation_errors( ) return errors + def _inspect_nwbfile(self, **kwargs: Any) -> Iterator[Any]: + """ + Yield the messages from inspecting the file with nwbinspector, reading + the content from `content_source` if it is set (in which case what + `nwbinspector.inspect_nwbfile()` does for a local file is replicated + here for the streamed content) + """ + # Avoid heavy import by importing within function: + from nwbinspector import ( + Importance, + InspectorMessage, + inspect_nwbfile, + inspect_nwbfile_object, + ) + + if self.content_source is None: + yield from inspect_nwbfile( + nwbfile_path=self.filepath, skip_validate=True, **kwargs + ) + return + + # Avoid heavy import by importing within function: + import h5py + from pynwb import NWBHDF5IO + + from dandi.pynwb_utils import open_readable + + try: + with open_readable(self.content_source) as fp, h5py.File( + fp, "r" + ) as h5, NWBHDF5IO(file=h5, mode="r", load_namespaces=True) as io: + nwbfile = io.read() + for message in inspect_nwbfile_object(nwbfile_object=nwbfile, **kwargs): + if message is not None: + message.file_path = str(self.filepath) + yield message + except Exception as e: + # Report the failure the same way inspect_nwbfile() does for a + # local file that PyNWB cannot read + yield InspectorMessage( + message=traceback.format_exc(), + importance=Importance.ERROR, + check_function_name=( + f"During io.read(), an error occurred: " + f"{type(e).__module__}.{type(e).__name__}. " + "This indicates that PyNWB was unable to read the file. " + "See the traceback message for more details." + ), + file_path=str(self.filepath), + ) + class VideoAsset(LocalFileAsset): pass diff --git a/dandi/files/bids.py b/dandi/files/bids.py index 008b085dd..7624c40a3 100644 --- a/dandi/files/bids.py +++ b/dandi/files/bids.py @@ -250,7 +250,7 @@ def get_metadata( ) -> BareAsset: metadata = self.bids_dataset_description.get_asset_metadata(self) start_time = end_time = datetime.now().astimezone() - add_common_metadata(metadata, self.filepath, start_time, end_time, digest) + add_common_metadata(metadata, self._content(), start_time, end_time, digest) metadata.path = self.path return metadata diff --git a/dandi/pynwb_utils.py b/dandi/pynwb_utils.py index c49293f57..eb530b74e 100644 --- a/dandi/pynwb_utils.py +++ b/dandi/pynwb_utils.py @@ -611,8 +611,9 @@ def rename_if_matched(_name: str, obj: Any) -> None: f.visititems(rename_if_matched) -@validate_cache.memoize_path -def validate(path: str | Path, devel_debug: bool = False) -> list[ValidationResult]: +def validate( + path: str | Path, devel_debug: bool = False, *, readable: Readable | None = None +) -> list[ValidationResult]: """Run validation on a file and return errors In case of an exception being thrown, an error message added to the @@ -621,8 +622,38 @@ def validate(path: str | Path, devel_debug: bool = False) -> list[ValidationResu Parameters ---------- path: str or Path + Path of the file, as reported in the returned results + devel_debug: bool, optional + Whether to re-raise exceptions instead of reporting them as errors + readable: Readable, optional + If given, the file's content is read from this `Readable` instead of from + ``path`` (e.g., to stream the content of an annexed file which is not + present locally); the results are then cached only if it is an + `AnnexedReadableFile` (see `annex_fingerprint`). + """ + source: str | Readable = readable if readable is not None else str(path) + # fscacher is untyped, so the memoized _validate returns Any for mypy + return cast( + "list[ValidationResult]", + _validate(source, str(path), devel_debug=devel_debug), + ) + + +@validate_cache.memoize_path(custom_fingerprint=annex_fingerprint) +def _validate( + source: str | Readable, path: str, devel_debug: bool = False +) -> list[ValidationResult]: + """`validate` proper, with the content source as first argument for caching + + Parameters + ---------- + source: str or Readable + What to read the file's content from: its path or a `Readable` + path: str + Path of the file, as reported in the returned results + devel_debug: bool + Whether to re-raise exceptions instead of reporting them as errors """ - path = str(path) # Might come in as pathlib's PATH errors: list[ValidationResult] = [] # To overcome @@ -634,7 +665,7 @@ def validate(path: str | Path, devel_debug: bool = False) -> list[ValidationResu ) version = None try: - version = get_nwb_version(path, sanitize=False) + version = get_nwb_version(source, sanitize=False) except Exception: # we just will not remove any errors, it is required so should be some pass @@ -680,9 +711,15 @@ def validate(path: str | Path, devel_debug: bool = False) -> list[ValidationResu ) try: - # Validates against the namespaces cached in the file; pynwb falls back - # to its own namespaces if the file has none cached - error_outputs = pynwb.validate(path=path) + # Either way, validates against the namespaces cached in the file; pynwb + # falls back to its own namespaces if the file has none cached + if isinstance(source, Readable): + with open_readable(source) as fp, h5py.File(fp, "r") as h5, NWBHDF5IO( + file=h5, mode="r", load_namespaces=True + ) as reader: + error_outputs = pynwb.validate(io=reader) + else: + error_outputs = pynwb.validate(path=source) except Exception as exc: if devel_debug: raise diff --git a/dandi/validate/_core.py b/dandi/validate/_core.py index 371ce5a30..e3c0e4f33 100644 --- a/dandi/validate/_core.py +++ b/dandi/validate/_core.py @@ -10,9 +10,10 @@ from __future__ import annotations from collections.abc import Iterator +from importlib.util import find_spec import os from pathlib import Path -from typing import Any +import shutil from ._types import ( ORIGIN_VALIDATION_DANDI_LAYOUT, @@ -26,7 +27,8 @@ Validator, ) from ..consts import dandiset_metadata_file -from ..files import find_dandi_files +from ..files import DandiFile, LocalFileAsset, find_dandi_files +from ..support.datalad_fuse import AnnexedReadableFile, get_annexed_readable from ..utils import find_parent_directory_containing BIDS_TO_DANDI = { @@ -155,7 +157,8 @@ def _is_broken_symlink(filepath: Path) -> bool: # BIDS error codes that require reading file content (headers, pixel data). -# When ``only-non-data`` is active these are suppressed for broken-symlink files. +# When ``only-non-data`` or ``stream`` is active these are suppressed for +# broken-symlink files (the BIDS validator cannot stream content). _BIDS_CONTENT_DEPENDENT_CODES = frozenset( { "BIDS.NIFTI_HEADER_UNREADABLE", @@ -181,13 +184,20 @@ def validate( Policy for files whose content is unavailable (e.g. broken symlinks in a datalad dataset without fetched data). ``error`` emits a concise error, ``skip`` skips the file with a warning, ``only-non-data`` skips - content-dependent validators but still validates path layout. + content-dependent validators but still validates path layout, and + ``stream`` streams the content of annexed files with datalad-fuse (see + `dandi.support.datalad_fuse`) so that + content-dependent validators run without the content being present + locally. Yields ------ path, errors errors for a path """ + if missing_file_content == MissingFileContent.stream: + _check_streaming_requirements() + # Archive of unique `ValidationResult` objects obtained through # `DandiFile.get_validation_errors()` # Note: This is needed to hold on to the unique `ValidationResult` objects @@ -214,34 +224,43 @@ def validate( p, dandiset_path=dandiset_path, allow_all=allow_any_path ): # Handle broken symlinks (missing file content) - if _is_broken_symlink(df.filepath): - r = _handle_missing_content(df, missing_file_content) - if r is not None: - r_id = id(r) - if r_id not in df_result_ids: - df_results.append(r) - df_result_ids.add(r_id) - yield r - if missing_file_content in ( - MissingFileContent.skip, - MissingFileContent.error, - ): + is_broken = _is_broken_symlink(df.filepath) + if is_broken: + if missing_file_content == MissingFileContent.stream: + readable = _prepare_streaming(df) + # A file whose content cannot be streamed gets an error + # and is otherwise skipped + skip_file = readable is None + else: + readable = None + skip_file = missing_file_content in ( + MissingFileContent.skip, + MissingFileContent.error, + ) + r = _handle_missing_content(df, missing_file_content, readable) + r_id = id(r) + if r_id not in df_result_ids: + df_results.append(r) + df_result_ids.add(r_id) + yield r + if skip_file: continue - # only-non-data: fall through but pass the flag to validators + # only-non-data & stream: fall through but pass the policy to + # the validators - is_broken = _is_broken_symlink(df.filepath) for r in df.get_validation_errors( schema_version=schema_version, devel_debug=devel_debug, missing_file_content=(missing_file_content if is_broken else None), ): - # For broken-symlink files under only-non-data, suppress - # BIDS errors that require reading file content (e.g. + # For broken-symlink files under only-non-data and stream, + # suppress BIDS errors that require reading file content (e.g. # NIFTI_HEADER_UNREADABLE). The validator ran in full so # real files still get those checks. if ( is_broken - and missing_file_content == MissingFileContent.only_non_data + and missing_file_content + in (MissingFileContent.only_non_data, MissingFileContent.stream) and r.id in _BIDS_CONTENT_DEPENDENT_CODES ): continue @@ -252,21 +271,74 @@ def validate( yield r +def _check_streaming_requirements() -> None: + """Raise an informative error if what is needed for streaming file content + is not installed""" + if find_spec("datalad_fuse") is None: + raise RuntimeError( + "Streaming file content requires datalad-fuse; install it with " + "`pip install 'dandi[datalad]'`" + ) + if shutil.which("git-annex") is None: + raise RuntimeError("Streaming file content requires git-annex") + + +def _prepare_streaming(df: DandiFile) -> AnnexedReadableFile | None: + """ + Set up streaming of the content of the annexed file represented by ``df``, + and return the `Readable` that will be used for it, or `None` if the + content cannot be streamed (see `get_annexed_readable()`) + """ + if not isinstance(df, LocalFileAsset): + return None + readable = get_annexed_readable(df.filepath) + if readable is not None: + df.content_source = readable + return readable + + def _handle_missing_content( - df: Any, + df: DandiFile, policy: MissingFileContent, -) -> ValidationResult | None: + readable: AnnexedReadableFile | None = None, +) -> ValidationResult: """Produce a single :class:`ValidationResult` for a file with missing content. - Returns ``None`` when *policy* is ``only-non-data`` (a warning is not - needed because validation still proceeds on the non-data aspects). + For the ``stream`` policy, ``readable`` is the `Readable` streaming the + file's content, or `None` if the content cannot be streamed. """ - from ..files import DandiFile - - assert isinstance(df, DandiFile) filepath = df.filepath - if policy == MissingFileContent.error: + if policy == MissingFileContent.stream: + if readable is not None: + return ValidationResult( + id="DANDI.FILE_CONTENT_STREAMED", + origin=ORIGIN_VALIDATION_DANDI_LAYOUT, + severity=Severity.INFO, + scope=Scope.FILE, + path=filepath, + dandiset_path=df.dandiset_path, + message=( + f"File content is not present locally (git-annex key " + f"{readable.key}); content-dependent validation streams " + f"it from {readable.url}" + ), + ) + return ValidationResult( + id="DANDI.FILE_CONTENT_MISSING", + origin=ORIGIN_VALIDATION_DANDI_LAYOUT, + severity=Severity.ERROR, + scope=Scope.FILE, + path=filepath, + dandiset_path=df.dandiset_path, + message=( + f"File content is not available (broken symlink: " + f"{filepath} -> {os.readlink(filepath)}) and cannot be " + f"streamed: not an annexed file in a git-annex repository " + f"with a URL known to git-annex." + ), + ) + elif policy == MissingFileContent.error: return ValidationResult( id="DANDI.FILE_CONTENT_MISSING", origin=ORIGIN_VALIDATION_DANDI_LAYOUT, @@ -277,8 +349,9 @@ def _handle_missing_content( message=( f"File content is not available (broken symlink: " f"{filepath} -> {os.readlink(filepath)}). " - f"Use --missing-file-content=skip or " - f"--missing-file-content=only-non-data to handle gracefully." + f"Use --missing-file-content=skip, " + f"--missing-file-content=only-non-data, or " + f"--missing-file-content=stream to handle gracefully." ), ) elif policy == MissingFileContent.skip: diff --git a/dandi/validate/_types.py b/dandi/validate/_types.py index f41c8402e..48b18b45a 100644 --- a/dandi/validate/_types.py +++ b/dandi/validate/_types.py @@ -26,6 +26,12 @@ class MissingFileContent(StrEnum): skip = auto() """Skip the file entirely; emit a WARNING noting that validation was skipped.""" + stream = auto() + """Stream the content of annexed files with datalad-fuse (see + `dandi.support.datalad_fuse`) so that content-dependent validators run + without the content being present locally. An INFO result is emitted + for each streamed file, and an ERROR for each file that cannot be streamed.""" + lgr = logging.getLogger(__name__) diff --git a/dandi/validate/tests/test_core.py b/dandi/validate/tests/test_core.py index 7d331bb61..ab3759542 100644 --- a/dandi/validate/tests/test_core.py +++ b/dandi/validate/tests/test_core.py @@ -1,5 +1,7 @@ import json +import os from pathlib import Path +import shutil from typing import Any import pytest @@ -17,7 +19,14 @@ ) from ... import __version__ from ...consts import dandiset_metadata_file -from ...tests.fixtures import BIDS_TESTDATA_SELECTION +from ...pynwb_utils import validate as pynwb_validate +from ...support.datalad_fuse import get_annexed_readable +from ...tests.fixtures import ( + BIDS_TESTDATA_SELECTION, + RangeHTTPServer, + make_git_annex_dandiset, +) +from ...tests.skip import skipif def test_validate_nwb_error(simple3_nwb: Path) -> None: @@ -310,3 +319,150 @@ def test_validate_broken_symlink_real_file_still_validated(tmp_path: Path) -> No assert ( len(broken_pynwb) == 0 ), f"policy={policy.value}: pynwb should not run on the broken symlink" + + +# ---- Tests for streaming the content of annexed files (missing_file_content=stream) ---- + + +@pytest.fixture +def streamable_dandiset( + range_http_server: tuple[RangeHTTPServer, str], simple3_nwb: Path, tmp_path: Path +) -> tuple[Path, RangeHTTPServer]: + """A git-annex dandiset looking like a DataLad clone without fetched content. + + ``sub-001/sub-001.nwb`` is an annexed copy of *simple3_nwb* whose content + is served by the HTTP server, and ``sub-001/sub-001_video.mp4`` an annexed + video registered at a URL that is never read (only the size recorded in + its key is needed). + """ + skipif.no_git_annex() + pytest.importorskip("datalad_fuse.fsspec") + server, base_url = range_http_server + shutil.copy(simple3_nwb, tmp_path / "served" / "content.nwb") + video = tmp_path / "video.mp4" + video.write_bytes(b"not really a video") + ds = tmp_path / "ds" + make_git_annex_dandiset( + ds, + { + "sub-001/sub-001.nwb": (simple3_nwb, f"{base_url}/content.nwb"), + "sub-001/sub-001_video.mp4": (video, f"{base_url}/never-read.mp4"), + }, + ) + return ds, server + + +def _content_results(results: list[ValidationResult], name: str) -> list[tuple]: + """Path-independent summary of the results for the file called *name*.""" + return sorted( + (r.id, r.severity, r.origin.validator, r.message) + for r in results + if r.path is not None + and r.path.name == name + and r.id != "DANDI.FILE_CONTENT_STREAMED" + ) + + +@pytest.mark.ai_generated +def test_validate_stream( + streamable_dandiset: tuple[Path, RangeHTTPServer], + simple3_nwb: Path, + tmp_path: Path, +) -> None: + """stream policy validates the content of annexed files with datalad-fuse.""" + ds, server = streamable_dandiset + results = list(validate(ds, missing_file_content=MissingFileContent.stream)) + + streamed = [r for r in results if r.id == "DANDI.FILE_CONTENT_STREAMED"] + assert sorted(r.path.name for r in streamed if r.path is not None) == [ + "sub-001.nwb", + "sub-001_video.mp4", + ] + assert all(r.severity == Severity.INFO for r in streamed) + assert not [r for r in results if r.id.startswith("DANDI.FILE_CONTENT_MISSING")] + assert server.ranged_requests > 0 + + # Validating the video only needs the size recorded in its key + assert _content_results(results, "sub-001_video.mp4") == [] + + # Content-dependent validation of the NWB file gives the same results as + # for a regular dandiset containing the file itself + local = tmp_path / "local" + (local / "sub-001").mkdir(parents=True) + shutil.copy(ds / dandiset_metadata_file, local / dandiset_metadata_file) + shutil.copy(simple3_nwb, local / "sub-001" / "sub-001.nwb") + expected = list(validate(local)) + # simple3_nwb lacks a subject_id, which only content-based checks notice + assert any(r.origin.validator == Validator.nwbinspector for r in expected) + assert _content_results(results, "sub-001.nwb") == _content_results( + expected, "sub-001.nwb" + ) + + +@pytest.mark.ai_generated +@pytest.mark.skipif( + os.environ.get("DANDI_CACHE") == "ignore", reason="the validation cache is disabled" +) +def test_validate_stream_cached_by_key( + streamable_dandiset: tuple[Path, RangeHTTPServer], +) -> None: + """pynwb validation results of a streamed file are cached under its annex key.""" + ds, server = streamable_dandiset + nwb = ds / "sub-001" / "sub-001.nwb" + readable = get_annexed_readable(nwb) + assert readable is not None + results = pynwb_validate(nwb, readable=readable) + assert not [r for r in results if r.id == "pynwb.GENERIC"] + requests = server.ranged_requests + assert requests > 0 + # Served from the cache, without the content being read again + assert pynwb_validate(nwb, readable=get_annexed_readable(nwb)) == results + assert server.ranged_requests == requests + + +@pytest.mark.ai_generated +def test_validate_stream_not_streamable(tmp_path: Path) -> None: + """stream policy emits an error for a broken symlink that cannot be streamed.""" + pytest.importorskip("datalad_fuse.fsspec") + skipif.no_git_annex() + ds = _make_dandiset_with_broken_symlink(tmp_path) + results = list(validate(ds, missing_file_content=MissingFileContent.stream)) + errs = [r for r in results if r.id == "DANDI.FILE_CONTENT_MISSING"] + assert len(errs) == 1 + assert errs[0].severity == Severity.ERROR + assert errs[0].message is not None + assert "cannot be streamed" in errs[0].message + assert not [r for r in results if r.id == "DANDI.FILE_CONTENT_STREAMED"] + assert not [ + r + for r in results + if r.origin.validator in (Validator.pynwb, Validator.nwbinspector) + ] + + +@pytest.mark.ai_generated +def test_validate_stream_unreadable_url( + streamable_dandiset: tuple[Path, RangeHTTPServer], tmp_path: Path +) -> None: + """A registered URL that cannot be read yields errors, not an exception.""" + ds, _ = streamable_dandiset + (tmp_path / "served" / "content.nwb").unlink() + results = list(validate(ds, missing_file_content=MissingFileContent.stream)) + nwb = ds / "sub-001" / "sub-001.nwb" + assert [ + r for r in results if r.path == nwb and r.id == "DANDI.FILE_CONTENT_STREAMED" + ] + errs = [r for r in results if r.path == nwb and r.severity == Severity.ERROR] + assert {r.origin.validator for r in errs} == { + Validator.pynwb, + Validator.nwbinspector, + } + + +@pytest.mark.ai_generated +def test_validate_stream_requires_datalad_fuse( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setattr("dandi.validate._core.find_spec", lambda name: None) + with pytest.raises(RuntimeError, match=r"dandi\[datalad\]"): + list(validate(tmp_path, missing_file_content=MissingFileContent.stream)) diff --git a/docs/source/cmdline/validate.rst b/docs/source/cmdline/validate.rst index 3a07d7e7a..a789f6203 100644 --- a/docs/source/cmdline/validate.rst +++ b/docs/source/cmdline/validate.rst @@ -56,7 +56,7 @@ Options Limit the number of results shown per group (or in total when not grouping); the excess is replaced by a count of omitted results -.. option:: --missing-file-content [error|only-non-data|skip] +.. option:: --missing-file-content [error|only-non-data|skip|stream] How to handle files whose content is unavailable, such as the broken symbolic links of a DataLad_ dataset (a git-annex_ repository) whose @@ -73,6 +73,12 @@ Options Skip content-dependent validators (pynwb, nwbinspector, ...) for each such file but still validate its path layout + ``stream`` + Stream the content of each such file with datalad-fuse_ so that + content-dependent validators run without the file having to be + downloaded; see `Validating DataLad Dandisets Remotely`_ + below. + .. option:: --load Instead of running validation, load previously saved results from the @@ -84,6 +90,60 @@ Options .. _git-annex: https://git-annex.branchable.com +Validating DataLad Dandisets Remotely +------------------------------------- + +Every Dandiset on the DANDI Archive is mirrored as a DataLad dataset at +https://github.com/dandisets (with https://github.com/dandisets/dandisets as +the superdataset containing all of them). In such a dataset, the assets are +annexed files: symbolic links that remain broken until the content is fetched, +which for some Dandisets would mean downloading terabytes of data. The +``stream`` policy of :option:`--missing-file-content` lets ``dandi validate`` +run the content-dependent validators (pynwb and nwbinspector for NWB files) on +such files by streaming their content on demand with datalad-fuse_ (without +mounting anything), which asks git-annex where the content is, reading only +the parts of each file that the validators need. This requires git-annex and +``pip install "dandi[datalad]"``. + +For example, to produce a JSON Lines record of *all* validation results for +a Dandiset:: + + datalad clone https://github.com/dandisets/000003 + dandi validate --missing-file-content=stream --min-severity=INFO \ + --format=json_lines --output=000003.jsonl 000003 + +Each streamed file yields an ``INFO``-level ``DANDI.FILE_CONTENT_STREAMED`` +result naming the URL its content was read from; a file that cannot be streamed +(not an annexed file, or no URL known to git-annex) yields a +``DANDI.FILE_CONTENT_MISSING`` error instead. + +Notes: + +- git-annex must be initialized in the clone, which ``datalad clone`` does + (after ``git clone``, run ``git annex init``). +- Some nwbinspector checks read data arrays (e.g., timestamps), so the amount + of data streamed for a file depends on its content; it is nevertheless + usually a small fraction of the file. +- The pynwb validation results and the metadata of a streamed file are cached + under its git-annex key (a digest of its content), just as those of a local + file are cached under its modification time and size, so re-running the + command skips that work for files whose key has not changed since. As for + local files, set the :envvar:`DANDI_CACHE` environment variable to ``clear`` + to start afresh or to ``ignore`` to bypass the cache. +- Zarr assets are stored as separate subdatasets (https://github.com/dandizarrs) + and are not streamed yet: an uninstalled Zarr subdataset is an empty directory + that is not validated at all. Streaming them is a follow-up for when NWB Zarr + support has matured across the ecosystem. +- The BIDS validator cannot stream content, so BIDS errors that require reading + a file (e.g., unreadable NIfTI headers) are suppressed for annexed files under + the ``stream`` and ``only-non-data`` policies. For NWB datasets, the primary + use case, nothing is lost: everything the BIDS validator needs is either + present (the non-annexed sidecar files, which are kept in git) or encoded in + the file and folder names of the annexed files themselves. + +.. _datalad-fuse: https://github.com/datalad/datalad-fuse + + Development Options ------------------- From 97a7889b4d7e8db4356b54a84ccfbb2ddb10a351 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 21:18:18 +0000 Subject: [PATCH 17/25] Document URL order and proxy behavior of stream policy Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- docs/source/cmdline/validate.rst | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/docs/source/cmdline/validate.rst b/docs/source/cmdline/validate.rst index a789f6203..00b76eb97 100644 --- a/docs/source/cmdline/validate.rst +++ b/docs/source/cmdline/validate.rst @@ -113,7 +113,8 @@ a Dandiset:: --format=json_lines --output=000003.jsonl 000003 Each streamed file yields an ``INFO``-level ``DANDI.FILE_CONTENT_STREAMED`` -result naming the URL its content was read from; a file that cannot be streamed +result naming the first URL known for its content (for a Dandiset, the DANDI +Archive's API download URL, which redirects to S3); a file that cannot be streamed (not an annexed file, or no URL known to git-annex) yields a ``DANDI.FILE_CONTENT_MISSING`` error instead. @@ -121,6 +122,8 @@ Notes: - git-annex must be initialized in the clone, which ``datalad clone`` does (after ``git clone``, run ``git annex init``). +- datalad-fuse does not use the HTTP proxy environment variables + (``HTTPS_PROXY`` etc.), so the URLs must be reachable directly. - Some nwbinspector checks read data arrays (e.g., timestamps), so the amount of data streamed for a file depends on its content; it is nevertheless usually a small fraction of the file. From 1821e2fbc5b770a9513ca4104ea422963a14ebbc Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 01:40:01 +0000 Subject: [PATCH 18/25] TEMP: CI diagnostics for datalad job hang (to be reverted) Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- .github/workflows/run-tests.yml | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml index 97c9ab948..6e259c273 100644 --- a/.github/workflows/run-tests.yml +++ b/.github/workflows/run-tests.yml @@ -152,7 +152,17 @@ jobs: # Start coverage before pytest, which loads dandi's pytest plugin (and # thus imports most of dandi) before pytest-cov would start it run: | - python -m coverage run -m pytest -s -v -m "not obolibrary" dandi + set +e + python -X faulthandler -m coverage run -m pytest -s -v -m "not obolibrary" dandi 2>&1 | tee "$RUNNER_TEMP/pytest.log" + status=${PIPESTATUS[0]} + if [ "$status" -ne 0 ]; then + # TEMPORARY diagnostics: surface the hang/crash tracebacks as an annotation + grep -n -B3 -A80 -E "Timeout|Fatal Python|Current thread|Thread 0x" "$RUNNER_TEMP/pytest.log" | head -400 > "$RUNNER_TEMP/diag.txt" + grep -E "^dandi/.*::" "$RUNNER_TEMP/pytest.log" | tail -5 >> "$RUNNER_TEMP/diag.txt" + echo "::error title=pytest diagnostics::$(sed -e 's/%/%25/g' -e 's/\r//g' "$RUNNER_TEMP/diag.txt" | sed ':a;N;$!ba;s/\n/%0A/g')" + exit "$status" + fi + set -e python -m coverage combine python -m coverage xml From 106f661e1eacdf64ce026cfc8b079fb8c835fa18 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 02:40:26 +0000 Subject: [PATCH 19/25] Revert "TEMP: CI diagnostics for datalad job hang (to be reverted)" This reverts commit d18843d73c449559c5c79b66c3bb7b770b98a584. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- .github/workflows/run-tests.yml | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml index 6e259c273..97c9ab948 100644 --- a/.github/workflows/run-tests.yml +++ b/.github/workflows/run-tests.yml @@ -152,17 +152,7 @@ jobs: # Start coverage before pytest, which loads dandi's pytest plugin (and # thus imports most of dandi) before pytest-cov would start it run: | - set +e - python -X faulthandler -m coverage run -m pytest -s -v -m "not obolibrary" dandi 2>&1 | tee "$RUNNER_TEMP/pytest.log" - status=${PIPESTATUS[0]} - if [ "$status" -ne 0 ]; then - # TEMPORARY diagnostics: surface the hang/crash tracebacks as an annotation - grep -n -B3 -A80 -E "Timeout|Fatal Python|Current thread|Thread 0x" "$RUNNER_TEMP/pytest.log" | head -400 > "$RUNNER_TEMP/diag.txt" - grep -E "^dandi/.*::" "$RUNNER_TEMP/pytest.log" | tail -5 >> "$RUNNER_TEMP/diag.txt" - echo "::error title=pytest diagnostics::$(sed -e 's/%/%25/g' -e 's/\r//g' "$RUNNER_TEMP/diag.txt" | sed ':a;N;$!ba;s/\n/%0A/g')" - exit "$status" - fi - set -e + python -m coverage run -m pytest -s -v -m "not obolibrary" dandi python -m coverage combine python -m coverage xml From 6d23a5438a97325339ee79687975e5970e41edae Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 22:00:54 +0000 Subject: [PATCH 20/25] Read streamed annexed content in small cached blocks datalad-fuse opens the files with fsspec's defaults: 5 MiB of read-ahead, discarded whenever a read jumps elsewhere. h5py reads many small pieces scattered over an NWB file, so each open downloaded about as much as the whole file (64 MB for the neurodata types of a 16 MB file in the new test), and dandi opens each file several times during validation. Read in cached blocks of 256 KiB instead (1.3 MB for the same file). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- dandi/support/datalad_fuse.py | 22 ++++++++++++++ dandi/support/tests/test_datalad_fuse.py | 37 ++++++++++++++++++++++++ dandi/tests/fixtures.py | 5 +++- 3 files changed, 63 insertions(+), 1 deletion(-) diff --git a/dandi/support/datalad_fuse.py b/dandi/support/datalad_fuse.py index 1dfaf117c..4629e9f0f 100644 --- a/dandi/support/datalad_fuse.py +++ b/dandi/support/datalad_fuse.py @@ -188,6 +188,27 @@ def annex_fingerprint(source: Any) -> tuple[str, str] | None: return cast("tuple[str, str] | None", annex_key_fingerprint(source)) +#: Size of the blocks in which streamed content is read and cached +STREAM_BLOCK_SIZE = 256 * 1024 + + +def _read_in_small_blocks(dsap: Any) -> None: + """ + Make the files of a dataset adapter be read in cached blocks of + `STREAM_BLOCK_SIZE` bytes rather than with fsspec's default: h5py reads + many small pieces scattered over the file, and with the default (5 MiB of + read-ahead, discarded whenever a read jumps elsewhere) reading an NWB file + downloads about as much as the whole file each time it is opened + """ + # Avoid heavy import when this module is imported: + from fsspec.implementations.http import HTTPFileSystem + + fs = getattr(dsap, "fs", None) + if isinstance(fs, HTTPFileSystem): + fs.block_size = STREAM_BLOCK_SIZE + fs.cache_type = "blockcache" + + def get_annexed_readable(path: str | Path) -> AnnexedReadableFile | None: """ Return an `AnnexedReadableFile` for streaming the content of the annexed @@ -208,6 +229,7 @@ def get_annexed_readable(path: str | Path) -> AnnexedReadableFile | None: return None try: dsap, relpath = get_adapter().resolve_dataset(filepath) + _read_in_small_blocks(dsap) state, key = dsap.get_file_state(relpath) except Exception as e: # E.g., a repository without any commit diff --git a/dandi/support/tests/test_datalad_fuse.py b/dandi/support/tests/test_datalad_fuse.py index d75b20c32..8e58bc036 100644 --- a/dandi/support/tests/test_datalad_fuse.py +++ b/dandi/support/tests/test_datalad_fuse.py @@ -1,11 +1,15 @@ from __future__ import annotations +from datetime import datetime, timezone import gc from pathlib import Path import shutil from fscacher import PersistentCache import h5py +from hdmf.backends.hdf5 import H5DataIO +import numpy as np +from pynwb import NWBHDF5IO, NWBFile, TimeSeries import pytest from ..datalad_fuse import AnnexedReadableFile, annex_fingerprint, get_annexed_readable @@ -77,6 +81,39 @@ def test_annexed_readable_pauses_gc( assert gc.isenabled() +@pytest.mark.ai_generated +def test_annexed_readable_reads_little( + range_http_server: tuple[RangeHTTPServer, str], tmp_path: Path +) -> None: + """Reading an NWB file's structure downloads only a small part of it""" + pytest.importorskip("datalad_fuse.fsspec") + server, base_url = range_http_server + nwbfile = NWBFile("streamed", "id", datetime(2026, 1, 1, tzinfo=timezone.utc)) + for i in range(4): + nwbfile.add_acquisition( + TimeSeries( + name=f"ts{i}", + data=H5DataIO(np.arange(1_000_000, dtype="f4"), chunks=(10_000,)), + unit="V", + rate=1.0, + ) + ) + local = tmp_path / "local.nwb" + with NWBHDF5IO(local, "w") as io: + io.write(nwbfile) + size = local.stat().st_size + assert size > 15_000_000 + shutil.copy(local, tmp_path / "served" / "content.nwb") + make_git_annex_dandiset( + tmp_path / "ds", {"sub-01/sub-01.nwb": (local, f"{base_url}/content.nwb")} + ) + r = get_annexed_readable(tmp_path / "ds" / "sub-01" / "sub-01.nwb") + assert r is not None + assert get_neurodata_types.__wrapped__(r) == get_neurodata_types.__wrapped__(local) + # With fsspec's default 5 MiB read-ahead, most of the file was downloaded + assert 0 < server.bytes_served < size / 10 + + @pytest.mark.ai_generated def test_annexed_readable_none( streamed_nwb: tuple[Path, RangeHTTPServer], simple2_nwb: Path, tmp_path: Path diff --git a/dandi/tests/fixtures.py b/dandi/tests/fixtures.py index 438219c99..ffdfd611d 100644 --- a/dandi/tests/fixtures.py +++ b/dandi/tests/fixtures.py @@ -398,9 +398,10 @@ def git(*args: str) -> str: class RangeHTTPServer(ThreadingHTTPServer): - """An HTTP server counting the range requests it served""" + """An HTTP server counting the range requests and the bytes it served""" ranged_requests: int = 0 + bytes_served: int = 0 class RangeHTTPRequestHandler(SimpleHTTPRequestHandler): @@ -449,6 +450,8 @@ def do_GET(self) -> None: ) else: self._send_headers(HTTPStatus.OK, size) + assert isinstance(self.server, RangeHTTPServer) + self.server.bytes_served += end - start + 1 with open(path, "rb") as fp: fp.seek(start) self.wfile.write(fp.read(end - start + 1)) From 611ffb63fcbccd04b6e247ca8382f27f43cc2119 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 22:04:08 +0000 Subject: [PATCH 21/25] Apply the small-reads bound only when fsspec reads the file With datalad/datalad-fuse#131 and remfile installed, remfile reads HDF5 files, with its own read-ahead (chunks growing up to 100 MiB) that datalad-fuse does not let us configure. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- dandi/support/tests/test_datalad_fuse.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/dandi/support/tests/test_datalad_fuse.py b/dandi/support/tests/test_datalad_fuse.py index 21763c6ca..d827a44a0 100644 --- a/dandi/support/tests/test_datalad_fuse.py +++ b/dandi/support/tests/test_datalad_fuse.py @@ -110,8 +110,15 @@ def test_annexed_readable_reads_little( r = get_annexed_readable(tmp_path / "ds" / "sub-01" / "sub-01.nwb") assert r is not None assert get_neurodata_types.__wrapped__(r) == get_neurodata_types.__wrapped__(local) - # With fsspec's default 5 MiB read-ahead, most of the file was downloaded - assert 0 < server.bytes_served < size / 10 + assert server.bytes_served > 0 + with r.open() as fp: + remfile_used = type(fp._fp).__name__ == "RemfileWrapper" # type: ignore[attr-defined] + if not remfile_used: + # With fsspec's default 5 MiB read-ahead, most of the file was + # downloaded. (remfile, used for HDF5 files by + # datalad/datalad-fuse#131 when installed, reads ahead by itself, in + # chunks growing up to 100 MiB that datalad-fuse does not configure.) + assert server.bytes_served < size / 10 @pytest.mark.ai_generated From 2d461ddbeaf9b187cd91288a12dda05b3ceb18e1 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 22:26:56 +0000 Subject: [PATCH 22/25] Fetch consecutive missing blocks of streamed content in one request fsspec's "blockcache" fetches each block of a read with its own request, so a large read (e.g., of a dataset's data, by NWB Inspector) took one round trip per 256 KiB, i.e., a couple of MB/s however fast the connection. Use instead a block cache that fetches each run of consecutive missing blocks with a single request, and keeps up to 512 blocks (128 MiB) per open file rather than 32, which h5py's scattered reads of metadata kept evicting and fetching again. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- dandi/support/datalad_fuse.py | 76 +++++++++++++++++++++++- dandi/support/tests/test_datalad_fuse.py | 63 +++++++++++++++++++- 2 files changed, 137 insertions(+), 2 deletions(-) diff --git a/dandi/support/datalad_fuse.py b/dandi/support/datalad_fuse.py index 4629e9f0f..3765860b8 100644 --- a/dandi/support/datalad_fuse.py +++ b/dandi/support/datalad_fuse.py @@ -191,6 +191,79 @@ def annex_fingerprint(source: Any) -> tuple[str, str] | None: #: Size of the blocks in which streamed content is read and cached STREAM_BLOCK_SIZE = 256 * 1024 +#: Maximum number of blocks cached for each open streamed file +STREAM_MAX_BLOCKS = 512 + +#: Name of the fsspec cache defined by `_coalescing_block_cache()` +COALESCING_BLOCK_CACHE = "dandi-coalescing-blockcache" + + +@cache +def _coalescing_block_cache() -> type: + """ + Define (once, so as not to import fsspec when this module is imported) and + register with fsspec a cache that, like fsspec's ``"blockcache"``, keeps + the least recently used blocks of a file, but fetches the consecutive + blocks missing for a read with a single request rather than with one + request per block, which would make reading a large dataset take one + round trip per `STREAM_BLOCK_SIZE` bytes + """ + from collections import OrderedDict + + from fsspec.caching import BaseCache, register_cache + + class CoalescingBlockCache(BaseCache): # type: ignore[misc] + name = COALESCING_BLOCK_CACHE + + def __init__( + self, + blocksize: int, + fetcher: Any, + size: int, + maxblocks: int = STREAM_MAX_BLOCKS, + ) -> None: + super().__init__(blocksize, fetcher, size) + self.maxblocks = maxblocks + self.blocks: OrderedDict[int, bytes] = OrderedDict() + + def _fetch(self, start: int | None, end: int | None) -> bytes: + if start is None: + start = 0 + if end is None or end > self.size: + end = self.size + if start >= end: + return b"" + first = start // self.blocksize + last = (end - 1) // self.blocksize + numbers = range(first, last + 1) + missing = [n for n in numbers if n not in self.blocks] + # Fetch each run of consecutive missing blocks in one request + i = 0 + while i < len(missing): + j = i + while j + 1 < len(missing) and missing[j + 1] == missing[j] + 1: + j += 1 + run_start = missing[i] * self.blocksize + run_end = min((missing[j] + 1) * self.blocksize, self.size) + data = self.fetcher(run_start, run_end) + for n in range(missing[i], missing[j] + 1): + offset = (n - missing[i]) * self.blocksize + self.blocks[n] = data[offset : offset + self.blocksize] + i = j + 1 + out = [] + for n in numbers: + self.blocks.move_to_end(n) + out.append(self.blocks[n]) + # Evict the least recently used blocks, but not those just read + while len(self.blocks) > max(self.maxblocks, len(numbers)): + self.blocks.popitem(last=False) + data = b"".join(out) + offset = first * self.blocksize + return data[start - offset : end - offset] + + register_cache(CoalescingBlockCache, clobber=True) + return CoalescingBlockCache + def _read_in_small_blocks(dsap: Any) -> None: """ @@ -206,7 +279,8 @@ def _read_in_small_blocks(dsap: Any) -> None: fs = getattr(dsap, "fs", None) if isinstance(fs, HTTPFileSystem): fs.block_size = STREAM_BLOCK_SIZE - fs.cache_type = "blockcache" + _coalescing_block_cache() + fs.cache_type = COALESCING_BLOCK_CACHE def get_annexed_readable(path: str | Path) -> AnnexedReadableFile | None: diff --git a/dandi/support/tests/test_datalad_fuse.py b/dandi/support/tests/test_datalad_fuse.py index 8e58bc036..00a0674a0 100644 --- a/dandi/support/tests/test_datalad_fuse.py +++ b/dandi/support/tests/test_datalad_fuse.py @@ -12,7 +12,12 @@ from pynwb import NWBHDF5IO, NWBFile, TimeSeries import pytest -from ..datalad_fuse import AnnexedReadableFile, annex_fingerprint, get_annexed_readable +from ..datalad_fuse import ( + AnnexedReadableFile, + _coalescing_block_cache, + annex_fingerprint, + get_annexed_readable, +) from ...pynwb_utils import get_neurodata_types from ...tests.fixtures import RangeHTTPServer, make_git_annex_dandiset from ...tests.skip import mark @@ -114,6 +119,62 @@ def test_annexed_readable_reads_little( assert 0 < server.bytes_served < size / 10 +@pytest.mark.ai_generated +def test_coalescing_block_cache() -> None: + pytest.importorskip("fsspec") + content = bytes(range(256)) * 40 + requests: list[tuple[int, int]] = [] + + def fetcher(start: int, end: int) -> bytes: + requests.append((start, end)) + return content[start:end] + + c = _coalescing_block_cache()(100, fetcher, len(content), maxblocks=5) + # The blocks missing for a read are fetched with one request + assert c._fetch(150, 420) == content[150:420] + assert requests == [(100, 500)] + # Cached blocks are not fetched again, and runs of missing blocks around + # them are fetched with one request each + assert c._fetch(30, 720) == content[30:720] + assert requests[1:] == [(0, 100), (500, 800)] + # Reads at the end of the content, and beyond + assert c._fetch(10_190, 20_000) == content[10_190:] + assert requests[3:] == [(10_100, 10_240)] + assert c._fetch(20_000, 20_100) == b"" + # Only the 5 most recently used blocks are kept (but at least the blocks + # of the last read) + assert sorted(c.blocks) == [5, 6, 7, 101, 102] + requests.clear() + assert c._fetch(0, 10_240) == content + assert requests == [(0, 500), (800, 10_100)] + assert len(c.blocks) == 103 + + +@pytest.mark.ai_generated +def test_annexed_readable_large_read( + range_http_server: tuple[RangeHTTPServer, str], tmp_path: Path +) -> None: + """A large read of streamed content is fetched with one request""" + pytest.importorskip("datalad_fuse.fsspec") + server, base_url = range_http_server + local = tmp_path / "local.bin" + content = np.random.default_rng(0).bytes(10_000_000) + local.write_bytes(content) + shutil.copy(local, tmp_path / "served" / "content.bin") + make_git_annex_dandiset( + tmp_path / "ds", {"sub-01/data.bin": (local, f"{base_url}/content.bin")} + ) + r = get_annexed_readable(tmp_path / "ds" / "sub-01" / "data.bin") + assert r is not None + with r.open() as fp: + fp.seek(123) + assert fp.read(9_000_000) == content[123:9_000_123] + assert server.ranged_requests == 1 + fp.seek(5_000_000) + assert fp.read(100) == content[5_000_000:5_000_100] + assert server.ranged_requests == 1 + + @pytest.mark.ai_generated def test_annexed_readable_none( streamed_nwb: tuple[Path, RangeHTTPServer], simple2_nwb: Path, tmp_path: Path From b2f56f45b65498e6513b8dea54071a74a40f85a3 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 22:30:40 +0000 Subject: [PATCH 23/25] Cache NWB Inspector results, like those of PyNWB validation The results of inspecting an NWB file are cached in the validation cache, under the file (by stat() for a local file, or by its git-annex key for streamed content; see `annex_fingerprint`) and the version of nwbinspector, so that validating again does not inspect (and for streamed content, download) unchanged files again. A failure to read streamed content, which might be transient, is not cached. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- dandi/files/bases.py | 144 +++++++++++++++++++++--------- dandi/validate/tests/test_core.py | 50 +++++++++++ 2 files changed, 153 insertions(+), 41 deletions(-) diff --git a/dandi/files/bases.py b/dandi/files/bases.py index 086298e88..26f89e00a 100644 --- a/dandi/files/bases.py +++ b/dandi/files/bases.py @@ -2,16 +2,17 @@ from abc import ABC, abstractmethod from collections import deque -from collections.abc import Iterator +from collections.abc import Callable, Iterator from concurrent.futures import ThreadPoolExecutor, as_completed from dataclasses import dataclass from datetime import datetime +from functools import cache import os from pathlib import Path import re from threading import Lock import traceback -from typing import IO, Any, Generic +from typing import IO, Any, Generic, cast from xml.etree.ElementTree import fromstring import dandischema @@ -34,6 +35,7 @@ ) from dandi.metadata.core import get_default_metadata from dandi.misctypes import DUMMY_DANDI_ETAG, Digest, LocalReadableFile, P, Readable +from dandi.support.datalad_fuse import annex_fingerprint from dandi.utils import post_upload_size_check, pre_upload_size_check, yaml_load from dandi.validate._types import ( ORIGIN_INTERNAL_DANDI, @@ -636,56 +638,116 @@ def get_validation_errors( ) return errors - def _inspect_nwbfile(self, **kwargs: Any) -> Iterator[Any]: + def _inspect_nwbfile( + self, config: dict | None = None, importance_threshold: Any = None + ) -> list[Any]: """ - Yield the messages from inspecting the file with nwbinspector, reading + Return the messages from inspecting the file with nwbinspector, reading the content from `content_source` if it is set (in which case what `nwbinspector.inspect_nwbfile()` does for a local file is replicated here for the streamed content) """ # Avoid heavy import by importing within function: - from nwbinspector import ( - Importance, - InspectorMessage, - inspect_nwbfile, - inspect_nwbfile_object, - ) - - if self.content_source is None: - yield from inspect_nwbfile( - nwbfile_path=self.filepath, skip_validate=True, **kwargs - ) - return - - # Avoid heavy import by importing within function: - import h5py - from pynwb import NWBHDF5IO + from importlib.metadata import version - from dandi.pynwb_utils import open_readable + from nwbinspector import Importance, InspectorMessage + if importance_threshold is None: + importance_threshold = Importance.BEST_PRACTICE_SUGGESTION + source = self.content_source try: - with open_readable(self.content_source) as fp, h5py.File( - fp, "r" - ) as h5, NWBHDF5IO(file=h5, mode="r", load_namespaces=True) as io: - nwbfile = io.read() - for message in inspect_nwbfile_object(nwbfile_object=nwbfile, **kwargs): - if message is not None: - message.file_path = str(self.filepath) - yield message - except Exception as e: + return _cached_inspect_nwbfile()( + source if source is not None else str(self.filepath), + str(self.filepath), + version("nwbinspector"), + config, + importance_threshold, + ) + except _StreamedReadError as e: # Report the failure the same way inspect_nwbfile() does for a # local file that PyNWB cannot read - yield InspectorMessage( - message=traceback.format_exc(), - importance=Importance.ERROR, - check_function_name=( - f"During io.read(), an error occurred: " - f"{type(e).__module__}.{type(e).__name__}. " - "This indicates that PyNWB was unable to read the file. " - "See the traceback message for more details." - ), - file_path=str(self.filepath), - ) + exc = e.__cause__ + assert exc is not None + return [ + InspectorMessage( + message="".join(traceback.format_exception(exc)), + importance=Importance.ERROR, + check_function_name=( + f"During io.read(), an error occurred: " + f"{type(exc).__module__}.{type(exc).__name__}. " + "This indicates that PyNWB was unable to read the file. " + "See the traceback message for more details." + ), + file_path=str(self.filepath), + ) + ] + + +class _StreamedReadError(Exception): + """ + Failure to read or inspect streamed content, which is not cached as a + result of `_inspect_nwbfile()`, as it might be transient (e.g., a network + error) + """ + + +def _inspect_nwbfile( + source: str | Readable, + filepath: str, + nwbinspector_version: str, + config: dict | None, + importance_threshold: Any, +) -> list[Any]: + """ + Inspect the NWB file with nwbinspector, reading its content from ``source`` + (its path or a `Readable`), and return the messages, reporting + ``filepath`` as their file path + + The results are cached under ``source`` (see `annex_fingerprint`) and the + version of nwbinspector (which the caller passes so that it is part of the + key). + """ + # Avoid heavy import by importing within function: + from nwbinspector import inspect_nwbfile, inspect_nwbfile_object + + del nwbinspector_version + kwargs = {"config": config, "importance_threshold": importance_threshold} + if isinstance(source, str): + return list(inspect_nwbfile(nwbfile_path=source, skip_validate=True, **kwargs)) + + # Avoid heavy import by importing within function: + import h5py + from pynwb import NWBHDF5IO + + from dandi.pynwb_utils import open_readable + + messages = [] + try: + with open_readable(source) as fp, h5py.File(fp, "r") as h5, NWBHDF5IO( + file=h5, mode="r", load_namespaces=True + ) as io: + nwbfile = io.read() + for message in inspect_nwbfile_object(nwbfile_object=nwbfile, **kwargs): + if message is not None: + message.file_path = filepath + messages.append(message) + except Exception as e: + raise _StreamedReadError() from e + return messages + + +@cache +def _cached_inspect_nwbfile() -> Callable[..., list[Any]]: + """`_inspect_nwbfile()` memoized in the validation cache""" + # Avoid heavy import by importing within function: + from dandi.pynwb_utils import validate_cache + + return cast( + "Callable[..., list[Any]]", + validate_cache.memoize_path(custom_fingerprint=annex_fingerprint)( + _inspect_nwbfile + ), + ) class VideoAsset(LocalFileAsset): diff --git a/dandi/validate/tests/test_core.py b/dandi/validate/tests/test_core.py index ab3759542..39244178a 100644 --- a/dandi/validate/tests/test_core.py +++ b/dandi/validate/tests/test_core.py @@ -19,6 +19,7 @@ ) from ... import __version__ from ...consts import dandiset_metadata_file +from ...files import NWBAsset, dandi_file from ...pynwb_utils import validate as pynwb_validate from ...support.datalad_fuse import get_annexed_readable from ...tests.fixtures import ( @@ -420,6 +421,55 @@ def test_validate_stream_cached_by_key( assert server.ranged_requests == requests +def _streamed_nwb_asset(ds: Path) -> NWBAsset: + nwb = ds / "sub-001" / "sub-001.nwb" + df = dandi_file(nwb, ds) + assert isinstance(df, NWBAsset) + df.content_source = get_annexed_readable(nwb) + assert df.content_source is not None + return df + + +@pytest.mark.ai_generated +@pytest.mark.skipif( + os.environ.get("DANDI_CACHE") == "ignore", reason="the validation cache is disabled" +) +def test_validate_stream_inspection_cached_by_key( + streamable_dandiset: tuple[Path, RangeHTTPServer], +) -> None: + """NWB Inspector results of a streamed file are cached under its annex key.""" + ds, server = streamable_dandiset + results = _streamed_nwb_asset(ds).get_validation_errors() + inspector = [r for r in results if r.origin.validator == Validator.nwbinspector] + # simple3_nwb lacks a subject_id + assert inspector + requests = server.ranged_requests + # Served from the cache, without the content being read again + assert _streamed_nwb_asset(ds).get_validation_errors() == results + assert server.ranged_requests == requests + + +@pytest.mark.ai_generated +def test_validate_stream_inspection_failure_not_cached( + streamable_dandiset: tuple[Path, RangeHTTPServer], tmp_path: Path +) -> None: + """A failure to read streamed content for NWB Inspector is not cached.""" + ds, _ = streamable_dandiset + served = tmp_path / "served" / "content.nwb" + content = served.read_bytes() + served.unlink() + df = _streamed_nwb_asset(ds) + [msg] = df._inspect_nwbfile() + assert msg.importance.name == "ERROR" + assert msg.check_function_name.startswith("During io.read()") + assert msg.file_path == str(df.filepath) + served.write_bytes(content) + assert all( + m.check_function_name != msg.check_function_name + for m in _streamed_nwb_asset(ds)._inspect_nwbfile() + ) + + @pytest.mark.ai_generated def test_validate_stream_not_streamable(tmp_path: Path) -> None: """stream policy emits an error for a broken symlink that cannot be streamed.""" From 421f0c6d2e53b56ad90159b05fbedd517808cd5d Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 22:39:16 +0000 Subject: [PATCH 24/25] Add `dandi validate -J/--jobs` to validate NWB files in parallel Validating a streamed file mostly waits for its content to be downloaded, one request at a time (h5py serializes all reading of HDF5 files in a process), so validate NWB files (other than those of BIDS datasets, which are validated together) in a pool of processes, started with "spawn" since this process may run threads (e.g., fsspec's). The results are yielded in the same order as without the pool; the workers' log records are handled by the handlers of the main process. Streamed content is set up again in the worker, as its source cannot be sent to another process. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- dandi/cli/cmd_validate.py | 20 ++- dandi/validate/_core.py | 256 +++++++++++++++++++++++------- dandi/validate/tests/test_core.py | 24 +++ 3 files changed, 240 insertions(+), 60 deletions(-) diff --git a/dandi/cli/cmd_validate.py b/dandi/cli/cmd_validate.py index ea18707c0..4bc7a9861 100644 --- a/dandi/cli/cmd_validate.py +++ b/dandi/cli/cmd_validate.py @@ -61,6 +61,7 @@ def _collect_results( devel_debug: bool, allow_any_path: bool, missing_file_content: MissingFileContent = MissingFileContent.error, + jobs: int = 1, ) -> list[ValidationResult]: """Run validation and collect all results into a list.""" # Avoid heavy import by importing within function: @@ -87,6 +88,7 @@ def _collect_results( devel_debug=devel_debug, allow_any_path=allow_any_path, missing_file_content=missing_file_content, + jobs=jobs, ) ) @@ -179,6 +181,16 @@ def _filter_results( ), default="error", ) +@click.option( + "-J", + "--jobs", + type=click.IntRange(min=1), + default=1, + show_default=True, + help="Number of NWB files to validate in parallel, in separate processes. " + "Useful especially with '--missing-file-content stream', for which " + "validating a file mostly waits for its content to be downloaded.", +) @click.option( "--load", help="Load validation results from JSONL file(s) instead of running validation.", @@ -201,6 +213,7 @@ def validate( summary: bool = False, max_per_group: int | None = None, missing_file_content: str = "error", + jobs: int = 1, load: tuple[str, ...] = (), schema: str | None = None, devel_debug: bool = False, @@ -244,7 +257,12 @@ def validate( else: mfc = MissingFileContent(missing_file_content) results = _collect_results( - paths, schema, devel_debug, allow_any_path, missing_file_content=mfc + paths, + schema, + devel_debug, + allow_any_path, + missing_file_content=mfc, + jobs=jobs, ) # Auto-save companion right after collection, before filtering — so # all results are preserved regardless of display filters. diff --git a/dandi/validate/_core.py b/dandi/validate/_core.py index e3c0e4f33..8e441dc81 100644 --- a/dandi/validate/_core.py +++ b/dandi/validate/_core.py @@ -9,11 +9,18 @@ from __future__ import annotations +from collections import deque from collections.abc import Iterator +from concurrent.futures import Future, ProcessPoolExecutor +from copy import copy from importlib.util import find_spec +import logging +from logging.handlers import QueueHandler, QueueListener +from multiprocessing import get_context import os from pathlib import Path import shutil +from typing import Any from ._types import ( ORIGIN_VALIDATION_DANDI_LAYOUT, @@ -27,7 +34,7 @@ Validator, ) from ..consts import dandiset_metadata_file -from ..files import DandiFile, LocalFileAsset, find_dandi_files +from ..files import DandiFile, LocalFileAsset, NWBAsset, find_dandi_files from ..support.datalad_fuse import AnnexedReadableFile, get_annexed_readable from ..utils import find_parent_directory_containing @@ -173,6 +180,7 @@ def validate( devel_debug: bool = False, allow_any_path: bool = False, missing_file_content: MissingFileContent = MissingFileContent.error, + jobs: int = 1, ) -> Iterator[ValidationResult]: """Validate content @@ -189,6 +197,11 @@ def validate( `dandi.support.datalad_fuse`) so that content-dependent validators run without the content being present locally. + jobs : int + Number of processes in which to validate NWB files (other than those of + BIDS datasets) in parallel; the results are yielded in the same order + as with 1 (the default), with which everything is validated in this + process. Yields ------ @@ -208,67 +221,192 @@ def validate( # The ids of the objects in `df_results` obtain through the `id()` built-in function df_result_ids: set[int] = set() - for p in paths: - p = os.path.abspath(p) - dandiset_path = find_parent_directory_containing(dandiset_metadata_file, p) - if dandiset_path is None: - yield ValidationResult( - id="DANDI.NO_DANDISET_FOUND", - origin=ORIGIN_VALIDATION_DANDI_LAYOUT, - severity=Severity.ERROR, - scope=Scope.DANDISET, - path=Path(p), - message="Path is not inside a Dandiset", - ) - for df in find_dandi_files( - p, dandiset_path=dandiset_path, allow_all=allow_any_path - ): - # Handle broken symlinks (missing file content) - is_broken = _is_broken_symlink(df.filepath) - if is_broken: - if missing_file_content == MissingFileContent.stream: - readable = _prepare_streaming(df) - # A file whose content cannot be streamed gets an error - # and is otherwise skipped - skip_file = readable is None + def unique(results: list[ValidationResult]) -> Iterator[ValidationResult]: + for r in results: + r_id = id(r) + if r_id not in df_result_ids: + df_results.append(r) + df_result_ids.add(r_id) + yield r + + # The results of the files, in order, either computed or being computed + # in `pool` + pending: deque[list[ValidationResult] | Future[list[ValidationResult]]] + pending = deque() + pool = _ValidationPool(jobs) if jobs > 1 else None + try: + for p in paths: + p = os.path.abspath(p) + dandiset_path = find_parent_directory_containing(dandiset_metadata_file, p) + if dandiset_path is None: + pending.append( + [ + ValidationResult( + id="DANDI.NO_DANDISET_FOUND", + origin=ORIGIN_VALIDATION_DANDI_LAYOUT, + severity=Severity.ERROR, + scope=Scope.DANDISET, + path=Path(p), + message="Path is not inside a Dandiset", + ) + ] + ) + for df in find_dandi_files( + p, dandiset_path=dandiset_path, allow_all=allow_any_path + ): + # Handle broken symlinks (missing file content) + is_broken = _is_broken_symlink(df.filepath) + if is_broken: + if missing_file_content == MissingFileContent.stream: + readable = _prepare_streaming(df) + # A file whose content cannot be streamed gets an error + # and is otherwise skipped + skip_file = readable is None + else: + readable = None + skip_file = missing_file_content in ( + MissingFileContent.skip, + MissingFileContent.error, + ) + pending.append( + [_handle_missing_content(df, missing_file_content, readable)] + ) + if skip_file: + continue + # only-non-data & stream: fall through but pass the policy + # to the validators + + file_policy = missing_file_content if is_broken else None + if pool is not None and type(df) is NWBAsset: + pending.append( + pool.submit(df, schema_version, devel_debug, file_policy) + ) else: - readable = None - skip_file = missing_file_content in ( - MissingFileContent.skip, - MissingFileContent.error, + pending.append( + _validate_file(df, schema_version, devel_debug, file_policy) ) - r = _handle_missing_content(df, missing_file_content, readable) - r_id = id(r) - if r_id not in df_result_ids: - df_results.append(r) - df_result_ids.add(r_id) - yield r - if skip_file: - continue - # only-non-data & stream: fall through but pass the policy to - # the validators - - for r in df.get_validation_errors( - schema_version=schema_version, - devel_debug=devel_debug, - missing_file_content=(missing_file_content if is_broken else None), - ): - # For broken-symlink files under only-non-data and stream, - # suppress BIDS errors that require reading file content (e.g. - # NIFTI_HEADER_UNREADABLE). The validator ran in full so - # real files still get those checks. - if ( - is_broken - and missing_file_content - in (MissingFileContent.only_non_data, MissingFileContent.stream) - and r.id in _BIDS_CONTENT_DEPENDENT_CODES + # Yield what is ready, or wait (for the oldest file) if enough + # files are being validated + while pending and ( + isinstance(pending[0], list) + or pending[0].done() + or (pool is not None and len(pending) > 2 * jobs) ): - continue - r_id = id(r) - if r_id not in df_result_ids: - df_results.append(r) - df_result_ids.add(r_id) - yield r + head = pending.popleft() + yield from unique(head if isinstance(head, list) else head.result()) + while pending: + head = pending.popleft() + yield from unique(head if isinstance(head, list) else head.result()) + finally: + if pool is not None: + pool.shutdown() + + +def _validate_file( + df: DandiFile, + schema_version: str | None, + devel_debug: bool, + missing_file_content: MissingFileContent | None, +) -> list[ValidationResult]: + """ + Return the results of validating ``df``; ``missing_file_content`` is the + policy for its missing content, if it is missing + """ + results = [] + for r in df.get_validation_errors( + schema_version=schema_version, + devel_debug=devel_debug, + missing_file_content=missing_file_content, + ): + # For broken-symlink files under only-non-data and stream, suppress + # BIDS errors that require reading file content (e.g. + # NIFTI_HEADER_UNREADABLE). The validator ran in full so real files + # still get those checks. + if ( + missing_file_content + in (MissingFileContent.only_non_data, MissingFileContent.stream) + and r.id in _BIDS_CONTENT_DEPENDENT_CODES + ): + continue + results.append(r) + return results + + +def _validate_file_in_worker( + df: LocalFileAsset, + schema_version: str | None, + devel_debug: bool, + missing_file_content: MissingFileContent | None, +) -> list[ValidationResult]: + """`_validate_file()` in a worker process of a `_ValidationPool`""" + if missing_file_content == MissingFileContent.stream: + # The streamed content is opened in this process + _prepare_streaming(df) + return _validate_file(df, schema_version, devel_debug, missing_file_content) + + +def _init_worker(log_queue: Any, levels: dict[str, int]) -> None: + """ + Set up a worker process of a `_ValidationPool`: send the log records to + the parent process, and ignore the warnings that it ignores + """ + # Avoid heavy import by importing within function: + from ..pynwb_utils import ignore_benign_pynwb_warnings + + root = logging.getLogger() + root.handlers = [QueueHandler(log_queue)] + for name, level in levels.items(): + logging.getLogger(name).setLevel(level) + ignore_benign_pynwb_warnings() + + +class _ValidationPool: + """ + Processes in which files are validated in parallel (h5py serializes all + reading of HDF5 files in a process), whose log records are handled by the + handlers of this process + """ + + def __init__(self, jobs: int) -> None: + # Not "fork": this process may have started threads (e.g., fsspec's + # for streaming content) + context = get_context("spawn") + self.log_queue = context.Queue() + self.listener = QueueListener( + self.log_queue, *logging.getLogger().handlers, respect_handler_level=True + ) + self.listener.start() + levels = {name: logging.getLogger(name or None).level for name in ("", "dandi")} + self.executor = ProcessPoolExecutor( + max_workers=jobs, + mp_context=context, + initializer=_init_worker, + initargs=(self.log_queue, levels), + ) + + def submit( + self, + df: LocalFileAsset, + schema_version: str | None, + devel_debug: bool, + missing_file_content: MissingFileContent | None, + ) -> Future[list[ValidationResult]]: + # The source of streamed content cannot be sent to another process; + # it is set up again there + df = copy(df) + df.content_source = None + return self.executor.submit( + _validate_file_in_worker, + df, + schema_version, + devel_debug, + missing_file_content, + ) + + def shutdown(self) -> None: + self.executor.shutdown(cancel_futures=True) + self.listener.stop() + self.log_queue.close() def _check_streaming_requirements() -> None: diff --git a/dandi/validate/tests/test_core.py b/dandi/validate/tests/test_core.py index 39244178a..4e54fd9e2 100644 --- a/dandi/validate/tests/test_core.py +++ b/dandi/validate/tests/test_core.py @@ -470,6 +470,30 @@ def test_validate_stream_inspection_failure_not_cached( ) +@pytest.mark.ai_generated +def test_validate_jobs( + streamable_dandiset: tuple[Path, RangeHTTPServer], simple3_nwb: Path +) -> None: + """Validating in parallel processes gives the same results, in order.""" + ds, server = streamable_dandiset + for sub in ("sub-002", "sub-003"): + (ds / sub).mkdir() + shutil.copy(simple3_nwb, ds / sub / f"{sub}.nwb") + # In parallel first, so that the streamed content is read in a worker + # process rather than the results taken from the cache + parallel = list( + validate(ds, missing_file_content=MissingFileContent.stream, jobs=2) + ) + assert server.ranged_requests > 0 + serial = list(validate(ds, missing_file_content=MissingFileContent.stream)) + assert any(r.origin.validator == Validator.nwbinspector for r in serial) + + def summary(results: list[ValidationResult]) -> list[tuple]: + return [(r.id, r.path, r.origin.validator, r.message) for r in results] + + assert summary(parallel) == summary(serial) + + @pytest.mark.ai_generated def test_validate_stream_not_streamable(tmp_path: Path) -> None: """stream policy emits an error for a broken symlink that cannot be streamed.""" From a65f6ee8bf93d3bad160b29c5899058730823bdf Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 17:04:58 +0000 Subject: [PATCH 25/25] Log the progress of `dandi validate`, one message per file validated Validating streamed content can take a long time per file, during which nothing was shown. Find all the files to validate first, log their number, and log for each file when it is done ("[3/120] sub-01/...nwb: 2 issues"), including for the files validated in parallel with --jobs, as they finish. The `dataset_description.json` files of BIDS datasets, to which the assets found refer only weakly, are kept alive while their assets are validated. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- dandi/validate/_core.py | 183 ++++++++++++++++++++---------- dandi/validate/tests/test_core.py | 23 ++++ 2 files changed, 149 insertions(+), 57 deletions(-) diff --git a/dandi/validate/_core.py b/dandi/validate/_core.py index 8e441dc81..f02239c13 100644 --- a/dandi/validate/_core.py +++ b/dandi/validate/_core.py @@ -13,6 +13,7 @@ from collections.abc import Iterator from concurrent.futures import Future, ProcessPoolExecutor from copy import copy +from functools import partial from importlib.util import find_spec import logging from logging.handlers import QueueHandler, QueueListener @@ -20,6 +21,7 @@ import os from pathlib import Path import shutil +from threading import Lock from typing import Any from ._types import ( @@ -33,11 +35,21 @@ ValidationResult, Validator, ) +from .. import get_logger from ..consts import dandiset_metadata_file -from ..files import DandiFile, LocalFileAsset, NWBAsset, find_dandi_files +from ..files import ( + BIDSAsset, + DandiFile, + LocalAsset, + LocalFileAsset, + NWBAsset, + find_dandi_files, +) from ..support.datalad_fuse import AnnexedReadableFile, get_annexed_readable from ..utils import find_parent_directory_containing +lgr = get_logger() + BIDS_TO_DANDI = { "subject": "subject_id", "session": "session_id", @@ -233,67 +245,82 @@ def unique(results: list[ValidationResult]) -> Iterator[ValidationResult]: # in `pool` pending: deque[list[ValidationResult] | Future[list[ValidationResult]]] pending = deque() + # The files to validate (and the results for paths not in a Dandiset), all + # found first so that the progress can be reported as a count + items: list[DandiFile | ValidationResult] = [] + # The `dataset_description.json` files of BIDS datasets, which the + # `BIDSAsset` files found refer to only weakly + bids_descriptions = [] + for p in paths: + p = os.path.abspath(p) + dandiset_path = find_parent_directory_containing(dandiset_metadata_file, p) + if dandiset_path is None: + items.append( + ValidationResult( + id="DANDI.NO_DANDISET_FOUND", + origin=ORIGIN_VALIDATION_DANDI_LAYOUT, + severity=Severity.ERROR, + scope=Scope.DANDISET, + path=Path(p), + message="Path is not inside a Dandiset", + ) + ) + for df in find_dandi_files( + p, dandiset_path=dandiset_path, allow_all=allow_any_path + ): + if isinstance(df, BIDSAsset): + bids_descriptions.append(df.bids_dataset_description) + items.append(df) + progress = _Progress(sum(isinstance(i, DandiFile) for i in items)) + pool = _ValidationPool(jobs) if jobs > 1 else None try: - for p in paths: - p = os.path.abspath(p) - dandiset_path = find_parent_directory_containing(dandiset_metadata_file, p) - if dandiset_path is None: + for item in items: + if isinstance(item, ValidationResult): + pending.append([item]) + continue + df = item + # Handle broken symlinks (missing file content) + is_broken = _is_broken_symlink(df.filepath) + if is_broken: + if missing_file_content == MissingFileContent.stream: + readable = _prepare_streaming(df) + # A file whose content cannot be streamed gets an error + # and is otherwise skipped + skip_file = readable is None + else: + readable = None + skip_file = missing_file_content in ( + MissingFileContent.skip, + MissingFileContent.error, + ) pending.append( - [ - ValidationResult( - id="DANDI.NO_DANDISET_FOUND", - origin=ORIGIN_VALIDATION_DANDI_LAYOUT, - severity=Severity.ERROR, - scope=Scope.DANDISET, - path=Path(p), - message="Path is not inside a Dandiset", - ) - ] + [_handle_missing_content(df, missing_file_content, readable)] ) - for df in find_dandi_files( - p, dandiset_path=dandiset_path, allow_all=allow_any_path + if skip_file: + progress.done(df, None) + continue + # only-non-data & stream: fall through but pass the policy + # to the validators + + file_policy = missing_file_content if is_broken else None + if pool is not None and type(df) is NWBAsset: + future = pool.submit(df, schema_version, devel_debug, file_policy) + future.add_done_callback(partial(progress.future_done, df)) + pending.append(future) + else: + results = _validate_file(df, schema_version, devel_debug, file_policy) + progress.done(df, results) + pending.append(results) + # Yield what is ready, or wait (for the oldest file) if enough + # files are being validated + while pending and ( + isinstance(pending[0], list) + or pending[0].done() + or (pool is not None and len(pending) > 2 * jobs) ): - # Handle broken symlinks (missing file content) - is_broken = _is_broken_symlink(df.filepath) - if is_broken: - if missing_file_content == MissingFileContent.stream: - readable = _prepare_streaming(df) - # A file whose content cannot be streamed gets an error - # and is otherwise skipped - skip_file = readable is None - else: - readable = None - skip_file = missing_file_content in ( - MissingFileContent.skip, - MissingFileContent.error, - ) - pending.append( - [_handle_missing_content(df, missing_file_content, readable)] - ) - if skip_file: - continue - # only-non-data & stream: fall through but pass the policy - # to the validators - - file_policy = missing_file_content if is_broken else None - if pool is not None and type(df) is NWBAsset: - pending.append( - pool.submit(df, schema_version, devel_debug, file_policy) - ) - else: - pending.append( - _validate_file(df, schema_version, devel_debug, file_policy) - ) - # Yield what is ready, or wait (for the oldest file) if enough - # files are being validated - while pending and ( - isinstance(pending[0], list) - or pending[0].done() - or (pool is not None and len(pending) > 2 * jobs) - ): - head = pending.popleft() - yield from unique(head if isinstance(head, list) else head.result()) + head = pending.popleft() + yield from unique(head if isinstance(head, list) else head.result()) while pending: head = pending.popleft() yield from unique(head if isinstance(head, list) else head.result()) @@ -302,6 +329,48 @@ def unique(results: list[ValidationResult]) -> Iterator[ValidationResult]: pool.shutdown() +class _Progress: + """Report the progress of validating ``total`` files with log messages""" + + def __init__(self, total: int) -> None: + self.total = total + self.count = 0 + # Files may be done in the threads of a `_ValidationPool` + self.lock = Lock() + if total: + lgr.info("Validating %d file%s", total, "" if total == 1 else "s") + + def done( + self, + df: DandiFile, + results: list[ValidationResult] | None, + outcome: str = "skipped", + ) -> None: + """ + Report that validating ``df`` is done, with the given results, or + `None` if it was not validated, for the reason ``outcome`` + """ + with self.lock: + self.count += 1 + if results is None: + pass + elif results: + outcome = f"{len(results)} issue{'' if len(results) == 1 else 's'}" + else: + outcome = "OK" + path = df.path if isinstance(df, LocalAsset) else df.filepath + lgr.info("[%d/%d] %s: %s", self.count, self.total, path, outcome) + + def future_done( + self, df: DandiFile, future: Future[list[ValidationResult]] + ) -> None: + """Report that validating ``df`` in a `_ValidationPool` is done""" + if future.cancelled() or future.exception() is not None: + self.done(df, None, outcome="failed") + else: + self.done(df, future.result()) + + def _validate_file( df: DandiFile, schema_version: str | None, diff --git a/dandi/validate/tests/test_core.py b/dandi/validate/tests/test_core.py index 4e54fd9e2..23c296af5 100644 --- a/dandi/validate/tests/test_core.py +++ b/dandi/validate/tests/test_core.py @@ -1,4 +1,5 @@ import json +import logging import os from pathlib import Path import shutil @@ -470,6 +471,28 @@ def test_validate_stream_inspection_failure_not_cached( ) +@pytest.mark.ai_generated +@pytest.mark.parametrize("jobs", [1, 2]) +def test_validate_progress( + streamable_dandiset: tuple[Path, RangeHTTPServer], + simple3_nwb: Path, + caplog: pytest.LogCaptureFixture, + jobs: int, +) -> None: + """The progress of validation is logged, one message per file.""" + ds, _ = streamable_dandiset + (ds / "sub-002").mkdir() + shutil.copy(simple3_nwb, ds / "sub-002" / "sub-002.nwb") + caplog.set_level(logging.INFO, logger="dandi") + list(validate(ds, missing_file_content=MissingFileContent.stream, jobs=jobs)) + messages = [r.getMessage() for r in caplog.records] + messages = messages[messages.index("Validating 3 files") :] + done = [m for m in messages if m.startswith("[")] + assert sorted(m.split("] ")[0] for m in done) == ["[1/3", "[2/3", "[3/3"] + assert any(m.endswith("] sub-001/sub-001_video.mp4: OK") for m in done) + assert any("sub-002/sub-002.nwb: " in m and "issue" in m for m in done) + + @pytest.mark.ai_generated def test_validate_jobs( streamable_dandiset: tuple[Path, RangeHTTPServer], simple3_nwb: Path