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

Revamp Dockerfile #183

Merged

Conversation

vit-zikmund
Copy link
Collaborator

@vit-zikmund vit-zikmund commented Jan 2, 2025

Hello folks,
Being under a bit of time pressure, I wanted to get this done ASAP, which turned out to be before you got to review the PR #181 (so the change is built upon it and ⚠️ this PR actually targets the original PR branch).

I said I don't want to work on these improvements before that PR is resolved, but then again, I felt there's so much to improve - and indeed, I manged to shrink the new image size to a half of the original - that I wouldn't want to hold this effort any longer.

All the Dockerfile changes are in fact rather cosmetic and don't change any ARGs (only sorts them to the start of a stage). Same with ENTRYPOINT and CMD directives, which remain the same.

See the commit messages for details. Thank you!

Using a couple best practices for saving space and build cache optimizations to get better build times and much smaller final image.

Notably:
- Python packages are now kept in a virtual environment, which is fully prepared in the builder stage, later simply copied to the same path of the final image
- Any python cache .pyc files are not part of the final image
- Git binary needed to automatically determine the package version is not part of the final image
- Many auxiliary project source files (incl. the local .git dir(!)) are also omitted from the final image
- Use RUN --mount instead of COPY where the files are needed only for the command
- Keep various COPY operations to the end of a stage not to invalidate semantically unrelated cached layers
- Remove deprecated (and misplaced) MAINTAINER directive in favor of a particular "authors" LABEL
Copy link
Member

@demenech demenech left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@athornton athornton left a comment

Choose a reason for hiding this comment

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

LGTM

@athornton
Copy link
Collaborator

Let's go ahead and merge this, but I think ultimately I'd prefer to use uv pip over pip and it turns out we can just install the uv binary in the build container from its image in the Docker container (rather than pip install uv). Anyway, future improvement, speeds up container construction a bit, but that might be an improvement that isn't even really worth bothering with.

@demenech demenech merged commit 769e99f into feat/dockerhub-deprecation-warning Jan 8, 2025
14 checks passed
@demenech demenech deleted the chore/streamline-dockerfile branch January 8, 2025 18:13
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