search

bids-apps / freesurfer

BIDS app wrapping recon-all from FreeSurfer
Apache License 2.0
40 stars 35 forks source link

WIP: Add Freesurfer 7 #67

Closed Shotgunosine closed 1 year ago

Shotgunosine commented 3 years ago

I added Freesurfer 7 and some additional jobs on circleci to test and deploy it. Circlci config will likely need to be fixed before this is complete.

Shotgunosine commented 3 years ago

@PeerHerholz and @andysworth this resolves #57 with the potential exception of the replacement recon-all script. I'd be happy to get your feedback on it.

PeerHerholz commented 3 years ago

Ahoi hoi @Shotgunosine,

thx for the fantastic work and effort. The same to you @andysworth. I checked the code and tests, LGTM. The plan re the recon-all script would be include it once we have it, eh?

Shotgunosine commented 3 years ago

Yeah, That's what I'm thinking.

PeerHerholz commented 3 years ago

Awesome! @andysworth, do you want to have a look as well? After that or otherwise, we could go ahead and merge!?

AndysWorth commented 3 years ago

Sorry for the slow response. This looks good to me. I just noticed that I got a response on the Freesurfer mailing list from Doug Greve:

Hi Andy, this is a bug introduced in 7.1.1, some kind of race condition. You can fix it by commenting out this line in recon-all if($OMP_NUM_THREADS > 1 && ! $LHonly && ! $RHonly) set cmd = ($cmd --parallel) # just hemis doug

Thanks for your strong work @Shotgunosine!

PeerHerholz commented 3 years ago

Cool, thanks @AndysWorth! @Shotgunosine should we go ahead and merge? WDYT re the recon-all script adaptation above?

Shotgunosine commented 3 years ago

We should add a sed command to the docker command, probably the best place would be in neurodocker. @AndysWorth, do you think you could open an issue there and submit a PR?

On Fri, Nov 13, 2020 at 8:42 AM Peer Herholz notifications@github.com wrote:

Cool, thanks @AndysWorth https://github.com/AndysWorth! @Shotgunosine https://github.com/Shotgunosine should we go ahead and merge? WDYT re the recon-all script adaptation above?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/BIDS-Apps/freesurfer/pull/67#issuecomment-726771234, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABILLGBJ27Y7SRH3TGMV4MTSPUZU7ANCNFSM4TI2FGUA .

AndysWorth commented 3 years ago

Will do!

PeerHerholz commented 3 years ago

Hi folks,

just checking: we can merge this, right? @Shotgunosine @AndysWorth

AndysWorth commented 3 years ago

Sorry, I was looking forward to making that contribution but it seems that I'm slowing this down. It looks like I won't have time until I'm off work for the holidays. @Shotgunosine if you want to go ahead with your suggestion in the meanwhile, that would be great. I know it's a small change but I don't feel confident to just do it without taking the time to make sure I fully understand what's going on.

Shotgunosine commented 3 years ago

I might not be able to get to it until around the same time period. Maybe @llevitis would be able to take a look

AndysWorth commented 3 years ago

I've installed the fix here at Flywheel -- once I have had a chance to run it a few times I'll submit it to neurodocker.

Shotgunosine commented 3 years ago

@AndysWorth have you had a chance to send the fix over to neurodocker?

AndysWorth commented 3 years ago

@Shotgunosine Sorry, no 😞 . There are two places that need patching in recon-all and the fixes have worked well at Flywheel so it's about time! I'll work on making the time. Thanks for the reminder.

Shotgunosine commented 3 years ago

@AndysWorth, sounds good. It'd be great if you're able to make the changes this week, I've got a bunch of sessions I need to run and I'd love to be able to use this.

AndysWorth commented 3 years ago

Okay @Shotgunosine, I will!

AndysWorth commented 3 years ago

Hey @Shotgunosine , I (finally) managed to patch Freesurfer 7.7.1 in neurodocker. See neurodocker PR#384. I did this exact same change to the official Freesurfer docker image version of 7.1.1 and it has been run successfully many times on various Flywheel instances. I have not tested this new neurodocker version, which is for a slightly different version of centos (6 vs. 7). Is this PR sufficient for you to give it a try? All it should do is prevent crashing.

AndysWorth commented 3 years ago

