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

test opencv functionality separately #7414

Merged
merged 3 commits into from
Dec 22, 2022

Conversation

zacharyburnett
Copy link
Collaborator

@zacharyburnett zacharyburnett commented Dec 22, 2022

Addresses #7409
related to spacetelescope/stcal#136

This PR runs the tests requiring opencv in a separate toxenv with opencv-python installed, to test whether conditional usage of ellipse construction is handled gracefully.

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

@zacharyburnett zacharyburnett self-assigned this Dec 22, 2022
@github-actions github-actions bot added automation Continuous Integration (CI) and testing automation tools installation jump testing labels Dec 22, 2022
@zacharyburnett zacharyburnett added no-changelog-entry-needed jump testing installation automation Continuous Integration (CI) and testing automation tools and removed jump testing installation automation Continuous Integration (CI) and testing automation tools labels Dec 22, 2022
@zacharyburnett zacharyburnett marked this pull request as ready for review December 22, 2022 19:48
@codecov
Copy link

codecov bot commented Dec 22, 2022

Codecov Report

Base: 52.52% // Head: 52.52% // No change to project coverage 👍

Coverage data is based on head (15f1baf) compared to base (df6cb65).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7414   +/-   ##
=======================================
  Coverage   52.52%   52.52%           
=======================================
  Files         417      417           
  Lines       38087    38087           
=======================================
  Hits        20006    20006           
  Misses      18081    18081           
Flag Coverage Δ *Carryforward flag
nightly 18.92% <ø> (ø) Carriedforward from df6cb65
unit 52.10% <ø> (ø)

*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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@zacharyburnett zacharyburnett merged commit 522db0e into spacetelescope:master Dec 22, 2022
@zacharyburnett zacharyburnett deleted the opencv_tests branch December 22, 2022 20:44
@jdavies-st
Copy link
Collaborator

jdavies-st commented Jan 27, 2023

This PR breaks CRDS caching between CI jobs. The reason is that the opencv job only tests some very small subset of the unit test suite and pulls virtually nothing from CRDS, yet it uses and updates the general caching used by the jobs that run all the unit tests. Because this job is short, it finishes before any of the other tests and then sets the cache when a new key become available (incremented CRDS context). And of course uses virtually no reference files, so it is creating a mostly empty cache (instead of the normal ~350MB one). The normal unit tests never get a chance to set the cache using the new key.

The way to fix this is to move this test-opencv tox job into a different job in the workflow that does not use the cache action.

@zacharyburnett
Copy link
Collaborator Author

This PR breaks CRDS caching between CI jobs. The reason is that the opencv job only tests some very small subset of the unit test suite and pulls virtually nothing from CRDS, yet it uses and updates the general caching used by the jobs that run all the unit tests. Because this job is short, it finishes before any of the other tests and then sets the cache when a new key become available (incremented CRDS context). And of course uses virtually no reference files, so it is creating a mostly empty cache (instead of the normal ~350MB one). The normal unit tests never get a chance to set the cache using the new key.

The way to fix this is to move this test-opencv tox job into a different job in the workflow that does not use the cache action.

This should be fixed by adding a dedicated caching job in https://github.com/spacetelescope/jwst/pull/7384/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03f

@jdavies-st
Copy link
Collaborator

No, it is still broke, and that dedicated caching job makes it worse, as it runs crds sync --contexts in the unit test jobs, something that is never needed, as that only pulls over the imap, rmap and pmaps (~1.8MB), not any actual reference files (~350MB needed for the unit tests). Most of the *maps are not needed, and none of the needed reference files are cached. See the output of the caching job here:

https://github.com/spacetelescope/jwst/actions/runs/4009075221/jobs/6884024021#step:5:19

It has been broke since this PR. It used to restore ~350MB of reference files:

https://github.com/spacetelescope/jwst/actions/runs/3758608010/jobs/6387151447#step:6:20

See the discussion here about why crds sync --contexts is not necessary:

#7323 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation Continuous Integration (CI) and testing automation tools installation jump no-changelog-entry-needed testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants