From e38507bf04e4784841976ebaa25ca5d20b7df2e5 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 17:26:47 +0000 Subject: [PATCH 1/2] Do not recurse when getting the metadata of an NWB file in a BIDS dataset For an NWB file in a BIDS dataset, get_metadata() took the BIDS metadata from NWBBIDSAsset.get_metadata(), which also extracts the NWB metadata by calling nwb2asset() and thus get_metadata() again, and so on until RecursionError: ~100 levels, each running the BIDS validator. The result was still correct, as each level falls back on errors, but it took about a minute per file (and timed out CI on Windows). Take only the BIDS metadata, with BIDSAsset.get_metadata(). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- dandi/metadata/nwb.py | 5 ++++- dandi/tests/test_metadata.py | 25 +++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/dandi/metadata/nwb.py b/dandi/metadata/nwb.py index bf43dbcb2..15baa5d9e 100644 --- a/dandi/metadata/nwb.py +++ b/dandi/metadata/nwb.py @@ -69,7 +69,10 @@ def get_metadata( assert isinstance(df, bids.BIDSAsset) if not digest: digest = DUMMY_DANDI_ETAG - path_metadata = df.get_metadata(digest=digest) + # Only the BIDS metadata: `df` is an `NWBBIDSAsset`, whose own + # `get_metadata()` would extract the NWB metadata by calling this + # function again, recursing until `RecursionError` + path_metadata = bids.BIDSAsset.get_metadata(df, digest=digest) meta["bids_version"] = df.get_validation_bids_version() # there might be a more elegant way to do this: if path_metadata.wasAttributedTo: diff --git a/dandi/tests/test_metadata.py b/dandi/tests/test_metadata.py index f6b40ddaa..b62b83b33 100644 --- a/dandi/tests/test_metadata.py +++ b/dandi/tests/test_metadata.py @@ -36,6 +36,7 @@ from pynwb import NWBHDF5IO, NWBFile, TimeSeries from pynwb.file import Subject import pytest +from pytest_mock import MockerFixture import requests from semantic_version import Version @@ -44,6 +45,7 @@ from .. import __version__ from ..consts import metadata_nwb_subject_fields from ..dandiapi import RemoteBlobAsset +from ..metadata import nwb as metadata_nwb from ..metadata.core import prepare_metadata from ..metadata.nwb import get_metadata, nwb2asset from ..metadata.util import ( @@ -124,6 +126,29 @@ def test_bids_nwb_metadata_integration(bids_examples: Path, tmp_path: Path) -> N assert metadata["sex"] == "U" +@pytest.mark.ai_generated +def test_bids_nwb_metadata_no_recursion( + bids_examples: Path, tmp_path: Path, mocker: MockerFixture +) -> None: + # Getting the BIDS metadata of an NWB file in a BIDS dataset must not + # extract its NWB metadata again (which used to recurse until + # RecursionError, running the BIDS validator at every level) + dpath = tmp_path / "ieeg_epilepsyNWB" + shutil.copytree(bids_examples / "ieeg_epilepsyNWB", dpath) + file_path = ( + dpath + / "sub-01" + / "ses-postimp" + / "ieeg" + / "sub-01_ses-postimp_task-seizure_run-01_ieeg.nwb" + ) + spy = mocker.spy(metadata_nwb, "nwb2asset") + metadata = get_metadata.__wrapped__(file_path) + assert spy.call_count == 0 + assert metadata["subject_id"] == "01" + assert metadata["sex"] == "U" + + @pytest.mark.parametrize( "age,duration", [ From 3ee7aba6e0aba3736306dcb60b0857b4bf73c85a Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 02:17:40 +0000 Subject: [PATCH 2/2] Add BIDSAsset.get_bids_metadata() to get only the BIDS metadata of an asset Getting the metadata of an NWB file in a BIDS dataset recursed because of a cycle between two methods that each merge BIDS and NWB metadata: `NWBBIDSAsset.get_metadata()` extracts the NWB metadata with `dandi.metadata.nwb.get_metadata()`, which itself got the BIDS metadata of the file with `get_metadata()` of its `NWBBIDSAsset`. Calling `BIDSAsset.get_metadata()` unbound only patched this one call site. Instead, give BIDS assets a method returning only their BIDS metadata, which no subclass overrides to read the content, and use it wherever only the BIDS metadata is wanted: when extracting the metadata of an NWB file, and in `NWBBIDSAsset.get_metadata()` and `ZarrBIDSAsset.get_metadata()` (no change in behavior). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --- dandi/files/bids.py | 19 ++++++++++++++----- dandi/metadata/nwb.py | 7 +++---- dandi/tests/test_metadata.py | 33 ++++++++++++++++++++++++++++++++- 3 files changed, 49 insertions(+), 10 deletions(-) diff --git a/dandi/files/bids.py b/dandi/files/bids.py index 008b085dd..371f7cd78 100644 --- a/dandi/files/bids.py +++ b/dandi/files/bids.py @@ -248,6 +248,18 @@ def get_metadata( digest: Digest | None = None, ignore_errors: bool = True, ) -> BareAsset: + return self.get_bids_metadata(digest) + + def get_bids_metadata(self, digest: Digest | None = None) -> BareAsset: + """ + Return the metadata of the asset that comes from the BIDS dataset (its + path and files such as :file:`participants.tsv`) only. + + Unlike `get_metadata()`, which subclasses override to also extract + metadata from the content of the asset (e.g., `NWBBIDSAsset` from the + NWB file), this never reads the content, so that the extraction of + metadata from the content can use it without recursing. + """ 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) @@ -283,7 +295,7 @@ def get_metadata( digest: Digest | None = None, ignore_errors: bool = True, ) -> BareAsset: - bids_metadata = BIDSAsset.get_metadata(self, digest, ignore_errors) + bids_metadata = self.get_bids_metadata(digest) nwb_metadata = NWBAsset.get_metadata(self, digest, ignore_errors) return BareAsset( **{ @@ -318,10 +330,7 @@ def get_metadata( digest: Digest | None = None, ignore_errors: bool = True, ) -> 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) - metadata.path = self.path + metadata = self.get_bids_metadata(digest) metadata.encodingFormat = ZARR_MIME_TYPE return metadata diff --git a/dandi/metadata/nwb.py b/dandi/metadata/nwb.py index 15baa5d9e..fe7820f2a 100644 --- a/dandi/metadata/nwb.py +++ b/dandi/metadata/nwb.py @@ -69,10 +69,9 @@ def get_metadata( assert isinstance(df, bids.BIDSAsset) if not digest: digest = DUMMY_DANDI_ETAG - # Only the BIDS metadata: `df` is an `NWBBIDSAsset`, whose own - # `get_metadata()` would extract the NWB metadata by calling this - # function again, recursing until `RecursionError` - path_metadata = bids.BIDSAsset.get_metadata(df, digest=digest) + # Not `df.get_metadata()`, which for an NWB file would extract + # its NWB metadata, i.e., call this function again + path_metadata = df.get_bids_metadata(digest=digest) meta["bids_version"] = df.get_validation_bids_version() # there might be a more elegant way to do this: if path_metadata.wasAttributedTo: diff --git a/dandi/tests/test_metadata.py b/dandi/tests/test_metadata.py index b62b83b33..63e37488d 100644 --- a/dandi/tests/test_metadata.py +++ b/dandi/tests/test_metadata.py @@ -43,8 +43,9 @@ from .fixtures import SampleDandiset from .skip import mark from .. import __version__ -from ..consts import metadata_nwb_subject_fields +from ..consts import dandiset_metadata_file, metadata_nwb_subject_fields from ..dandiapi import RemoteBlobAsset +from ..files import NWBBIDSAsset, dandi_file, find_bids_dataset_description from ..metadata import nwb as metadata_nwb from ..metadata.core import prepare_metadata from ..metadata.nwb import get_metadata, nwb2asset @@ -149,6 +150,36 @@ def test_bids_nwb_metadata_no_recursion( assert metadata["sex"] == "U" +@pytest.mark.ai_generated +def test_nwb_bids_asset_metadata_no_recursion( + bids_examples: Path, tmp_path: Path, mocker: MockerFixture +) -> None: + # The metadata of an NWBBIDSAsset merges its BIDS and NWB metadata, + # extracting the latter (which itself includes BIDS metadata) only once + dpath = tmp_path / "ieeg_epilepsyNWB" + shutil.copytree(bids_examples / "ieeg_epilepsyNWB", dpath) + (dpath / dandiset_metadata_file).write_text("identifier: '000027'\n") + file_path = ( + dpath + / "sub-01" + / "ses-postimp" + / "ieeg" + / "sub-01_ses-postimp_task-seizure_run-01_ieeg.nwb" + ) + # (The asset refers to it only weakly) + bids_dataset_description = find_bids_dataset_description(file_path) + df = dandi_file(file_path, dpath, bids_dataset_description=bids_dataset_description) + assert isinstance(df, NWBBIDSAsset) + spy = mocker.spy(metadata_nwb, "nwb2asset") + metadata = df.get_metadata(digest=DUMMY_DANDI_ETAG, ignore_errors=False) + assert spy.call_count == 1 + assert metadata.wasAttributedTo is not None + participant = metadata.wasAttributedTo[0] + assert participant.identifier == "01" + assert participant.sex is not None + assert participant.sex.name == "Unknown" + + @pytest.mark.parametrize( "age,duration", [