Skip to content

Parameterize FreeSurfer license path and clean up README MIT-isms#5

Open
yibeichan wants to merge 3 commits into
mainfrom
fix/fs-license-and-readme-polish
Open

Parameterize FreeSurfer license path and clean up README MIT-isms#5
yibeichan wants to merge 3 commits into
mainfrom
fix/fs-license-and-readme-polish

Conversation

@yibeichan

Copy link
Copy Markdown
Collaborator

Summary

  • Fixes the hardcoded FreeSurfer license path in config_freesurfer-nidm.yaml that was causing the BABS workflow to fail at job runtime for any user outside MIT ORCD (the singularity bind to /orcd/scratch/bcs/001/yibei/prettymouth_babs/license.txt doesn't resolve on other clusters — reported recently from a TSCC run).
  • Wires FS_LICENSE through the FreeSurfer wrapper script with fail-fast validation: the script now aborts at submission time with a clear error if FS_LICENSE is unset or missing, rather than failing silently inside every SLURM job.
  • Cleans up remaining MIT-isms in the README that would mislead new users about output paths and post-processing layout, and adds a "what this is / where the containers come from" opener.

What changed

Code

  • config_freesurfer-nidm.yaml: --fs-license-file now uses the ${FS_LICENSE} placeholder, picked up by the existing babs_prepare_yaml_config substitution machinery.
  • freesurfer-nidm_babs_script.sh: validates FS_LICENSE is set and points at an existing file before babs init; passes FS_LICENSE=${FS_LICENSE} into the YAML substitution call.

README

  • New opening paragraph explaining what the wrapper scripts actually orchestrate (container lookup → YAML prep → babs init → submit), plus links to the three ReproNim BIDS App container repos so users know where the .sif files come from.
  • New Prerequisites checklist covering BABS env, Apptainer, .env setup, .sif build, FreeSurfer license (with the registration link), and DataLad inputs.
  • .env template:
    • Drops the unused DATA_DIR variable (confirmed unreferenced anywhere in the repo).
    • Adopts the shared-SCRATCH_DIR-parent idea from Clarify .env contract in README and align variable semantics with wrapper scripts #4, but in correct bash syntax — the merged form (SCRATCH_DIR = '...' with spaces, and SCRATCH_DIR_ANTS= SCRATCH_DIR + 'ants...' Python-style concat) would have silently mis-set the per-app paths when sourced. Now uses SCRATCH_DIR_ANTS=\"\${SCRATCH_DIR}/ants_bidsapp_babs\".
    • Adds FS_LICENSE with a "FreeSurfer-NIDM pipeline only" comment.
  • Replaces hardcoded /orcd/scratch/bcs/001/yibei/simple2/... paths in Output Structure, Manual BABS Commands, and Post-Processing with \${SCRATCH_DIR_*} placeholders and <dataset> / <site> / <RUN_DATE> tokens.
  • Generalizes the Git Safe Directories example to use \${DATALAD_SET_DIR}.
  • Folds SLURM partition and Incremental NIDM notes into the Configuration Files section where they belong; moves git-safe-dirs under a new Caveats heading; drops the duplicate FreeSurfer-license note (now covered in Prerequisites).

Test plan

  • Verify on TSCC (the original failure site): clone, set FS_LICENSE in .env, run ./freesurfer-nidm_babs_script.sh <site> <study>, confirm no license-related errors.
  • Unset FS_LICENSE or point it at a missing file — script should abort before submitting jobs.
  • Run the ANTs and MRIQC scripts to confirm they still work (no changes intended for those paths, but the merge touched the README they reference).
  • Render the README on GitHub and sanity-check link targets and code blocks.

🤖 Generated with Claude Code

yibeichan and others added 3 commits May 13, 2026 10:44
The FreeSurfer license path was hardcoded to
/orcd/scratch/bcs/001/yibei/prettymouth_babs/license.txt in
config_freesurfer-nidm.yaml, causing the BABS workflow to fail at job
runtime for any user outside that environment (the singularity bind
fails because the source path doesn't exist).

- Replace the hardcoded path with ${FS_LICENSE} in the YAML so the
  existing babs_prepare_yaml_config substitution machinery handles it.
- Wire FS_LICENSE through freesurfer-nidm_babs_script.sh: validate it
  is set and the file exists before submitting any jobs, then pass it
  into the YAML substitution call.
- Document FS_LICENSE in the README .env block and add a Prerequisites
  section covering BABS env, apptainer, .env contents, .sif files,
  the FreeSurfer license, and DataLad inputs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous README told users what scripts existed but not what the
repo is for, and hardcoded MIT ORCD paths in Output Structure, Manual
BABS Commands, and Post-Processing examples — the same class of bug
that broke FS_LICENSE for non-MIT users.

Changes:
- Add an opening paragraph explaining what the wrapper scripts do
  (container lookup, YAML prep, babs init + submit), and link out to
  the three ReproNim BIDS App container repos so users know where
  the .sif files come from.
- Point Prerequisites item 4 (container .sif files) at the same repos.
- Replace hardcoded /orcd/scratch/bcs/001/yibei/simple2/... in Output
  Structure, Manual BABS Commands, and Post-Processing with
  ${SCRATCH_DIR} / ${SCRATCH_DIR_*} placeholders and
  <dataset>/<site>/<RUN_DATE> tokens.
- Generalize the Git Safe Directories example to use ${DATALAD_SET_DIR}.
- Rewrite the .env explanatory paragraph in neutral docs voice.
- Restructure "Important Notes": fold SLURM partition and incremental
  NIDM into Configuration Files (where they belong), drop the
  duplicate FreeSurfer license note (now in Prerequisites), and keep
  the git safe-directory caveat under a new "Caveats" heading.

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

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request enhances the BABS wrapper scripts by improving documentation and parameterizing the FreeSurfer license path. The README.md was updated with detailed prerequisites and environment variable usage, while the FreeSurfer script now includes fail-fast validation for the license file. Review feedback recommends resolving the license path to an absolute path, explicitly binding it for Singularity containers, and ensuring the .env file is sourced in documentation examples.

Comment on lines +51 to +59
# Validate FreeSurfer license (fail fast before submitting jobs)
if [ -z "${FS_LICENSE:-}" ]; then
echo "ERROR: FS_LICENSE is not set. Add 'FS_LICENSE=/path/to/license.txt' to .env" >&2
exit 1
fi
if [ ! -f "$FS_LICENSE" ]; then
echo "ERROR: FreeSurfer license file not found at FS_LICENSE=$FS_LICENSE" >&2
exit 1
fi

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

It is recommended to resolve FS_LICENSE to an absolute path. This ensures the path remains valid if the script changes the current working directory (as seen on line 82) and provides a reliable path for container volume mounting. Using realpath is a standard way to achieve this in modern Linux environments.

Suggested change
# Validate FreeSurfer license (fail fast before submitting jobs)
if [ -z "${FS_LICENSE:-}" ]; then
echo "ERROR: FS_LICENSE is not set. Add 'FS_LICENSE=/path/to/license.txt' to .env" >&2
exit 1
fi
if [ ! -f "$FS_LICENSE" ]; then
echo "ERROR: FreeSurfer license file not found at FS_LICENSE=$FS_LICENSE" >&2
exit 1
fi
# Validate FreeSurfer license (fail fast before submitting jobs)
if [ -z "${FS_LICENSE:-}" ]; then
echo "ERROR: FS_LICENSE is not set. Add 'FS_LICENSE=/path/to/license.txt' to .env" >&2
exit 1
fi
if [ ! -f "$FS_LICENSE" ]; then
echo "ERROR: FreeSurfer license file not found at FS_LICENSE=$FS_LICENSE" >&2
exit 1
fi
# Resolve to absolute path to ensure it remains valid after directory changes
FS_LICENSE=$(realpath "$FS_LICENSE")

bids_app_args:
"$SUBJECT_SELECTION_FLAG": "--participant_label"
--fs-license-file: "/orcd/scratch/bcs/001/yibei/prettymouth_babs/license.txt"
--fs-license-file: "${FS_LICENSE}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To ensure the FreeSurfer license file is accessible inside the container, especially when using --no-home (line 24), it should be explicitly bound. If the license is located outside of standard auto-bind paths (like /scratch), the pipeline will fail. Consider adding a bind point to the singularity_args section, for example: - -B ${FS_LICENSE}:${FS_LICENSE}:ro.

Comment thread README.md
Comment on lines +144 to +145
# Navigate to the BABS project directory
cd "${SCRATCH_DIR_ANTS}/<dataset>_<RUN_DATE>/ants-nidm_bidsapp_<site>_<RUN_DATE>"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For these manual commands to work in a terminal session, the user must first source the environment variables so that variables like ${SCRATCH_DIR_ANTS} are defined. Adding a source .env line to the example makes it more complete and easier for users to follow.

Suggested change
# Navigate to the BABS project directory
cd "${SCRATCH_DIR_ANTS}/<dataset>_<RUN_DATE>/ants-nidm_bidsapp_<site>_<RUN_DATE>"
# Navigate to the BABS project directory
source .env
cd "${SCRATCH_DIR_ANTS}/<dataset>_<RUN_DATE>/ants-nidm_bidsapp_<site>_<RUN_DATE>"

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.

1 participant