Adding an option to fit BIDS-study layout (updated) by djarecka · Pull Request #369 · PennLINC/babs

djarecka · 2026-04-17T17:04:02Z

instead of #361 (after rewriting history went wrong); I included the newest changes to main.
From the previous description:

allowing for configuring analysis_path
ading rias to .babs and gitignore
adding initial configuration file to .babs, so it can be read whenever is neeeded

An example presenting the usage and the layout can be found on the
babs_demo repo, follow the section for the bids_layout

…rectory for config/RIA storage

codecov-commenter · 2026-04-17T17:13:54Z

Codecov Report

❌ Patch coverage is 87.87879% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.12%. Comparing base (95fd99d) to head (42a8cea).

Files with missing lines	Patch %	Lines
babs/bootstrap.py	71.42%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #369      +/-   ##
==========================================
+ Coverage   79.07%   79.12%   +0.04%     
==========================================
  Files          17       17              
  Lines        2079     2103      +24     
  Branches      362      366       +4     
==========================================
+ Hits         1644     1664      +20     
- Misses        300      302       +2     
- Partials      135      137       +2

☔ View full report in Codecov by Sentry.
📢 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.

asmacdo

I ran this against the https://github.com/djarecka/babs_demo with the bids layout setup, and it worked very smoothly!

Tested this end-to-end on a Dartmouth/Discovery slurm cluster using djarecka's babs_demo bids_layout walkthrough (simbids → init with analysis_path: . and .babs/{input,output}_ria → submit → merge → unzip). All jobs finished and the resulting layout under <study>/derivatives/<project>/ is exactly the BIDS-derivatives shape we (mechababs) want for the OpenNeuroStudies/OpenNeuroDerivatives integration. Strongly aligned with our roadmap — thanks!

Three small things noticed while verifying:

1. .gitattributes defaults annex small metadata files. .babs/babs_init_config.yaml (newly committed by this PR) and the extracted dataset_description.json both end up as annex symlinks. Filed as #378
2. .gitignore RIA rule uses op.basename, which is over-broad. With input_ria_path: .babs/input_ria, the basename input_ria becomes a gitignore rule that matches any path component named input_ria. I think this is just a nitpick and should be fine.

djarecka · 2026-05-28T15:38:55Z

Hi @tien-tong, just pinging if you have any questions or comments to this PR

tien-tong

I've tested the orig BABS project layout with real data and everything looks good!

I've also tested a BIDS-study layout config yaml file for freesurfer-post (my commit). This section from the BABSProject/README.md need to be changed to sourcedata for BIDS layout

## Dataset structure

- All inputs (i.e. building blocks from other sources) are located in
  `inputs/`.

Other than that, everything looks good to me!

tien-tong · 2026-05-21T16:12:23Z

+
+        analysis_dir = cfg.get('analysis_path', 'analysis')
+        self.analysis_path = op.normpath(op.join(self.project_root, analysis_dir))
        self._analysis_datalad_handle = None


analysis_path, input_ria_path, and output_ria_path are joined with project_root without validation. Path values are not validated, so absolute paths, .., "", and "." can escape or collapse into unintended locations

if a config uses analysis_path: ../outside, BABS may create/write outside the project root, while cleanup only deletes project_root.

Suggested fix: centralize path resolution with Path.resolve(), reject empty/non-string paths, reject absolute paths unless explicitly supported, and require resolved paths to stay inside project_root.

Added the _resolve_subpath function that should cover all your cases

tien-tong · 2026-05-21T16:18:02Z

            system,
            project_root=op.dirname(self.analysis_path),
+            analysis_dir=op.basename(self.analysis_path),
            shared_group_mode=shared_group_mode,


nested analysis_path are reconstructed from dirname + basename

but PROJECT_ROOT in the generated environment still means the real project root.

CONTAINER_SHARED="${PROJECT_ROOT}/{{ analysis_dir }}/${CONTAINER_JOB}"

This is fragile and confusing, and breaks easily for external callers or future script changes.

