Conditional Workflows

[YigitElma]

Resolves #1221

I assume it will not affect a lot of PR's but some small ones (like this one or just docs update) can be faster. The conditions are,

• if no change in desc or requirements don't run notebook tests
• if no change in desc, tests (without benchmarks) or requirements don't run unit and regression tests
• if no change in desc, tests/benchmarks or requirements don't run benchmarks

If there is a change in the workflow file itself, it will run. If unit and regression tests are skipped no coverage report will be uploaded and someone has to override these PRs to merge to master. I have tried uploading 100% fake coverage report but it could have unintentional effects as Rory mentioned.

[YigitElma]

#1235 (comment)

[github-actions]

|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     +0.52 +/- 3.88     | +2.73e-03 +/- 2.03e-02 |  5.26e-01 +/- 1.2e-02  |  5.23e-01 +/- 1.7e-02  |
 test_equilibrium_init_medres            |     -0.28 +/- 1.22     | -1.14e-02 +/- 5.05e-02 |  4.13e+00 +/- 3.1e-02  |  4.14e+00 +/- 4.0e-02  |
 test_equilibrium_init_highres           |     -0.19 +/- 0.89     | -1.03e-02 +/- 4.93e-02 |  5.50e+00 +/- 3.9e-02  |  5.51e+00 +/- 3.0e-02  |
 test_objective_compile_dshape_current   |     -0.21 +/- 1.67     | -7.99e-03 +/- 6.42e-02 |  3.83e+00 +/- 5.5e-02  |  3.84e+00 +/- 3.3e-02  |
 test_objective_compute_dshape_current   |     -0.18 +/- 1.82     | -6.03e-06 +/- 6.26e-05 |  3.43e-03 +/- 4.7e-05  |  3.43e-03 +/- 4.2e-05  |
 test_objective_jac_dshape_current       |     +0.32 +/- 7.99     | +1.30e-04 +/- 3.22e-03 |  4.04e-02 +/- 2.9e-03  |  4.03e-02 +/- 1.4e-03  |
 test_perturb_2                          |     +1.35 +/- 1.56     | +2.33e-01 +/- 2.71e-01 |  1.76e+01 +/- 2.1e-01  |  1.73e+01 +/- 1.7e-01  |
 test_proximal_freeb_jac                 |     -0.36 +/- 1.12     | -2.71e-02 +/- 8.41e-02 |  7.45e+00 +/- 6.9e-02  |  7.48e+00 +/- 4.8e-02  |
 test_solve_fixed_iter                   |     +1.03 +/- 60.80    | +5.10e-02 +/- 3.02e+00 |  5.02e+00 +/- 2.1e+00  |  4.97e+00 +/- 2.2e+00  |

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.44%. Comparing base (12441ac) to head (3f9a193).
Report is 25 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1239   +/-   ##
=======================================
  Coverage   95.44%   95.44%           
=======================================
  Files          95       95           
  Lines       23402    23402           
=======================================
  Hits        22337    22337           
  Misses       1065     1065           

see 1 file with indirect coverage changes

[f0uriest]

If the tests don't run, does github still consider them "passed"? IE, we require the tests to pass before merging, will this cause any issues if they don't run at all?

[YigitElma]

If the tests don't run, does github still consider them "passed"? IE, we require the tests to pass before merging, will this cause any issues if they don't run at all?

This is good question that I don't have the answer of. As a check I can delete the lines that says if the workflow files are changed run the test, for testing that and re add them after. Because basically this pr doesn't need any unit or regression tests.

EDIT: I tried that and it doesn't work, it successfully skips the tests but they still show up as expected: waiting for status to be reported

EDIT 2: Now it works, I upload fake 100% coverage data too

[f0uriest]

hmmm thats kind of annoying. Can you see if you can modify the action in some way to make them pass in that case?

[YigitElma]

hmmm thats kind of annoying. Can you see if you can modify the action in some way to make them pass in that case?

@f0uriest now, it works. The code coverage was failing since nothing was uploaded, I upload a fake 100% coverage data to make that pass (only if actual upload is skipped)

I will re add the paths to the workflow files now, so for this PR each test will run. But I have tested it with commit 2a6e7d3 and it is working, you can check the previous workflows. All the tests except black and linting pass around 20 sec.

[f0uriest]

Any chance this could have some weird unintended consequences?
IE, suppose we merge a PR that only changes some minor thing such that these tests don't run. This then tells codecov that the coverage is 100%. On the next PR where tests do run, we get coverage ~95%, does that mean the 2nd PR fails, even though the actual coverage is fine?

[YigitElma]

Hmm, I don't know how exactly the codecov works, does it compare to the last PR?

Tbh, this was also not my first preference but even tho the unit/regression tests pass the codecov was a problem. Can people with override authorization merge PRs before codecov tests start? Like maybe I can change it back to the version without the fake codecov upload and for these PRs we can override and merge?

[YigitElma]

I thought of it again, but I couldn't find any other way. So, the question is whether this PR will cause other PR's to have less relative coverage and fail, I guess this can be handled by just overwriting the test for that PR. Can you check if codecov not starting at all can be overridden? @f0uriest

[f0uriest]

possibly relevant:

• https://community.codecov.com/t/i-would-like-to-skip-the-codecov-when-source-code-when-files-unrelated-to-the-tests-are-modified/4433/8
• https://community.codecov.com/t/how-to-make-codecov-branch-status-checks-conditional/3549

another option might be removing codecov from the list of required checks? And we just manually verify that it's ok when we expect it to be?

[YigitElma]

possibly relevant:

• https://community.codecov.com/t/i-would-like-to-skip-the-codecov-when-source-code-when-files-unrelated-to-the-tests-are-modified/4433/8
• https://community.codecov.com/t/how-to-make-codecov-branch-status-checks-conditional/3549

another option might be removing codecov from the list of required checks? And we just manually verify that it's ok when we expect it to be?

I think the way it is now is fine. It won't upload anything and we will just override it for these kind of PRs which are small in number. The links you sent say this is still under development. I couldn't find a good way of doing this