-
Notifications
You must be signed in to change notification settings - Fork 651
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
[POC] Implement caching of .build for unit-tests #2874
Conversation
.github/workflows/swift_matrix.yml
Outdated
@@ -108,6 +112,16 @@ jobs: | |||
if: ${{ matrix.swift.enabled }} | |||
# https://github.com/actions/checkout/issues/766 | |||
run: git config --global --add safe.directory ${GITHUB_WORKSPACE} | |||
|
|||
- name: Restore .build | |||
if: ${{ matrix.swift.enabled && (inputs.cache_key_prefix != '') && !(github.run_attempt > 1) }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!(github.run_attempt > 1)
will be useful when something is inconsistent in CI and you're retrying the job.
Although you can delete cache from the GitHub Actions UI too, on web.
@FranzBusch WDYT? |
We have intentionally not adopted caching yet since it trades potential time saving for cost. I am going to kick-off the CI here since I want to see what the timing impact actually is and see if the trade off in time vs cost is worth it. |
@FranzBusch are you referring to GitHub costs? Furthermore, you can use an S3 bucket for caching with runs-on/cache. |
@FranzBusch based on the code i put there, it won't use the cache in a re-run. Not that, that code is mandatory, but i'll just push something to the branch to see how things will go with the current changes. |
@MahdiBM Can we make re-runs work. Then I can trigger the CI again to see the impact |
@FranzBusch there you go |
So it appears that for swift-nio, it decreases the nightly unit-tests runs from 4m40s to 4m20s. A pretty low difference. Though this difference will get noticeably bigger, the heavier the package / builds are. I'm not sure if there are such heavy repos in the swiftlang/Apple orgs. If you're considering maintaining standardized swift reusable workflows for public users, caching will be necessary IMO in such a workflow. I'll let you decide if you think it's worth it to have the option for caching in the current workflows, or not. |
I keep seeing this in the logs
Any idea @MahdiBM? |
That's kinda what I expected tbh. The time it takes to push and download the cache vs just running a build is too minimal.
These workflows for now are really only intended to be used by library packages in the swiftlang, apple and swift-server organization. So at this time I think we can hold off adding the caching. Though I appreciate you taking the time to create this PR to show the impact of it! |
06db28e
to
e4eca65
Compare
386afcf
to
bed1899
Compare
uses: actions/cache/restore@v4 | ||
with: | ||
path: .build | ||
key: "${{ inputs.cache_key_prefix }}${{ github.event.repository.name }}-build-${{ matrix.swift.swift_version }}-${{ runner.os }}-${{ (github.event.pull_request.base.sha != null) && github.event.pull_request.base.sha || github.event.after }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
${{ (github.event.pull_request.base.sha != null) && github.event.pull_request.base.sha || github.event.after }}
is the main thing that can be different for a specific swift version.
It allows the cache to be updated after each push to the main branch, since it'll resolve to github.event.after
.
It either resolves to the base branch's SHA in PR events, or to the new SHA of the branch in push events.
So each PR will be looking for their base branch's cached .build.
And on a push to the main branch, the cache is refreshed.
It will be suboptimal, but even the event isn't a push or PR, the CI should still use the cache. But maybe also waste some seconds on trying to re-save the cache.
One confusing behavior of the caches action is that in a PR, you cannot create a cache that the next commits in a PR can use. That's why the cache "key" doesn't try to do something like that.
You can only access the repo's primary-branch's caches. [Edit: actually not 100% sure about this, this "rule" might be even more weird, but still, what is in this PR makes sense]
FYI another gotcha of the caches action is that you cannot delete OR override value of a cache "key" (not programmatically, otherwise there is a delete button in GitHub web UI).
@FranzBusch Without the cache-save step which shouldn't be usually triggered, there will be a 30s more time cut, at which point it might be worth it (50s cut in this repo for nightly debug builds for unit tests). I figured out why the cache-save is hit at all, when it shouldn't be. Now, we can check out the package and everything will be fine, but the problem is this repository does not commit the |
@FranzBusch TLDR; The cache-save step should no longer be triggered now, if the cache save is going to fail anyways. |
Motivation:
Faster tests. Less CI time / costs.
Not sure if this is what you would want at all, but if you do, I think this (possibly with minor fixes) should work properly.
Modifications:
Add an option to
swift_matrix
to do caching of.build
.For now, enable this caching in unit-tests.
Result:
Faster tests. Less CI time / costs.