From 5b292aa5ef3e3adc7a223e287fc562da32bb60bf Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 16:29:56 +0000 Subject: [PATCH 01/16] Add --missing-file-content=stream to validate DataLad Dandisets by streaming annexed content `dandi validate` can now validate a DataLad/git-annex Dandiset (such as the clones at https://github.com/dandisets) whose content has not been fetched: with `--missing-file-content=stream`, the content of each annexed file is streamed on demand (via fsspec) from the URLs registered for it in git-annex, preferring direct S3 URLs over DANDI API download URLs, so that pynwb and nwbinspector run without downloading the (possibly terabytes of) data. - New `dandi.support.annex` module: parses git-annex keys from the broken symlinks, reads URL logs from the `git-annex` branch (local or remote-tracking) using only `git`, and provides an `AnnexReadableFile` `Readable` that streams from the first URL that can be opened. - `LocalFileAsset.content_source` lets an asset read its content from a `Readable` instead of `filepath`; `pynwb_utils.validate()` accepts a `readable`, and `NWBAsset` runs nwbinspector on the streamed file via `inspect_nwbfile_object()`. - Each streamed file yields an INFO `DANDI.FILE_CONTENT_STREAMED` result naming the URL; a file that cannot be streamed yields `DANDI.FILE_CONTENT_MISSING`. BIDS content-dependent errors are suppressed for annexed files under `stream` as under `only-non-data`. - `dandi validate` now accepts a broken symlink directly as a path argument (`click.Path(exists=True)` used to reject it). - Docs: describe validating DataLad Dandisets, including every Dandiset of the `dandisets` superdataset, and the datalad-fuse alternative. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01HFhMPs6vF5zyeHDvPTvRcA --- dandi/cli/cmd_validate.py | 32 ++- dandi/cli/tests/test_cmd_validate.py | 57 +++++ dandi/files/bases.py | 82 ++++++- dandi/files/bids.py | 2 +- dandi/pynwb_utils.py | 44 +++- dandi/support/annex.py | 352 +++++++++++++++++++++++++++ dandi/support/tests/test_annex.py | 345 ++++++++++++++++++++++++++ dandi/tests/fixtures.py | 86 ++++++- dandi/validate/_core.py | 135 +++++++--- dandi/validate/_types.py | 6 + dandi/validate/tests/test_core.py | 133 +++++++++- docs/source/cmdline/validate.rst | 100 +++++++- 12 files changed, 1322 insertions(+), 52 deletions(-) create mode 100644 dandi/support/annex.py create mode 100644 dandi/support/tests/test_annex.py diff --git a/dandi/cli/cmd_validate.py b/dandi/cli/cmd_validate.py index 375dbc578..36adef186 100644 --- a/dandi/cli/cmd_validate.py +++ b/dandi/cli/cmd_validate.py @@ -7,7 +7,7 @@ import os import re import sys -from typing import IO, Union, cast +from typing import IO, Any, Union, cast import warnings import click @@ -35,6 +35,25 @@ class TruncationNotice: """Number of validation results omitted from this group.""" +class ExistingPath(click.Path): + """ + Like ``click.Path(exists=True)``, but also accepting broken symbolic links, + such as annexed files whose content is not present, which ``dandi validate`` + knows how to handle (see ``--missing-file-content``) + """ + + def convert( + self, value: Any, param: click.Parameter | None, ctx: click.Context | None + ) -> Any: + if not os.path.lexists(value): + self.fail( + f"{self.name.title()} {os.fsdecode(value)!r} does not exist.", + param, + ctx, + ) + return super().convert(value, param, ctx) + + STRUCTURED_FORMATS = ("json", "json_pp", "json_lines", "yaml") _EXT_TO_FORMAT = { @@ -217,8 +236,13 @@ def validate_bids( "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 from the URLs " + "registered for them in git-annex so that content-dependent validators run " + "without the files having to be downloaded (requires fsspec; install with " + "`pip install 'dandi[extras]'`).", + type=click.Choice( + ["error", "only-non-data", "skip", "stream"], case_sensitive=True + ), default="error", ) @click.option( @@ -228,7 +252,7 @@ def validate_bids( multiple=True, default=(), ) -@click.argument("paths", nargs=-1, type=click.Path(exists=True, dir_okay=True)) +@click.argument("paths", nargs=-1, type=ExistingPath(dir_okay=True)) @click.pass_context @devel_debug_option() @map_to_click_exceptions diff --git a/dandi/cli/tests/test_cmd_validate.py b/dandi/cli/tests/test_cmd_validate.py index 6d0b503a0..eaa4a9d11 100644 --- a/dandi/cli/tests/test_cmd_validate.py +++ b/dandi/cli/tests/test_cmd_validate.py @@ -996,3 +996,60 @@ def test_validate_missing_file_content_no_broken_symlinks(tmp_path: Path) -> Non assert r_default.exit_code == r_skip.exit_code == 0 assert "FILE_CONTENT_MISSING" not in r_default.output 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 +) -> None: + """--missing-file-content=stream validates annexed content via registered URLs.""" + from ...tests.fixtures import annex_key_for_file, make_annexed_dandiset + from ...tests.skip import skipif + + skipif.no_git() + pytest.importorskip("fsspec") + ds = tmp_path / "ds" + make_annexed_dandiset( + ds, + { + "sub-001/sub-001.nwb": ( + annex_key_for_file(simple3_nwb), + [simple3_nwb.as_uri()], + ) + }, + ) + 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.""" + ds = _make_dandiset_with_broken_symlinks(tmp_path) + r = CliRunner().invoke(validate, [str(ds / "sub-001" / "sub-001.nwb")]) + assert r.exit_code == 1 + assert "does not exist" not in r.output + assert "FILE_CONTENT_MISSING" in r.output + r = CliRunner().invoke(validate, [str(ds / "sub-001" / "nonexistent.nwb")]) + assert r.exit_code == 2 + assert "does not exist" in r.output diff --git a/dandi/files/bases.py b/dandi/files/bases.py index db502dcc3..120121dc8 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 + #: from a URL registered in git-annex; see `dandi.support.annex`. + 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 80fba2a20..552a9a424 100644 --- a/dandi/pynwb_utils.py +++ b/dandi/pynwb_utils.py @@ -616,8 +616,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 @@ -626,8 +627,35 @@ 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 not cached. """ + if readable is not None: + return _validate(path, readable, devel_debug=devel_debug) + # The memoizing decorator hides the return type from mypy + return cast( + "list[ValidationResult]", _validate_cached(path, devel_debug=devel_debug) + ) + + +@validate_cache.memoize_path +def _validate_cached( + path: str | Path, devel_debug: bool = False +) -> list[ValidationResult]: + return _validate(path, None, devel_debug=devel_debug) + + +def _validate( + path: str | Path, readable: Readable | None, devel_debug: bool = False +) -> list[ValidationResult]: path = str(path) # Might come in as pathlib's PATH + # What to read the content from: + source: str | Readable = readable if readable is not None else path errors: list[ValidationResult] = [] # To overcome @@ -639,7 +667,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 @@ -685,7 +713,12 @@ def validate(path: str | Path, devel_debug: bool = False) -> list[ValidationResu ) try: - if Version(pynwb.__version__) >= Version("3.0.0"): + if readable is not None: + with open_readable(readable) as fp, h5py.File(fp, "r") as h5, NWBHDF5IO( + file=h5, mode="r", load_namespaces=True + ) as reader: + error_outputs = pynwb.validate(io=reader) + elif Version(pynwb.__version__) >= Version("3.0.0"): error_outputs = pynwb.validate(path=path) elif Version(pynwb.__version__) >= Version( "2.2.0" @@ -695,6 +728,9 @@ def validate(path: str | Path, devel_debug: bool = False) -> list[ValidationResu else: # Fallback if an older version with pynwb.NWBHDF5IO(path=path, mode="r", load_namespaces=True) as reader: error_outputs = pynwb.validate(io=reader) + if isinstance(error_outputs, tuple): + # Older pynwb versions return (errors, status) for some call forms + error_outputs = error_outputs[0] except Exception as exc: if devel_debug: raise diff --git a/dandi/support/annex.py b/dandi/support/annex.py new file mode 100644 index 000000000..79954a242 --- /dev/null +++ b/dandi/support/annex.py @@ -0,0 +1,352 @@ +""" +Support for reading the content of annexed files in git-annex_ repositories +(such as DataLad_ datasets) when that content is not present locally. + +In a git-annex repository, an annexed file is a symbolic link into +:file:`.git/annex/objects/`. Until the content is fetched (e.g., with ``git +annex get`` or ``datalad get``), the link is broken. The name of the link's +target is the file's *key*, which for the ``SHA256E`` and ``MD5E`` backends used +for DANDI Dandisets records the size of the content. The URLs from which the +content can be retrieved are recorded in the ``git-annex`` branch of the +repository (the "URL log" of the key). + +This module reads that information using only ``git`` (git-annex itself is not +required) and exposes the content of such files as a `Readable` that streams it +over HTTP(S) with fsspec_, so that, e.g., ``dandi validate`` can validate a +DataLad Dandiset without downloading the (possibly terabytes of) data. + +.. _git-annex: https://git-annex.branchable.com +.. _DataLad: https://www.datalad.org +.. _fsspec: https://github.com/fsspec/filesystem_spec +""" + +from __future__ import annotations + +from dataclasses import dataclass +from datetime import datetime +import hashlib +import logging +import os +from pathlib import Path, PurePosixPath +import re +import subprocess +from typing import IO, ClassVar, cast + +from ..misctypes import Readable + +lgr = logging.getLogger("dandi.support.annex") + +#: Regular expression matching DANDI Archive API "download" URLs, which +#: redirect to the actual storage location on every request and are thus less +#: efficient for streaming than the direct URLs also registered in git-annex +DANDI_API_DOWNLOAD_URL_RE = re.compile( + r"/api/(?:dandisets/[^/]+/versions/[^/]+/)?assets/[^/]+/download/?(?:[?#].*)?$" +) + +#: Block size (in bytes) for streaming reads of remote content +STREAM_BLOCK_SIZE = 4 * 1024 * 1024 + +#: Maximum number of blocks of remote content kept in memory per open file +STREAM_MAX_BLOCKS = 64 + + +@dataclass(frozen=True) +class AnnexKey: + """ + A parsed git-annex key, e.g., ``SHA256E-s1234--.nwb`` (see + https://git-annex.branchable.com/internals/key_format/) + """ + + #: The key as a whole + key: str + + #: The key backend, e.g., ``SHA256E`` or ``MD5E`` + backend: str + + #: The size of the content in bytes, if recorded in the key + size: int | None + + #: The backend-specific part of the key following ``--``; for the checksum + #: backends, this is the checksum followed (for the ``*E`` variants) by the + #: file extension + name: str + + @classmethod + def parse(cls, key: str) -> AnnexKey: + """Parse a git-annex key string""" + fields, sep, name = key.partition("--") + backend, *extra = fields.split("-") + if not sep or not name or not backend: + raise ValueError(f"Not a git-annex key: {key!r}") + size = None + for f in extra: + if f[:1] == "s" and f[1:].isdigit(): + size = int(f[1:]) + return cls(key=key, backend=backend, size=size, name=name) + + def __str__(self) -> str: + return self.key + + @property + def hashdir_lower(self) -> str: + """ + The two-level hash directory under which git-annex stores the key's + log files in the ``git-annex`` branch + """ + h = hashlib.md5(self.key.encode("utf-8"), usedforsecurity=False).hexdigest() + return f"{h[:3]}/{h[3:6]}" + + +def get_annex_key(path: str | Path) -> AnnexKey | None: + """ + Return the git-annex key of the annexed file at ``path``, or `None` if + ``path`` is not a symbolic link into a git-annex object store + """ + try: + target = os.readlink(path) + except OSError: + # Not a symlink (or does not exist at all) + return None + parts = PurePosixPath(target.replace(os.sep, "/")).parts + if not any(a == "annex" and b == "objects" for a, b in zip(parts, parts[1:])): + return None + try: + return AnnexKey.parse(parts[-1]) + except ValueError: + return None + + +def parse_url_log(text: str) -> list[str]: + """ + Parse the content of a git-annex URL log (a ``.log.web`` file in the + ``git-annex`` branch), whose lines are of the form ``s + ``, and return the URLs whose most recent status is ``1`` (present). + + Direct URLs are sorted before DANDI Archive API download URLs; otherwise, + the order is that of first appearance in the log. + """ + latest: dict[str, tuple[float, str]] = {} + for line in text.splitlines(): + try: + ts, status, url = line.split(" ", 2) + t = float(ts.rstrip("s")) + except ValueError: + lgr.debug("Ignoring unparsable git-annex URL log line: %r", line) + continue + if url not in latest or t > latest[url][0]: + latest[url] = (t, status) + urls = [url for url, (_, status) in latest.items() if status == "1"] + return sorted(urls, key=lambda u: DANDI_API_DOWNLOAD_URL_RE.search(u) is not None) + + +class AnnexRepo: + """ + Read-only access to the git-annex metadata (the ``git-annex`` branch) of a + Git repository, using only ``git``. Instances are obtained with `find()` + and are cached per repository root. + """ + + _instances: ClassVar[dict[Path, AnnexRepo]] = {} + + def __init__(self, root: Path) -> None: + #: The root (top-level directory) of the repository + self.root = root + self._annex_ref: str | None = None + self._annex_ref_resolved = False + self._urls: dict[str, list[str]] = {} + + def __repr__(self) -> str: + return f"{type(self).__name__}({str(self.root)!r})" + + @classmethod + def find(cls, path: str | Path) -> AnnexRepo | None: + """ + Return the `AnnexRepo` for the repository containing ``path`` (the + closest parent directory containing a ``.git`` directory or file), or + `None` if ``path`` is not inside a Git repository + """ + p = Path(os.path.abspath(path)) + for d in (p, *p.parents): + if os.path.lexists(d / ".git"): + return cls._instances.setdefault(d, cls(d)) + return None + + def _git(self, *args: str) -> str | None: + """ + Run a ``git`` command in the repository and return its standard output, + or `None` if it failed + """ + try: + r = subprocess.run( + ["git", "-C", str(self.root), *args], + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + ) + except FileNotFoundError: + lgr.warning( + "git is not installed; cannot read git-annex metadata of %s", self.root + ) + return None + if r.returncode != 0: + lgr.debug( + "git %s failed in %s: %s", " ".join(args), self.root, r.stderr.strip() + ) + return None + return r.stdout + + @property + def annex_ref(self) -> str | None: + """ + The ref of the ``git-annex`` branch: the local branch if there is one, + otherwise a remote-tracking branch (preferring that of ``origin``); or + `None` if the repository has no git-annex metadata + """ + if not self._annex_ref_resolved: + self._annex_ref_resolved = True + out = self._git( + "for-each-ref", + "--format=%(refname)", + "refs/heads/git-annex", + "refs/remotes/*/git-annex", + ) + if out is not None and (refs := out.split()): + for preferred in ( + "refs/heads/git-annex", + "refs/remotes/origin/git-annex", + ): + if preferred in refs: + self._annex_ref = preferred + break + else: + self._annex_ref = sorted(refs)[0] + return self._annex_ref + + def get_urls(self, key: AnnexKey | str) -> list[str]: + """ + Return the URLs registered in git-annex for the given key (see + `parse_url_log()` for their order), or an empty list if there are none + or the git-annex metadata cannot be read + """ + if isinstance(key, str): + key = AnnexKey.parse(key) + if (urls := self._urls.get(key.key)) is None: + urls = [] + if (ref := self.annex_ref) is not None: + out = self._git( + "cat-file", "-p", f"{ref}:{key.hashdir_lower}/{key.key}.log.web" + ) + if out is not None: + urls = parse_url_log(out) + self._urls[key.key] = urls + return urls + + +@dataclass +class AnnexReadableFile(Readable): + """ + A `Readable` for an annexed file whose content is not present locally and + is instead streamed from the URL(s) registered for it in git-annex. The + fsspec_ library must be installed with the ``http`` extra (e.g., ``pip + install "dandi[extras]"``) in order for `.open()` to be usable. + + Instances are obtained by calling `get_annex_readable()`. + + .. _fsspec: http://github.com/fsspec/filesystem_spec + """ + + #: The path of the (broken) symbolic link to the file's content + filepath: Path + + #: The git-annex key of the file + key: AnnexKey + + #: The URLs from which the content can be retrieved, in order of preference + urls: list[str] + + def open(self) -> IO[bytes]: + """ + Open the content for random-access reading from the first URL that can + be opened, streaming it in blocks on demand + """ + # Optional dependency: + from aiohttp import ClientTimeout + import fsspec + + if not self.urls: + raise RuntimeError(f"{self.filepath}: No URLs registered in git-annex") + # fsspec logs every block it fetches at INFO level, which is too noisy + # for the (INFO-level by default) dandi CLI output; quiet that unless + # the user configured that logger themselves + if (fsspec_lgr := logging.getLogger("fsspec.caching")).level == logging.NOTSET: + fsspec_lgr.setLevel(logging.WARNING) + error: Exception | None = None + for url in self.urls: + lgr.debug("%s: Opening %s for streaming", self.filepath, url) + try: + # We need to call open() on the return value of fsspec.open() + # because otherwise the filehandle will only be opened when + # used to enter a context manager. + return cast( + IO[bytes], + fsspec.open( + url, + mode="rb", + block_size=STREAM_BLOCK_SIZE, + cache_type="blockcache", + cache_options={"maxblocks": STREAM_MAX_BLOCKS}, + client_kwargs={ + # Explicit timeouts prevent indefinite hangs in + # fsspec's sync() wrapper on a stalled connection; + # see the same in `RemoteReadableAsset.open()`. + "timeout": ClientTimeout( + total=120, sock_read=60, sock_connect=30 + ), + # Honor HTTP(S)_PROXY etc. environment variables + "trust_env": True, + }, + ).open(), + ) + except Exception as e: + lgr.warning( + "%s: Could not open %s for streaming: %s: %s", + self.filepath, + url, + type(e).__name__, + e, + ) + error = e + assert error is not None + raise error + + def get_size(self) -> int: + if self.key.size is None: + raise ValueError(f"git-annex key {self.key} does not record a size") + return self.key.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_annex_readable(path: str | Path) -> AnnexReadableFile | None: + """ + Return an `AnnexReadableFile` for streaming the content of the annexed file + at ``path``, or `None` if ``path`` is not a symbolic link into a git-annex + object store or its key does not record the size of the content. The + returned object's ``urls`` list is empty if no URLs are registered for the + file (or the repository's git-annex metadata cannot be read). + """ + filepath = Path(path) + key = get_annex_key(filepath) + if key is None or key.size is None: + return None + repo = AnnexRepo.find(filepath.parent) + urls = repo.get_urls(key) if repo is not None else [] + return AnnexReadableFile(filepath=filepath, key=key, urls=urls) diff --git a/dandi/support/tests/test_annex.py b/dandi/support/tests/test_annex.py new file mode 100644 index 000000000..58b24fa07 --- /dev/null +++ b/dandi/support/tests/test_annex.py @@ -0,0 +1,345 @@ +from __future__ import annotations + +from collections.abc import Iterator +from functools import partial +from http import HTTPStatus +from http.server import SimpleHTTPRequestHandler, ThreadingHTTPServer +import os +from pathlib import Path +import re +import shutil +from subprocess import run +import threading +from typing import Any + +import h5py +import pytest + +from ..annex import ( + AnnexKey, + AnnexReadableFile, + AnnexRepo, + get_annex_key, + get_annex_readable, + parse_url_log, +) +from ...tests.fixtures import ( + annex_key_for_file, + create_git_annex_branch, + make_annexed_dandiset, +) +from ...tests.skip import skipif + +# The key of an actual asset of https://github.com/dandisets/000003 +REAL_KEY = ( + "SHA256E-s61510864725--" + "e063aec023141b5a11fb95a7605fb31057aab05944a81f37c5c6211739ed7f62.nwb" +) + + +@pytest.mark.ai_generated +@pytest.mark.parametrize( + "key,backend,size,name", + [ + ( + REAL_KEY, + "SHA256E", + 61510864725, + "e063aec023141b5a11fb95a7605fb31057aab05944a81f37c5c6211739ed7f62.nwb", + ), + ( + "MD5E-s123--0123456789abcdef0123456789abcdef.nwb", + "MD5E", + 123, + "0123456789abcdef0123456789abcdef.nwb", + ), + ("WORM-s512-m1700000000--foo.txt", "WORM", 512, "foo.txt"), + ("URL--http&c%%example.com%file", "URL", None, "http&c%%example.com%file"), + ], +) +def test_annex_key_parse(key: str, backend: str, size: int | None, name: str) -> None: + k = AnnexKey.parse(key) + assert (k.backend, k.size, k.name) == (backend, size, name) + assert str(k) == key + + +@pytest.mark.ai_generated +@pytest.mark.parametrize( + "key", ["", "SHA256E-s123", "--abc", "SHA256E-s1--", "-s1--abc"] +) +def test_annex_key_parse_invalid(key: str) -> None: + with pytest.raises(ValueError): + AnnexKey.parse(key) + + +@pytest.mark.ai_generated +def test_annex_key_hashdir_lower() -> None: + # This is where the key's log files are located in the git-annex branch of + # https://github.com/dandisets/000003 + assert AnnexKey.parse(REAL_KEY).hashdir_lower == "595/864" + + +@pytest.mark.ai_generated +def test_get_annex_key(tmp_path: Path) -> None: + (tmp_path / "regular.nwb").write_bytes(b"data") + assert get_annex_key(tmp_path / "regular.nwb") is None + assert get_annex_key(tmp_path / "nonexistent.nwb") is None + (tmp_path / "other.nwb").symlink_to("somewhere/else.nwb") + assert get_annex_key(tmp_path / "other.nwb") is None + (tmp_path / "notakey.nwb").symlink_to(".git/annex/objects/Xx/Yy/notakey/notakey") + assert get_annex_key(tmp_path / "notakey.nwb") is None + (tmp_path / "annexed.nwb").symlink_to( + f"../.git/annex/objects/Xx/Yy/{REAL_KEY}/{REAL_KEY}" + ) + key = get_annex_key(tmp_path / "annexed.nwb") + assert key is not None + assert key.key == REAL_KEY + assert key.size == 61510864725 + + +@pytest.mark.ai_generated +def test_parse_url_log() -> None: + s3_url = "https://dandiarchive.s3.amazonaws.com/blobs/9d7/5f6/uuid?versionId=abc" + api_url = "https://api.dandiarchive.org/api/assets/25564f6b/download/" + old_api_url = ( + "https://api.dandiarchive.org/api/dandisets/000003/versions/draft" + "/assets/25564f6b/download/" + ) + log = ( + f"1620050417.649816s 1 {s3_url}\n" + f"1630339841.045183s 1 {api_url}\n" + f"1630339840.983847s 0 {old_api_url}\n" + "1620050418.205986s 0 https://dandiarchive.s3.amazonaws.com/girder/71/54/old\n" + # A URL that was present and then removed, with the lines out of order + "1700000002s 0 https://example.com/removed\n" + "1700000001s 1 https://example.com/removed\n" + # A URL that was removed and then restored + "1700000001s 0 https://example.com/restored\n" + "1700000002s 1 https://example.com/restored\n" + "garbage line\n" + ) + assert parse_url_log(log) == [ + s3_url, + "https://example.com/restored", + # DANDI API download URLs come last + api_url, + ] + assert parse_url_log("") == [] + + +def _git(repo: Path, *args: str) -> None: + 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", + } + run(["git", "-C", str(repo), *args], check=True, env=env) + + +@pytest.mark.ai_generated +def test_annex_repo_get_urls(tmp_path: Path) -> None: + skipif.no_git() + repo = tmp_path / "repo" + repo.mkdir() + _git(repo, "init", "-q") + _git(repo, "commit", "-q", "--allow-empty", "-m", "Initial commit") + key = AnnexKey.parse(REAL_KEY) + + found = AnnexRepo.find(repo / "sub-01" / "sub-01.nwb") + assert found is not None + assert found.root == repo + assert AnnexRepo.find(repo) is found + assert AnnexRepo.find(tmp_path / "norepo" / "file.nwb") is None + + # Without a git-annex branch there are no URLs + annex_repo = AnnexRepo(repo) + assert annex_repo.annex_ref is None + assert annex_repo.get_urls(key) == [] + + create_git_annex_branch( + repo, + { + f"{key.hashdir_lower}/{REAL_KEY}.log.web": ( + "1s 1 https://api.dandiarchive.org/api/assets/x/download/\n" + "2s 1 https://example.com/direct\n" + ) + }, + ) + annex_repo = AnnexRepo(repo) + assert annex_repo.annex_ref == "refs/heads/git-annex" + urls = [ + "https://example.com/direct", + "https://api.dandiarchive.org/api/assets/x/download/", + ] + assert annex_repo.get_urls(key) == urls + assert annex_repo.get_urls(REAL_KEY) == urls + assert annex_repo.get_urls("MD5E-s1--00000000000000000000000000000000.nwb") == [] + + # A plain `git clone` only has the remote-tracking git-annex branch + clone = tmp_path / "clone" + _git(tmp_path, "clone", "-q", str(repo), str(clone)) + cloned = AnnexRepo(clone) + assert cloned.annex_ref == "refs/remotes/origin/git-annex" + assert cloned.get_urls(key) == urls + + +@pytest.mark.ai_generated +def test_get_annex_readable(tmp_path: Path, simple2_nwb: Path) -> None: + skipif.no_git() + ds = tmp_path / "ds" + key = annex_key_for_file(simple2_nwb) + make_annexed_dandiset( + ds, + { + "sub-01/sub-01.nwb": (key, [simple2_nwb.as_uri()]), + "sub-02/sub-02.nwb": ("SHA256E-s42--" + "0" * 64 + ".nwb", []), + "sub-03/sub-03.nwb": ("URL--http&c%%example.com%sub-03.nwb", ["x"]), + }, + ) + r = get_annex_readable(ds / "sub-01" / "sub-01.nwb") + assert r is not None + assert r.key.key == key + assert r.urls == [simple2_nwb.as_uri()] + assert r.get_size() == simple2_nwb.stat().st_size + assert r.get_filename() == "sub-01.nwb" + assert str(r) == str(ds / "sub-01" / "sub-01.nwb") + # No URLs registered + r = get_annex_readable(ds / "sub-02" / "sub-02.nwb") + assert r is not None + assert r.urls == [] + # Key without a size + assert get_annex_readable(ds / "sub-03" / "sub-03.nwb") is None + # Not an annexed file + assert get_annex_readable(ds / "dandiset.yaml") is None + + +@pytest.mark.ai_generated +def test_annex_readable_file_open_file_url(tmp_path: Path) -> None: + pytest.importorskip("fsspec") + content = tmp_path / "content.bin" + content.write_bytes(b"0123456789" * 100) + key = AnnexKey.parse(annex_key_for_file(content)) + assert key.size == 1000 + filepath = tmp_path / "ds" / "content.bin" + missing = (tmp_path / "missing.bin").as_uri() + r = AnnexReadableFile(filepath=filepath, key=key, urls=[missing, content.as_uri()]) + assert r.get_size() == 1000 + assert r.get_mtime() is None + assert r.get_filename() == "content.bin" + # The first URL cannot be opened, so the second one is used + fp = r.open() + try: + assert fp.read(10) == b"0123456789" + fp.seek(995) + assert fp.read() == b"56789" + finally: + fp.close() + with pytest.raises(FileNotFoundError): + AnnexReadableFile(filepath=filepath, key=key, urls=[missing]).open() + with pytest.raises(RuntimeError): + AnnexReadableFile(filepath=filepath, key=key, urls=[]).open() + with pytest.raises(ValueError): + AnnexReadableFile( + filepath=filepath, key=AnnexKey.parse("URL--x"), urls=[] + ).get_size() + + +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`` over HTTP with support for range requests""" + server = RangeHTTPServer( + ("127.0.0.1", 0), partial(RangeHTTPRequestHandler, directory=str(tmp_path)) + ) + 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() + + +@pytest.mark.ai_generated +def test_annex_readable_file_open_http( + range_http_server: tuple[RangeHTTPServer, str], simple2_nwb: Path, tmp_path: Path +) -> None: + pytest.importorskip("fsspec") + pytest.importorskip("aiohttp") + server, base_url = range_http_server + shutil.copy(simple2_nwb, tmp_path / "content.nwb") + key = AnnexKey.parse(annex_key_for_file(tmp_path / "content.nwb")) + r = AnnexReadableFile( + filepath=tmp_path / "ds" / "sub-01" / "sub-01.nwb", + key=key, + # The first URL does not exist, so the second one is used + urls=[f"{base_url}/missing.nwb", f"{base_url}/content.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"] + assert "session_description" in h5 + finally: + fp.close() + assert server.ranged_requests > 0 diff --git a/dandi/tests/fixtures.py b/dandi/tests/fixtures.py index 9b128a23d..50da14b0c 100644 --- a/dandi/tests/fixtures.py +++ b/dandi/tests/fixtures.py @@ -3,6 +3,7 @@ from collections.abc import Callable, Iterator from dataclasses import dataclass, field, replace from datetime import datetime, timezone +import hashlib import logging import os from pathlib import Path @@ -10,7 +11,7 @@ import shutil from subprocess import DEVNULL, check_output, run from time import sleep -from typing import Any, Literal +from typing import Any, Literal, cast from uuid import uuid4 from click.testing import CliRunner @@ -39,6 +40,7 @@ ) from ..dandiapi import DandiAPIClient, RemoteDandiset from ..pynwb_utils import make_nwb_file +from ..support.annex import AnnexKey from ..upload import upload lgr = get_logger() @@ -352,6 +354,88 @@ def fixture( return fixture +def annex_key_for_file(path: Path) -> str: + """ + Return the git-annex key (using the ``SHA256E`` backend, as DANDI Dandisets + do) that git-annex would assign to the file at ``path`` + """ + digest = hashlib.sha256(path.read_bytes()).hexdigest() + return f"SHA256E-s{path.stat().st_size}--{digest}{path.suffix}" + + +def create_git_annex_branch( + repo: Path, files: dict[str, str], ref: str = "refs/heads/git-annex" +) -> None: + """ + Create (or replace) the ``git-annex`` branch of the Git repository at + ``repo`` so that its tree consists of the given files (a mapping from paths + to contents), using only ``git`` plumbing commands, i.e., without needing + git-annex. This is for testing code that reads git-annex metadata. + """ + index = repo / ".git" / "dandi-test-index" + env = { + **os.environ, + "GIT_INDEX_FILE": str(index), + "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, **kwargs: Any) -> str: + out = check_output( + ["git", "-C", str(repo), *args], env=env, text=True, **kwargs + ) + return cast(str, out).strip() + + for path, content in files.items(): + blob = git("hash-object", "-w", "--stdin", input=content) + git("update-index", "--add", "--cacheinfo", f"100644,{blob},{path}") + tree = git("write-tree") + commit = git("commit-tree", tree, "-m", "git-annex metadata") + git("update-ref", ref, commit) + index.unlink(missing_ok=True) + + +def make_annexed_dandiset( + dandiset: Path, files: dict[str, tuple[str, list[str]]] +) -> None: + """ + Make the directory ``dandiset`` (created if needed, with a minimal + :file:`dandiset.yaml` if it does not have one) look like a DataLad Dandiset + whose annexed content has not been fetched: for each ``relpath: (key, + urls)`` item of ``files``, ``relpath`` is created as a broken symbolic link + into the git-annex object store for ``key``, and the ``urls`` are + registered for ``key`` in the ``git-annex`` branch of the repository. + """ + dandiset.mkdir(parents=True, exist_ok=True) + metadata_file = dandiset / dandiset_metadata_file + if not metadata_file.exists(): + metadata_file.write_text( + "identifier: '000027'\nname: Test\ndescription: Test dandiset\n" + ) + if not (dandiset / ".git").exists(): + run(["git", "init", "-q", str(dandiset)], check=True) + logs: dict[str, str] = {} + for relpath, (key, urls) in files.items(): + link = dandiset / relpath + link.parent.mkdir(parents=True, exist_ok=True) + link.symlink_to( + Path(os.path.relpath(dandiset, link.parent)) + / ".git" + / "annex" + / "objects" + / "Xx" + / "Yy" + / key + / key + ) + logs[f"{AnnexKey.parse(key).hashdir_lower}/{key}.log.web"] = "".join( + f"{1700000000 + i}s 1 {url}\n" for i, url in enumerate(urls) + ) + create_git_annex_branch(dandiset, logs) + + def _make_subdirs_dandisets(path: Path) -> None: for bids_dataset_path in path.iterdir(): if bids_dataset_path.is_dir(): diff --git a/dandi/validate/_core.py b/dandi/validate/_core.py index 371ce5a30..6fe3e6699 100644 --- a/dandi/validate/_core.py +++ b/dandi/validate/_core.py @@ -10,9 +10,9 @@ 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 from ._types import ( ORIGIN_VALIDATION_DANDI_LAYOUT, @@ -26,7 +26,8 @@ Validator, ) from ..consts import dandiset_metadata_file -from ..files import find_dandi_files +from ..files import DandiFile, LocalFileAsset, find_dandi_files +from ..support.annex import AnnexReadableFile, get_annex_readable from ..utils import find_parent_directory_containing BIDS_TO_DANDI = { @@ -155,7 +156,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 +183,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 from the URLs registered + for them in git-annex (see `dandi.support.annex`) 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 +223,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 +270,75 @@ def validate( yield r +def _check_streaming_requirements() -> None: + """Raise an informative error if the optional dependencies needed for + streaming file content are not installed""" + for module in ("fsspec", "aiohttp"): + if find_spec(module) is None: + raise RuntimeError( + f"Streaming file content requires the {module} package; " + "install it with `pip install 'dandi[extras]'`" + ) + + +def _prepare_streaming(df: DandiFile) -> AnnexReadableFile | None: + """ + Set up streaming of the content of the annexed file represented by ``df`` + from the URLs registered for it in git-annex, and return the `Readable` + that will be used for it, or `None` if the content cannot be streamed + (``df`` is not a file asset, not an annexed file, or has no URLs registered) + """ + if not isinstance(df, LocalFileAsset): + return None + readable = get_annex_readable(df.filepath) + if readable is None or not readable.urls: + return None + df.content_source = readable + return readable + + def _handle_missing_content( - df: Any, + df: DandiFile, policy: MissingFileContent, -) -> ValidationResult | None: + readable: AnnexReadableFile | 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.urls[0]}" + ), + ) + 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 with a URL registered in " + f"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..8fbb74fd3 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 from the URLs registered for them in + git-annex (see `dandi.support.annex`) 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..07a8ba614 100644 --- a/dandi/validate/tests/test_core.py +++ b/dandi/validate/tests/test_core.py @@ -1,5 +1,6 @@ import json from pathlib import Path +import shutil from typing import Any import pytest @@ -17,7 +18,12 @@ ) from ... import __version__ from ...consts import dandiset_metadata_file -from ...tests.fixtures import BIDS_TESTDATA_SELECTION +from ...tests.fixtures import ( + BIDS_TESTDATA_SELECTION, + annex_key_for_file, + make_annexed_dandiset, +) +from ...tests.skip import skipif def test_validate_nwb_error(simple3_nwb: Path) -> None: @@ -310,3 +316,128 @@ 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) ---- + + +def _make_streamable_dandiset( + tmp_path: Path, nwb: Path, url: str | None = None +) -> Path: + """Create a dandiset looking like a DataLad clone without fetched content. + + ``sub-001/sub-001.nwb`` is an annexed copy of *nwb* whose content is + registered at *url* (a ``file://`` URL for *nwb* by default), and + ``sub-001/sub-001_video.mp4`` is an annexed video with a registered URL + that is never read (only the size recorded in its key is needed). + """ + ds = tmp_path / "ds" + make_annexed_dandiset( + ds, + { + "sub-001/sub-001.nwb": ( + annex_key_for_file(nwb), + [url if url is not None else nwb.as_uri()], + ), + "sub-001/sub-001_video.mp4": ( + "MD5E-s4242--0123456789abcdef0123456789abcdef.mp4", + ["https://example.com/video.mp4"], + ), + }, + ) + return ds + + +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(tmp_path: Path, simple3_nwb: Path) -> None: + """stream policy validates the content of annexed files via their URLs.""" + skipif.no_git() + pytest.importorskip("fsspec") + ds = _make_streamable_dandiset(tmp_path, simple3_nwb) + 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 any( + r.message is not None and simple3_nwb.as_uri() in r.message for r in streamed + ) + assert not [r for r in results if r.id.startswith("DANDI.FILE_CONTENT_MISSING")] + + # 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 +def test_validate_stream_not_streamable(tmp_path: Path) -> None: + """stream policy emits an error for a broken symlink with no URL to stream from.""" + pytest.importorskip("fsspec") + 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(tmp_path: Path, simple3_nwb: Path) -> None: + """A registered URL that cannot be read yields errors, not an exception.""" + skipif.no_git() + pytest.importorskip("fsspec") + ds = _make_streamable_dandiset( + tmp_path, simple3_nwb, url=(tmp_path / "gone.nwb").as_uri() + ) + 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_fsspec( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setattr("dandi.validate._core.find_spec", lambda name: None) + with pytest.raises(RuntimeError, match=r"dandi\[extras\]"): + list(validate(tmp_path, missing_file_content=MissingFileContent.stream)) diff --git a/docs/source/cmdline/validate.rst b/docs/source/cmdline/validate.rst index 111ccd441..4ffff5cbb 100644 --- a/docs/source/cmdline/validate.rst +++ b/docs/source/cmdline/validate.rst @@ -22,10 +22,108 @@ Options Ignore any validation errors & warnings whose ID matches the given regular expression -.. option:: --min-severity [HINT|WARNING|ERROR] +.. option:: --min-severity [INFO|HINT|WARNING|ERROR|CRITICAL] Only display issues with severities above this level (HINT by default) +.. 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 + content has not been fetched: + + ``error`` + Emit a concise ``DANDI.FILE_CONTENT_MISSING`` error for each such file + (default) + + ``skip`` + Skip each such file, emitting a warning + + ``only-non-data`` + Skip content-dependent validators (pynwb, nwbinspector, ...) for each + such file but still validate its path layout + + ``stream`` + Stream the content of each such file from the URLs registered for it in + git-annex so that content-dependent validators run without the file + having to be downloaded; see `Validating DataLad Dandisets`_ below. + +.. option:: -f, --format [text|json|json_pp|json_lines|yaml] + + Output format (``text`` by default) + +.. option:: -o, --output + + Write the output to the given file instead of standard output. The format + is inferred from the file's extension unless :option:`--format` is given. + +.. _DataLad: https://www.datalad.org +.. _git-annex: https://git-annex.branchable.com + + +Validating DataLad Dandisets +---------------------------- + +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 from the URLs registered in +git-annex (preferring the direct S3 URLs), reading only the parts of each file +that the validators need. Only ``git`` is needed to read that information +(git-annex and DataLad do not need to be installed), plus fsspec_ for the +streaming itself (``pip install "dandi[extras]"``). + +For example, to produce a JSON Lines record of *all* validation results for +a Dandiset:: + + git 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 registered for it) yields a +``DANDI.FILE_CONTENT_MISSING`` error instead. To do this for every Dandiset, +install the subdatasets of the superdataset without fetching any content and +validate each one (the superdataset itself is not a Dandiset):: + + datalad clone https://github.com/dandisets/dandisets + cd dandisets + datalad get -n -r . # or: git submodule update --init + for ds in 0*/; do + dandi validate --missing-file-content=stream --min-severity=INFO \ + --format=json_lines --output="records/${ds%/}.jsonl" "$ds" + done + +Notes: + +- The URLs are read from the ``git-annex`` branch of the repository (or its + remote-tracking counterpart, e.g., ``origin/git-annex``), so the clone must + include that branch: do not clone with ``--single-branch`` or a + ``--depth`` that excludes it. +- 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. +- Zarr assets are stored as separate subdatasets (https://github.com/dandizarrs) + and are not streamed: an uninstalled Zarr subdataset is an empty directory + that is not validated at all. +- 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. + +Alternatively, `datalad-fuse`_ can mount a DataLad dataset as a file system +that fetches content transparently on read, in which case plain ``dandi +validate`` (without :option:`--missing-file-content`) can be run on the mount +point; that approach requires FUSE and thus does not work in every +environment (e.g., many containers). + +.. _fsspec: https://github.com/fsspec/filesystem_spec +.. _datalad-fuse: https://github.com/datalad/datalad-fuse + Development Options ------------------- From 10658f81fd18d8e44c342af9cc4de88723152e14 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 16:54:19 +0000 Subject: [PATCH 02/16] Support fsspec versions that register the block cache as "block" fsspec renamed its LRU block cache type from "block" to "blockcache" in 2023, and the declared minimum (2022.11.0) only knows the old name, so `AnnexReadableFile.open()` failed with `KeyError: 'blockcache'` in the lowest-deps CI job. Pick whichever name the installed fsspec registers. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01HFhMPs6vF5zyeHDvPTvRcA --- dandi/support/annex.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/dandi/support/annex.py b/dandi/support/annex.py index 79954a242..6848707bd 100644 --- a/dandi/support/annex.py +++ b/dandi/support/annex.py @@ -273,9 +273,13 @@ def open(self) -> IO[bytes]: # Optional dependency: from aiohttp import ClientTimeout import fsspec + from fsspec.caching import caches as fsspec_caches if not self.urls: raise RuntimeError(f"{self.filepath}: No URLs registered in git-annex") + # fsspec's LRU block cache (which suits h5py's random access) was + # registered as "block" before it was renamed to "blockcache" in 2023 + cache_type = "blockcache" if "blockcache" in fsspec_caches else "block" # fsspec logs every block it fetches at INFO level, which is too noisy # for the (INFO-level by default) dandi CLI output; quiet that unless # the user configured that logger themselves @@ -294,7 +298,7 @@ def open(self) -> IO[bytes]: url, mode="rb", block_size=STREAM_BLOCK_SIZE, - cache_type="blockcache", + cache_type=cache_type, cache_options={"maxblocks": STREAM_MAX_BLOCKS}, client_kwargs={ # Explicit timeouts prevent indefinite hangs in From 0344543c4f9eff9eb5e66b2612433f4e0f0fccce Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 16:56:58 +0000 Subject: [PATCH 03/16] Docs: trim the DataLad validation section per review - Drop the recipe for looping over the whole `dandisets` superdataset. - Note that streaming Zarr subdatasets is a follow-up for when NWB Zarr support has matured across the ecosystem. - Explain that suppressing content-dependent BIDS checks loses nothing for NWB datasets: the sidecar files are in git and the rest is in the names. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01HFhMPs6vF5zyeHDvPTvRcA --- docs/source/cmdline/validate.rst | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/docs/source/cmdline/validate.rst b/docs/source/cmdline/validate.rst index 4ffff5cbb..6c915c33b 100644 --- a/docs/source/cmdline/validate.rst +++ b/docs/source/cmdline/validate.rst @@ -87,17 +87,7 @@ a Dandiset:: 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 registered for it) yields a -``DANDI.FILE_CONTENT_MISSING`` error instead. To do this for every Dandiset, -install the subdatasets of the superdataset without fetching any content and -validate each one (the superdataset itself is not a Dandiset):: - - datalad clone https://github.com/dandisets/dandisets - cd dandisets - datalad get -n -r . # or: git submodule update --init - for ds in 0*/; do - dandi validate --missing-file-content=stream --min-severity=INFO \ - --format=json_lines --output="records/${ds%/}.jsonl" "$ds" - done +``DANDI.FILE_CONTENT_MISSING`` error instead. Notes: @@ -109,11 +99,15 @@ Notes: of data streamed for a file depends on its content; it is nevertheless usually a small fraction of the file. - Zarr assets are stored as separate subdatasets (https://github.com/dandizarrs) - and are not streamed: an uninstalled Zarr subdataset is an empty directory - that is not validated at all. + 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. + 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. Alternatively, `datalad-fuse`_ can mount a DataLad dataset as a file system that fetches content transparently on read, in which case plain ``dandi From fb34954aa394ebc4a38482e6858914cc60c57ad2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 17:15:47 +0000 Subject: [PATCH 04/16] Docs: drop the datalad-fuse paragraph per review Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01HFhMPs6vF5zyeHDvPTvRcA --- docs/source/cmdline/validate.rst | 7 ------- 1 file changed, 7 deletions(-) diff --git a/docs/source/cmdline/validate.rst b/docs/source/cmdline/validate.rst index 6c915c33b..1df8cda8b 100644 --- a/docs/source/cmdline/validate.rst +++ b/docs/source/cmdline/validate.rst @@ -109,14 +109,7 @@ Notes: present (the non-annexed sidecar files, which are kept in git) or encoded in the file and folder names of the annexed files themselves. -Alternatively, `datalad-fuse`_ can mount a DataLad dataset as a file system -that fetches content transparently on read, in which case plain ``dandi -validate`` (without :option:`--missing-file-content`) can be run on the mount -point; that approach requires FUSE and thus does not work in every -environment (e.g., many containers). - .. _fsspec: https://github.com/fsspec/filesystem_spec -.. _datalad-fuse: https://github.com/datalad/datalad-fuse Development Options From d22177c3de131e8fa544de391dd2ad42d16be177 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 17:38:02 +0000 Subject: [PATCH 05/16] Accept broken symlinks as path arguments of `dandi validate` `dandi validate` already handles annexed files whose content has not been fetched (the broken symlinks of a DataLad dataset) according to `--missing-file-content`: `error`, `skip`, or `only-non-data`. Those policies were applied only when such a file was reached through its directory, though: given directly on the command line, the file never got past argument parsing, because `click.Path(exists=True)` follows the link and rejected it with "does not exist". Check the path with `lexists()` instead, so that dandi validate --missing-file-content=skip sub-01/sub-01.nwb works the same as validating `sub-01/`, while paths that truly do not exist are still rejected with the same error. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01HFhMPs6vF5zyeHDvPTvRcA --- dandi/cli/cmd_validate.py | 26 ++++++++++++++++++++++++-- dandi/cli/tests/test_cmd_validate.py | 22 ++++++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/dandi/cli/cmd_validate.py b/dandi/cli/cmd_validate.py index 375dbc578..78a86bb6a 100644 --- a/dandi/cli/cmd_validate.py +++ b/dandi/cli/cmd_validate.py @@ -7,7 +7,7 @@ import os import re import sys -from typing import IO, Union, cast +from typing import IO, Any, Union, cast import warnings import click @@ -35,6 +35,28 @@ class TruncationNotice: """Number of validation results omitted from this group.""" +class ExistingPath(click.Path): + """ + Like ``click.Path(exists=True)``, but also accepting broken symbolic links, + such as the annexed files of a DataLad dataset whose content has not been + fetched. ``click.Path(exists=True)`` follows the link and rejects those as + nonexistent, which prevented the ``--missing-file-content`` policies + (``error``, ``skip``, ``only-non-data``) from ever being applied to a file + given directly on the command line rather than via its directory. + """ + + def convert( + self, value: Any, param: click.Parameter | None, ctx: click.Context | None + ) -> Any: + if not os.path.lexists(value): + self.fail( + f"{self.name.title()} {os.fsdecode(value)!r} does not exist.", + param, + ctx, + ) + return super().convert(value, param, ctx) + + STRUCTURED_FORMATS = ("json", "json_pp", "json_lines", "yaml") _EXT_TO_FORMAT = { @@ -228,7 +250,7 @@ def validate_bids( multiple=True, default=(), ) -@click.argument("paths", nargs=-1, type=click.Path(exists=True, dir_okay=True)) +@click.argument("paths", nargs=-1, type=ExistingPath(dir_okay=True)) @click.pass_context @devel_debug_option() @map_to_click_exceptions diff --git a/dandi/cli/tests/test_cmd_validate.py b/dandi/cli/tests/test_cmd_validate.py index 6d0b503a0..1da5c16ca 100644 --- a/dandi/cli/tests/test_cmd_validate.py +++ b/dandi/cli/tests/test_cmd_validate.py @@ -996,3 +996,25 @@ def test_validate_missing_file_content_no_broken_symlinks(tmp_path: Path) -> Non assert r_default.exit_code == r_skip.exit_code == 0 assert "FILE_CONTENT_MISSING" not in r_default.output assert "FILE_CONTENT_MISSING" not in r_skip.output + + +@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. + + The --missing-file-content policies then apply to it just as they do when + the containing directory is given. + """ + ds = _make_dandiset_with_broken_symlinks(tmp_path) + link = ds / "sub-001" / "sub-001.nwb" + r = CliRunner().invoke(validate, [str(link)]) + assert r.exit_code == 1 + assert "does not exist" not in r.output + assert "FILE_CONTENT_MISSING" in r.output + r = CliRunner().invoke(validate, ["--missing-file-content", "skip", str(link)]) + assert r.exit_code == 0 + assert "FILE_CONTENT_MISSING_SKIPPED" in r.output + # Paths that truly do not exist are still rejected + r = CliRunner().invoke(validate, [str(ds / "sub-001" / "nonexistent.nwb")]) + assert r.exit_code == 2 + assert "does not exist" in r.output From f89f30647e804bad1e327b968dcecfafd4771217 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 17:57:06 +0000 Subject: [PATCH 06/16] Docs: deduplicate the validate page after merging the docs branch Git merged the synced option reference and this branch's additions without conflict but with the `--missing-file-content`, `--format`, and `--output` entries and the link targets appearing twice, which Sphinx rejects. Keep the synced entries, adding the `stream` policy to `--missing-file-content`, and the "Validating DataLad Dandisets" section. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01HFhMPs6vF5zyeHDvPTvRcA --- docs/source/cmdline/validate.rst | 39 ++++---------------------------- 1 file changed, 5 insertions(+), 34 deletions(-) diff --git a/docs/source/cmdline/validate.rst b/docs/source/cmdline/validate.rst index 57aa4799c..7fddb2a06 100644 --- a/docs/source/cmdline/validate.rst +++ b/docs/source/cmdline/validate.rst @@ -56,33 +56,6 @@ 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] - - How to handle files whose content is unavailable, such as the broken - symbolic links of a DataLad_ dataset (a git-annex_ repository) whose - content has not been fetched: - - ``error`` - Emit a concise ``DANDI.FILE_CONTENT_MISSING`` error for each such file - (default) - - ``skip`` - Skip each such file, emitting a warning - - ``only-non-data`` - Skip content-dependent validators (pynwb, nwbinspector, ...) for each - such file but still validate its path layout - -.. option:: --load - - Instead of running validation, load previously saved results from the - given JSON Lines file (e.g., an automatically saved companion file) and - render them. Can be specified multiple times; cannot be combined with - paths. - -.. _DataLad: https://www.datalad.org -.. _git-annex: https://git-annex.branchable.com - .. option:: --missing-file-content [error|only-non-data|skip|stream] How to handle files whose content is unavailable, such as the broken @@ -105,14 +78,12 @@ Options git-annex so that content-dependent validators run without the file having to be downloaded; see `Validating DataLad Dandisets`_ below. -.. option:: -f, --format [text|json|json_pp|json_lines|yaml] - - Output format (``text`` by default) - -.. option:: -o, --output +.. option:: --load - Write the output to the given file instead of standard output. The format - is inferred from the file's extension unless :option:`--format` is given. + Instead of running validation, load previously saved results from the + given JSON Lines file (e.g., an automatically saved companion file) and + render them. Can be specified multiple times; cannot be combined with + paths. .. _DataLad: https://www.datalad.org .. _git-annex: https://git-annex.branchable.com From c5dc0854a58a74c55d8098eda41e1ccea8f2ca7d Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 18:01:41 +0000 Subject: [PATCH 07/16] Docs: rename the section to "Validating DataLad Dandisets Remotely" Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01HFhMPs6vF5zyeHDvPTvRcA --- docs/source/cmdline/validate.rst | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/source/cmdline/validate.rst b/docs/source/cmdline/validate.rst index 7fddb2a06..a4f82be1e 100644 --- a/docs/source/cmdline/validate.rst +++ b/docs/source/cmdline/validate.rst @@ -76,7 +76,8 @@ Options ``stream`` Stream the content of each such file from the URLs registered for it in git-annex so that content-dependent validators run without the file - having to be downloaded; see `Validating DataLad Dandisets`_ below. + having to be downloaded; see `Validating DataLad Dandisets Remotely`_ + below. .. option:: --load @@ -89,8 +90,8 @@ Options .. _git-annex: https://git-annex.branchable.com -Validating DataLad Dandisets ----------------------------- +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 From 552274af8778efb04c792d424d94531eb76666c4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 19:10:08 +0000 Subject: [PATCH 08/16] Drop pynwb < 3.1 compatibility code from pynwb_utils The declared floor is pynwb >= 3.1.0 (and nwbinspector >= 0.7.0 requires pynwb >= 3.1 anyway), which makes several version guards unreachable: - validate(): only the `pynwb.validate(path=...)` branch (pynwb >= 3.0) can run; the `paths=[...]` tuple-returning form (2.2 to 2.x) and the `io=` fallback for even older releases are gone. - copy_nwb_file(): `cache_spec` has been accepted by `export()` since pynwb 2.8.2, so pass it unconditionally. - _get_external_images(): `ExternalImage` exists in every supported pynwb, so import it at module level instead of guarding an ImportError. The now-unused `packaging.version.Version` import is removed as well. The NWB *schema* version checks (e.g. the < 2.1.0 error filter) are about the file being validated, not about pynwb, and are unchanged. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01HFhMPs6vF5zyeHDvPTvRcA --- dandi/pynwb_utils.py | 30 +++++++----------------------- 1 file changed, 7 insertions(+), 23 deletions(-) diff --git a/dandi/pynwb_utils.py b/dandi/pynwb_utils.py index 80fba2a20..297df05f3 100644 --- a/dandi/pynwb_utils.py +++ b/dandi/pynwb_utils.py @@ -27,9 +27,9 @@ import h5py import hdmf import numpy as np -from packaging.version import Version import pynwb from pynwb import NWBHDF5IO +from pynwb.base import ExternalImage import semantic_version from . import __version__, get_logger @@ -450,12 +450,6 @@ def _get_external_images(nwb: pynwb.NWBFile) -> list[dict]: list of dicts : [{id: , name: , external_files=[ExternalImage.data], field: "data"}] """ - try: - from pynwb.base import ExternalImage - except ImportError: - # `ExternalImage` was added in pynwb 3.1.0; nothing to collect on an older one. - return [] - out = [] for ob in nwb.objects.values(): if not isinstance(ob, ExternalImage) or ob.data is None: @@ -685,16 +679,9 @@ def validate(path: str | Path, devel_debug: bool = False) -> list[ValidationResu ) try: - if Version(pynwb.__version__) >= Version("3.0.0"): - error_outputs = pynwb.validate(path=path) - elif Version(pynwb.__version__) >= Version( - "2.2.0" - ): # Use cached namespace feature - # argument get_cached_namespaces is True by default - error_outputs, _ = pynwb.validate(paths=[path]) - else: # Fallback if an older version - with pynwb.NWBHDF5IO(path=path, mode="r", load_namespaces=True) as reader: - error_outputs = pynwb.validate(io=reader) + # 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) except Exception as exc: if devel_debug: raise @@ -808,18 +795,15 @@ def copy_nwb_file(src: str | Path, dest: str | Path) -> str: dest = op.join(dest, op.basename(src)) else: os.makedirs(op.dirname(dest), exist_ok=True) - kws = {} - if Version(pynwb.__version__) >= Version("2.8.2"): - # we might make it leaner by not caching the spec if original - # file did not have it. Possible only since 2.8.2.dev11 - kws["cache_spec"] = bool(pynwb.NWBHDF5IO.get_namespaces(src)) with pynwb.NWBHDF5IO(src, "r") as ior, pynwb.NWBHDF5IO(dest, "w") as iow: data = ior.read() data.generate_new_id() iow.export( ior, nwbfile=data, - **kws, + # keep the copy leaner by not caching the spec if the original + # file did not have it cached either + cache_spec=bool(pynwb.NWBHDF5IO.get_namespaces(src)), ) return str(dest) From 4c46020503e00390c27c7ab235ed9382e3422bae Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 19:51:07 +0000 Subject: [PATCH 09/16] Cache results for Readables that know their content fingerprint fscacher's memoize_path derives its cache key from a stat of the file at the given path, so it never caches a call made on a `Readable` (there is nothing to stat). Some readables can nevertheless vouch for their content better than mtime/inode ever could, e.g. with a content digest such as a git-annex key: results computed from one such resource are valid for any other with the same fingerprint. - `Readable.get_fingerprint()`: new optional hook returning a content fingerprint string, `None` (the default) meaning "unknown, do not cache". - `pynwb_utils.memoize_source(cache, tokens)`: a decorator that behaves exactly like `cache.memoize_path` for path arguments and, for a `Readable` with a fingerprint, caches under (file name, fingerprint, tokens, other arguments) via `cache.memoize` instead. The tokens are passed explicitly since plain `memoize` does not add them by itself. - Applied to `get_metadata`, `get_neurodata_types`, and `nwb_has_external_links`, the memoized functions that already accept a `Readable`. No behavior change for paths or for readables without a fingerprint (still none of them at this point). Since the decorator preserves the signature, mypy now sees through `get_metadata`, whose return annotation claimed `dict | None` although it never returns `None`; fixed to `dict[str, Any]`. isort also reordered a pair of local imports in `RemoteReadableAsset.open()` while at it. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01HFhMPs6vF5zyeHDvPTvRcA --- dandi/metadata/nwb.py | 6 +- dandi/misctypes.py | 18 +++++- dandi/pynwb_utils.py | 77 +++++++++++++++++++++--- dandi/tests/fixtures.py | 24 +++++++- dandi/tests/test_metadata.py | 22 ++++++- dandi/tests/test_pynwb_utils.py | 103 ++++++++++++++++++++++++++++++-- 6 files changed, 232 insertions(+), 18 deletions(-) diff --git a/dandi/metadata/nwb.py b/dandi/metadata/nwb.py index 38c2ac4e7..59d2e1ddb 100644 --- a/dandi/metadata/nwb.py +++ b/dandi/metadata/nwb.py @@ -19,7 +19,9 @@ get_neurodata_types, get_nwb_version, ignore_benign_pynwb_warnings, + memoize_source, metadata_cache, + metadata_cache_tokens, nwb_has_external_links, ) from ..utils import find_parent_directory_containing @@ -28,10 +30,10 @@ # Disable this for clean hacking -@metadata_cache.memoize_path +@memoize_source(metadata_cache, metadata_cache_tokens) def get_metadata( path: str | Path | Readable, digest: Digest | None = None -) -> dict | None: +) -> dict[str, Any]: """ Get "flatdata" from a .nwb file diff --git a/dandi/misctypes.py b/dandi/misctypes.py index b50cffc42..0d970b944 100644 --- a/dandi/misctypes.py +++ b/dandi/misctypes.py @@ -284,6 +284,21 @@ def get_filename(self) -> str: """ ... + def get_fingerprint(self) -> str | None: + """ + .. versionadded:: 0.81.0 + + Returns a fingerprint of the resource's *content*, such as a content + digest (e.g., a git-annex key), or `None` if none is known + + Two resources with equal fingerprints must have identical bytes: the + fingerprint is what lets results derived from the content (metadata, + validation results) be cached and reused across instances, see + `dandi.pynwb_utils.memoize_source`. With the default of `None`, such + results are never cached for the resource. + """ + return None + class LocalReadableFile(Readable): """ @@ -343,9 +358,8 @@ class RemoteReadableAsset(Readable): def open(self) -> IO[bytes]: # Optional dependency: - import fsspec - from aiohttp import ClientTimeout + import fsspec # We need to call open() on the return value of fsspec.open() because # otherwise the filehandle will only be opened when used to enter a diff --git a/dandi/pynwb_utils.py b/dandi/pynwb_utils.py index 297df05f3..d197d8218 100644 --- a/dandi/pynwb_utils.py +++ b/dandi/pynwb_utils.py @@ -12,8 +12,9 @@ from __future__ import annotations from collections import Counter -from collections.abc import Callable +from collections.abc import Callable, Sequence from datetime import timedelta +from functools import wraps import inspect import os import os.path as op @@ -63,15 +64,77 @@ get_module_version(hdmf), get_module_version(h5py), ] +metadata_cache_tokens = dandi_cache_tokens metadata_cache = PersistentCache( - name="dandi-metadata", tokens=dandi_cache_tokens, envvar="DANDI_CACHE" + name="dandi-metadata", tokens=metadata_cache_tokens, envvar="DANDI_CACHE" ) +validate_cache_tokens = dandi_cache_tokens + [get_module_version(dandischema)] validate_cache = PersistentCache( - name="dandi-validate", - tokens=dandi_cache_tokens + [get_module_version(dandischema)], - envvar="DANDI_CACHE", + name="dandi-validate", tokens=validate_cache_tokens, envvar="DANDI_CACHE" ) +F = TypeVar("F", bound=Callable[..., Any]) + + +def memoize_source(cache: PersistentCache, tokens: Sequence[Any]) -> Callable[[F], F]: + """ + Decorator caching the results of a function of a local path or a `Readable` + + For a path argument (`str` or `Path`), this is exactly ``cache.memoize_path``: + the result is cached under the file's location, a fingerprint of the file + (mtime, ctime, size, ...), and ``tokens``. A `Readable` cannot be + fingerprinted that way, so `PersistentCache.memoize_path` leaves calls on + one uncached; here, a `Readable` whose `~Readable.get_fingerprint` returns + a value is cached under its file name, that fingerprint, and ``tokens`` + instead. A `Readable` without a fingerprint is still never cached. + + ``tokens`` should be the ones ``cache`` was created with (versions of the + software producing the results): `PersistentCache.memoize`, unlike + `~PersistentCache.memoize_path`, does not add them by itself. + """ + + def decorator(f: F) -> F: + by_path = cache.memoize_path(f) + + def by_fingerprint( + filename: str, + fingerprint: str, + cache_tokens: tuple[Any, ...], + *args: Any, + _source: Readable, + **kwargs: Any, + ) -> Any: + return f(_source, *args, **kwargs) + + # Give it an identity of its own under `f`'s module, so that joblib + # keeps its results in a per-function directory next to `by_path`'s + by_fingerprint.__module__ = f.__module__ + by_fingerprint.__name__ = f"{f.__name__}__by_fingerprint" + by_fingerprint.__qualname__ = f"{f.__qualname__}__by_fingerprint" + cached_by_fingerprint = cache.memoize( + by_fingerprint, exclude_kwargs=["_source"] + ) + + @wraps(f) + def wrapper(source: Any, *args: Any, **kwargs: Any) -> Any: + if not isinstance(source, Readable): + return by_path(source, *args, **kwargs) + fingerprint = source.get_fingerprint() + if fingerprint is None: + return f(source, *args, **kwargs) + return cached_by_fingerprint( + source.get_filename(), + fingerprint, + tuple(tokens), + *args, + _source=source, + **kwargs, + ) + + return cast(F, wrapper) + + return decorator + def _sanitize_nwb_version( v: Any, @@ -194,7 +257,7 @@ def get_neurodata_types_to_modalities_map() -> dict[str, str]: return ndtypes -@metadata_cache.memoize_path +@memoize_source(metadata_cache, metadata_cache_tokens) 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 +871,7 @@ def copy_nwb_file(src: str | Path, dest: str | Path) -> str: return str(dest) -@metadata_cache.memoize_path +@memoize_source(metadata_cache, metadata_cache_tokens) 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/tests/fixtures.py b/dandi/tests/fixtures.py index 9b128a23d..efe80b25a 100644 --- a/dandi/tests/fixtures.py +++ b/dandi/tests/fixtures.py @@ -10,7 +10,7 @@ import shutil from subprocess import DEVNULL, check_output, run from time import sleep -from typing import Any, Literal +from typing import IO, Any, Literal from uuid import uuid4 from click.testing import CliRunner @@ -38,6 +38,7 @@ metadata_nwb_file_fields, ) from ..dandiapi import DandiAPIClient, RemoteDandiset +from ..misctypes import LocalReadableFile from ..pynwb_utils import make_nwb_file from ..upload import upload @@ -94,6 +95,27 @@ def simple1_nwb_metadata() -> dict[str, Any]: return metadata +class FingerprintedReadable(LocalReadableFile): + """ + A local file posing as a `Readable` with a content fingerprint of its own + + Opening it is counted, to tell results served from a cache from those + computed from the content. + """ + + def __init__(self, filepath: str | Path, fingerprint: str | None) -> None: + super().__init__(filepath) + self.fingerprint = fingerprint + self.opened = 0 + + def open(self) -> IO[bytes]: + self.opened += 1 + return super().open() + + def get_fingerprint(self) -> str | None: + return self.fingerprint + + @pytest.fixture(scope="session") def simple1_nwb( simple1_nwb_metadata: dict[str, Any], tmp_path_factory: pytest.TempPathFactory diff --git a/dandi/tests/test_metadata.py b/dandi/tests/test_metadata.py index 26f29acbb..b28d71997 100644 --- a/dandi/tests/test_metadata.py +++ b/dandi/tests/test_metadata.py @@ -4,9 +4,11 @@ from datetime import datetime, timedelta from itertools import chain import json +import os from pathlib import Path import shutil from typing import Any +from uuid import uuid4 from anys import ANY_AWARE_DATETIME, ANY_INT, AnyFullmatch, AnyIn from dandischema.consts import DANDI_SCHEMA_VERSION @@ -38,7 +40,7 @@ import requests from semantic_version import Version -from .fixtures import SampleDandiset +from .fixtures import FingerprintedReadable, SampleDandiset from .skip import mark from .. import __version__ from ..consts import metadata_nwb_subject_fields @@ -1413,3 +1415,21 @@ def test_nwb2asset_remote_asset(nwb_dandiset: SampleDandiset) -> None: approach=[], relatedResource=[], ) + + +@pytest.mark.ai_generated +@pytest.mark.skipif( + os.environ.get("DANDI_CACHE") == "ignore", reason="the metadata cache is disabled" +) +def test_get_metadata_fingerprinted_readable(simple1_nwb: Path, tmp_path: Path) -> None: + # Unique per run: the module-level cache persists across test runs + fingerprint = f"test-{uuid4()}" + first = FingerprintedReadable(simple1_nwb, fingerprint) + metadata = get_metadata(first) + assert metadata == get_metadata(simple1_nwb) + assert first.opened > 0 + # Its twin is served from the cache by the fingerprint: never even opened, + # which it could not be + twin = FingerprintedReadable(tmp_path / "gone" / simple1_nwb.name, fingerprint) + assert get_metadata(twin) == metadata + assert twin.opened == 0 diff --git a/dandi/tests/test_pynwb_utils.py b/dandi/tests/test_pynwb_utils.py index 52b49367e..1424e9487 100644 --- a/dandi/tests/test_pynwb_utils.py +++ b/dandi/tests/test_pynwb_utils.py @@ -2,21 +2,29 @@ from collections.abc import Callable from datetime import datetime, timezone +import os from pathlib import Path import re +import shutil +import time from types import SimpleNamespace from typing import Any, NoReturn +from fscacher import PersistentCache import h5py import numpy as np -import pytest from pynwb import NWBHDF5IO, NWBFile, TimeSeries +import pytest from pytest_mock import MockerFixture +from .fixtures import FingerprintedReadable +from ..misctypes import Readable from ..pynwb_utils import ( _rename_pose_estimation_original_videos, _sanitize_nwb_version, + memoize_source, nwb_has_external_links, + open_readable, rename_nwb_external_files, ) @@ -66,7 +74,11 @@ def search(v: str) -> None: def test_rename_pose_estimation_original_videos() -> None: pose = SimpleNamespace( neurodata_type="PoseEstimation", - original_videos=[b"camera\\raw.mp4", "https://example.com/remote.mp4", "other.mp4"], + original_videos=[ + b"camera\\raw.mp4", + "https://example.com/remote.mp4", + "other.mp4", + ], ) unrelated = SimpleNamespace( neurodata_type="OtherContainer", original_videos=["camera/raw.mp4"] @@ -116,9 +128,7 @@ def test_rename_pose_estimation_original_videos_persists_hdf5( with h5py.File(filepath, "w") as f: f.create_dataset( "original_videos", - data=np.asarray( - ["camera/raw.mp4", "camera/other.mp4"], dtype=string_type - ), + data=np.asarray(["camera/raw.mp4", "camera/other.mp4"], dtype=string_type), ) with h5py.File(filepath, "r+") as f: @@ -228,3 +238,86 @@ def test_nwb_has_external_links(tmp_path): assert not nwb_has_external_links(filename1) assert nwb_has_external_links(filename4) + + +@pytest.mark.ai_generated +def test_memoize_source(tmp_path: Path, simple1_nwb: Path) -> None: + cache = PersistentCache(path=tmp_path / "cache", tokens=["t1"]) + calls: list[Any] = [] + + def size(source: str | Path | Readable, flag: bool = False) -> str: + calls.append(source) + with open_readable(source) as fp: + return f"{len(fp.read())}:{flag}" + + cached = memoize_source(cache, ["t1"])(size) + nbytes = simple1_nwb.stat().st_size + expected = f"{nbytes}:False" + + # A path is cached the memoize_path way (which skips a file modified "just + # now", as the session-wide fixture may well have been: age a copy) + nwb = tmp_path / simple1_nwb.name + shutil.copyfile(simple1_nwb, nwb) + hour_ago = time.time() - 3600 + os.utime(nwb, (hour_ago, hour_ago)) + assert cached(nwb) == expected + assert cached(nwb) == expected + assert len(calls) == 1 + + # A Readable without a fingerprint is never cached + assert cached(FingerprintedReadable(simple1_nwb, None)) == expected + assert cached(FingerprintedReadable(simple1_nwb, None)) == expected + assert len(calls) == 3 + + # A Readable with a fingerprint is cached by it: its twin is served from + # the cache without being read (it could not be) + first = FingerprintedReadable(simple1_nwb, "A") + assert cached(first) == expected + assert first.opened == 1 + twin = FingerprintedReadable(tmp_path / "gone" / simple1_nwb.name, "A") + assert cached(twin) == expected + assert twin.opened == 0 + assert len(calls) == 4 + + # The file name, the other arguments, and the tokens are part of the key + with pytest.raises(FileNotFoundError): + cached(FingerprintedReadable(tmp_path / "gone" / "other.nwb", "A")) + assert cached(FingerprintedReadable(simple1_nwb, "B")) == expected + assert ( + cached(FingerprintedReadable(simple1_nwb, "A"), flag=True) == f"{nbytes}:True" + ) + assert len(calls) == 7 + other_tokens = memoize_source(cache, ["t2"])(size) + assert other_tokens(FingerprintedReadable(simple1_nwb, "A")) == expected + assert len(calls) == 8 + + # Different functions of the same cache do not share entries + def name(source: str | Path | Readable, flag: bool = False) -> str: + return source.get_filename() if isinstance(source, Readable) else str(source) + + cached_name = memoize_source(cache, ["t1"])(name) + assert cached_name(FingerprintedReadable(simple1_nwb, "A")) == simple1_nwb.name + assert cached(FingerprintedReadable(simple1_nwb, "A")) == expected + assert len(calls) == 8 + + +@pytest.mark.ai_generated +def test_memoize_source_ignored_cache( + tmp_path: Path, simple1_nwb: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setenv("DANDI_TEST_CACHE", "ignore") + cache = PersistentCache( + path=tmp_path / "cache", tokens=["t1"], envvar="DANDI_TEST_CACHE" + ) + calls = 0 + + def count(source: str | Path | Readable) -> int: + nonlocal calls + calls += 1 + return calls + + cached = memoize_source(cache, ["t1"])(count) + assert cached(FingerprintedReadable(simple1_nwb, "A")) == 1 + assert cached(FingerprintedReadable(simple1_nwb, "A")) == 2 + assert cached(simple1_nwb) == 3 + assert cached(simple1_nwb) == 4 From c463b3bfc52a7470f15eff72d07d86e7653763dc Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 19:57:01 +0000 Subject: [PATCH 10/16] Cache streamed validation and metadata under the git-annex key `AnnexReadableFile.get_fingerprint()` returns the file's git-annex key, which is a digest of the content (plus its size), and `pynwb_utils.validate` now goes through `memoize_source` for its `readable=` as well, with the content source as the first argument of the memoized `_validate_cached`. The metadata side (`get_metadata` & co.) picked the caching up already by accepting a `Readable`. So re-running `dandi validate --missing-file-content=stream` on a clone skips the pynwb validation and the metadata extraction of every file whose key has not changed, instead of streaming it again. nwbinspector's checks are not cached (they are not for local files either). Documented in the notes of the "Validating DataLad Dandisets Remotely" section. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01HFhMPs6vF5zyeHDvPTvRcA --- dandi/pynwb_utils.py | 19 ++++++++--------- dandi/support/annex.py | 5 +++++ dandi/support/tests/test_annex.py | 2 ++ dandi/validate/tests/test_core.py | 34 +++++++++++++++++++++++++++++++ docs/source/cmdline/validate.rst | 6 ++++++ 5 files changed, 56 insertions(+), 10 deletions(-) diff --git a/dandi/pynwb_utils.py b/dandi/pynwb_utils.py index c969b98dc..cb945dbde 100644 --- a/dandi/pynwb_utils.py +++ b/dandi/pynwb_utils.py @@ -690,21 +690,20 @@ def validate( 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 not cached. + present locally); the results are then cached only if it has a + `~Readable.get_fingerprint`. """ - if readable is not None: - return _validate(path, readable, devel_debug=devel_debug) - # The memoizing decorator hides the return type from mypy - return cast( - "list[ValidationResult]", _validate_cached(path, devel_debug=devel_debug) - ) + source: str | Readable = readable if readable is not None else str(path) + return _validate_cached(source, str(path), devel_debug=devel_debug) -@validate_cache.memoize_path +@memoize_source(validate_cache, validate_cache_tokens) def _validate_cached( - path: str | Path, devel_debug: bool = False + source: str | Path | Readable, path: str, devel_debug: bool = False ) -> list[ValidationResult]: - return _validate(path, None, devel_debug=devel_debug) + """`_validate` with the content source first, for `memoize_source`""" + readable = source if isinstance(source, Readable) else None + return _validate(path, readable, devel_debug=devel_debug) def _validate( diff --git a/dandi/support/annex.py b/dandi/support/annex.py index 6848707bd..51fee0ecd 100644 --- a/dandi/support/annex.py +++ b/dandi/support/annex.py @@ -335,6 +335,11 @@ def get_mtime(self) -> datetime | None: def get_filename(self) -> str: return self.filepath.name + def get_fingerprint(self) -> str: + # The key is a digest of the content (plus its size), so results + # computed from it (validation, metadata) can be cached under it + return self.key.key + def __str__(self) -> str: return str(self.filepath) diff --git a/dandi/support/tests/test_annex.py b/dandi/support/tests/test_annex.py index 58b24fa07..b1d4f5651 100644 --- a/dandi/support/tests/test_annex.py +++ b/dandi/support/tests/test_annex.py @@ -204,6 +204,7 @@ def test_get_annex_readable(tmp_path: Path, simple2_nwb: Path) -> None: assert r.urls == [simple2_nwb.as_uri()] assert r.get_size() == simple2_nwb.stat().st_size assert r.get_filename() == "sub-01.nwb" + assert r.get_fingerprint() == r.key.key assert str(r) == str(ds / "sub-01" / "sub-01.nwb") # No URLs registered r = get_annex_readable(ds / "sub-02" / "sub-02.nwb") @@ -228,6 +229,7 @@ def test_annex_readable_file_open_file_url(tmp_path: Path) -> None: assert r.get_size() == 1000 assert r.get_mtime() is None assert r.get_filename() == "content.bin" + assert r.get_fingerprint() == key.key # The first URL cannot be opened, so the second one is used fp = r.open() try: diff --git a/dandi/validate/tests/test_core.py b/dandi/validate/tests/test_core.py index 07a8ba614..73d2b5374 100644 --- a/dandi/validate/tests/test_core.py +++ b/dandi/validate/tests/test_core.py @@ -1,4 +1,5 @@ import json +import os from pathlib import Path import shutil from typing import Any @@ -18,6 +19,8 @@ ) from ... import __version__ from ...consts import dandiset_metadata_file +from ...pynwb_utils import validate as pynwb_validate +from ...support.annex import AnnexKey, AnnexReadableFile, get_annex_readable from ...tests.fixtures import ( BIDS_TESTDATA_SELECTION, annex_key_for_file, @@ -395,6 +398,37 @@ def test_validate_stream(tmp_path: Path, simple3_nwb: Path) -> None: ) +@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(tmp_path: Path, simple3_nwb: Path) -> None: + """pynwb validation results of a streamed file are cached under its annex key.""" + skipif.no_git() + pytest.importorskip("fsspec") + ds = _make_streamable_dandiset(tmp_path, simple3_nwb) + nwb = ds / "sub-001" / "sub-001.nwb" + readable = get_annex_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"] + + # The same file under the same key, but with a URL that cannot be opened: + # served from the cache (an attempt to read it would have been reported as + # a pynwb.GENERIC error instead) + twin = AnnexReadableFile( + filepath=nwb, key=readable.key, urls=["file:///nonexistent/sub-001.nwb"] + ) + assert pynwb_validate(nwb, readable=twin) == results + # ... unlike the same file with a different key + other = AnnexReadableFile( + filepath=nwb, + key=AnnexKey.parse(f"SHA256E-s{readable.key.size}--{'0' * 64}.nwb"), + urls=["file:///nonexistent/sub-001.nwb"], + ) + assert [r.id for r in pynwb_validate(nwb, readable=other)] == ["pynwb.GENERIC"] + + @pytest.mark.ai_generated def test_validate_stream_not_streamable(tmp_path: Path) -> None: """stream policy emits an error for a broken symlink with no URL to stream from.""" diff --git a/docs/source/cmdline/validate.rst b/docs/source/cmdline/validate.rst index a4f82be1e..2ac46da9a 100644 --- a/docs/source/cmdline/validate.rst +++ b/docs/source/cmdline/validate.rst @@ -127,6 +127,12 @@ Notes: - 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 From ef659b8cda145a60898064a7c8a89067079ecab9 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 20:02:44 +0000 Subject: [PATCH 11/16] Support fscacher < 0.4, whose memoize() calls the ignore list `ignore` `PersistentCache.memoize(exclude_kwargs=...)` only exists from fscacher 0.4 on; the declared floor is 0.3.0, where the argument is called `ignore`, so the lowest-deps job failed at import time. Pick the name from the signature. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01HFhMPs6vF5zyeHDvPTvRcA --- dandi/pynwb_utils.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/dandi/pynwb_utils.py b/dandi/pynwb_utils.py index d197d8218..5637ff116 100644 --- a/dandi/pynwb_utils.py +++ b/dandi/pynwb_utils.py @@ -111,8 +111,14 @@ def by_fingerprint( by_fingerprint.__module__ = f.__module__ by_fingerprint.__name__ = f"{f.__name__}__by_fingerprint" by_fingerprint.__qualname__ = f"{f.__qualname__}__by_fingerprint" + # fscacher >= 0.4 calls the argument `exclude_kwargs`, older ones `ignore` + ignore_kwarg = ( + "exclude_kwargs" + if "exclude_kwargs" in inspect.signature(cache.memoize).parameters + else "ignore" + ) cached_by_fingerprint = cache.memoize( - by_fingerprint, exclude_kwargs=["_source"] + by_fingerprint, **{ignore_kwarg: ["_source"]} ) @wraps(f) From a0b7aff146a6cb7d2a17fe0b66dfbb6e48f1ec80 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 20:06:49 +0000 Subject: [PATCH 12/16] memoize_source: pass the other arguments by name joblib 1.3 (the declared floor) mistakes positional arguments beyond the fingerprint ones for the keyword-only `_source` parameter of the memoized function ("Keyword-only parameter '_source' was passed as positional parameter"), which broke `validate(path, readable=...)` under lowest-deps. Bind the decorated function's other arguments by name instead (defaults included), which also makes the cache key independent of how they were passed; the decorated function must not take *args as a consequence. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01HFhMPs6vF5zyeHDvPTvRcA --- dandi/pynwb_utils.py | 21 +++++++++++++++++---- dandi/tests/test_pynwb_utils.py | 10 ++++++++++ 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/dandi/pynwb_utils.py b/dandi/pynwb_utils.py index 5637ff116..356e388c3 100644 --- a/dandi/pynwb_utils.py +++ b/dandi/pynwb_utils.py @@ -94,17 +94,20 @@ def memoize_source(cache: PersistentCache, tokens: Sequence[Any]) -> Callable[[F """ def decorator(f: F) -> F: + sig = inspect.signature(f) + if any(p.kind is p.VAR_POSITIONAL for p in sig.parameters.values()): + raise TypeError(f"memoize_source: {f.__qualname__}() must not take *args") by_path = cache.memoize_path(f) def by_fingerprint( filename: str, fingerprint: str, cache_tokens: tuple[Any, ...], - *args: Any, + *, _source: Readable, **kwargs: Any, ) -> Any: - return f(_source, *args, **kwargs) + return f(_source, **kwargs) # Give it an identity of its own under `f`'s module, so that joblib # keeps its results in a per-function directory next to `by_path`'s @@ -128,13 +131,23 @@ def wrapper(source: Any, *args: Any, **kwargs: Any) -> Any: fingerprint = source.get_fingerprint() if fingerprint is None: return f(source, *args, **kwargs) + # Pass `f`'s other arguments by name (defaults included), so that the + # cache key does not depend on how they were passed, and so that + # joblib < 1.4 does not mistake positional ones for `_source` + bound = sig.bind(source, *args, **kwargs) + bound.apply_defaults() + others: dict[str, Any] = {} + for name, value in list(bound.arguments.items())[1:]: + if sig.parameters[name].kind is inspect.Parameter.VAR_KEYWORD: + others.update(value) + else: + others[name] = value return cached_by_fingerprint( source.get_filename(), fingerprint, tuple(tokens), - *args, _source=source, - **kwargs, + **others, ) return cast(F, wrapper) diff --git a/dandi/tests/test_pynwb_utils.py b/dandi/tests/test_pynwb_utils.py index 1424e9487..394f04072 100644 --- a/dandi/tests/test_pynwb_utils.py +++ b/dandi/tests/test_pynwb_utils.py @@ -287,6 +287,9 @@ def size(source: str | Path | Readable, flag: bool = False) -> str: cached(FingerprintedReadable(simple1_nwb, "A"), flag=True) == f"{nbytes}:True" ) assert len(calls) == 7 + # ... however those other arguments are passed + assert cached(FingerprintedReadable(simple1_nwb, "A"), True) == f"{nbytes}:True" + assert len(calls) == 7 other_tokens = memoize_source(cache, ["t2"])(size) assert other_tokens(FingerprintedReadable(simple1_nwb, "A")) == expected assert len(calls) == 8 @@ -300,6 +303,13 @@ def name(source: str | Path | Readable, flag: bool = False) -> str: assert cached(FingerprintedReadable(simple1_nwb, "A")) == expected assert len(calls) == 8 + # The other arguments are keyed by name, so `f` must not take *args + def varargs(source: str | Path | Readable, *args: Any) -> None: + pass + + with pytest.raises(TypeError, match="must not take"): + memoize_source(cache, ["t1"])(varargs) + @pytest.mark.ai_generated def test_memoize_source_ignored_cache( From b544f0332e96f8831bf8ff2de86d87dc11c96d6c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 20:50:22 +0000 Subject: [PATCH 13/16] Fold _validate_cached into _validate The adapter only existed because `_validate` kept its pre-caching parameter order; with the content source as its first parameter, `memoize_source` can decorate `_validate` directly. Same arguments and values reach the memoized function, so cache keys are unchanged. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01HFhMPs6vF5zyeHDvPTvRcA --- dandi/pynwb_utils.py | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/dandi/pynwb_utils.py b/dandi/pynwb_utils.py index 0b6fd7778..7638ce9a7 100644 --- a/dandi/pynwb_utils.py +++ b/dandi/pynwb_utils.py @@ -713,24 +713,15 @@ def validate( `~Readable.get_fingerprint`. """ source: str | Readable = readable if readable is not None else str(path) - return _validate_cached(source, str(path), devel_debug=devel_debug) + return _validate(source, str(path), devel_debug=devel_debug) @memoize_source(validate_cache, validate_cache_tokens) -def _validate_cached( - source: str | Path | Readable, path: str, devel_debug: bool = False -) -> list[ValidationResult]: - """`_validate` with the content source first, for `memoize_source`""" - readable = source if isinstance(source, Readable) else None - return _validate(path, readable, devel_debug=devel_debug) - - def _validate( - path: str | Path, readable: Readable | None, devel_debug: bool = False + source: str | Readable, path: str, devel_debug: bool = False ) -> list[ValidationResult]: - path = str(path) # Might come in as pathlib's PATH - # What to read the content from: - source: str | Readable = readable if readable is not None else path + """`validate` with the content source first, as `memoize_source` needs it""" + readable = source if isinstance(source, Readable) else None errors: list[ValidationResult] = [] # To overcome From ba2f2b38caa195ee8135ae5ee5bef68141f0dc2c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 21:25:50 +0000 Subject: [PATCH 14/16] _validate: take the content source and the reported path as plain arguments Instead of re-deriving a `readable` from the type of `source`, read whatever the source is through `open_readable()` and validate via `pynwb.validate(io=)` for local files too; since pynwb 3.0 that is the same code path as `validate(path=)`, cached namespaces included (`_get_pynwb_metadata` already reads local files this way). Document both arguments on the function. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01HFhMPs6vF5zyeHDvPTvRcA --- dandi/pynwb_utils.py | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/dandi/pynwb_utils.py b/dandi/pynwb_utils.py index 7638ce9a7..0109fa632 100644 --- a/dandi/pynwb_utils.py +++ b/dandi/pynwb_utils.py @@ -720,8 +720,17 @@ def validate( def _validate( source: str | Readable, path: str, devel_debug: bool = False ) -> list[ValidationResult]: - """`validate` with the content source first, as `memoize_source` needs it""" - readable = source if isinstance(source, Readable) else None + """`validate` proper, with its arguments arranged for `memoize_source` + + 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 + """ errors: list[ValidationResult] = [] # To overcome @@ -779,15 +788,12 @@ def _validate( ) try: - # Either way, validates against the namespaces cached in the file; pynwb - # falls back to its own namespaces if the file has none cached - if readable is not None: - with open_readable(readable) 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=path) + # Validates against the namespaces cached in the file; pynwb falls back + # to its own namespaces if the file has none cached + 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) except Exception as exc: if devel_debug: raise From 22e296b614bbec114723ba4817bf1e45b0cfbd12 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 22:37:30 +0000 Subject: [PATCH 15/16] _validate: validate local files by path again ba2f2b38 routed local files through open_readable() + h5py's file-object driver + pynwb.validate(io=...), the same way streamed content is validated. The lowest-deps CI job started failing on that commit while the same tests pass locally under the same dependency floors, so go back to pynwb.validate(path=...) for local files and keep the io route for Readables only. The explicit (source, path) arguments stay. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01HFhMPs6vF5zyeHDvPTvRcA --- dandi/pynwb_utils.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/dandi/pynwb_utils.py b/dandi/pynwb_utils.py index 0109fa632..6dc702362 100644 --- a/dandi/pynwb_utils.py +++ b/dandi/pynwb_utils.py @@ -788,12 +788,15 @@ def _validate( ) try: - # Validates against the namespaces cached in the file; pynwb falls back - # to its own namespaces if the file has none cached - 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) + # 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 From 9ef57964ec9d5e13510d230ed1a75a369b1f8f76 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 19:56:49 +0000 Subject: [PATCH 16/16] validate: cast the memoized _validate's result for mypy fscacher is untyped, so since _validate is decorated with fscacher's memoize_path (instead of the typed memoize_source) mypy sees it as returning Any. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- dandi/pynwb_utils.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/dandi/pynwb_utils.py b/dandi/pynwb_utils.py index 6fb384ca9..b65753b39 100644 --- a/dandi/pynwb_utils.py +++ b/dandi/pynwb_utils.py @@ -648,7 +648,11 @@ def validate( `~Readable.get_fingerprint`. """ source: str | Readable = readable if readable is not None else str(path) - return _validate(source, str(path), devel_debug=devel_debug) + # 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=readable_fingerprint)