@Shotgunosine I've created a repository recon-all-v7.1.1-parallel-patch. Right now it just has the patch file that can be installed with patch /usr/local/freesurfer/bin/recon-all parallel.patch Is this enough? If not, what else should I do?

Shotgunosine commented 3 years ago

@AndysWorth, can you update the neurodocker PR to pull that repo run the bash script?

AndysWorth commented 3 years ago

@Shotgunosine I'm not familiar enough with neurodocker. Where would I do that?

AndysWorth commented 3 years ago

@Shotgunosine It sounds like they don't want the patch in neurodocker so the following should go somewhere around here?

curl -LJO https://github.com/AndysWorth/recon-all-v7.1.1-parallel-patch/tarball/master
tar zxf AndysWorth-recon-all-v7.1.1-parallel-patch-e5ff2fe.tar.gz
cd AndysWorth-recon-all-v7.1.1-parallel-patch-e5ff2fe
patch $FREESURFER_HOME/bin/recon-all parallel.patch 
patch $FREESURFER_HOME/bin/quantifyThalamicNuclei.sh quantifyThalamicNuclei.patch
cd ..
rm -rf AndysWorth-recon-all-v7.1.1-parallel-patch-e5ff2fe
Shotgunosine commented 3 years ago

I’ll see if I can use the script you wrote to get a command into neurodocker that’ll make them happy.

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

@Shotgunosine https://github.com/Shotgunosine It sounds like they don't want the patch in neurodocker so the following should go somewhere around here https://github.com/Shotgunosine/freesurfer/blob/fs_7/generate_freesurfer_images.sh#L96 ?

curl -LJO https://github.com/AndysWorth/recon-all-v7.1.1-parallel-patch/tarball/master tar zxf AndysWorth-recon-all-v7.1.1-parallel-patch-e5ff2fe.tar.gz cd AndysWorth-recon-all-v7.1.1-parallel-patch-e5ff2fe patch $FREESURFER_HOME/bin/recon-all parallel.patch patch $FREESURFER_HOME/bin/quantifyThalamicNuclei.sh quantifyThalamicNuclei.patch cd .. rm -rf AndysWorth-recon-all-v7.1.1-parallel-patch-e5ff2fe

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/BIDS-Apps/freesurfer/pull/67#issuecomment-812168436, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABILLGC46EKVCXGLUICFAPTTGTM4FANCNFSM4TI2FGUA .

Shotgunosine commented 3 years ago

With the new clarifications can you take care of it @Andysworth?

On Thu, Apr 1, 2021 at 5:15 PM Dylan Nielson @.***> wrote:

I’ll see if I can use the script you wrote to get a command into neurodocker that’ll make them happy.

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

@Shotgunosine https://github.com/Shotgunosine It sounds like they don't want the patch in neurodocker so the following should go somewhere around here https://github.com/Shotgunosine/freesurfer/blob/fs_7/generate_freesurfer_images.sh#L96 ?

curl -LJO https://github.com/AndysWorth/recon-all-v7.1.1-parallel-patch/tarball/master tar zxf AndysWorth-recon-all-v7.1.1-parallel-patch-e5ff2fe.tar.gz cd AndysWorth-recon-all-v7.1.1-parallel-patch-e5ff2fe patch $FREESURFER_HOME/bin/recon-all parallel.patch patch $FREESURFER_HOME/bin/quantifyThalamicNuclei.sh quantifyThalamicNuclei.patch cd .. rm -rf AndysWorth-recon-all-v7.1.1-parallel-patch-e5ff2fe

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/BIDS-Apps/freesurfer/pull/67#issuecomment-812168436, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABILLGC46EKVCXGLUICFAPTTGTM4FANCNFSM4TI2FGUA .

bpinsard commented 1 year ago

Hi, are there any blockers to merging that very old PR? It would be great to have latest versions integrated into repronim/containers. Anything I can do to help? Should we restart a PR from https://github.com/e-dads/freesurfer ? Thanks!

Shotgunosine commented 1 year ago

I haven't looked at this in a while, but I think this convo with Satra is where things were left off:

https://github.com/ReproNim/neurodocker/pull/384#issuecomment-812187770

@bpinsard, are you comfortable with the structure of everything to just make that change? It'd take me a while to reestablish context here.

Shotgunosine commented 1 year ago

Actually, looks like we could just jump to Freesurfer 7.4.1. Would that be alright?

bpinsard commented 1 year ago

