bids-apps / freesurfer

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

Update circleci to 2.1 api and fix dockerfile #53

Closed Shotgunosine closed 4 years ago

Shotgunosine commented 4 years ago

Took a quick and dirty pass to see if I could update the old broken circle.yml that used circleci api 1.0 to the current circle api 2.1.

closes #50

sappelhoff commented 4 years ago

good idea, we did this recently in BIDS-Apps/SPM and BIDS-Apps/example as well :+1:

Shotgunosine commented 4 years ago

@sappelhoff Any chance you know what's up with all of the colons at the ends of the lines as in: https://github.com/BIDS-Apps/freesurfer/blob/753bd76ad1cf835f1a1fe38236f9261155fd6b04/circle.yml#L32

    - docker run -ti --rm --read-only -v $PWD/license.txt:/license.txt -v /tmp:/tmp -v /var/tmp:/var/tmp -v ${HOME}/data/ds114_test1:/bids_dataset bids/${CIRCLE_PROJECT_REPONAME,,} --version
    - docker run -ti --rm --read-only -v $PWD/license.txt:/license.txt -v /tmp:/tmp -v /var/tmp:/var/tmp -v ${HOME}/data/ds114_test1:/bids_dataset bids/${CIRCLE_PROJECT_REPONAME,,} -h
    - docker run -ti --rm --read-only -v $PWD/license.txt:/license.txt -v /tmp:/tmp -v /var/tmp:/var/tmp -v ${HOME}/data/ds114_test1:/bids_dataset -v ${HOME}/outputs1:/outputs bids/${CIRCLE_PROJECT_REPONAME,,} /bids_dataset /outputs participant --participant_label 01 --license_file=/license.txt --stages autorecon1 && cat ${HOME}/outputs1/sub-01/scripts/recon-all.done :
Shotgunosine commented 4 years ago

@PeerHerholz This is ready for you to review. @sappelhoff, or @leej3 I'm not exactly an expert at CircleCI so any suggestions are welcome.

PeerHerholz commented 4 years ago

Thx a lot @Shotgunosine, @sappelhoff and @leej3 for the work and comments! One thing I thought about was to include neurodocker for the creation of docker and singularity files. WDYT? We can also discuss it in a different issue/PR as it would introduce a substantial change.

Shotgunosine commented 4 years ago

I think moving to neurodocker is a good idea, should probably be a new PR though.

Shotgunosine commented 4 years ago

Alright, I think this is good to go.

leej3 commented 4 years ago

You need to modify the deploy step a little. You need to restore the image cache and use docker load as you have done in the test step.

Also I think you should change the save key for the image to v_0-image_cache-{{ checksum "Dockerfile" }}-{{ .Revision }}-{{ epoch }}. The build restore key can be kept the same but test and deploy steps should probably restore using v_0-image_cache-{{ checksum "Dockerfile" }}-{{ .Revision }} to avoid funny bugs where you build a new image but don't save it (and so repeatedly test and deploy the same thing).

EDIT: Example would be run.py changes and the image is rebuilt appropriately but is not cached because the dockerfile did not change and so the key used for saving the image cache has already been used to write a cache and so cannot be reused.

Shotgunosine commented 4 years ago

Yep, good catch. Are the fixes in that last commit what you had in mind?

leej3 commented 4 years ago

Yes, except the restore cache keys. The restore key will match all caches that start with the provided key. So a restore key of v_0-image_cache-{{ checksum "Dockerfile" }}-{{ .Revision }} will match all image caches for the current git commit (and dockerfile version). It loads the cache that was most recently saved. We don't know (or need to know) the exact name of the save key at runtime: we use a unique and arbitrary suffix of epoch time in seconds when the cache was written. I don't think the key you have used will match anything.

Shotgunosine commented 4 years ago

@leej3 good now?

leej3 commented 4 years ago

Looks good

Shotgunosine commented 4 years ago

@sappelhoff I think this is good to merge if you're happy with it.

sappelhoff commented 4 years ago

Thanks a lot @Shotgunosine and @leej3 + @PeerHerholz for the review!