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.

Sign up for GitHub

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

Jump to bottom

improve WebbPSF data caching in GitHub CI #923

Merged
merged 8 commits into from
Oct 16, 2023
Merged

improve WebbPSF data caching in GitHub CI #923

merged 8 commits into from
Oct 16, 2023

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Oct 3, 2023 •
edited
Loading

This PR changes how webbpsf data is downloaded and cached in the github actions CI.

This PR adds a new reusable workflow data.yml that is used by:

  • roman_ci.yml
  • roman_ci_cron.yaml
  • test_devdeps.yml
    and handles:
  • (optionally) fetching webbpsf data (more on this below)
  • fetching the current crds context

Checklist

  • added entry in CHANGES.rst under the corresponding subsection
  • updated relevant tests
  • updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below. How to run regression tests on a PR

@codecov
Copy link

codecov bot commented Oct 3, 2023 •
edited
Loading

Codecov Report

All modified lines are covered by tests ✅

📢 Thoughts on this report? Let us know!.

@braingram braingram force-pushed the webbpsf_data_cache branch 11 times, most recently from a1d7883 to be68c69 Compare October 3, 2023 20:33
@braingram
Copy link
Collaborator Author

There are many failed jobs because of #922 and the devdeps are failing due to numpy 2.0 deepdiff incompatibility (the devdeps job does not appear to be installing the dev version and appears related to #910).

@zacharyburnett I requested a review on this PR even though it will stay draft until #922 can be fixed and #920 merged. Thanks for giving it a look!

@zacharyburnett
Copy link
Collaborator

made a PR at braingram#1

@braingram
Copy link
Collaborator Author

@zacharyburnett feel free to push to my branch if any more errors pop up

@braingram
Copy link
Collaborator Author

The webbpsf hash does not appear to be propagated to the test jobs:
https://github.com/spacetelescope/romancal/actions/runs/6409720129/job/17401548600?pr=923#step:3:4
shows:

    key: data--roman_0051.pmap
    restore-keys: data-639888f0376b13e17a71a27e1eff514f8c64b18e-

@zacharyburnett
Copy link
Collaborator

zacharyburnett commented Oct 4, 2023 •
edited
Loading

The webbpsf hash does not appear to be propagated to the test jobs: https://github.com/spacetelescope/romancal/actions/runs/6409720129/job/17401548600?pr=923#step:3:4 shows:

    key: data--roman_0051.pmap
    restore-keys: data-639888f0376b13e17a71a27e1eff514f8c64b18e-

fixed with 2c6ef24
https://github.com/spacetelescope/romancal/actions/runs/6410194510/job/17403071740#step:3:4

    key: data-639888f0376b13e17a71a27e1eff514f8c64b18e-roman_0051.pmap
    restore-keys: data-639888f0376b13e17a71a27e1eff514f8c64b18e

braingram
braingram commented Oct 4, 2023
View reviewed changes
Copy link
Collaborator Author

@braingram braingram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it!

Just 2 minor edits for the use of crds_path instead of path.

I'd also like to see some of the combinations "exercised" once changes are finished including:

  • removing the "update webbpsf data" label from this PR and confirming that the data is not downloaded
  • removing the "webbpsf-{hash}" cache and triggering the "data.yml" (with a workflow_dispatch seems good enough)

It would also be great to see the CI green so the final combined cache can be generated but that will have to wait for changes outside the scope of this PR.
@mairanteodoro Does it make sense to delay this PR until those are done?

.github/workflows/tests_devdeps.yml Show resolved Hide resolved
@zacharyburnett
Copy link
Collaborator

removing the update webbpsf data label retriggered the devdeps and scheduled jobs, and they did skip downloading the data and were able to grab the existing cache https://github.com/spacetelescope/romancal/actions/runs/6410785985/job/17404827427#step:3:16

@zacharyburnett
Copy link
Collaborator

zacharyburnett commented Oct 4, 2023 •
edited
Loading

I removed the webbpsf-{hash} cache, but the Run workflow button is missing from the data.yml workflow; I think because it's not present on the main branch

@braingram
Copy link
Collaborator Author

Thanks for running the tests! All looks good to me.

@zacharyburnett zacharyburnett enabled auto-merge (squash) October 5, 2023 02:28
@zacharyburnett zacharyburnett changed the title Change webbpsf data cache in github CI improve WebbPSF data caching in GitHub CI Oct 5, 2023
@braingram
Copy link
Collaborator Author

@zacharyburnett I tried adding the update webbpsf data label here as there no longer appears to be any webbpsf- caches and that portion of the workflow does not appear to be running. If you have a chance would you take a look?

@zacharyburnett
Copy link
Collaborator

@zacharyburnett I tried adding the update webbpsf data label here as there no longer appears to be any webbpsf- caches and that portion of the workflow does not appear to be running. If you have a chance would you take a look?

I'm not sure what the issue was, but I reran the workflow and it looks like the caches are generated again and the tests are running.

@braingram braingram mentioned this pull request Oct 13, 2023
Merged
6 tasks
@braingram
change webbpsf data caching in github CI
a6e2e7e 
download webbpsf data once per week to update
a cache ``webbpsf-<hash>`` that can be used as
a ``cache-restore-keys`` entry for the cache in
other ci jobs. This can also be triggered when
a PR is specifically labeled to update webbpsf
data.

Update other ``roman_ci.yml`` to use most recent
``webbpsf-<hash>`` to look up hash to then construct
a combined cache key (for the combined crds/webbpsf data
cache) using the webbpsf data as a restore key to initialize
the cache with the pre-fetched webbpsf data.

``roman_ci_cron.yml`` will need similar updates.
@braingram
attempt at reusable workflow
03258f7
@braingram
simplifying paths and outputs
c9616e2
@braingram
rename webbpsf_data.yml to data.yml
09bb1c9
@braingram
fix webbpsf path
42cf4c8
@braingram
debug
e378bc0
@braingram
remove debug
e14e228
@braingram
add comment about webbpsf download skip
1ed817b
@braingram
Copy link
Collaborator Author

@zacharyburnett should this get another review since we've both contributed? If not, I'm happy merging it as the CI appears green (except for the devdeps jobs which are failing due to unrelated issues fixed in: #940)

@zacharyburnett
Copy link
Collaborator

@zacharyburnett should this get another review since we've both contributed? If not, I'm happy merging it as the CI appears green (except for the devdeps jobs which are failing due to unrelated issues fixed in: #940)

I think it looks good to merge, at least to me

@braingram braingram merged commit c2671f0 into spacetelescope:main Oct 16, 2023
30 of 38 checks passed
@braingram braingram deleted the webbpsf_data_cache branch October 16, 2023 14:22
@braingram
Copy link
Collaborator Author

Note that I manually triggered the workflow after merging to generate a webbpsf- cache on the main branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zacharyburnett zacharyburnett zacharyburnett approved these changes

Assignees
No one assigned
Labels
automation no-changelog-entry-needed run devdeps tests run scheduled tests testing update webbpsf data
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@braingram @zacharyburnett