fix parallel problem and quantifyThalamicNuclei.sh

[AndysWorth]

This is a patch to recon-all that removes parallelism in two places to remove race conditions that sometimes lead to failure. One error message is "Cannot find rh.white.H". See here. It also has the updated version of quantifyThalamicNuclei.sh see here.

% diff recon-all recon-all.bak
3474,3475c3474,3481
<     if($RunIt) $fs_time $cmd |& tee -a $LF
<     if($status) goto error_exit;
---
>     if($DoParallel) then
>       set CMDF = mris_curvature_white_${hemi}.cmd
>       echo "$cmd" > $CMDF
>       set CMDFS = ( $CMDFS $CMDF )
>     else
>       if($RunIt) $fs_time $cmd |& tee -a $LF
>       if($status) goto error_exit;
>     endif
3484,3488c3490,3503
<       if($RunIt) then
<         $cmd1 | tee -a $LF
<         if($status) goto error_exit
<         $cmd2 | tee -a $LF
<         if($status) goto error_exit
---
>       if($DoParallel) then
>         set CMDF = rm_curvature_white_${hemi}.$suffix.cmd
>         echo "$cmd1" > $CMDF
>         set CMDFS = ( $CMDFS $CMDF )
>         set CMDF = ln_curvature_white_${hemi}.$suffix.cmd
>         echo "$cmd2" > $CMDF
>         set CMDFS = ( $CMDFS $CMDF )
>       else
>         if($RunIt) then
>           $cmd1 | tee -a $LF
>           if($status) goto error_exit
>           $cmd2 | tee -a $LF
>           if($status) goto error_exit
>         endif
4202c4217
< #  if($OMP_NUM_THREADS > 1) set cmd = ($cmd --parallel) # just hemis
---
>   if($OMP_NUM_THREADS > 1) set cmd = ($cmd --parallel) # just hemis

[Shotgunosine]

@AndysWorth, are you able to look into why the singularity build is failing?https://app.circleci.com/pipelines/github/ReproNim/neurodocker/148/workflows/454ff119-e13c-4370-b9e8-41d1e7fc9bfd/jobs/1529

[AndysWorth]

I added this patch to master and the master branch was failing already. There is a lot of activity on other branches. Should I close this pull request and add it somewhere else? I guess it depends on what version/branch of neurodocker you want to use.

[Shotgunosine]

@kaczmarj, @satra, should this PR be submitted to a different branch?

[satra]

@Shotgunosine and @AndysWorth - this should really go into freesurfer rather than neurodocker. we don't want to necessarily patch upstream tools here, just provide a way to build it.

this specific thing sounds like the neurodocker generate command simply appending this file into the freesurfer distribution using the standard add/copy commands in docker/singularity.

[Shotgunosine]

@satra, that's what it's doing, but Freesurfer's current version has some small broken bits and as far as I know, has no plans on releasing an update to fix these issues in the near future. The advice from the mailing list was to make these tweaks manually. In my opinion, that makes these tweaks part of the Freesurfer install process.

[satra]

perhaps i'm missing something. this PR distributes it as a patch to freesurfer, which we want to avoid. whereas i would make patching a separate action in the neurodocker generate command itself, pointing to a commitish in some git repo, so that even the generation is potentially reproducible.

[Shotgunosine]

Ah, ok, I think I understand. Make a little repo with a bash script that executes the patch. Then add an action to the neurodocker generate command that pulls that repo and executes the script to patch freesurfer. @Satra, do I have that right? @AndysWorth, does that make sense?

[satra]

yup. that's it.

[AndysWorth]

I can set up a repo. Should it be a script that does the patch or should it just be the patched recon-all file? And then the fixed version of quantifyThalamicNuclei.sh could be pulled in from its original source as well?

[Shotgunosine]

I think a script that applies the patch.

On Thu, Apr 1, 2021 at 12:08 PM Andrew J. Worth @.***> wrote:

I can set up a repo. Should it be a script that does the patch or should it just be the patched recon-all file? And then the fixed vesion of quantifyThalamicNuclei.sh could be pulled in from it's original source as well?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ReproNim/neurodocker/pull/384#issuecomment-812011864, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABILLGCFPRQZ7MXNDHYJRYDTGSLABANCNFSM4ZEJ474A .

[AndysWorth]

Sounds good.

[AndysWorth]

https://github.com/AndysWorth/recon-all-v7.1.1-parallel-patch

[kaczmarj]

thank you all!

@AndysWorth - we can probably close this pull request. but it would be helpful if you included an example of how to use the path with a neurodocker generate command. future users will likely find it useful.

you can include the command in another comment on this thread.

[Shotgunosine]

Oh, I thought we’d add another step in the neurodocker install for free surfer, maybe controlled by a parameter, that would pull the patch repo and apply it. Isn’t that what you were suggesting @Satra?

I can try to work on that tomorrow or this weekend.

edit Scratch that, I now realize that you said to add it as an action to neurodcker generate so it'd be something like this:

docker run --rm repronim/neurodocke generate docker \
  --base ubuntu:xenial \
  --pkg-manager apt \
  --install tcsh bc tar libgomp1 perl-modules wget curl \
    libsm-dev libx11-dev libxt-dev libxext-dev libglu1-mesa libpython2.7-stdlib\
  --freesurfer version=7.1.1 install_path=/opt/freesurfer \
  --run git clone https://github.com/AndysWorth/recon-all-v7.1.1-parallel-patch \
      && patch $FREESURFER_HOME/bin/recon-all recon-all-v7.1.1-parallel-patch/parallel.patch \
      && patch $FREESURFER_HOME/bin/quantifyThalamicNuclei.sh recon-all-v7.1.1-parallel-patch/quantifyThalamicNuclei.patch

I'll make sure that works and replay back to this PR.

On Thu, Apr 1, 2021 at 4:40 PM Jakub Kaczmarzyk @.***> wrote:

thank you all!

@AndysWorth https://github.com/AndysWorth - we can probably close this pull request. but it would be helpful if you included an example of how to use the path with a neurodocker generate command. future users will likely find it useful.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ReproNim/neurodocker/pull/384#issuecomment-812159701, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABILLGBGFOTRNSZX3BRVAF3TGTK5TANCNFSM4ZEJ474A .

[satra]

i was thinking of something super simple. either something like this

neurodocker generate docker -b ubuntu -p apt --freesurfer version=7.1.1 \
 --run-bash "curl -sSL https://path/to/7.1.1-patch.sh | bash -"

or in your own repo

neurodocker generate docker -b ubuntu -p apt --freesurfer version=7.1.1 \
 --copy path1 /path/to/recon-all --copy quantifyThalamicNuclei.sh /location/to/where this file should go

[Shotgunosine]

Cool, thanks!

On Thu, Apr 1, 2021 at 5:42 PM Satrajit Ghosh @.***> wrote:

i was thinking of something super simple. either something like this

neurodocker generate docker -b ubuntu -p apt --freesurfer version=7.1.1
--run-bash "curl -sSL https://path/to/7.1.1-patch.sh | bash -"

or in your own repo

neurodocker generate docker -b ubuntu -p apt --freesurfer version=7.1.1
--copy path1 /path/to/recon-all --copy quantifyThalamicNuclei.sh /location/to/where this file should go

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ReproNim/neurodocker/pull/384#issuecomment-812187770, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABILLGEEJXIJ2MQ53EM4QJDTGTSDJANCNFSM4ZEJ474A .