separate tox testenvs to cache CRDS reference files retrieved by initial test #7323

zacharyburnett · 2022-10-28T13:37:27Z

This PR sets up dependent CI jobs and attempts to cache CRDS reference files, succeeding #6867 with suggestions from #6867 (review).

Checklist for maintainers

added entry in CHANGES.rst within the relevant release section
updated or added relevant tests
updated relevant documentation
added relevant milestone
added relevant label(s)
ran regression tests, post a link to the Jenkins job below.
How to run regression tests on a PR
Make sure the JIRA ticket is resolved properly

jdavies-st

This looks good! Some comments below.

.github/workflows/ci.yml

tox.ini

.github/workflows/ci.yml

.github/workflows/ci_cron.yml

codecov · 2022-10-31T21:42:11Z

Codecov Report

Base: 79.66% // Head: 79.63% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (dc70d1a) compared to base (91cde26).
Patch has no changes to coverable lines.

❗ Current head dc70d1a differs from pull request most recent head 1b703b3. Consider uploading reports for the commit 1b703b3 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7323      +/-   ##
==========================================
- Coverage   79.66%   79.63%   -0.04%     
==========================================
  Files         411      411              
  Lines       37347    37350       +3     
==========================================
- Hits        29752    29743       -9     
- Misses       7595     7607      +12

Flag	Coverage Δ		*Carryforward flag
nightly	`79.61% <ø> (-0.04%)`	⬇️	Carriedforward from 88b8747
unit	`51.98% <ø> (-0.01%)`	⬇️

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files	Coverage Δ
jwst/tweakreg/tweakreg_catalog.py	`89.47% <0.00%> (-5.27%)`	⬇️
jwst/tweakreg/tweakreg_step.py	`55.00% <0.00%> (-4.62%)`	⬇️
jwst/regtest/conftest.py	`88.04% <0.00%> (-1.64%)`	⬇️
jwst/associations/lib/rules_level3_base.py	`93.50% <0.00%> (-0.30%)`	⬇️
jwst/lib/wcs_utils.py	`78.57% <0.00%> (+3.57%)`	⬆️
jwst/pipeline/calwebb_image3.py	`88.63% <0.00%> (+4.54%)`	⬆️
jwst/transforms/integration.py	`92.85% <0.00%> (+7.14%)`	⬆️
jwst/datamodels/integration.py	`90.90% <0.00%> (+9.09%)`	⬆️

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

…ial test

…hash)

jdavies-st

This is a hard nut to crack.

I really like the refactor of tox.ini. Structurally much better and easier to work with, and importantly, a good distinction between checks and tests. 🚀

I don't like the needs: [ quick_test ] dependency, as this dramatically increases the runtime of the test suite, likely doubling it. And it seems the main reason behind it is to get CRDS caching to work. Would be better to get the caching to work and keep the flat, concurrent set of checks we already have, I think.

The main problem with the CRDS cache is that we are trying to restore a cache whose contents we are using to make a hash. But it needs to be there before we can hash it, but we need to hash it before it is restored. Chicken, egg.

The next-best thing would be to hash files that exist after repo checkout, or some other state that corresponds with a state of the CRDS cache. While the cache of files needed for the tests will not change with every increment of the default context, it's probably the best proxy we have. Of course it needs to be created first and cached before it can be used, so still the chicken-egg problem, which I recall having a helluva time trying to get sorted when I first implemented (then unimplemented) this caching long ago.

I've been pinging some ideas around today and might have a solution based off the nice tox refactor you did here. Look for a PR.

jdavies-st · 2022-11-02T11:57:35Z

.github/workflows/ci.yml

+      - uses: actions/cache@v3
+        with:
+          path: .tox
+          key: tox-${{ matrix.toxenv }}-${{ env.pythonLocation }}-${{ runner.os }}-${{ hashFiles('pyproject.toml', 'setup.*') }}


I think it is not a good idea to cache .tox, especially since the requirements in setup.cfg do not accurately represent the state of the packages installed on any given run of the CI. I.e. setup.cfg could not change for months, meanwhile there may be patch releases of numpy and other direct dependencies, or other indirect dependencies. This caching would mask such updates to dependencies and defeat the purpose of daily CI testing.

Oh, nevermind, I see this has been removed. 👍

jdavies-st · 2022-11-03T20:30:48Z

.github/workflows/ci.yml

          - name: Code style check
-            os: ubuntu-latest
-            python-version: 3.9
-            toxenv: style
-
+            toxenv: check-style


These and the other 2 are already in the matrix above. Probably no need to add them here.

jdavies-st · 2022-11-03T20:32:11Z

.github/workflows/ci.yml

-        if: ${{ contains(matrix.toxenv,'-cov') }}
+      - run: pip install tox
+      - run: tox -e ${{ matrix.toxenv }}
+  quick_test:


Is the sole purpose of this quick_test to generate a cache that the other tests can use?

jdavies-st · 2022-11-03T20:32:53Z

.github/workflows/ci.yml

+          path: reference_files_hash.txt
+  test:
+    name: ${{ matrix.toxenv }} (Python ${{ matrix.python-version }}, ${{ matrix.os }})
+    needs: [ quick_test ]


Since these tests don't even kick off until quick_test is finished, this is going to ~double the runtime of the test suite.

jdavies-st · 2022-11-03T20:36:22Z

.github/workflows/ci_cron.yml

-      - name: Upload coverage to codecov
-        if: ${{ contains(matrix.toxenv,'-cov') }}
-        uses: codecov/codecov-action@v3


Removing coverage reporting here is probably a good idea since nothing is changing on master when this cron job runs.

jdavies-st · 2022-11-03T20:51:51Z

.github/workflows/ci.yml

+      matrix:
+        toxenv: [ test-xdist, test-oldestdeps, test-sdpdeps-xdist ]
+        python-version: [ '3.8', '3.9', '3.10', '3.11' ]
+        os: [ ubuntu-latest, macos-latest ]


And I don't think we should be running on macos. The runners are too slow, and they don't test anything extra compared to the linux runners. If our test suite was 3 minutes, fine. But it is over an hour on the macos runners.

Likewise, oldestdeps probably only needs to run on the oldest supported version of python (and in fact probably only can), and the sdpdeps probably only should run on whatever version of python DMS/SDP is currently using, python 3.9 currently, i.e. whatever our JenkinsfileRT says.

jdavies-st · 2022-11-03T20:56:03Z

The other thing to keep in mind is that caches created in branches cannot be used on master, though caches created on master can be used on branches.

https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows#restrictions-for-accessing-a-cache

So this complicates any rollout. Probably will have to be 2 successive PRs. Chicken. Egg.

zacharyburnett self-assigned this Oct 28, 2022

github-actions bot added automation Continuous Integration (CI) and testing automation tools testing labels Oct 28, 2022

zacharyburnett force-pushed the cache_crds branch 2 times, most recently from 5a9a659 to 1637313 Compare October 28, 2022 13:43

zacharyburnett mentioned this pull request Oct 28, 2022

use GitHub Actions to cache environments and use dependent CI workflow structure #6867

Closed

5 tasks

zacharyburnett requested a review from jdavies-st October 28, 2022 14:46

zacharyburnett force-pushed the cache_crds branch from 268228f to ffbae47 Compare October 28, 2022 18:49

jdavies-st reviewed Oct 31, 2022

View reviewed changes

zacharyburnett force-pushed the cache_crds branch 2 times, most recently from 01ee2df to 2013cf4 Compare October 31, 2022 19:34

zacharyburnett force-pushed the cache_crds branch 3 times, most recently from c849a15 to 752d22b Compare November 1, 2022 17:26

zacharyburnett added 15 commits November 2, 2022 12:05

separate tox testenvs to cache CRDS reference files retrieved by init…

752620e

…ial test

simplify cron CI

