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

Speed up Github Actions CI #2635

Merged
merged 1 commit into from
Nov 22, 2022
Merged

Speed up Github Actions CI #2635

merged 1 commit into from
Nov 22, 2022

Conversation

levskaya
Copy link
Collaborator

Also enable more parallelism in CI tests.

@codecov-commenter
Copy link

codecov-commenter commented Nov 19, 2022

Codecov Report

Merging #2635 (20cca7b) into main (3b27a69) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2635   +/-   ##
=======================================
  Coverage   80.53%   80.53%           
=======================================
  Files          50       50           
  Lines        5313     5313           
=======================================
  Hits         4279     4279           
  Misses       1034     1034           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@levskaya
Copy link
Collaborator Author

So this shaves off some time (maybe 3-5 min on average?) from our CI tests by using a larger 16-core test-runner, esp. due to faster pytype runs.

However we spend about half the time for all the pytest/doctest/pytype runs on just re-installing dependencies. I've tried caching the downloaded packages, but I feel like we might save more time by caching the installed packages - that would save another 2-3 min per run. See this interesting comment about caching the installed packages... maybe we should think about doing something like that to speed this up further.

Side note: We can also soon drop the 3.7 tests in favor of 3.10 tests. We may also want to add a mypy typecheck job as well.

@levskaya levskaya requested a review from marcvanzee November 19, 2022 09:27
@levskaya
Copy link
Collaborator Author

Addresses #2620

@levskaya
Copy link
Collaborator Author

NB: for caching e.g. a full virtualenv directory, the usual recommendation is to hash on the requirements.txt file, and to pin versions so as to be specific... however that's annoying - maybe we can invalidate caches every week using the date instead.

@levskaya levskaya force-pushed the fasterci branch 4 times, most recently from 72d267a to fc5b55a Compare November 20, 2022 05:07
@levskaya
Copy link
Collaborator Author

levskaya commented Nov 20, 2022

OK I updated this to directly cache the virtual-environment directory post-installation so we don't waste any time installing all of our extensive set of deps for every single test. As part of the cache key I added the week-number and year, so that the caches will automatically invalidate every week, so that we're testing our unpinned dependencies at their newest versions. (Note that caches can be manually deleted under actions -> management -> caches)

Github supports 10G of caches per repo, our installed venv comes in at ~2.2Gb currently, allowing us to store enough for our 3 python version testing matrix.

So far the faster runners and caching drop our CI testing time: 17-19min --> 6m 15s

@levskaya levskaya changed the title Enable faster test runners. Speed up Github Actions CI Nov 20, 2022
@levskaya levskaya force-pushed the fasterci branch 2 times, most recently from 9af1472 to 9761482 Compare November 20, 2022 05:50
Enable more parallelism in CI tests.
Add caching of venv installation with weekly forced invalidation.
@levskaya
Copy link
Collaborator Author

The last major issue is that the codecov uploader can often take up to 1 minute to upload... this is horrible, and I don't think delaying every single CI test by ~30-60s is really worth it for a simple coverage indicator that none of us even probably look at.

uses: actions/cache@v3
with:
path: venv
key: pip-${{ steps.setup_python.outputs.python-version }}-${{ steps.week_year.outputs.DATE }}-${{ hashFiles('**/requirements.txt', 'setup.py') }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the recommended way to launch a Github build that forces an uncached environment?
(This might be useful to test dependency interplay at HEAD).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can just delete the caches from the Actions submenu. We could also probably figure out some way to signal against using caches, but in practice just deleting them is probably enough.

Copy link
Collaborator

@marcvanzee marcvanzee left a comment

Choose a reason for hiding this comment

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

Nice, thanks for adding this! Out of curiosity: do you have an idea how much of the speed improvement is due to the caching and how much is due to the improved pytype testing parallelism?

@levskaya
Copy link
Collaborator Author

Nice, thanks for adding this! Out of curiosity: do you have an idea how much of the speed improvement is due to the caching and how much is due to the improved pytype testing parallelism?

Kinda 50% / 50% - pytype is slow and really benefits from a larger machine (as do the rest of the tests) - but we were also wasting >3min or more for every test runner re-installing everything from scratch.

@copybara-service copybara-service bot merged commit d640b5a into google:main Nov 22, 2022
@marcvanzee marcvanzee mentioned this pull request Dec 13, 2022
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.

4 participants