From c09ac629385302bb5e86713e234836708b1fc600 Mon Sep 17 00:00:00 2001 From: Austin Macdonald Date: Sun, 19 Jul 2026 15:25:32 -0500 Subject: [PATCH] Drop `session` from the fmap entry of the BIDS filter file 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) --- babs/templates/bidsapp_pipeline_run.sh.jinja2 | 5 +- babs/templates/bidsapp_run.sh.jinja2 | 5 +- babs/templates/filter_file.sh.jinja2 | 22 ------- tests/test_generate_bidsapp_runscript.py | 63 +++++++++++++++++++ 4 files changed, 71 insertions(+), 24 deletions(-) delete mode 100644 babs/templates/filter_file.sh.jinja2 diff --git a/babs/templates/bidsapp_pipeline_run.sh.jinja2 b/babs/templates/bidsapp_pipeline_run.sh.jinja2 index 90198b20..729b2b89 100644 --- a/babs/templates/bidsapp_pipeline_run.sh.jinja2 +++ b/babs/templates/bidsapp_pipeline_run.sh.jinja2 @@ -15,7 +15,10 @@ sesid="$2" filterfile="${PWD}/${sesid}_filter.json" {% raw %}{{% endraw %} echo "{" - echo "'fmap': {'datatype': 'fmap', 'session': '$sesid'}," + {# The fmap entry must NOT carry 'session'. fmriprep passes this entry to sdcflows' + find_estimators() together with the session it resolved itself, and sdcflows rejects + receiving the session twice. The session is already enforced by the sparse checkout. #} + echo "'fmap': {'datatype': 'fmap'}," {% set first_filter_step = steps_with_filter[0] %} {% if 'fmriprep' in first_filter_step['container_name'].lower() %} echo "'bold': {'datatype': 'func', 'session': '$sesid', 'suffix': 'bold'}," diff --git a/babs/templates/bidsapp_run.sh.jinja2 b/babs/templates/bidsapp_run.sh.jinja2 index 12b5da0e..4e59f586 100644 --- a/babs/templates/bidsapp_run.sh.jinja2 +++ b/babs/templates/bidsapp_run.sh.jinja2 @@ -14,7 +14,10 @@ sesid="$2" filterfile="${PWD}/${sesid}_filter.json" {% raw %}{{% endraw %} echo "{" - echo "'fmap': {'datatype': 'fmap', 'session': '$sesid'}," + {# The fmap entry must NOT carry 'session'. fmriprep passes this entry to sdcflows' + find_estimators() together with the session it resolved itself, and sdcflows rejects + receiving the session twice. The session is already enforced by the sparse checkout. #} + echo "'fmap': {'datatype': 'fmap'}," {% if 'fmriprep' in container_name.lower() %} echo "'bold': {'datatype': 'func', 'session': '$sesid', 'suffix': 'bold'}," {% elif 'qsiprep' in container_name.lower() %} diff --git a/babs/templates/filter_file.sh.jinja2 b/babs/templates/filter_file.sh.jinja2 deleted file mode 100644 index 3898a7e7..00000000 --- a/babs/templates/filter_file.sh.jinja2 +++ /dev/null @@ -1,22 +0,0 @@ -filterfile=${PWD}/${sesid}_filter.json -echo "{" > ${filterfile} - -echo "'fmap': {'datatype': 'fmap'}," >> ${filterfile} - -{% if 'fmriprep' in container_name.lower() %} -echo "'bold': {'datatype': 'func', 'session': '$sesid', 'suffix': 'bold'}," >> ${filterfile} -{% elif 'qsiprep' in container_name.lower() %} -echo "'dwi': {'datatype': 'dwi', 'session': '$sesid', 'suffix': 'dwi'}," >> ${filterfile} -{% elif 'aslprep' in container_name.lower() %} -echo "'perf': {'datatype': 'perf', 'session': '$sesid', 'suffix': 'asl'}," >> ${filterfile} -{% endif %} -echo "'sbref': {'datatype': 'func', 'session': '$sesid', 'suffix': 'sbref'}," >> ${filterfile} -echo "'flair': {'datatype': 'anat', 'session': '$sesid', 'suffix': 'FLAIR'}," >> ${filterfile} -echo "'t2w': {'datatype': 'anat', 'session': '$sesid', 'suffix': 'T2w'}," >> ${filterfile} -echo "'t1w': {'datatype': 'anat', 'session': '$sesid', 'suffix': 'T1w'}," >> ${filterfile} -echo "'roi': {'datatype': 'anat', 'session': '$sesid', 'suffix': 'roi'}" >> ${filterfile} -echo "}" >> ${filterfile} - -# remove ses and get valid json -sed -i "s/'/\\\"/g" ${filterfile} -sed -i "s/ses-//g" ${filterfile} \ No newline at end of file diff --git a/tests/test_generate_bidsapp_runscript.py b/tests/test_generate_bidsapp_runscript.py index 19a34402..4f10343e 100644 --- a/tests/test_generate_bidsapp_runscript.py +++ b/tests/test_generate_bidsapp_runscript.py @@ -1,3 +1,5 @@ +import json +import os import subprocess from pathlib import Path @@ -139,6 +141,67 @@ def test_generate_bidsapp_runscript(input_datasets, config_file, processing_leve assert passed, status +def generate_session_filter_file(config_file, input_datasets, tmp_path): + """Render a session-level runscript, run its filter-file block, and parse the result. + + The filter file is written by the job script at runtime, so the only way to see what + a BIDS app actually receives is to execute the block that builds it. + """ + config = read_yaml(NOTEBOOKS_DIR / config_file) + _, bids_app_output_dir = app_output_settings_from_config(config) + script_content = generate_bidsapp_runscript( + input_datasets, + 'session', + container_name=config_file.split('_')[1], + relative_container_path='containers/.datalad/containers/app/image', + bids_app_output_dir=bids_app_output_dir, + dict_zip_foldernames=config['zip_foldernames'], + bids_app_args=config['bids_app_args'], + singularity_args=config['singularity_args'], + templateflow_home='/path/to/templateflow_home', + ) + + # the block runs from `filterfile=...` through the last `sed -i` that repairs the JSON + lines = script_content.splitlines() + start = next(i for i, line in enumerate(lines) if line.startswith('filterfile=')) + end = max(i for i, line in enumerate(lines) if line.startswith('sed -i')) + block = '\n'.join(lines[start : end + 1]) + + subprocess.run( + ['bash', '-c', block], + cwd=tmp_path, + env={'sesid': 'ses-1', 'PATH': os.environ['PATH']}, + check=True, + ) + return json.loads((tmp_path / 'ses-1_filter.json').read_text()) + + +@pytest.mark.parametrize( + 'config_file', + [ + 'eg_fmriprep-24-1-1_regular.yaml', + 'eg_qsiprep-1-0-0_regular.yaml', + 'eg_aslprep-0-7-5.yaml', + ], +) +def test_session_filter_file_fmap_carries_no_session(config_file, tmp_path): + """The fmap entry must not pin a session. + + fmriprep passes this entry straight to sdcflows' ``find_estimators()`` alongside the + session it resolved itself, and sdcflows raises "Filters include session, but session + is already defined." when it receives both. That aborts workflow construction, so no + session-level job can run. The session is already enforced by the sparse checkout. + """ + filters = generate_session_filter_file(config_file, input_datasets_prep, tmp_path) + + assert filters['fmap'] == {'datatype': 'fmap'} + + # the remaining entries still restrict to this session ('ses-' is stripped by sed) + others = {key: value for key, value in filters.items() if key != 'fmap'} + assert others + assert all(value.get('session') == '1' for value in others.values()) + + def run_shellcheck(script_path): """Run shellcheck on a shell script string and return the result.