Skip to content

Do not recurse when getting the metadata of an NWB file in a BIDS dataset - #1958

Draft
CodyCBakerPhD wants to merge 1 commit into
masterfrom
claude/fix-bids-nwb-metadata-recursion
Draft

CodyCBakerPhD wants to merge 1 commit into
masterfrom
claude/fix-bids-nwb-metadata-recursion

Conversation

@CodyCBakerPhD

Copy link
Copy Markdown
Contributor

Problem

For an NWB file in a BIDS dataset, dandi.metadata.nwb.get_metadata() took the BIDS part of the metadata from df.get_metadata(). df is an NWBBIDSAsset, and its get_metadata() also extracts the NWB metadata, via NWBAsset.get_metadata() → nwb2asset() → get_metadata(). That calls back into the same BIDS branch, so the cycle repeats until RecursionError.

The result was still correct, because NWBAsset.get_metadata() falls back to default metadata on errors at each level. But the BIDS validator ran at every one of roughly 100 levels. On ieeg_epilepsyNWB from bids-examples, one get_metadata() call made 98 nested nwb2asset() calls and took about 60 s. That is what made test_bids_nwb_metadata_integration time out on Windows CI.

Fix

Take only the BIDS metadata, via bids.BIDSAsset.get_metadata(df, ...), which was the intent of that branch. The NWB metadata is extracted by the rest of get_metadata() itself.

Measured on the same file:

nwb2asset() calls time subject_id / sex
before 98 (nested) 60.9 s 01 / U
after 0 0.7 s 01 / U

Tests

  • Added test_bids_nwb_metadata_no_recursion, which spies on nwb2asset(). It fails on the current master (68 calls) and passes with the fix.
  • dandi/tests/test_metadata.py, dandi/tests/test_files.py, dandi/files and dandi/validate/tests/test_core.py pass locally: 198 passed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr


Generated by Claude Code

…aset

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
@codecov

codecov Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.80%. Comparing base (ad8f64d) to head (e38507b).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1958      +/-   ##
==========================================
+ Coverage   90.77%   90.80%   +0.03%     
==========================================
  Files          92       92              
  Lines       14130    14142      +12     
==========================================
+ Hits        12826    12842      +16     
+ Misses       1304     1300       -4     
Flag Coverage Δ
unittests 90.80% <100.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants