Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
yibeichan opened this issue Aug 24, 2023 · 15 comments
Labels
documentation Improvements or additions to documentation enhancement New feature or request

Comments

@yibeichan
Copy link
Collaborator

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
Copy link
Collaborator Author

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

@mattcieslak
Copy link
Collaborator

unfortunately it looks like it. This bug is awful

@yibeichan
Copy link
Collaborator Author

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

@yibeichan yibeichan changed the title midthickness (freesurfer) failed during fmriprep due to .git in the path [FIX] path generated by BABS is too long for freesurfer to function/run properly Sep 1, 2023
@zhao-cy
Copy link
Collaborator

zhao-cy commented Sep 21, 2023

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.

@zhao-cy zhao-cy closed this as completed Sep 21, 2023
@yibeichan
Copy link
Collaborator Author

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
Copy link
Collaborator

zhao-cy commented Sep 27, 2023

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?

@zhao-cy zhao-cy reopened this Sep 27, 2023
@yibeichan
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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

@zhao-cy
Copy link
Collaborator

zhao-cy commented Sep 28, 2023

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

btw, this is where related to binding, right? and we need more than one -B

babs/babs/utils.py

Lines 431 to 432 in 6d6e5d9

# Get env var's value, to be used for binding `-B` in `singularity run`:
env_var_value = os.getenv(env_var_name)

@zhao-cy
Copy link
Collaborator

zhao-cy commented Sep 28, 2023

Notes:

  • There will be multiple functions to be changed - need to spend some time to identify them;
  • also we need to consider how to handle the case of more than one input dataset (e.g., FreeSurfer ingressed one).
  • For docs: can make a webpage of FAQs or so to explain the newly defined paths. (sorry I had a bit misunderstanding earlier)

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

@zhao-cy zhao-cy added documentation Improvements or additions to documentation enhancement New feature or request labels Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants