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

fix(op-program): minimize docker context #13580

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

sigma
Copy link
Contributor

@sigma sigma commented Jan 4, 2025

Description

This change minimizes the docker context needed to create the repro
artifacts.

Rationale: by default, the context is basically the entire monorepo
(minus whatever is excluded in the top-level .dockerignore file).

This is way larger than needed (and gets worse as the working
directory gets dirtier), and leads to unnecessary cache misses,
therefore rebuilds.

Tests

Additional context

Metadata

Copy link

codecov bot commented Jan 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.06%. Comparing base (6fa5e37) to head (a0dd9f5).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #13580      +/-   ##
===========================================
- Coverage    47.48%   43.06%   -4.42%     
===========================================
  Files          945      779     -166     
  Lines        79032    69671    -9361     
  Branches       761        0     -761     
===========================================
- Hits         37530    30007    -7523     
+ Misses       38654    37087    -1567     
+ Partials      2848     2577     -271     
Flag Coverage Δ
cannon-go-tests-32 ?
cannon-go-tests-64 ?
contracts-bedrock-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

see 172 files with indirect coverage changes

@sigma
Copy link
Contributor Author

sigma commented Jan 4, 2025

I should add that the downside is that this file needs to be adjusted if new dependencies within the monorepo appear.
So it's not a free improvement.

@sigma sigma marked this pull request as ready for review January 4, 2025 00:12
@sigma sigma requested review from a team as code owners January 4, 2025 00:12
@axelKingsley
Copy link
Contributor

Awesome find, do you have an estimate of before/after size?

I am in favor of this, but I wonder if we have any sort of "Making a top-level application in the Monorepo" document that would need to be updated with this. And if we don't have such a thing, I wonder if we should... 🤔

@sigma
Copy link
Contributor Author

sigma commented Jan 6, 2025

Awesome find, do you have an estimate of before/after size?

I am in favor of this, but I wonder if we have any sort of "Making a top-level application in the Monorepo" document that would need to be updated with this. And if we don't have such a thing, I wonder if we should... 🤔

I don't have a very good answer for the before/after, but here are some numbers:

  • I got pissed off when the context accumulated ~3.5GB and the transfer tool several minutes, but some of it came from repo dirtiness
  • with this change, the context for this particular build is at ~150MB, which arguably is still too large. I think I need to be more specific with op-program/. Anyways, the transfer takes seconds at this level

Possibly more important is the fact that we should expect that context to be more or less constant? (I think? we might have to whitelist .go files explicitly to be sure)

I don't know of such a document. I'm happy to start one if I feel it's useful

@sigma sigma force-pushed the sigma/kurtosis-devnet-pq-17 branch from 87ec3ee to b653d80 Compare January 6, 2025 16:49
@sigma
Copy link
Contributor Author

sigma commented Jan 6, 2025

just added a fix to exclude op-program/bin.
Now the context is ~250k, which is as good as we're gonna get I think.
And for reference, the build time is ~30s vs multiple minutes

@sigma sigma force-pushed the sigma/kurtosis-devnet-pq-17 branch from b653d80 to a0dd9f5 Compare January 7, 2025 10:11
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