Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 129 #151

Merged
merged 11 commits into from
Oct 22, 2020
Merged

Issue 129 #151

merged 11 commits into from
Oct 22, 2020

Conversation

tgallagh
Copy link

Made a dockerfile to build image for metaphlan3.0.3. And updated README.md and travis.yml. Note that Docker version >= 17.05 is necessary to build the image which uses multistage builds to reduce image size (https://docs.docker.com/develop/develop-images/multistage-build/), but the built image should work with any docker version.

Also, this is my first time contributing (and hopefully won't be my last). Please let me know if there's anything I should do differently for future PRs! Thanks!!!

@kapsakcj
Copy link
Collaborator

Hey Tara, thanks so much for the PR. And thank you for updating the readme and travis tests! Sorry it took me so long to respond - I hope this docker image isn't mission critical.

I haven't played with multi-stage builds, but have been helping Lee K. as he is working on a multi-stage build for one of his projects. So that being said, I'm not sure the best practices for optimizing multi-stage builds. And I've never used metaphlan either...I'll leave the testing up to you 😄

I have a few requests that I list here, and for nitpicky comments, I'll make comments to the specific lines in the code.

  • Please add a line in the table in Program_licenses.md with the info for metaphlan
  • Consider adding a nearly identical dockerfile that does not include the metaphlan database. Generally it's not a good idea to keep large files inside a docker image and we have seen some issues (@erinyoung could elaborate) with kraken1 & 2 images that contain the minikraken db that is 8GB. For this reason, we've added kraken dockerfiles that do not download the database, and instead we've provided instructions for users to mount a volume e.g. docker run -v /local-kraken-db:/kraken-db-inside-container to make the image building and downloading process smoother.

Curtis

tests/metaphlan.sh Outdated Show resolved Hide resolved
tests/metaphlan.sh Outdated Show resolved Hide resolved
@tgallagh
Copy link
Author

Hi Curtis,

Thank you again for your feedback and help with this! Sorry the long response - I've been doing a lot of wetlab lately. I don't need to use the metaphlan docker anytime soon, so no need to rush for reviewing. 😄

As requested, I updated the program licenses file. I also made two versions of the dockerfile: one that builds the ~3GB metaphlan database (3.0.3) and one without (3.0.3-no-db). For 3.0.3.-no-db, I included instructions on how to install the metaphlan database in metaphlan/3.0.3-no-db/README.md.

I recently realized that the multistage builds do not automatically delete or overwrite intermediate stages, so the builder will need to delete any intermediate stages after the final image build is complete. I labeled the intermediate stages so they can be easily deleted and noted this in the Dockerfile comments:

For 3.0.3:
docker image ls --filter "label=stage=builder_metaphlan"
docker image prune --filter "label=stage=builder_metaphlan"

For 3.0.3-no-db:
docker image ls --filter "label=stage=builder_metaphlan_nodb"
docker image prune --filter "label=stage=builder_metaphlan_nodb"

Sorry about that - let me know if you want me to avoid multistage builds in the future!

I tested both versions in docker, still need to test in singularity.

thanks again!
Tara

Program_Licenses.md Outdated Show resolved Hide resolved
@kapsakcj
Copy link
Collaborator

Thanks for the additional dockerfile, the readme, and for making those changes.

If you think multi-stage build is the way to go, I say go for it. I just have less/almost no experience with multi-stage so it's a little harder for me to test & debug.

If you're happy with these dockerfiles, we can merge the PR, and I can set up the builds in dockerhub. Once they're built, we can download/convert them with singularity and test.

Curtis

@tgallagh
Copy link
Author

Thanks Curtis. Before you merge, I'll incorp those 2 changes and I also want to test in singularity. Will be in touch soon!

@tgallagh
Copy link
Author

Hi Curtis,

Thanks for waiting, I think it's ready!

Quick update - I think I misunderstood your first request about running perl to check locale settings in the env, so I removed those perl lines from each dockerfile (they didn't do anything to check LC settings during the build process). I tested both builds in singularity and added examples to each README.

Let me know what else I can do. And please let me know if there's anything I should do differently for future Dockerfiles and PRs.

Thanks,
Tara

@kapsakcj
Copy link
Collaborator

Awesome work. I think this is ready to be merged. Thank you so much for the contribution.

I will set up the builds on dockerhub using the name and tags you have specified in the READMEs

@kapsakcj kapsakcj merged commit e34f97c into StaPH-B:master Oct 22, 2020
@kapsakcj kapsakcj mentioned this pull request Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants