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.

Sign up for GitHub

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

Jump to bottom

Docker dependencies cache #39

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Docker dependencies cache #39

wants to merge 3 commits into from

Conversation

dikwickley
Copy link

Moving the pip install command above copying /teuthology_api to take advantage of docker caching.
Fixes: #37

running locally:

(env) aniket@air teuthology-api % docker build -t t-api2 .
[+] Building 4.8s (12/12) FINISHED                                                                                         docker:desktop-linux
 => [internal] load .dockerignore                                                                                                          0.1s
 => => transferring context: 2B                                                                                                            0.0s
 => [internal] load build definition from Dockerfile                                                                                       0.1s
 => => transferring dockerfile: 584B                                                                                                       0.0s
 => [internal] load metadata for docker.io/library/ubuntu:focal                                                                            1.3s
 => [internal] load build context                                                                                                          3.3s
 => => transferring context: 10.66MB                                                                                                       3.2s
 => [1/7] FROM docker.io/library/ubuntu:focal@sha256:ed4a42283d9943135ed87d4ee34e542f7f5ad9ecf2f244870e23122f703f91c2                      0.0s
 => CACHED [2/7] RUN apt-get update &&     apt-get install -y     git     qemu-utils     python3-dev     libssl-dev     ipmitool     pyth  0.0s
 => CACHED [3/7] COPY setup.py /                                                                                                           0.0s
 => CACHED [4/7] RUN pip3 install -e .                                                                                                     0.0s
 => CACHED [5/7] COPY .teuthology.yaml /root                                                                                               0.0s
 => CACHED [6/7] WORKDIR /teuthology_api                                                                                                   0.0s
 => CACHED [7/7] COPY . /teuthology_api/                                                                                                   0.0s
 => exporting to image                                                                                                                     0.0s
 => => exporting layers                                                                                                                    0.0s
 => => writing image sha256:e0c6d8ed232068a7b37ab68fe8f406a6088ca58eaaa883f350a9e56c76fb076a                                               0.0s
 => => naming to docker.io/library/t-api2                                                                                                  0.0s

@VallariAg can you please review.

@dikwickley
Updated docker file to pip install before copying dir.
e27c89d 
Signed-off-by: Aniket Singh Rawat <sprx@gmail.com>
@dikwickley
Removed extra line.
c41cae4 
Signed-off-by: Aniket Singh Rawat <sprx@gmail.com>
@VallariAg
Copy link
Member

@dikwickley I'm unable to run the container with docker run after building it. It's probably because the dependencies are set as install_requires in setup.cfg which isn't even copied to the container at this stage.

Maybe you can look into something like pip-compile which would create the requirements.txt from setup.cfg automatically. Then in Dockerfile, install requirements from requirements.txt before copying all other files.

@dikwickley
Copy link
Author

@VallariAg What if copy setup.cfg as well when copying setup.py

@dikwickley
added requirements.txt
e80951e 
Signed-off-by: Aniket Singh Rawat <sprx@gmail.com>
@dikwickley
Copy link
Author

@VallariAg I have made the suggested changes and it works now, the container starts up.

Though I suggest we move to a conda-based env. Thoughts on this?

@VallariAg
Copy link
Member

VallariAg commented Nov 27, 2023 •
edited

What if copy setup.cfg as well when copying setup.py

I'm not sure if that would work (it didn't when I tested that) - you can explore more ways to install dependencies before copying other project content for a pyscaffold packaged python project if you want.

Though I suggest we move to a conda-based env. Thoughts on this?

This project is structured using pyscaffold. They added instructions to use conda in https://github.com/ceph/teuthology-api/blob/main/CONTRIBUTING.md#create-an-environment I'm curious to know what prompted this?

@dikwickley
Copy link
Author

This project is structured using pyscaffold. They added instructions to use conda in https://github.com/ceph/teuthology-api/blob/main/CONTRIBUTING.md#create-an-environment I'm curious to know what prompted this?

Those instructions are just for creating a blank environment, in which install all dependencies from pip install -U pip setuptools -e .

I'm not sure if that would work (it didn't when I tested that) - you can explore more ways to install dependencies before copying other project content for a pyscaffold packaged python project if you want.

I also tried this, and this didn't work out. I compiled requirements.txt using pip-compile and copied that along with setup.py

VallariAg
VallariAg reviewed Nov 27, 2023
View reviewed changes
Dockerfile
@@ -18,9 +18,13 @@ RUN apt-get update && \
lsb-release && \
apt-get clean all

COPY setup.py .
Copy link
Member

Choose a reason for hiding this comment

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

we can just copy requirements.txt and install dependencies with that, I don't think copying setup.py is required

also copying requirements.txt to /teuthology_api would be better, maybe something like:

COPY .teuthology.yaml /root
WORKDIR /teuthology_api
COPY requirements.txt .
RUN pip3 install -r requirements.txt
COPY . /teuthology_api/
RUN pip3 install -e .

@VallariAg
Copy link
Member

@dikwickley I'm looking at ways to cache pip3 packages like suggested here https://docs.docker.com/build/cache/#use-the-dedicated-run-cache I think this might be a good direction to go from here.
This solution would not require creating and maintaining requirement.txt.
Note: please rebase your branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VallariAg VallariAg VallariAg left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

docker: all dependencies are reinstalled on each run
2 participants
@dikwickley @VallariAg