I am not sure I have played enough with neurodocker to fully grasp the issue to be fixed. If I understand, that would mean:

The latest version would be great! I don't think there is really an interest in redeploying each older version.

Remi-Gau commented 1 year ago

I can have a look to try to fix some of the merge conflicts on the circle-CI config. I have tried to "standardize" across bids apps to make the maintenance easier.

Shotgunosine commented 1 year ago

I'm working on 7.4.1 now, to do that I've got to update the neurodocker version, which breaks some of the syntax. I'm working on it though.

bpinsard commented 1 year ago

I had the generate_freesurfer_images.sh running with the following changes: https://github.com/courtois-neuromod/freesurfer/tree/pr/67 (requires neurodocker master) If I understand correctly the patches are not needed with that version, right?

Shotgunosine commented 1 year ago

@Remi-Gau I took a first pass at resolving merge conflicts on the circleci config, but I've been out of the circleci game for a minute. Do you think you can fix the config now, or have I just made a mess of things?

Remi-Gau commented 1 year ago

had a quick look it seems to make sense but may need to go for a deeper dive: those diffs are hard to read

Remi-Gau commented 1 year ago

also seems that some file changes are showing in the PR when they are already in the default branch, no?

Shotgunosine commented 1 year ago

I tried to rebase the PR thinking that that would be more straightfoward, but may have just made a mess. Happy to do a force rollback if you think that'd be easier.

Remi-Gau commented 1 year ago

yeah if you can try to get a cleaner diff it'd be great

let me know if I can help

Shotgunosine commented 1 year ago

Alright, should be cleaner now.

Remi-Gau commented 1 year ago

yup indeed

Remi-Gau commented 1 year ago

also just thinking: if we agree that we want both freesurfer 6 and 7 to be available it should probably be mentioned in the readme how to pull each from docker hub. Does not have to be part of this PR: can be done once a release has been done and a tagged image has been pushed

Shotgunosine commented 1 year ago

I think we will want both freesurfer 6 and 7 available. HCP still uses freesurfer 6.

Remi-Gau commented 1 year ago

having a look locally at the whole config.

it looks very much like this is doing exactly the same thing for both images, no?

like literally running the same commands with both containers

sounds like this should probably be refactored, no?

Shotgunosine commented 1 year ago

Definitely, do you think you could refactor it? It should be straightforward, I just don't know circle's syntax very well.

Remi-Gau commented 1 year ago

Definitely, do you think you could refactor it? It should be straightforward, I just don't know circle's syntax very well.

yup I will give it a go

Remi-Gau commented 1 year ago

ok refactored things now let's see how this baby flies on the circle-ci side...

Remi-Gau commented 1 year ago

Note that given that one of the preivous build did not complete because we hit the circle CI "wall time" I think we may have to move to github actions that will give us 6 hours before we get cancelled. I am not as good with caching things in GHA action though...

https://app.circleci.com/pipelines/github/bids-apps/freesurfer/82/workflows/bc88a5ed-4c03-4c1e-ba7a-dd0b33577e60/jobs/340

But let's wait and see how things are going on this build... Not holding my breath.

Remi-Gau commented 1 year ago

note that the image is using an antiquated bids-validator version (0.19.8 when the current one is 1.12.0): may be worth bumping this up.

Remi-Gau commented 1 year ago

ok the image build is failing because of some conda hanging: https://app.circleci.com/pipelines/github/bids-apps/freesurfer/87/workflows/d6fe415d-5abd-427d-95c9-792ab1bf0073/jobs/355

trying locally

Remi-Gau commented 1 year ago

The only that changes in the bash script used to generate the different recipe is the freesurfer version, no?

I think this should be refactored too

Remi-Gau commented 1 year ago

@Shotgunosine can you check if you can build the image using the docker recipe: it seems to fail in CI and for me locally.

May need to adapt the neurodocker command

Shotgunosine commented 1 year ago

If hangs forever solving the environment for me locally as well. I'm trying to see if there's a way to install pandas that doesn't hang, but I haven't found anything yet. Might need to add mamba to neurodocker.

Remi-Gau commented 1 year ago

If hangs forever solving the environment for me locally as well. I'm trying to see if there's a way to install pandas that doesn't hang, but I haven't found anything yet. Might need to add mamba to neurodocker.

maybe specifying the exact version of each package may help?