1a230a9

fix checks job

9bca5be

checkout tags

9aaf5a0

fix toxenv

5e51b43

rename toxenvs

ed8aeee

consolidate jobs

f30c7b7

run full matrix

cee87d6

test pyargs once

0cccaaa

fix CRDS path and disable fail-fast

800ed16

use string concatenation with join()

f660d92

use string concatenation with format()

790e04c

rename jobs

0d53fcf

fill matrix

91d9fec

print home dir

1da95ad

zacharyburnett added 18 commits November 2, 2022 12:05

print CRDS dir

4d13fb4

print CRDS dir

310b9c0

hash reference files

f8791fd

add change log entry

a88e1ba

set desired CRDS context from input

26e0464

install CRDS

94d80d6

attempt cache after first test (to construct key from reference file …

dd54c22

…hash)

remove -n auto from default testenv

63cdbb3

use Python 3.10 for checks

8edcd82

remove manual usage of crds sync

f1e5f98

fix comma

208dbe7

save reference files hash for future cache keys

045e5bd

only hash files once

0380292

use latest Python in checks and build

4f6569f

use pytest-xdist in bottleneck

dc8c61f

rename jobs

40b7843

refactor matrices

2f921f5

cache .tox environment

f77fc21

zacharyburnett force-pushed the cache_crds branch from 962eeb3 to f77fc21 Compare November 2, 2022 16:06

exclude .eggs and build directories from test discovery

bd23c6c

github-actions bot added the installation label Nov 2, 2022

don't cache tox environment

1b703b3

zacharyburnett force-pushed the cache_crds branch from c96fd33 to 1b703b3 Compare November 3, 2022 13:03

jdavies-st reviewed Nov 3, 2022

View reviewed changes

jdavies-st mentioned this pull request Nov 4, 2022

Cache CRDS_PATH reffile contents in CI workflows #7333

Merged

zacharyburnett closed this Nov 29, 2022

zacharyburnett deleted the cache_crds branch November 29, 2022 14:00

jdavies-st mentioned this pull request Jan 27, 2023

test opencv functionality separately #7414

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

separate tox testenvs to cache CRDS reference files retrieved by initial test #7323

separate tox testenvs to cache CRDS reference files retrieved by initial test #7323

zacharyburnett commented Oct 28, 2022 •

edited

Loading

jdavies-st left a comment

codecov bot commented Oct 31, 2022 •

edited

Loading

jdavies-st left a comment

jdavies-st Nov 2, 2022

jdavies-st Nov 3, 2022

jdavies-st Nov 3, 2022

jdavies-st Nov 3, 2022

jdavies-st Nov 3, 2022

jdavies-st Nov 3, 2022

jdavies-st Nov 3, 2022

jdavies-st commented Nov 3, 2022 •

edited

Loading

separate tox testenvs to cache CRDS reference files retrieved by initial test #7323

separate tox testenvs to cache CRDS reference files retrieved by initial test #7323

Conversation

zacharyburnett commented Oct 28, 2022 • edited Loading

jdavies-st left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 31, 2022 • edited Loading

Codecov Report

jdavies-st left a comment

Choose a reason for hiding this comment

jdavies-st Nov 2, 2022

Choose a reason for hiding this comment

jdavies-st Nov 3, 2022

Choose a reason for hiding this comment

jdavies-st Nov 3, 2022

Choose a reason for hiding this comment

jdavies-st Nov 3, 2022

Choose a reason for hiding this comment

jdavies-st Nov 3, 2022

Choose a reason for hiding this comment

jdavies-st Nov 3, 2022

Choose a reason for hiding this comment

jdavies-st Nov 3, 2022

Choose a reason for hiding this comment

jdavies-st commented Nov 3, 2022 • edited Loading

zacharyburnett commented Oct 28, 2022 •

edited

Loading

codecov bot commented Oct 31, 2022 •

edited

Loading

jdavies-st commented Nov 3, 2022 •

edited

Loading