-
Notifications
You must be signed in to change notification settings - Fork 28
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
Fix unit tests #1462
Fix unit tests #1462
Conversation
Probably related to python/cpython#124651 |
If we can't get this working can we just drop the caching and go back to installing each time? It only takes like ~1 minute |
|
||
- name: Restore Python environment cache | ||
if: env.has_changes == 'true' | ||
id: restore-env | ||
uses: actions/cache/restore@v4 | ||
with: | ||
path: .venv-${{ matrix.python-version }} | ||
key: ${{ runner.os }}-venv-${{ matrix.python-version }}-${{ hashFiles('devtools/dev-requirements.txt', 'requirements.txt') }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of using matrix.python-version
as the key why can't we use the actual version from python --version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can. I don't know if the syntax will allow it, but that could resolve inconsistent Python version problem. However, we might need to clean some old cache created with older Python versions every 3-4 months.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw this problem will probably be solved when every Github runner starts using 3.12.8 and 3.11.11. For now, some of them use 3.12.7 and this causes the issue. They made some security changes regarding the venv
activation with the latest patch
| benchmark_name | dt(%) | dt(s) | t_new(s) | t_old(s) |
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
test_build_transform_fft_lowres | +10.37 +/- 9.99 | +5.43e-02 +/- 5.24e-02 | 5.78e-01 +/- 2.9e-02 | 5.24e-01 +/- 4.4e-02 |
-test_equilibrium_init_medres | +14.06 +/- 3.44 | +5.75e-01 +/- 1.41e-01 | 4.66e+00 +/- 9.9e-02 | 4.09e+00 +/- 1.0e-01 |
test_equilibrium_init_highres | +1.34 +/- 5.34 | +7.42e-02 +/- 2.95e-01 | 5.61e+00 +/- 2.3e-01 | 5.53e+00 +/- 1.8e-01 |
test_objective_compile_dshape_current | +2.02 +/- 5.36 | +7.96e-02 +/- 2.11e-01 | 4.01e+00 +/- 4.1e-02 | 3.93e+00 +/- 2.1e-01 |
test_objective_compute_dshape_current | +0.01 +/- 2.66 | +5.25e-07 +/- 1.38e-04 | 5.19e-03 +/- 3.9e-05 | 5.19e-03 +/- 1.3e-04 |
test_objective_jac_dshape_current | +3.25 +/- 8.99 | +1.40e-03 +/- 3.88e-03 | 4.46e-02 +/- 2.3e-03 | 4.32e-02 +/- 3.1e-03 |
test_perturb_2 | +0.46 +/- 5.94 | +9.16e-02 +/- 1.18e+00 | 2.00e+01 +/- 1.1e+00 | 1.99e+01 +/- 3.8e-01 |
test_proximal_freeb_jac | -0.40 +/- 0.87 | -2.95e-02 +/- 6.43e-02 | 7.36e+00 +/- 4.0e-02 | 7.39e+00 +/- 5.1e-02 |
test_solve_fixed_iter | -3.48 +/- 2.13 | -1.19e+00 +/- 7.28e-01 | 3.31e+01 +/- 5.6e-01 | 3.42e+01 +/- 4.6e-01 |
test_LinearConstraintProjection_build | -5.49 +/- 4.84 | -5.89e-01 +/- 5.19e-01 | 1.01e+01 +/- 2.2e-01 | 1.07e+01 +/- 4.7e-01 |
test_build_transform_fft_midres | -0.81 +/- 3.49 | -4.96e-03 +/- 2.15e-02 | 6.09e-01 +/- 1.6e-02 | 6.14e-01 +/- 1.4e-02 |
test_build_transform_fft_highres | -0.10 +/- 1.89 | -1.01e-03 +/- 1.84e-02 | 9.74e-01 +/- 1.5e-02 | 9.75e-01 +/- 1.1e-02 |
test_equilibrium_init_lowres | -1.30 +/- 1.52 | -5.10e-02 +/- 5.95e-02 | 3.87e+00 +/- 3.1e-02 | 3.92e+00 +/- 5.1e-02 |
test_objective_compile_atf | +1.06 +/- 4.47 | +8.62e-02 +/- 3.64e-01 | 8.22e+00 +/- 2.5e-01 | 8.13e+00 +/- 2.7e-01 |
test_objective_compute_atf | -0.21 +/- 2.89 | -3.33e-05 +/- 4.63e-04 | 1.60e-02 +/- 3.0e-04 | 1.60e-02 +/- 3.5e-04 |
test_objective_jac_atf | +0.02 +/- 2.43 | +4.42e-04 +/- 4.81e-02 | 1.98e+00 +/- 3.6e-02 | 1.98e+00 +/- 3.2e-02 |
test_perturb_1 | -0.54 +/- 1.34 | -8.05e-02 +/- 2.00e-01 | 1.48e+01 +/- 1.2e-01 | 1.49e+01 +/- 1.6e-01 |
test_proximal_jac_atf | +1.04 +/- 0.73 | +8.65e-02 +/- 6.05e-02 | 8.37e+00 +/- 4.5e-02 | 8.29e+00 +/- 4.1e-02 |
test_proximal_freeb_compute | -0.79 +/- 1.03 | -1.60e-03 +/- 2.09e-03 | 2.00e-01 +/- 1.5e-03 | 2.02e-01 +/- 1.4e-03 |
test_solve_fixed_iter_compiled | +0.47 +/- 1.10 | +1.03e-01 +/- 2.42e-01 | 2.21e+01 +/- 1.7e-01 | 2.20e+01 +/- 1.7e-01 | |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1462 +/- ##
=======================================
Coverage 95.57% 95.57%
=======================================
Files 98 98
Lines 25156 25156
=======================================
+ Hits 24043 24044 +1
+ Misses 1113 1112 -1 |
The new releases for Python 3.12 and 3.11 introduced some changes to the activation of virtual environments that made the
venv
s incompatible between Python patched 3.12.8 and 3.12.7 which Github runners use. This caused the unit tests to fail (since they are the only ones that use 3.12. and 3.11).This PR changes the caching procedure to include the full Python version and use it if it is the same as the runners use. Otherwise, create a new environment for that job, so it will run either way.
Note: I believe this problem will be resolved automatically once all Github runners use the same Python version (they released new patch last week so it might take another week). But these changes will make it ready for future releases.