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

Higher splitting for Github Actions and Cache venv #1213

Merged
merged 68 commits into from
Aug 29, 2024
Merged

Conversation

rahulgaur104
Copy link
Collaborator

@rahulgaur104 rahulgaur104 commented Aug 21, 2024

Here is the description for this PR by @YigitElma

  • I created a new workflow that runs every 2 days and updates the requirements and stores them in a venv that is cached
  • Black, unit, regression, notebook and benchmark workflows use the cached venv (which takes around 4-8seconds whereas installing dependencies usually more than a minute)
  • Increase the spitting of unit, notebook, and benchmark workflows.
  • For some reason I couldn't make the linting test faster using venv (so set it back to old version which is fine), actually for some reason venv made it way slower around 8 minutes?!!!?

Other Fixes

  • We stopped using Python 3.8 in our tests with Bump python versions #923 but we forgot to remove them from weekly tests. Since then (I guess the dates check) the weekly tests fail (for the last 3 months). I removed Python 3.8 from weekly tests.

@dpanici
Copy link
Collaborator

dpanici commented Aug 21, 2024

Does this work with pytest-split?

@rahulgaur104
Copy link
Collaborator Author

rahulgaur104 commented Aug 21, 2024

Does this work with pytest-split?

I am using pytest-xdist. It definitely works on della and my laptop. Is pytest-split better or already being used?

Copy link
Contributor

github-actions bot commented Aug 21, 2024

|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     +0.82 +/- 4.27     | +4.23e-03 +/- 2.20e-02 |  5.18e-01 +/- 1.3e-02  |  5.14e-01 +/- 1.8e-02  |
 test_equilibrium_init_medres            |     +0.97 +/- 1.08     | +3.93e-02 +/- 4.38e-02 |  4.11e+00 +/- 3.3e-02  |  4.07e+00 +/- 2.8e-02  |
 test_equilibrium_init_highres           |     +0.97 +/- 1.91     | +5.24e-02 +/- 1.04e-01 |  5.48e+00 +/- 8.0e-02  |  5.43e+00 +/- 6.6e-02  |
 test_objective_compile_dshape_current   |     +0.61 +/- 1.14     | +2.30e-02 +/- 4.33e-02 |  3.82e+00 +/- 3.9e-02  |  3.80e+00 +/- 1.8e-02  |
 test_objective_compute_dshape_current   |     +1.62 +/- 2.29     | +5.58e-05 +/- 7.90e-05 |  3.51e-03 +/- 6.2e-05  |  3.45e-03 +/- 4.9e-05  |
 test_objective_jac_dshape_current       |     -1.30 +/- 6.81     | -5.35e-04 +/- 2.80e-03 |  4.05e-02 +/- 1.7e-03  |  4.10e-02 +/- 2.2e-03  |
 test_perturb_2                          |     +0.79 +/- 1.33     | +1.37e-01 +/- 2.29e-01 |  1.74e+01 +/- 1.6e-01  |  1.73e+01 +/- 1.6e-01  |
 test_proximal_freeb_jac                 |     -0.13 +/- 1.19     | -9.85e-03 +/- 8.90e-02 |  7.48e+00 +/- 7.8e-02  |  7.49e+00 +/- 4.3e-02  |
 test_solve_fixed_iter                   |     -0.31 +/- 59.23    | -1.52e-02 +/- 2.91e+00 |  4.90e+00 +/- 2.0e+00  |  4.92e+00 +/- 2.1e+00  |
 test_build_transform_fft_midres         |     +0.46 +/- 6.97     | +2.77e-03 +/- 4.16e-02 |  6.01e-01 +/- 2.5e-02  |  5.98e-01 +/- 3.3e-02  |
 test_build_transform_fft_highres        |     +0.12 +/- 2.10     | +1.18e-03 +/- 2.07e-02 |  9.88e-01 +/- 1.2e-02  |  9.86e-01 +/- 1.7e-02  |
 test_equilibrium_init_lowres            |     +0.62 +/- 1.85     | +2.31e-02 +/- 6.91e-02 |  3.76e+00 +/- 4.6e-02  |  3.74e+00 +/- 5.2e-02  |
 test_objective_compile_atf              |     +0.41 +/- 3.58     | +3.14e-02 +/- 2.75e-01 |  7.72e+00 +/- 2.5e-01  |  7.69e+00 +/- 1.2e-01  |
 test_objective_compute_atf              |     +1.08 +/- 2.24     | +1.09e-04 +/- 2.28e-04 |  1.03e-02 +/- 1.6e-04  |  1.02e-02 +/- 1.7e-04  |
 test_objective_jac_atf                  |     +1.56 +/- 4.85     | +2.96e-02 +/- 9.20e-02 |  1.92e+00 +/- 7.6e-02  |  1.90e+00 +/- 5.1e-02  |
 test_perturb_1                          |     +9.60 +/- 3.97     | +1.16e+00 +/- 4.81e-01 |  1.33e+01 +/- 4.5e-01  |  1.21e+01 +/- 1.8e-01  |
 test_proximal_jac_atf                   |     +0.04 +/- 0.72     | +2.92e-03 +/- 5.87e-02 |  8.14e+00 +/- 4.3e-02  |  8.14e+00 +/- 3.9e-02  |
 test_proximal_freeb_compute             |     +1.47 +/- 1.43     | +2.65e-03 +/- 2.58e-03 |  1.83e-01 +/- 2.5e-03  |  1.81e-01 +/- 7.1e-04  |

