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

[RunAllTests] Remove caching for Bazel CI workflows #2884

Merged
merged 3 commits into from
Mar 11, 2021

Conversation

BenHenning
Copy link
Member

@BenHenning BenHenning commented Mar 11, 2021

Bazel caching is currently not cost effective: it only caches 25-30% of actions, and we're not seeing worthwhile performance benefits from it. This PR makes caching configurable for both unit tests and binary builds, and disables it for CI workflows. #1861 is tracking improving this over time, including upstream patching Bazel itself to fix some of the caching issues. We may revisit
reenabling caching at that time.

Runs verifying that caching is now disabled:

Builds
image

Unit tests
image

Remove caching from Bazel tests.
Remove caching for Bazel builds.
@BenHenning
Copy link
Member Author

@seanlip @rt4914 @anandwana001 assigning this broadly to try and get the PR in sooner since it's a priority. Please feel free to let it get auto-merged if any of you approve it (no need for more than one approval here, I think). Please reach out if you have any questions or concerns.

Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@BenHenning BenHenning changed the title Remove caching for Bazel CI workflows [RunAllTests] Remove caching for Bazel CI workflows Mar 11, 2021
@BenHenning BenHenning closed this Mar 11, 2021
@seanlip seanlip removed their assignment Mar 11, 2021
@seanlip
Copy link
Member

seanlip commented Mar 11, 2021

(or not?)

EDIT: Ah, ignore that. I was confused about why the PR got closed, which is explained in the next comment.

@BenHenning
Copy link
Member Author

Restarting CI to force Bazel tests to run to verify that they are actually not using caching anymore.

@BenHenning BenHenning reopened this Mar 11, 2021
Fix environment variable conditions.
@BenHenning
Copy link
Member Author

Thanks for the super fast review @seanlip!

@BenHenning BenHenning enabled auto-merge (squash) March 11, 2021 07:55
@BenHenning BenHenning merged commit 6875935 into develop Mar 11, 2021
@BenHenning BenHenning deleted the remove-caching-bazel-ci branch March 11, 2021 08:20
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