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

GitHub: cache more things #127

Merged
merged 3 commits into from
Jan 16, 2024
Merged

GitHub: cache more things #127

merged 3 commits into from
Jan 16, 2024

Conversation

garymm
Copy link
Owner

@garymm garymm commented Jan 13, 2024

also remove unnecessary remote_download_minimal. We're not using a
remote cache so it's pointless.

Change-Id: I375b456a064ccbd57d6cdc94f9b548371d92ddaf

@garymm garymm force-pushed the garymm/github-cache branch from acb05bc to c856f0f Compare January 13, 2024 19:51
Copy link

codecov bot commented Jan 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2e6a83d) 82.45% compared to head (5b59f8f) 82.45%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #127   +/-   ##
=======================================
  Coverage   82.45%   82.45%           
=======================================
  Files          16       16           
  Lines         627      627           
  Branches       38       38           
=======================================
  Hits          517      517           
  Misses         94       94           
  Partials       16       16           

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

@garymm garymm force-pushed the garymm/github-cache branch from c856f0f to b59704d Compare January 13, 2024 20:08
@garymm garymm requested a review from oliverlee January 13, 2024 20:11
@garymm
Copy link
Owner Author

garymm commented Jan 13, 2024

@oliverlee I'm a bit worried about this in the post-cache step:
Warning: Path Validation Error: Path(s) specified in the action for caching do(es) not exist, hence no cache is being saved.

I tried to reproduce locally but couldn't. Maybe it will be fixed once this runs on master and the cache is created.

@garymm garymm enabled auto-merge (squash) January 13, 2024 20:12
@oliverlee
Copy link
Collaborator

Github actions cache cannot be updated once it is created (except for separate versions on feature branches).
https://github.com/actions/cache/blob/main/tips-and-workarounds.md#update-a-cache

This is how I handle repo cache in geometry:
oliverlee/geometry@0ada9e3
oliverlee/geometry@c3c0a34
The jobs run in the check workflow do not save repo cache.

From looking at profiling data in CI logs, ctx.download_and_extract of the llvm toolchain (which occurs in almost all CI jobs) takes ~2 mins and dominates the load+analyze phases. By setting up actions/cache/restore (which takes ~ 1 min), the load+analyze phase is reduced to a few seconds and results in ~1 minute saved overall.

Here's a snippet from a recent build:

=== PHASE SUMMARY INFORMATION ===

Total launch phase time                              1.581 s   11.32%
Total init phase time                                1.769 s   12.67%
Total target pattern evaluation phase time           0.254 s    1.82%
Total interleaved loading, analysis and execution phase time   10.326 s   73.93%
Total finish phase time                              0.035 s    0.25%
---------------------------------------------------------------------
Total run time                                      13.966 s  100.00%

https://github.com/oliverlee/geometry/actions/runs/7514778513/job/20458026966

@oliverlee
Copy link
Collaborator

Also, the single saved repo cache in geometry is ~4GB. I assume it will be similar here if successful since the same hermetic toolchains are used. Since each job is creating a separate cache entry, each workflow run will probably hit the cache limit and result in cache evictions.

https://github.com/actions/cache#cache-limits

also remove unnecessary remote_download_minimal. We're not using a
remote cache so it's pointless.

Change-Id: I375b456a064ccbd57d6cdc94f9b548371d92ddaf
@garymm garymm force-pushed the garymm/github-cache branch from b59704d to afbcd16 Compare January 14, 2024 00:16
@garymm
Copy link
Owner Author

garymm commented Jan 14, 2024

Hmm OK I guess not sure if this is the best, but seems a bit simpler that what you did to me. I split it into "bazel disk cache" which every job writes and reads, and "bazel other" which only one job writes and everyoone else just restores.

.github/workflows/check.yml Outdated Show resolved Hide resolved
"~/.cache/bazel_output_base/external"
"~/.cache/bazel_repository_cache"
"~/.cache/bazelisk"
key: bazel-other
Copy link
Collaborator

Choose a reason for hiding this comment

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

just fyi, you will only be able to write this cache once per branch.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks. I did not know that's how the cache action worked but I think that's fine for these.

@oliverlee
Copy link
Collaborator

Jobs show about 0 second duration for cache restore steps. Maybe you can try manually clearing the caches and then triggering the workflow twice?

@garymm garymm merged commit 3cc6b47 into master Jan 16, 2024
15 checks passed
@garymm garymm deleted the garymm/github-cache branch January 16, 2024 04:39
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