@rahulgaur104
Copy link
Collaborator Author

Nah, everything slows down. But I think using this on della and your laptop is still a good idea.

@f0uriest
Copy link
Member

Yeah I tried this a while back. IIRC another issue is it would often crash on CI causing random failures, so even if it was faster it's also a lot more annoying to have to restart jobs constantly

@YigitElma
Copy link
Collaborator

@rahulgaur104 Can I use this PR to test if increasing the splitting helps?

@rahulgaur104
Copy link
Collaborator Author

@YigitElma Go ahead!

@YigitElma
Copy link
Collaborator

So, I counted the jobs running at the same time, and I think we dont have team account, so only 20 jobs at a time. With this change, I increased the number jobs per pr to 21 but 2 of them (black and linting) takes a minute to complete, hence 19 actual jobs. This means CI will test a single pr at a time but since each job is shorter, we might still be better off.

@YigitElma
Copy link
Collaborator

This is not a solution to @f0uriest s point of running tests locally under 30mins, but still using github resources more efficiently.

@YigitElma
Copy link
Collaborator

Also, we can try to add a dependency cache which gets updated every week. https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/caching-dependencies-to-speed-up-workflows

@YigitElma YigitElma changed the title Speeding up pytest using mutliple process Higher splitting for Github Actions Aug 21, 2024
@YigitElma YigitElma changed the title Higher splitting for Github Actions Higher splitting for Github Actions and Cache venv Aug 21, 2024
@PlasmaControl PlasmaControl deleted a comment from codecov bot Aug 27, 2024
Copy link

codecov bot commented Aug 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.33%. Comparing base (f21d65b) to head (76dad87).
Report is 69 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1213      +/-   ##
==========================================
- Coverage   95.33%   95.33%   -0.01%     
==========================================
  Files          90       90              
  Lines       22702    22702              
==========================================
- Hits        21643    21642       -1     
- Misses       1059     1060       +1     

see 1 file with indirect coverage changes

@YigitElma YigitElma marked this pull request as ready for review August 27, 2024 14:37
@YigitElma YigitElma added the testing Adding a new test or fixing an existing one label Aug 27, 2024
strategy:
matrix:
python-version: ['3.9']
group: [1, 2]
Copy link
Member

Choose a reason for hiding this comment

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

im not sure if we want to split the benchmarks, since they need to run on the same hardware for it to be a meaningful comparison

Copy link
Collaborator

Choose a reason for hiding this comment

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

When we split master and latest PR still runs on the same hardware. The only difference is some portion of the benchmarks runs on the second job. I am assuming pytest-split always splits the same tests. SO, compile and jac tests may run on different hardware but the same test for aster and PR runs on the same job and thus hardware

.github/workflows/pythonpip.yml Outdated Show resolved Hide resolved
.github/workflows/pythonpip.yml Outdated Show resolved Hide resolved
.github/workflows/regression_test.yml Show resolved Hide resolved
.github/workflows/unittest.yml Show resolved Hide resolved
@dpanici
Copy link
Collaborator

dpanici commented Aug 28, 2024

mark new actions as "required to pass" before merging in repo settings once this is in

@YigitElma
Copy link
Collaborator

mark new actions as "required to pass" before merging in repo settings once this is in

I tried to do that but I don't have access to those settings. For me, the settings page is very limited. @f0uriest or @dpanici can you do that?

@YigitElma
Copy link
Collaborator

YigitElma commented Aug 28, 2024

And also you may squash merge this PR, because I committed relentlessly to test the github actions. So, the commit history is really ugly

@f0uriest f0uriest merged commit 416b5fb into master Aug 29, 2024
24 checks passed
@f0uriest f0uriest deleted the rg/parallel-testing branch August 29, 2024 01:18
f0uriest added a commit that referenced this pull request Aug 29, 2024
Some stuff that was missing from #1213 

- Updates readme badges to point to the renamed gh actions
- Tells codecov to expect 14 batches of coverage data, not 10 (we used
to have 6 regression + 4 unit, now 6 regression + 8 unit)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Adding a new test or fixing an existing one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants