PennLINC / babs

BIDS App Bootstrap (BABS)
https://pennlinc-babs.readthedocs.io
MIT License
7 stars 7 forks source link

[FIX] path generated by BABS is too long for freesurfer to function/run properly #138

Open yibeichan opened 1 year ago

yibeichan commented 1 year ago

as mentioned in this issue #137 i'm using fmriprep thru babs and encountered problem with midthickness.

Cmdline:
    mris_expand -pial /om2/scratch/Sun/yibei/budapest/output/job-32194722-sub-sid000030/ds/.git/tmp/wkdir/fmriprep_23_1_wf/single_subject_sid000030_wf/anat_preproc_wf/surface_recon_wf/gifti_surface_wf/midthickness/mapflow/_midthickness0/lh.pial -thickness -thickness_name /om2/scratch/Sun/yibei/budapest/output/job-32194722-sub-sid000030/ds/.git/tmp/wkdir/fmriprep_23_1_wf/single_subject_sid000030_wf/anat_preproc_wf/surface_recon_wf/gifti_surface_wf/midthickness/mapflow/_midthickness0/lh.thickness /om2/scratch/Sun/yibei/budapest/output/job-32194722-sub-sid000030/ds/.git/tmp/wkdir/fmriprep_23_1_wf/single_subject_sid000030_wf/anat_preproc_wf/surface_recon_wf/gifti_surface_wf/midthickness/mapflow/_midthickness0/lh.white 0.5 /om2/scratch/Sun/yibei/budapest/output/job-32194722-sub-sid000030/ds/.git/tmp/wkdir/fmriprep_23_1_wf/single_subject_sid000030_wf/anat_preproc_wf/surface_recon_wf/gifti_surface_wf/midthickness/mapflow/_midthickness0/lh.midthickness
Stdout:
    reading pial surface from /om2/scratch/Sun/yibei/budapest/output/job-32194722-sub-sid000030/ds/.git/tmp/wkdir/fmriprep_23_1_wf/single_subject_sid000030_wf/anat_preproc_wf/surface_recon_wf/gifti_surface_wf/midthickness/mapflow/_midthickness0/lh.pial
    using distance as a % of thickness
    using thickness file /om2/scratch/Sun/yibei/budapest/output/job-32194722-sub-sid000030/ds/.git/tmp/wkdir/fmriprep_23_1_wf/single_subject_sid000030_wf/anat_preproc_wf/surface_recon_wf/gifti_surface_wf/midthickness/mapflow/_midthickness0/lh.thickness
    expanding surface /om2/scratch/Sun/yibei/budapest/output/job-32194722-sub-sid000030/ds/.git/tmp/wkdir/fmriprep_23_1_wf/single_subject_sid000030_wf/anat_preproc_wf/surface_recon_wf/gifti_surface_wf/midthickness/mapflow/_midthickness0/lh.white by 50.0% of thickness and writing it to /om2/scratch/Sun/yibei/budapest/output/job-32194722-sub-sid000030/ds/.git/tmp/wkdir/fmriprep_23_1_wf/single_subject_sid000030_wf/anat_preproc_wf/surface_recon_wf/gifti_surface_wf/midthickness/mapflow/_midthickness0/lh.midthickness
    reading thickness...
Stderr:
    *** buffer overflow detected ***: terminated
    Aborted (core dumped)
Traceback:
    RuntimeError: subprocess exited with code 134.

I reported the details about this error on neurostars (I thought it was a fmriprep/freesurfer issue) it turns out that the .git folder having a period in it is causing issues

@zhao-cy you have tested BABS with freesurfer before, right? did you encounter anything similar?

yibeichan commented 1 year ago

ah, is it because the path is too long for freesurfer? @mattcieslak

mattcieslak commented 1 year ago

unfortunately it looks like it. This bug is awful

yibeichan commented 1 year ago

oh no... that doesn't sound good... let me talk to @djarecka tomorrow first and see whether we can come up with some plans to fix it. will keep you updated

zhao-cy commented 1 year ago

I'm closing the issue for now, as it seems the lengthy path is mainly generated by fMRIPrep, so it's probably out of the scope of BABS.

yibeichan commented 1 year ago

some updates for this one, see my post here, maybe BABS can do something to make this path shorter as it does in The Script?

zhao-cy commented 1 year ago

Hi Yibei! @mattcieslak suggested this: since the issue is when running singularity run, maybe bind the main folder to singularity, so that it won't appear long within singularity? i.e., your original path: /om2/scratch/Sun/yibei/budapest/output/job-32194541-sub-sid000005/ds/.git/tmp/wkdir/fmriprep_23_1_wf/single_subject_sid000005_wf/anat_preproc_wf/surface_recon_wf/gifti_surface_wf/midthickness/mapflow/_midthickness0

