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

Do not cache packages in images #1927

Merged
merged 1 commit into from
Oct 31, 2023
Merged

Do not cache packages in images #1927

merged 1 commit into from
Oct 31, 2023

Conversation

eapolinario
Copy link
Collaborator

TL;DR

Disable caching of packages in images

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Pass the --no-cache-dir flag in invocation of pip install in dockerfiles. This will have a bigger impact in the agent image, since currently we are using about ~800MB:

 docker run --rm -it ghcr.io/flyteorg/flyteagent:1.10.0 du -kh --max-depth=1 /root/.cache
813M    /root/.cache/pip
813M    /root/.cache

Tracking Issue

https://github.com/flyteorg/flyte/issues/

Follow-up issue

NA
OR
https://github.com/flyteorg/flyte/issues/

Signed-off-by: Eduardo Apolinario <[email protected]>
@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8491f70) 54.70% compared to head (aeadcc6) 54.73%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1927      +/-   ##
==========================================
+ Coverage   54.70%   54.73%   +0.02%     
==========================================
  Files         306      306              
  Lines       22788    22811      +23     
  Branches     2255     3459    +1204     
==========================================
+ Hits        12466    12485      +19     
+ Misses      10166    10154      -12     
- Partials      156      172      +16     

see 25 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Dockerfile Show resolved Hide resolved
@@ -8,7 +8,7 @@ ARG VERSION
RUN apt-get update && apt-get install build-essential -y

RUN pip install prometheus-client
Copy link
Contributor

Choose a reason for hiding this comment

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

Missed one. Why is this separate anyway?

Copy link
Member

Choose a reason for hiding this comment

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

since we don't want to add prometheus-client to the default flytekit dependencies.

@jeevb
Copy link
Contributor

jeevb commented Oct 31, 2023

build-essential adds an insignificant overhead. It should only be needed for building wheels, and not needed in the final image. Also apt cache is not removed. We can clean this up in a subsequent PR.

@eapolinario
Copy link
Collaborator Author

build-essential adds an insignificant overhead. It should only be needed for building wheels, and not needed in the final image. Also apt cache is not removed. We can clean this up in a subsequent PR.

we need build-essential for this exact reason. We build wheels for some of our dependencies, e.g. pyspark in the case of the agents. I also did a quick test removing the apt cache and we could shave another 20-30MB.

@eapolinario eapolinario merged commit e1cf84d into master Oct 31, 2023
70 checks passed
ringohoffman pushed a commit to ringohoffman/flytekit that referenced this pull request Nov 24, 2023
Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
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.

4 participants