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

Testing dependencies in Docker #3

Merged
merged 6 commits into from
Nov 5, 2024

Conversation

marissafujimoto
Copy link
Collaborator

@marissafujimoto marissafujimoto commented Oct 15, 2024

  • rearranging
  • Fix Bowtie2 installation command
  • Add seqtk installation to dockerfile
  • Add hello world bash script
  • Call dependencies in bash script
  • Pass arguments in dependency POC

Description

Addressing #2. Right now just focused on quick and fast test of dependencies. Running through bash with

docker run -it $(docker build -q --platform linux/amd64 .) bash

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Terminal output

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@marissafujimoto marissafujimoto self-assigned this Oct 15, 2024
@cansavvy
Copy link
Collaborator

cansavvy commented Oct 16, 2024

Thanks for getting this started! This is great! We're moving forward.

I need to check with Berger lab that the the fasta files I'm adding here are the correct ones for the samples we're processing for the example but it does work if from the extdata directory I run:

docker run -it $(docker build -q --platform linux/amd64 .) bash /home/scripts/dependency-poc.sh

Couple overall thoughts we should think about after getting the proof of concept here through:

  1. Probably will have users use volumes instead of copying the files over to the image. This way users won't have to rebuild the image each time they want to run it. Plus it will allow files to be saved more easily. https://docs.docker.com/engine/storage/volumes/

  2. Probably will want these software programs to be called from anywhere as opposed to /home/biodocker/bin/bowtie2 type deals so that way if "power" users decide to use the image interactively that doesn't become a roadblock for them.

@marissafujimoto
Copy link
Collaborator Author

Thanks for getting this started! This is great! We're moving forward.

I need to check with Berger lab that the the fasta files I'm adding here are the correct ones for the samples we're processing for the example but it does work if from the extdata directory I run:

docker run -it $(docker build -q --platform linux/amd64 .) bash /home/scripts/dependency-poc.sh

Couple overall thoughts we should think about after getting the proof of concept here through:

  1. Probably will have users use volumes instead of copying the files over to the image. This way users won't have to rebuild the image each time they want to run it. Plus it will allow files to be saved more easily. https://docs.docker.com/engine/storage/volumes/
  2. Probably will want these software programs to be called from anywhere as opposed to /home/biodocker/bin/bowtie2 type deals so that way if "power" users decide to use the image interactively that doesn't become a roadblock for them.
  1. I was actually testing earlier with volumes so I think that shouldn't be an issue for the final version.
  2. I can organize the dependencies on the docker image if that's a use case. Just basically would move the binaries to /usr/local/bin and add them to the path. Maybe we could also move any source code to /usr/src just to make the directories a little cleaner. Let me know what you think!

Anything blocking this pr though?

@marissafujimoto
Copy link
Collaborator Author

@cansavvy small update here: 4778b3d to sym link / install all the dependencies to /usr/local/bin. Just in case anyone needs to run them within the container.

@marissafujimoto marissafujimoto marked this pull request as ready for review October 29, 2024 22:21
Comment on lines +23 to +28
RUN apt-get update && apt-get -y upgrade && \
apt-get install -y build-essential wget \
libncurses5-dev zlib1g-dev libbz2-dev liblzma-dev libcurl3-dev && \
apt-get clean && apt-get purge && \
rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/*

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just moved this up so that the image layers are doing the more general / slow steps first. Just might save someone a couple minutes if they need to add a dependency later. https://docs.docker.com/get-started/docker-concepts/building-images/understanding-image-layers/

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though looking at it now we are definitely pretty far from the most efficient dockerfile...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's okay, we will polish. we're still in the stage of figuring out what is needed here.

@cansavvy
Copy link
Collaborator

cansavvy commented Nov 5, 2024

I have further edits here that we'll need to incorporate at some point: https://github.com/FredHutch/pgmap/pull/8/files#diff-25e231b330e6b4280e139dcfb0868218749cdbd08abb6c4baa396b8478642a2d

but this will still be in flux for a bit I suspect.

@cansavvy
Copy link
Collaborator

cansavvy commented Nov 5, 2024

@cansavvy small update here: 4778b3d to sym link / install all the dependencies to /usr/local/bin. Just in case anyone needs to run them within the container.

Oh great. This is good. thanks!

Comment on lines +23 to +28
RUN apt-get update && apt-get -y upgrade && \
apt-get install -y build-essential wget \
libncurses5-dev zlib1g-dev libbz2-dev liblzma-dev libcurl3-dev && \
apt-get clean && apt-get purge && \
rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/*

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's okay, we will polish. we're still in the stage of figuring out what is needed here.

@marissafujimoto marissafujimoto merged commit eb17b33 into FredHutch:main Nov 5, 2024
2 of 7 checks passed
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