And for the singularity run command in analysis/code/fmriprep*_zip.sh, add a line of -B /om2/scratch/Sun/yibei/budapest/output/job-32194541-sub-sid000005/ds:/data, i.e., bind the path as a folder called /data or so within the singularity. The Job ID can be retrieved by some env variable, and subject ID is also available in that script. Please make sure you datalad save the script before moving forward.

After changing this, you might or might not need to change other relative paths in the scripts - may be try out and see?

yibeichan commented 1 year ago

Hi Chenying! Yes, I agree, it should be something to do with binding. See the code (original) below, it already bound ${PWD}, which is something like /om2/scratch/Sun/yibei/budapest/output/job-32194541-sub-sid000005/ds/

if I change that line to -B ${PWD}:/any_dirname \, I get fmriprep: error: Path does not exist: <inputs/data/BIDS>.

mkdir -p ${PWD}/.git/tmp/wkdir
singularity run --cleanenv \
    -B ${PWD} \
    -B /om2/user/$(whoami)/images/license.txt:/SGLR/FREESURFER_HOME/license.txt \
    containers/.datalad/environments/fmriprep-23-1-4/image \
    inputs/data/BIDS \
    outputs/fmriprep \
    participant \
    -w ${PWD}/.git/tmp/wkdir \
    --stop-on-first-crash \
    --nthreads 16 \
    --omp-nthreads 8 \
    --mem-mb 40000 \
    --fd-spike-threshold 0.9 \
    --dvars-spike-threshold 5 \
    --fs-license-file /SGLR/FREESURFER_HOME/license.txt \
    --skip-bids-validation \
    --output-layout legacy \
    --force-bbr \
    --notrack \
    --cifti-output 91k \
    -v -v \
    --participant-label "${subid}"
mattcieslak commented 1 year ago

You'd need to change your command to

singularity run --cleanenv \
    -B ${PWD}/inputs/data/BIDS:/bids_data \
        -B ${PWD} \
        -B ${PWD}/.git/tmp/wkdir:/work \
    -B /om2/user/$(whoami)/images/license.txt:/SGLR/FREESURFER_HOME/license.txt \
    containers/.datalad/environments/fmriprep-23-1-4/image \
    /bids_data \
    outputs/fmriprep \
    participant \
    -w /work \
    --stop-on-first-crash \
    --nthreads 16 \
    --omp-nthreads 8 \
    --mem-mb 40000 \
    --fd-spike-threshold 0.9 \
    --dvars-spike-threshold 5 \
    --fs-license-file /SGLR/FREESURFER_HOME/license.txt \
    --skip-bids-validation \
    --output-layout legacy \
    --force-bbr \
    --notrack \
    --cifti-output 91k \
    -v -v \
    --participant-label "${subid}"
yibeichan commented 1 year ago

thanks! it works!

before changing, fmriprep node path is /om2/scratch/tmp/yibei/budapest_babs/job-32807164-sub-sid000009/ds/.git/tmp/wkdir/fmriprep_23_1_wf/single_subject_sid000009_wf/anat_preproc_wf/brain_extraction_wf/full_wm after this change: /work/fmriprep_23_1_wf/single_subject_sid000010_wf/anat_preproc_wf/brain_extraction_wf/full_wm

I think this is the solution then!

should we make a PR for babs? I can work on it if @zhao-cy tells me which file I should look into.

mattcieslak commented 1 year ago

I think it might be our only option to ensure the filenames are ok, but it does have some negative side effects.

For example, if someone looks through their error logs, they'll see an error regarding a file in /work/something and will not know where this file is on their file system. If we go this route we should definitely add some documentation to help explain the new file paths!

yibeichan commented 1 year ago

sounds good! (now thinking where do we want to put this documentation, do we want a separate FAQ page?

zhao-cy commented 1 year ago

Hi Yibei! How about we create a new webpage, called What if something goes wrong? or so, in parallel with other docs (like overview, walkthrough), and include potential "failures" when using BABS? Let me know if you'd like to start that or I can also do it later.

yibeichan commented 1 year ago

sound good! I can do it, mostly likely next week unless I get bored tomorrow then I'll do it tomorrow (very low probability haha)

yibeichan commented 1 year ago

btw, this is where related to binding, right? and we need more than one -B https://github.com/PennLINC/babs/blob/6d6e5d9dbe1c3890a5a64c02ce7c12c53c8766b5/babs/utils.py#L431-L432

zhao-cy commented 1 year ago

Notes:

Current plan: enhance this before the next major release of BABS.