-
Notifications
You must be signed in to change notification settings - Fork 167
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
Cache CRDS_PATH reffile contents in CI workflows #7333
Cache CRDS_PATH reffile contents in CI workflows #7333
Conversation
Codecov ReportBase: 79.63% // Head: 79.64% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #7333 +/- ##
=======================================
Coverage 79.63% 79.64%
=======================================
Files 412 412
Lines 37572 37572
=======================================
+ Hits 29922 29924 +2
+ Misses 7650 7648 -2
*This pull request uses carry forward flags. Click here to find out more.
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. |
f5059da
to
1b72ddf
Compare
Zach will review when he's back from vacation. |
1b72ddf
to
85f5599
Compare
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.
this LGTM! Are there any instances where we might want to explicitly run tests with a previous CRDS context with workflow_dispatch
?
Possibly! Maybe a separate PR for that? Here I'm mostly trying to make the CI fast and reliable. Before this is merged, someone who has admin credentials for this repo will have to change which are the required tests to pass. They've been renamed, and rejiggered here, so for instance merging is blocked because the "Code style check" test hasn't passed yet, but it's there, just labeled "CI / check-style". Feedback on the CI check names/descriptions would be appreciated. This PR changes them, hopefully in a more structured way. Is this an improvement for the daily developers or just another irritating change? We can go back to the old names. |
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 have no idea if this code will actually do what you claim it does, but I'm very much in favor of the claim!
python-version: 3.9 | ||
toxenv: security | ||
|
||
# MacOS job is flaky on Github actions and fails routinely. |
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.
Are MacOS jobs still flaky? Would be nice to be able to test both platforms.
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.
They are slower. Perhaps a token macos test with latest dependencies?
Can you indicate which names have been changed, so that we can make sure to cover all of them when modifying the list of required passing tests?
The new ones look OK to me. |
All of the following have changed:
|
After poking around in the page of required CI test settings, it appears that we may have to merge this PR before the new test names will show up and hence allow me to change the list. So any objections to merging this now? I have the power to merge this PR even though the (old) required tests are not passing. (it's good being king ...) |
I'm fine with that, though perhaps we should rebase first to see if the |
de2920b
to
4e94a30
Compare
are these test failures important, or ephemeral? They seemed to appear in the metacls = <class 'enum.EnumType'>, cls = 'ListCategory'
bases = (<enum 'ListCategory'>,), classdict = {}, boundary = None
_simple = False, kwds = {}, ignore = ['_ignore_'], key = '_ignore_'
def __new__(metacls, cls, bases, classdict, *, boundary=None, _simple=False, **kwds):
# an Enum class is final once enumeration items have been defined; it
# cannot be mixed with other types (int, float, etc.) if it has an
# inherited __new__ unless a new __new__ is defined (or the resulting
# class will fail).
#
if _simple:
return super().__new__(metacls, cls, bases, classdict, **kwds)
#
# remove any keys listed in _ignore_
classdict.setdefault('_ignore_', []).append('_ignore_')
ignore = classdict['_ignore_']
for key in ignore:
classdict.pop(key, None)
#
# grab member names
> member_names = classdict._member_names
E AttributeError: 'dict' object has no attribute '_member_names' https://github.com/spacetelescope/jwst/actions/runs/3489141823/jobs/5840043311#step:7:362 |
Probably need @stscieisenhamer to take a look. |
Odd error. It was not there before It was introduced with #7332. I don't understand https://github.com/spacetelescope/jwst/actions/runs/3489141823/jobs/5840043311#step:7:338 jwst/jwst/associations/registry.py Line 323 in f6b3973
where a ListCategory of type >>> from enum import Enum
>>> type("foo", (Enum,), {})
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/jdavies/miniconda3/envs/jwst/lib/python3.11/enum.py", line 482, in __new__
member_names = classdict._member_names
^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'dict' object has no attribute '_member_names' and instead you should do >>> Enum("foo", {})
<enum 'foo'> But maybe the bug is that |
I verified that changes introduced in #7332 fail under Python 3.11 but not under Python 3.10.8. The rules to the registry by the test are the same. So there must be some subtle changes in 3.11 vs 3.10, though I found this And that's from a year ago, well before 3.11 was released. 😕 |
No doubt I am being over-clever, as usual. However, point of information: If installing jwst from master, all tests succeed under Python 3.11. It is only this PR that the errors appears. I can investigate further next week. |
So ... something in this PR is causing the failures or what? |
I see the same bug on
|
Ok, then I think this PR should be merged now, and then another PR opened for fixes for Python 3.11 for those tests. Alternatively, we could try and solve those issues here. |
4e94a30
to
84cdbab
Compare
I'm good with merging this now and investigating the related errors later. @zacharyburnett however you wish to handle the out-of-date issue and then merge is good. |
84cdbab
to
4690040
Compare
This PR re-enables caching of the CRDS_PATH for the CI workflows, after having been turned off in #6165. It should make the per-PR CI much more reliable (no failed reffile downloads) and slightly faster overall.
This takes advantage of @zacharyburnett 's excellent tox.ini refactor in PR #7323 (with some minor additions), and uses the new clean structure to distinguish which tox envs are running unit tests which use CRDS and which are not.
Here we use the CRDS_CONTEXT as the cache key. The context will change more often than the contents of CRDS_PATH itself, but that's pretty much the only way to do it that makes sense, as we are trying to cache the snapshot of reffiles that are pulled during the running of the unit tests, and thus need a proxy for the contents of this snapshot before the tests are run. We cannot hash the contents of CRDS_PATH as it doesn't exist until after the tests are run, but we need the hash before the tests are run. Chicken. Egg.
This means that every increment of the default CRDS_CONTEXT will mean that the first CI test run will not have a cached CRDS_PATH, but after it runs, it will create one, and then all subsequent runs will use it until the next CRDS_CONTEXT change.
I've tested this separately in the CI system by running one of the test matrix jobs, and results can be seen here:
https://github.com/jdavies-st/jwst/actions/workflows/ci.yml
particularly the "CI use caching" job numbers 89 through 92, where in 4 stages, respectively, the
I've previously also checked that running matrix works with the cache both when created for the first time and incremented with a new key.
Note:
actions/setup-python@v4
which saves having to download the likely packages from pypi. This works the same here as on one's local machine, which also maintains a pip cache of packages that have been pulled from pypi. The install step still queries pypi.org to get the latest version of a package dependency and will download it if not in the pip cache. This is all default pip behavior. This should speed up installs a bit and has no downsides.crds
to do so by using the crds server json api. This speeds things up considerably.