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 bf43dbcb2..fe7820f2a 100644 --- a/dandi/metadata/nwb.py +++ b/dandi/metadata/nwb.py @@ -69,7 +69,9 @@ def get_metadata( assert isinstance(df, bids.BIDSAsset) if not digest: digest = DUMMY_DANDI_ETAG - path_metadata = df.get_metadata(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 f6b40ddaa..63e37488d 100644 --- a/dandi/tests/test_metadata.py +++ b/dandi/tests/test_metadata.py @@ -36,14 +36,17 @@ 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 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 from ..metadata.util import ( @@ -124,6 +127,59 @@ 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.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", [