Drop session from the fmap BIDS filter so session-level *prep runs can start - #395
Draft
asmacdo wants to merge 1 commit into
Draft
Drop session from the fmap BIDS filter so session-level *prep runs can start#395asmacdo wants to merge 1 commit into
session from the fmap BIDS filter so session-level *prep runs can start#395asmacdo wants to merge 1 commit into
Conversation
Session-level runs of any `*prep` app abort during workflow construction:
sdcflows/utils/wrangler.py, in find_estimators
ValueError: Filters include session, but session is already defined.
fmriprep passes the fmap entry of `--bids-filter-file` to sdcflows'
`find_estimators()` alongside the session it resolved itself, and sdcflows
refuses to receive the session from both sources. The session is already
enforced by the sparse checkout, so the key can simply go.
Also removes `filter_file.sh.jinja2`, which nothing references.
The new test executes the generated filter-file block and asserts on the
parsed JSON, so it covers what the BIDS app actually receives rather than
the rendered text.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #395 +/- ##
=======================================
Coverage 79.19% 79.19%
=======================================
Files 17 17
Lines 2168 2168
Branches 385 385
=======================================
Hits 1717 1717
Misses 308 308
Partials 143 143 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
14 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Session-level
fmriprepaborts during workflow construction with:The
fmapentry of the generated--bids-filter-filepins a session, and fmriprep independently passes the session to sdcflows'find_estimators(). sdcflows treats receiving the session from both sources as a conflict and raises, so no session-level job can start. Droppingsessionfrom that one entry unblocks the run. Every other entry keeps its session.History — this restores the pre-#337 fmap entry
The fmap entry carried no
sessionfrom #231 until #337 (@tien-tong's sparse-checkout PR) added'session': '$sesid'to it. This PR puts it back to{'datatype': 'fmap'}. So the question isn't "is it safe to remove a key" but "was #337's addition of the key intentional, or a blanket session-scoping of every filter entry that didn't notice fmap conflicts with sdcflows?"Why it should be safe — reasoning, please poke holes
The argument: since #337, each job's input is
git sparse-checkout'd down to a singlesub-XX/ses-YY(participant_job.sh.jinja2), so the working tree contains only the target session's files. If that holds, a{'datatype': 'fmap'}filter with no session can't match a fieldmap from another session — no other session's files are on disk — so dropping the key shouldn't broaden what any app sees, whatever app reads the filter. (Note the same PR that made the checkout single-session is the one that added the key — so on this reading the key was never needed.)I have not proven this beyond the one fmriprep run, so I'd rather the meeting kick the tires than take it on faith. The place I'd look first: that per-dataset narrowing is best-effort —
participant_job.sh.jinja2wraps it in|| true. If it ever fails, the full dataset tree is present and a session-less fmap filter could match other sessions. The mitigation is thatdatalad get -nonly fetched the target session's content, so other-session files are annex pointers without content — the app would likely error rather than silently use the wrong fieldmap — but I haven't confirmed that path.Scope — verified vs. expected
The filter file is generated for any
*prepapp at session level (gate:'prep' in container_name.lower()), so the same fmap entry reaches fmriprep / qsiprep / aslprep alike.ds004044— crashes without this change, runs with it.find_estimators()path, so they should hit the same conflict and the same fix — but I have not run them end-to-end.Open questions for review
@tien-tong: was adding'session': '$sesid'to the fmap entry in Usegit sparse-checkoutinparticipant_jobfor more efficient dataset cloning #337 intentional, or a side effect of scoping every filter entry to the session? Anything sparse-checkout doesn't already cover that it was meant to catch?Changes
bidsapp_run.sh.jinja2/bidsapp_pipeline_run.sh.jinja2— fmap entry is now{'datatype': 'fmap'}.filter_file.sh.jinja2— unreferenced.test_session_filter_file_fmap_carries_no_session(fmriprep / qsiprep / aslprep). It executes the rendered filter-file block underbashand asserts on the parsed JSON, so it covers what the app receives, not template text. Fails without the fix on all three (asserted diff is exactly the extra{'session': '1'}).Call path
Three conditions must coincide to hit this, which is why it went unnoticed:
processing_level == 'session', an app withprepin its name (theflag_filterfilegate), and not--anat-only(which skips fieldmap estimation entirely). Subject-level runs never render the block.🤖 Generated with Claude Code