Suggested fix: pass a single analysis_path or analysis_relpath into the template. Rename shell variables to what they mean, e.g. BABS_PROJECT_ROOT and BABS_ANALYSIS_PATH, and stop reconstructing nested paths from basename.

@tien-tong - can you take a look at my changes in this commit to see if this is what you had in mind?

tien-tong · 2026-05-21T16:37:30Z


-    babs_proj = BABSBootstrap(project_root)
+    babs_proj = BABSBootstrap(project_root, container_config=container_config)
    try:


Direct API usage like silently ignores custom analysis_path.

class BABSBootstrap(BABS): def __init__(self, project_root, container_config=None): super().__init__(project_root, container_config=container_config)

Suggested fix: either require container_config in BABSBootstrap.__init__ or resolve init paths inside babs_bootstrap() before any path-dependent work.

I don't want to make it require, but since we are already resolving container_config is resolved in __init__, I mage this argument optional in babs_bootstrap, see this commit

tien-tong · 2026-05-21T16:41:29Z


-cmd_template: '{{ submit_head }} {{ env_flags }} {{ name_flag_str }}{{ job_name }} {{ eo_args }} {{ array_args }} {% if test %}{{ babs.analysis_path }}/code/check_setup/call_test_job.sh{% else %}{{ babs.analysis_path }}/code/participant_job.sh {{ dssource }} {{ pushgitremote }} {{ babs.job_submit_path_abs }} {{ babs.project_root }}{% endif %}'
+cmd_template: '{{ submit_head }} {{ env_flags }} {{ name_flag_str }}{{ job_name }} {{ eo_args }} {{ array_args }} {% if test %}{{ babs.analysis_path }}/code/check_setup/call_test_job.sh{% else %}{{ babs.analysis_path }}/code/participant_job.sh {{ dssource }} {{ pushgitremote }} {{ babs.job_submit_path_abs }} {{ babs.analysis_root }}{% endif %}'
 job_name_template: '{{ job_name }}'


generated command strings are split with plain .split().

job_id = sbatch_get_job_id(cmd.split(), analysis_path)

spaces in project paths, RIA paths, or analysis paths will break job submission.

Suggested fix: build argv as structured data, or render shell-quoted strings and parse with shlex.split().

i think this is a bigger change, that is not related to this PR, can we do it in a separate PR?

djarecka · 2026-06-16T12:54:23Z

@tien-tong - thank you so much for review. I have been traveling and was not able to work on this before the conference. Now our HPC is down, so I will try to answer your comments next week.

