bids-apps / CPAC

BIDS Application for the Configurable Pipeline for the Analysis of Connectomes (C-PAC)
Apache License 2.0
14 stars 18 forks source link

FIX,ENH: upgraded basebox to xenial, updated cpac_install to reflect … #26

Closed jdkent closed 6 years ago

jdkent commented 6 years ago

…CPAC, updated version to reflect the version of CPAC this repository is based on.

Purpose

Fixes #25

Changes

good idea chrisfilo, ping @ccraddock

chrisgorgo commented 6 years ago

Pinging @ccradock

jdkent commented 6 years ago

To encourage not recreated the wheel (or the environment in this case), I pushed most of the building of the dockerfile to cpac_install.sh, I'll submit a pull request on the main CPAC repository to keep the cpac_install.sh files equivalent.

The only frustrating thing is that I have to build the environment at runtime, but I think this will simplify maintenance of the BIDS-App to keep in step with the main CPAC repository, so in this scenario, the benefits may outweigh the risks.

I still need to test this container with actual data to test if it works.

Comments welcome!

ccraddock commented 6 years ago

I don't have a lot of time now, will look more closely later, but have a comment now. We have had trouble in the past with how NeuroDebian builds AFNI where some of the applications, for example 3dSkullStrip, fail to work, complaining that they need to open a display. Also, CPAC doesn't need the full AFNI, or FSL to execute. If possible I would like to try to find a solution that allows us to install just parts of these tools. I agree that the afni_minimal way isn't the best. But hopefully we can find another way ...

Also I would like to get away from cpac_setup.py. It defeats the dependency change detection mechanism of docker builds, and results in re-builds taking longer than they really should need to. I am sure that you have experienced this quite a bit.

Also would like to make it so that the user doesn't run as root, messes up permissions for generated files.

Thank you very much for help with maintaining the CPAC BIDS app!

jdkent commented 6 years ago

We have had trouble in the past with how NeuroDebian builds AFNI where some of the applications, for example 3dSkullStrip, fail to work, complaining that they need to open a display.

RE: 3dSkullstrip, I did run into that, which is found in version 16.2, but appears to be fixed in 18. You can see a similar conversation in mriqc.

Also, CPAC doesn't need the full AFNI, or FSL to execute. If possible I would like to try to find a solution that allows us to install just parts of these tools.

I agree, extra bloat isn't great, again in line with with mriqc, perhaps osf versions of utilities needed could be uploaded and installed here, moving some of the installation/configuration outside of cpac_install.sh which in my mind is a con, but I think the reduced size is worth it, but I'm not comfortable with knowing exactly which utilities are needed from both packages. EDIT: the container is too big to even pass automated tests.

Also I would like to get away from cpac_setup.py. It defeats the dependency change detection mechanism of docker builds, and results in re-builds taking longer than they really should need to. I am sure that you have experienced this quite a bit.

Ahh, this is what I was trying to avoid earlier (assuming you meant cpac_install.sh), since I thought the dependencies were made "just so" in the cpac_install.sh and I thought it would be difficult to keep track of the dependencies needed in two separate places (Dockerfile and cpac_install.sh). I agree a nice feature of the Dockerfile is being able to separate out the steps to reduce build time... Now I see it as being better just using the dockerfile as you said, I think my confusion when I was changing the dockerfile was why there was a hybrid approach, and I wanted building to mostly happen in one environment.

Also would like to make it so that the user doesn't run as root, messes up permissions for generated files.

Is this possible within docker? I don't know the ins and outs of docker, but I thought you could only run containers as root.

BTW thank you very much for help with maintaining the CPAC BIDS app!

No Problem! Open source only works if we help.

ccraddock commented 6 years ago

Looks like the installation script is too effusive for travisci, the build failed with"

"The log length has exceeded the limit of 4 MB (this usually means that the test suite is raising the same exception over and over).

The job has been terminated"

ccraddock commented 6 years ago

Yes, I did mean cpac_install.sh, it looks like you did a lot of good work to it. I will use it to update the install script in the main CPAC repository, that way it will still go to good use if we decide to go to a Dockerfile only build.

jdkent commented 6 years ago

Error message from travis-ci:

(9/9) Moving the image to the output folder...
 10,322,182,176 100%   36.28MB/s    0:04:31 (xfr#1, to-chk=0/1)
rsync: write failed on "/output/bids_cpac-2018-01-26-66919f7d2caa.img": No space left on device (28)
rsync error: error in file IO (code 11) at receiver.c(393) [receiver=3.1.2]
The command "docker run -ti --rm --privileged -v /var/run/docker.sock:/var/run/docker.sock -v ${HOME}/singularity_images:/output filo/docker2singularity "bids/cpac"" failed and exited with 11 during .
chrisgorgo commented 6 years ago

a) try singularityware/docker2singularity b) check storage on your destination hard drive c) check if your virtual machine (you are using a mac?) did not run out of space; if in doubt reset docker

jdkent commented 6 years ago

thanks @chrisfilo, I'm still wet behind the ears when it comes to travis, how do I determine how much disk space travis has when it's testing my build?

chrisgorgo commented 6 years ago

oh - that's the travis error - it might mean this image is too big to convert on travis

ccraddock commented 6 years ago

Oh yeah, that’s one of the reasons we reduced AFNI. Instead of using neurodebian, can we install from the official AFNI tarball but only extract the required files?

Sent from my iPhone

On Jan 25, 2018, at 10:30 PM, Chris Filo Gorgolewski notifications@github.com wrote:

oh - that's the travis error - it might mean this image is too big to convert on travis

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jdkent commented 6 years ago

woo! it passed, ready for comments again, Should I include cpac_templates.tar.gz in the container? or was there a url it was downloaded from?