Skip to content

[Github Actions] Cache builds of cmdstan in the CI #423

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 10 commits into from
Aug 8, 2021

Conversation

WardBrian
Copy link
Member

@WardBrian WardBrian commented Aug 5, 2021

Submission Checklist

  • Run unit tests
  • Declare copyright holder and open-source license: see below

Summary

This PR changes the github actions CI to cache the build of cmdstan for each platform

This means that subsequent re-runs of the CI will not need to build CmdStan from source unless either

  1. install_cmdstan.py has changed (to ensure continued correctness of the install script)
  2. cmdstan releases a new version (determined in the same way cmdstanpy does, looking at the github release API json)

Why?

Nearly half of the test time was spent building cmdstan, which changes much less often than other parts of the package. This speeds that up dramatically, from ~10 minutes on MacOS down to ~1 minute, ~3 minutes on Linux to ~15 seconds, and ~5 minutes on Windows to ~1 minute.

We can even decide to make this faster, if we want. Currently, the CI still downloads the tarball and builds the example model each time, because of this check:

and os.path.exists(
os.path.join(
cmdstan_version,
'examples',
'bernoulli',
'bernoulli' + EXTENSION,
)

and the fact that the CI runs scripts/clean_examples.py each time. If we remove this script call, cmdstanpy will notice that the build has been cached and the 'install cmdstan' step should take <10 seconds on all platforms unless one of the two above cache-miss conditions occurs.

Copyright and Licensing

Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company):
Simons Foundation

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

@WardBrian WardBrian changed the title Try cacheing [Github Actions] Cache builds of cmdstan in the CI Aug 5, 2021
@WardBrian
Copy link
Member Author

I'd love to discuss this with you @mitzimorris

@stan-dev stan-dev deleted a comment from codecov-commenter Aug 5, 2021
@mitzimorris
Copy link
Member

speeding up unit tests would be a huge savings ($$$) to project, so yes, we should get rid of the clean_examples.py script.

@WardBrian
Copy link
Member Author

After this CI run completes I'll push another formatting commit which will also serve to test that we're getting the best times possible. If that all goes smoothly this will be ready to merge and should cut our test run times in half or more, especially for macos/windows

We will want to squash this merge I think, no reason seeing all the test commits

@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2021

Codecov Report

Merging #423 (c762c28) into develop (ea0f3f2) will decrease coverage by 0.09%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #423      +/-   ##
===========================================
- Coverage    77.31%   77.21%   -0.10%     
===========================================
  Files           27       27              
  Lines         8556     8559       +3     
===========================================
- Hits          6615     6609       -6     
- Misses        1941     1950       +9     
Impacted Files Coverage Δ
a/cmdstanpy/cmdstanpy/cmdstanpy/model.py 80.43% <0.00%> (-0.61%) ⬇️
cmdstanpy/cmdstanpy/model.py 79.34% <0.00%> (-0.60%) ⬇️
...runner/work/cmdstanpy/cmdstanpy/cmdstanpy/model.py 79.34% <0.00%> (-0.60%) ⬇️
cmdstanpy/cmdstanpy/compiler_opts.py 93.33% <0.00%> (-0.22%) ⬇️
a/cmdstanpy/cmdstanpy/cmdstanpy/compiler_opts.py 93.33% <0.00%> (-0.22%) ⬇️
...ork/cmdstanpy/cmdstanpy/cmdstanpy/compiler_opts.py 93.33% <0.00%> (-0.22%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea0f3f2...c762c28. Read the comment docs.

@WardBrian
Copy link
Member Author

Actually, seems to be a small issue with the macos tests - not sure if I can use curl to get the cmdstan version. I'll fix it up tomorrow first thing

@ahartikainen
Copy link
Contributor

huge savings ($$$) to project

Do we pay something on these tests? (And we rarely run them)

@mitzimorris
Copy link
Member

yes, our CI is mostly done on AWS, and the monthly server bills are considerable. granted, CmdStanPy is a small fraction of the CI load, so maybe not that big savings of $$, but yes, savings.
from the dev point of view, when submitting a PR, it's nice if tests complete quickly, because that means that can keep working on a problem, as opposed to picking it up and then putting it down while waiting on CI. granted, sometimes it's useful to switch gears. other times, just frustrating.

@WardBrian
Copy link
Member Author

While I was assuming this would have some financial benefit, my main motivator for looking into it was the speed of the tests. We would often spend 10+ minutes in CI before the first test was actually run by pytest. Now the bulk of the time should be either running our linters (flake8/pylint/mypy seem to take ~2 minutes; I believe at least mypy supports caching it's result, which may be worth looking into) and pytest (variable depending on platform, 5-10 minutes)

@ahartikainen
Copy link
Contributor

yes, our CI is mostly done on AWS, and the monthly server bills are considerable. granted, CmdStanPy is a small fraction of the CI load, so maybe not that big savings of $$, but yes, savings.
from the dev point of view, when submitting a PR, it's nice if tests complete quickly, because that means that can keep working on a problem, as opposed to picking it up and then putting it down while waiting on CI. granted, sometimes it's useful to switch gears. other times, just frustrating.

These are run on Github Action (Azure)?

(I'm not against caching stuff)

@WardBrian WardBrian marked this pull request as ready for review August 6, 2021 14:31
@WardBrian WardBrian marked this pull request as draft August 6, 2021 14:32
@WardBrian
Copy link
Member Author

Still running into an issue where some tests don't get the version number populated. I thought this was a MacOS problem, as it only seems to affect it there, but now it seems to only fail some of the time, so I'm starting to think this is making too many requests to the api in short succession and the later ones are getting 403'd.

Unsure of a consistent/safe way around this, I'll try to look around

@mitzimorris
Copy link
Member

mitzimorris commented Aug 6, 2021

These are run on Github Action (Azure)?

OK, maybe I'm wrong - @serban-nicusor-toptal - do you know which CI cycles are metered and which are free? also, any advice w/ current snag in this PR? (see above)

@WardBrian
Copy link
Member Author

Reorganized to only look up the cmdstan version once per CI run and use that for all jobs. This means the issue with some jobs getting 403'd should not occur, so this is good to go now.

Again, squash when merging IMO -- it got a little messy with the testing

@WardBrian WardBrian marked this pull request as ready for review August 6, 2021 16:17
@mitzimorris
Copy link
Member

lgtm, but I defer to @ahartikainen on this one.

@serban-nicusor-toptal
Copy link
Contributor

serban-nicusor-toptal commented Aug 7, 2021

Hey @mitzimorris afaik we're into the open source category which gives us free 2000 ci minutes/month for Github Actions.
This is a very nice optimization of the pipeline @WardBrian thanks!

PS: LGTM!

Copy link
Contributor

@ahartikainen ahartikainen left a comment

Choose a reason for hiding this comment

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

LGTM

@ahartikainen ahartikainen merged commit 1f01d72 into develop Aug 8, 2021
@WardBrian WardBrian deleted the cache-actions-ci branch August 9, 2021 13:10
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.

5 participants