Skip to content

CI add ccache for GitHub Actions #31895

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

Merged
merged 13 commits into from
Aug 11, 2025
Merged

Conversation

StefanieSenger
Copy link
Member

@StefanieSenger StefanieSenger commented Aug 7, 2025

Reference Issues/PRs

Follow up on #31832

What does this implement/fix? Explain your changes.

This PR adds a step to use a cache for ccache on the GitHub Actions job(s) in the unit-tests.yml.

Thanks for your support @lesteve and @adrinjalali! 🫶

Comments

From my experiments on another branch it seems that this speeds up the build of scikit-learn by 1 min (1 min instead of 2 mins) on ARM.

This won't work on windows.

Copy link

github-actions bot commented Aug 7, 2025

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: bbf689b. Link to the linter CI: here

@StefanieSenger
Copy link
Member Author

Three comments for reviewers:

  1. The actions output of "build scikit-learn" step outputs the ccache statistics at the very end.

  2. The Set compiler ID step is an experiment and I am happy to remove/replace if it's not adding enough value. Then the key would be shorter/less specific. Reasoning:

  • I have looked at how we define key and restore-keys in build_tools/azure/posix.yml and found doing it the same way is not so useful here. Using something like Agent.JobName doesn't help, because we don't use several jobs that build (lint job doesn't build). Build.BuildNumber doesn't seem to have an equivalent in github actions(?).
  • Instead, I have tried to use the compiler as part of the key. ccache then implements one cache per os-compiler combination.
  • The Set compiler ID step extracts the compiler from the runner and it gets re-used in the key later. The alternative way using ${{ matrix.compiler }} would mean that we would have the define a compiler on the matrix and we have done that nowhere else, so I assume that we don't want that to keep it more flexible?
  1. The cache size is pretty low (I had 6MB after multiple experiments on the other branch), but he have set the maximum size to ccache -M 256M in install.sh. Should it be decreased or would we wait for the other jobs to be added and then re-evaluate this?

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Pretty cool!

uses: actions/cache@v4
with:
path: ${{ github.workspace }}/ccache
key: ccache-${{ runner.os }}-${{ steps.get-compiler.outputs.compiler }}
Copy link
Member

Choose a reason for hiding this comment

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

do we want to make sure this is only within each PR, and not shared between PRs, to make sure there's no cache poisoning happening?

Copy link
Member Author

@StefanieSenger StefanieSenger Aug 8, 2025

Choose a reason for hiding this comment

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

@lesteve and I had talked about this. If I understood correctly then it would be preferable to allow sharing the ccache across several PRs, because ccache is smart enough to compare the contents of the source c and cpp files before reusing their build. I also think it would quickly clutter if we had a cache for each PR.

Or do you refer to security questions?

Another thought that came to my mind:
Does it make sense to have an action/repository setting/makefile command (not sure how it should look like) (here is an actions workflow that would do that) that deletes the ccache to clean out old min dependency builds that are not used anymore or when we need a fresh start. That could be regular or on demand.

Copy link
Member

Choose a reason for hiding this comment

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

My understanding is that GHA cache already has some builtin protections, see doc.

@adrinjalali
Copy link
Member

can we test this by having another commit and see if cache works?

@lesteve
Copy link
Member

lesteve commented Aug 8, 2025

Nice, it seems to work fine. There may be some tweaks that we can do which I will suggest soon:

  • first commit build log, building from scratch takes ~2 minutes:
+ ccache -s
Cacheable calls:    73 / 162 (45.06%)
  Hits:              0 /  73 ( 0.00%)
    Direct:          0
    Preprocessed:    0
  Misses:           73 /  73 (100.0%)
Uncacheable calls:  89 / 162 (54.94%)
Local storage:
  Cache size (GB): 0.0 / 0.3 ( 2.71%)
  Hits:              0 /  73 ( 0.00%)
  Misses:           73 /  73 (100.0%)
  • second commit build log, reuses ccache and building takes ~1 minute and you can see some ccache reuse:
+ ccache -s
Cacheable calls:    73 / 162 (45.06%)
  Hits:             71 /  73 (97.26%)
    Direct:         71 /  71 (100.0%)
    Preprocessed:    0 /  71 ( 0.00%)
  Misses:            2 /  73 ( 2.74%)
Uncacheable calls:  89 / 162 (54.94%)
Local storage:
  Cache size (GB): 0.0 / 0.3 ( 2.81%)
  Hits:             71 /  73 (97.26%)
  Misses:            2 /  73 ( 2.74%)

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

LGTM. I'll let @lesteve decide if improvements need to be done here or other PRs.

Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
@lesteve
Copy link
Member

lesteve commented Aug 8, 2025

Here is the thinking behind the cache key:

  • you can not update an existing cache entry, see doc, so you need to create a cache entry with a different name (and use restore-key to be able to use partial match to reuse a previous cache key)
  • we chose to create a new cache entry if there is a "good reason" to, so we added the following files in the hash
    • Cython (including .tp files), C, C++ files (that seems a no-brainer)
    • lock-file. I am not 100% sure about this one, but I guess if the lock-file changes it may be a good enough reason to generate a cache entry e.g. compiler or numpy version change? The ccache doc seems to say that cache will not be reused with a different compiler version (it uses the size and the mtime of the compiler in the hash key by default)
  • Azure is creating a cache entry in every build. This was done in MAINT Increase ccache hit rate on Azure Pipelines #22213 to maximize cache reuse. We can always go back to this simpler strategy if we think it's better.

@lesteve
Copy link
Member

lesteve commented Aug 11, 2025

Let's merge this one and adapt if we notice possible improvements to the cache key strategy.

@lesteve lesteve merged commit a9a7b7d into scikit-learn:main Aug 11, 2025
36 checks passed
@StefanieSenger StefanieSenger deleted the ccache_for_gha branch August 11, 2025 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants