search

Bioconductor / bioc_docker

[DEPRECATED] Docker containers for Bioconductor
https://github.com/bioconductor/bioconductor_docker
Artistic License 2.0
49 stars 27 forks source link

Optimize src #89

Closed nturaga closed 5 years ago

nturaga commented 5 years ago

Reopen PR #88. (It got accidentally merged because of my git foo)

This PR makes the release and devel base / core images more efficient.

The image sizes are reduced quite a bit as a result of this clean up. 

bioconductor/release_core2        latest              3002bf31af4f        2 hours ago         2GB
bioconductor/devel_core2          latest              9c5632d5bfa6        2 hours ago         2GB
bioconductor/release_base2        latest              59afb44f26e5        3 hours ago         1.48GB
bioconductor/devel_base2          latest              9bc5a41e1779        3 hours ago         1.48GB

The changes made need to be run through rake. To produce the out directory.

The main aim is to:

  1. reduce duplication

  2. clear cache

  3. remove dead code

  4. remove comments which are not relevant anymore

This effect of reduced size will trickle down to all the other docker images which inherit from these base / core images.

I'm tagging all the authors of these files as they should be aware of the changes in bioc_docker.

nturaga commented 5 years ago

Hi @mtmorgan

I've added the changes you've suggested on #88. I had a git snafu on the earlier Pull request. I did a git push without adding upstream optimize_src. Because of which the PR got merged.

@lgatto and @lshep I'm attaching you to this as you have contributed to this repo. Please be aware of the changes i've made. If you have questions, i'm happy to answer.

As an additional check to make sure all the containers are working as expected, I've built them with the same names as the bioconductor docker hub organization under my username (nitesh1989).

https://cloud.docker.com/u/nitesh1989/repository/docker/nitesh1989/devel_base2

https://cloud.docker.com/u/nitesh1989/repository/docker/nitesh1989/release_base2

https://cloud.docker.com/u/nitesh1989/repository/docker/nitesh1989/devel_core2

https://cloud.docker.com/u/nitesh1989/repository/docker/nitesh1989/release_core2

https://cloud.docker.com/u/nitesh1989/repository/docker/nitesh1989/release_mscore2

https://cloud.docker.com/u/nitesh1989/repository/docker/nitesh1989/devel_mscore2

etc.

sneumann commented 5 years ago

Hi, I also checked out your PR branch, and can confirm that release_base2 -> ... -> releasemetabolomics2 all build. I haven't checked the devel ones yet. I saw that you didn't touch the _core2 files, I guess there was nothing to optimize. And I realised that mscore is built off base, but instead shoud be built off core, but that's something @lgatto and myself will attend to. I also haven't made any attempts to optimise mscore, protmetcore nor metabolomics, but I guess that should be done in the future. Yours, Steffen

nturaga commented 5 years ago

@sneumann Thanks a lot for the review and response. Yes, you can make those changes once this PR is merged and the new containers get built.

lshep commented 5 years ago

Looks good to me so long as the core images are building off the new base image.

One little note: I left the commented out BiocManager install from github code in src/base/install.R.in because of the issues we had towards the end of last release where there was a version of R given to devel but no version in BiocManager but we didn't want to push the new version to CRAN until it was official, etc (or at least I think that was the reason) . I figured we would have to make this switch again towards the spring release again but maybe not?

Once this is merged I or @nturaga will have to tag and kick off new release builds and devel build - and possible look back into the reason why the devel images aren't cascade building

nturaga commented 5 years ago

Looks good to me so long as the core images are building off the new base image.

One little note: I left the commented out BiocManager install from github code in src/base/install.R.in because of the issues we had towards the end of last release where there was a version of R given to devel but no version in BiocManager but we didn't want to push the new version to CRAN until it was official, etc (or at least I think that was the reason) . I figured we would have to make this switch again towards the spring release again but maybe not?

Once this is merged I or @nturaga will have to tag and kick off new release builds and devel build - and possible look back into the reason why the devel images aren't cascade building

The images are building off the new images. i've tested it a few times on my local machine and I can confirm they work.

And, I'm not aware of that issue. But we could recreate that code if it's needed again during release time. 

-#install.packages("devtools", repos="https://cran.rstudio.com")
-#devtools::install_github("Bioconductor/BiocManager")
nturaga commented 5 years ago

@sneumann @lgatto The base images were updated, as a result of which there should be a cascading effect on all the images which inherit from them.

Please make any updates you think are appropriate for the mscore and other images which you maintain.

I feel now is a good time to have these "improved" docker images up and running.

Thanks for your effort in maintaining these images.

Best

Nitesh

lshep commented 5 years ago

@mikejiang You shouldn't need to make any changes since you don't add any system dependencies and just build off the base image but so you are aware of @nturaga efforts and the result will be an optimized image for yours as well.

mikejiang commented 5 years ago

Great,Thanks!