Trim the RIA .gitignore comment to its essential skip-condition, and explain why babs_bootstrap re-derives the project paths (the Python-API / test-fixture construction pattern behind tien-tong's thread-7 concern). Both surfaced in Austin Macdonald's review of the fixup; the re-derive rationale answers tien-tong's original review on PennLINC#369.

…ve paths

…reconstructions

…ding self.container_config in init; adding an additional test for this option

asmacdo · 2026-07-10T14:06:07Z


 for CONTAINER_JOB in "${CONTAINER_IMAGE_PATHS[@]}"; do
-  CONTAINER_SHARED="${PROJECT_ROOT}/analysis/${CONTAINER_JOB}"
+  CONTAINER_SHARED="{{ analysis_path }}/${CONTAINER_JOB}"


I like your solution a lot more than the one I hacked up, it simplified out nicely!

Claude noticed that this leaves some dangling dead code. Now that analysis_path is templated in, $PROJECT_ROOT isnt used anymore and the stuff that feeds it should go and this simplifies even more. (I found that surprising, but glancing around, it looks like claude is right.)

babs/templates/participant_job.sh.jinja2

16-17 PROJECT_ROOT set, read by nothing.

babs/templates/job_submit.yaml.jinja2

5 {{ babs.analysis_root }} passed as $4, feeds only that.

babs/container.py

268: only needs the lockfile part, the PROJECT_ROOT can go

177: stale docstring

(project_root arg is threaded through here in other places and can go)

babs/base.py

167 — self.analysis_root exists only to be that $4.

generate_submit_script.py:

also doesnt need project_root anymore

thanks @asmacdo ! I cleaned the code with claude

analysis_path is now baked into the script at render time, so PROJECT_ROOT is never read at runtime. Remove the full chain: self.analysis_root, $4/PROJECT_ROOT in the template, babs.analysis_root from job_submit.yaml, ,PROJECT_ROOT= from sbatch --export, and project_root threaded through generate_bash_participant_job and generate_submit_script. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

tien-tong · 2026-07-11T01:14:13Z

This PR should address this #392

asmacdo · 2026-07-13T17:00:31Z

Implemented #392 as a PR against this PR djarecka#3

Happy to have this go in together, or as a followup. Sidenote: I fixed up the ruff check on that branch as well in a separate commit.

Add custom analysis_path support: configurable analysis dir, .babs di…

9bb4256

…rectory for config/RIA storage

ruff fix

3535cd8

tien-tong self-requested a review April 23, 2026 13:04

tien-tong reviewed Apr 23, 2026

View reviewed changes

Comment thread babs/container.py Outdated

tien-tong added 3 commits April 23, 2026 09:27

Merge branch 'main' into structurechange_1

24570d1

Apply Ruff formatting to babs/base.py

6295329

fix test

42a8cea

tien-tong linked an issue Apr 24, 2026 that may be closed by this pull request

Are RIAs necessary? #357

Closed

asmacdo mentioned this pull request May 21, 2026

Default .gitattributes should be BIDS-friendly #378

Open

tien-tong self-requested a review May 21, 2026 15:59

asmacdo approved these changes May 21, 2026

View reviewed changes

asmacdo mentioned this pull request May 21, 2026

Make logs/ entry in .gitignore optional #372

Open

tien-tong added the enhancement New feature or request label May 22, 2026

This was referenced Jun 4, 2026

Output-structure gaps (today → target) asmacdo/mechababs#4

Open

contain babs config within result code/ asmacdo/mechababs#7

Open

Epic: upstream babs dependencies (M2) asmacdo/mechababs#38

Open

Add freesurfer-post BIDS study layout example

39b58f7

tien-tong requested changes Jun 8, 2026

View reviewed changes

asmacdo mentioned this pull request Jul 1, 2026

iterate: datalad save the campaign superdataset after each tick asmacdo/mechababs#52

Open

This was referenced Jul 8, 2026

Add pre_run/post_run hooks; make zipping an optional built-in hook #381

Open

Add option to fit BIDS-study layout (refresh of #369) #390

Open

djarecka added 5 commits July 9, 2026 11:53

checking the analysis and ria paths to ensure there are proper relati…

b559a95

…ve paths

changing the template to use directly analysis_path and removing the …

450037e

…reconstructions

making container_config optional in the babs_bootstrap method, but ad…

af6bd9e

…ding self.container_config in init; adding an additional test for this option

reverting changes to conftest to keep the tests structure

71d1c19

resolving the conflict

a27f07d

updating test

d2655eb

djarecka requested review from asmacdo and tien-tong July 9, 2026 23:13

asmacdo reviewed Jul 10, 2026

View reviewed changes

asmacdo mentioned this pull request Jul 10, 2026

Output folder name hardcoded to outputs/ — make it derivatives/ (or configurable) for BIDS-study validity #392

Open

tien-tong linked an issue Jul 11, 2026 that may be closed by this pull request

Output folder name hardcoded to outputs/ — make it derivatives/ (or configurable) for BIDS-study validity #392

Open

asmacdo mentioned this pull request Jul 13, 2026

Add configurable output folder via output_dir config key djarecka/babs#3

Open

Uh oh!

Conversation

djarecka commented Apr 17, 2026

Uh oh!

codecov-commenter commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

asmacdo left a comment

Choose a reason for hiding this comment

Uh oh!

djarecka commented May 28, 2026

Uh oh!

tien-tong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

djarecka commented Jun 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tien-tong commented Jul 11, 2026

Uh oh!

asmacdo commented Jul 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Apr 17, 2026 •

edited

Loading