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

updating checkm2 docker build #1138

Closed
wants to merge 0 commits into from

Conversation

taylorpaisie
Copy link
Contributor

Pull Request (PR) checklist:

  • Include a description of what is in this pull request in this message.
  • The dockerfile successfully builds to a test target for the user creating the PR. (i.e. docker build --tag samtools:1.15test --target test docker-builds/samtools/1.15 )
  • Directory structure as name of the tool in lower case with special characters removed with a subdirectory of the version number (i.e. spades/3.12.0/Dockerfile)
    • (optional) All test files are located in same directory as the Dockerfile (i.e. shigatyper/2.0.1/test.sh)
  • Create a simple container-specific README.md in the same directory as the Dockerfile (i.e. spades/3.12.0/README.md)
    • If this README is longer than 30 lines, there is an explanation as to why more detail was needed
  • Dockerfile includes the recommended LABELS
  • Main README.md has been updated to include the tool and/or version of the dockerfile(s) in this PR
  • Program_Licenses.md contains the tool(s) used in this PR and has been updated for any missing

@taylorpaisie
Copy link
Contributor Author

I am creating this PR for the Checkm2 docker container. I know the same version is already on master but I was building this container for our workflow already and thought I would submit and get some feedback for the container. Let me know what you think!

@Kincekara
Copy link
Collaborator

Kincekara commented Dec 23, 2024

@taylorpaisie What is the reason for trying to update this image? Do you want to add its database to the image? If there is an issue in the current image, please submit an issue first. So we can avoid similar problems in future releases.

First of all, we don't overwrite the existing images except on very rare occasions. It can break people's pipelines and validations.
Secondly, we try to avoid adding large databases to images. Because it significantly increases the loading time, decreases efficiency, and may cause problems in cloud applications.

However, we can still add it if you need it as is. Please change your PR name to something like checkm2/1.0.2-{current db name or version} first. So, we can add it without overwriting the current image.

Is it possible to use Jammy as the base image?

I'm just curious, have you compared the image sizes of micromaba and staged builds?

@taylorpaisie
Copy link
Contributor Author

The purpose of me creating this new image is to not use micromamba in the build. I realize that micromamba creates a smaller image size, but I was also under the impression that micromamba, or anything conda related, in these docker builds should be avoided.

@erinyoung
Copy link
Contributor

I think it would be great to use a slimmer image. When we changed how things were installed for quast, we add a -slim to directory path and version tag (i.e. 5.3.0-slim).

I think we need to keep the original dockerfile unchanged, and then copy these edits into a new Dockerfile in a build-files/checkm2/1.0.2-slim subdirectory.

Or something like it.

Maybe focal instead of slim?

@Kincekara
Copy link
Collaborator

@erinyoung I do agree we should keep the original dockerfile unchanged.
This image is not smaller than the current one. On the contrary, it is much larger since it includes ~3.1 GB of diamond database.
We can add it with a database name or date like bakta or busco images.

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.

3 participants