Skip to content

Stage BIDS-inherited metadata for every job; common_paths for explicit extras - #385

Merged
tien-tong merged 12 commits into
PennLINC:mainfrom
asmacdo:pr-376-fixup
Jul 23, 2026
Merged

Stage BIDS-inherited metadata for every job; common_paths for explicit extras#385
tien-tong merged 12 commits into
PennLINC:mainfrom
asmacdo:pr-376-fixup

Conversation

@asmacdo

@asmacdo asmacdo commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Closes #374. Builds on @djarecka's common_paths commits (included here) #376.

Problem

A per-subject (or per-session) sparse-checkout drops dataset-level BIDS metadata that sits above the subject in the directory hierarchy — e.g. a top-level task-rest_bold.json carrying RepetitionTime/PhaseEncodingDirection, or participants.tsv. Under the BIDS Inheritance Principle those files apply to the data below them, so dropping them makes BIDS Apps fail on otherwise-valid data (KeyError: 'RepetitionTime', "Missing readout timing information").

@djarecka's common_paths (#374) replaced the hard-coded dataset_description.json with a configurable list, but a user still has to enumerate every inherited sidecar — which they can't, since the names vary per dataset.

What changed

Automatic BIDS inheritance. For every non-zipped input dataset, each job now stages every metadata blob at the tiers above its checkout:

  • the dataset root — always;
  • the subject tier (sub-XX/) — additionally for session-level jobs, whose sub-XX/ses-YY checkout would otherwise miss files sitting directly under sub-XX/ (subject-level jobs already check out the whole sub-XX/ subtree).

Resolution is positional and happens at job runtime: resolve_tier() uses git ls-tree HEAD <tier>, which is non-recursive (anchored to the tier, never crosses into other subjects) and reads the committed tree (independent of sparse/checkout state). It resolves to literal paths because datalad get / git sparse-checkout can't take a glob. The resolved paths feed all three consumers: datalad get -n, git sparse-checkout set --stdin, and datalad run -i.

common_paths recast as explicit-extras. Kept from @djarecka's work for a non-inherited file the grab doesn't reach (e.g. a shared sourcedata/NIDM/nidm.ttl). Its default flips to [], since inheritance now covers dataset_description.json automatically.

Why positional, not a glob list

Inheritance is about position in the hierarchy, not file extension. In a valid BIDS dataset no data files sit at the root or directly under sub-XX/ (imaging data is always inside a datatype dir under sub-XX/[ses-YY/]), so every blob at those tiers is metadata by construction. Grabbing "all upper-tier blobs" is therefore BIDS-correct and future-proof, and — crucially — needs no user configuration to get right.

Testing

  • Unit (render/structure): root tier resolved for every job, subject tier only for session-level jobs, zipped inputs skipped, common_paths threaded into all three consumers, and the [] default.
  • End-to-end: validated via the e2e-slurm walkthrough (simbids), both projects green. The walkthrough sets no common_paths, so with the new [] default dataset_description.json reaches fmriprep --anat-only only via the grab — making a green walkthrough an implicit regression test for the inheritance staging.

Follow-up

A faithful runtime staging test (positive + negative assertions, incl. the session subject-tier) is awkward today: the e2e walkthrough only checks job outcomes and runs subject-level. Tracked as #384 — to land as a pre-app-hook test once the splice-point hooks (#381) merge.

🤖 Generated with Claude Code

djarecka and others added 9 commits May 13, 2026 15:00
Replaces the hard-coded `dataset_description.json` in sparse-checkout,
`datalad get`, and `datalad run -i` with a configurable `common_paths`
list on each input dataset entry. Defaults to `["dataset_description.json"]`
to preserve existing behaviour. An empty list disables all common-path
inclusion.

Closes PennLINC#374

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add common_paths to the section overview list and optional sections list,
add a stub required_files section, and add a full common_paths section
with examples and usage notes.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add common paths to the submit script test inputs.

No need to add to the zipped ones, they are skipped.
`common_paths` (PR PennLINC#376) replaces the hard-coded `dataset_description.json`
but still drops root-level inherited BIDS sidecars (e.g. a top-level
`task-*_bold.json` carrying RepetitionTime/PhaseEncodingDirection) from the
per-job sparse-checkout, so fmriprep fails on valid data with
`KeyError: 'RepetitionTime'` / "Missing readout timing information".

Make BIDS inheritance automatic and positional: each job now grabs every
metadata blob at the tiers ABOVE its checkout -- the dataset root always,
plus the subject tier (sub-XX/) for session-level jobs, whose sub-XX/ses-YY
checkout would otherwise miss files sitting directly under sub-XX/. In valid
BIDS no data files sit at those tiers, so every blob there is inheritable
metadata. resolve_tier() uses `git ls-tree HEAD` -- non-recursive (anchored,
never crosses into other subjects) and reading the committed tree
(independent of sparse/checkout state) -- and resolves to literal paths
because datalad/sparse-checkout cannot take a glob. The resolved paths feed
all three consumers: `datalad get -n`, `git sparse-checkout set --stdin`,
and `datalad run -i`.

`common_paths` is kept as the explicit-extras escape hatch for a
non-inherited file the grab doesn't reach (e.g. `sourcedata/.../nidm.ttl`);
its default flips from `['dataset_description.json']` to `[]` since
inheritance now covers `dataset_description.json` automatically.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Render-string assertions (matching this module's existing render+shellcheck
style): root tier resolved for every job, subject tier only for session-level
jobs, zipped inputs skipped, and an explicit common_paths entry reaching
`datalad get` / sparse-checkout / `datalad run -i`. Plus an object-level test
that common_paths defaults to [].

These check the generated script text; runtime resolve_tier behavior is left
to the e2e walkthrough (a faithful staging assertion is awkward until the
splice-point hooks land -- see PR description / follow-up).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The fixup makes BIDS metadata inheritance automatic, so dataset_description.json
and other upper-tier sidecars are staged without configuration, and common_paths
now defaults to []. Add a "BIDS metadata inheritance (automatic)" section and
recast common_paths as the explicit-extras escape hatch (e.g. a non-inherited
sourcedata/.../nidm.ttl), replacing the old dataset_description.json-default and
"keep the default + add" examples.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@asmacdo

asmacdo commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator Author

CI failure is just codecov failing to upload, which happens on my other PR too.

@codecov-commenter

codecov-commenter commented Jul 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.21%. Comparing base (2cc536a) to head (3d11215).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #385      +/-   ##
==========================================
+ Coverage   79.19%   79.21%   +0.01%     
==========================================
  Files          17       17              
  Lines        2168     2170       +2     
  Branches      385      385              
==========================================
+ Hits         1717     1719       +2     
  Misses        308      308              
  Partials      143      143              

☔ 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.

@yarikoptic

Copy link
Copy Markdown
Collaborator

@asmacdo please address this fail (let's hope for small fix)

image

@asmacdo

asmacdo commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator Author

Fixed in 3d11215 — the failing check was a test that needed updating after the #369 merge; CI is green now.

I suggest holding the merge until I've re-validated on one of the datasets that originally hit the KeyError: 'RepetitionTime'. I'll start that job today and hopefully be ready to confirm tomorrow, then request review.

@tien-tong
tien-tong merged commit 0a2890c into PennLINC:main Jul 23, 2026
7 checks passed
@tien-tong

Copy link
Copy Markdown
Contributor

Oops was in review window and did not see this comment until after merging @asmacdo! LMK how the test goes, can revert main if needed.

@asmacdo

asmacdo commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator Author

Verified on real data: ran fmriprep --level minimal via BABS on https://github.com/OpenNeuroDatasets/ds003097 (Unity cluster). The babs was a branch off main carrying this change plus #394 (which the anat→minimal chaining also needs). All three subjects built the BOLD workflow and finished successfully — the root-inherited task-*_bold.json was staged into each per-job sparse checkout as intended, with no RepetitionTime KeyError. Thanks for landing this!

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.

Add extra_paths to input dataset config to retain non-subject files (e.g. nidm.ttl)

5 participants