Move QPY tests to GitHub Actions and increase inter-symengine tests (backport #13273) #13380
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![mergify[bot]](https://avatars.githubusercontent.com/in/10562?s=60&v=4){kind=small}
…13273) This commit has two major goals: - fix the caching of the QPY files for both the `main` and `stable/*` branches - increase the number of compatibility tests between the different symengine versions that might be involved in the generation and loading of the QPY files. Achieving both of these goals also means that it is sensible to move the job to GitHub Actions at the same time, since it will put more pressure on the Azure machine concurrency we use. Caching ------- The previous QPY tests attempted to cache the generated files for each historical version of Qiskit, but this was unreliable. The cache never seemed to hit on backport branches, which was a huge slowdown in the critical path to getting releases out. The cache restore keys were also a bit lax, meaning that we might accidentally have invalidated files in the cache by changing what we wanted to test, but the restore keys wouldn't have changed. The cache files would fail to restore as a side-effect of ed79d42 (gh-11526); QPY was moved to be on the tail end of the lint run, rather than in a test run. This meant that it was no longer run as part of the push event when updating `main` or one of the `stable/*` branches. In Azure (and GitHub Actions), the "cache" action accesses a _scoped_ cache, not a universal one for the repository [^1][^2]. Approximately, base branches each have their own scope, and PR events open a new scope that is a child of the target branch, the default branch, and the source branch, if appropriate. A cache task can read from any of its parent scopes, but write events go to the most local scope. This means that we haven't been writing to long-standing caches for some time now. PRs would typically miss the cache on the first attempt, hit their cache for updates, then miss again once entering the merge queue. The fix for this is to run the QPY job on branch-update events as well. The post-job cache action will then write out to a reachable cache for all following events. Cross-symengine tests --------------------- We previously were just running a single test with differing versions of symengine between the loading and generation of the QPY files. This refactors the QPY `run_tests.sh` script to run a full pairwise matrix of compatibility tests, to increase the coverage. [^1]: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/caching-dependencies-to-speed-up-workflows#restrictions-for-accessing-a-cache [^2]: https://learn.microsoft.com/en-us/azure/devops/pipelines/release/caching?view=azure-devops#cache-isolation-and-security (cherry picked from commit af8be25)
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the following people are relevant to this code:
|
github-actions
bot
added
type: qa
jakelishman
approved these changes
Oct 29, 2024
Pull Request Test Coverage Report for Build 11583319838Details
💛 - Coveralls |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This commit has two major goals:
fix the caching of the QPY files for both the
mainandstable/*branchesincrease the number of compatibility tests between the different symengine versions that might be involved in the generation and loading of the QPY files.
Achieving both of these goals also means that it is sensible to move the job to GitHub Actions at the same time, since it will put more pressure on the Azure machine concurrency we use.
Caching
The previous QPY tests attempted to cache the generated files for each historical version of Qiskit, but this was unreliable. The cache never seemed to hit on backport branches, which was a huge slowdown in the critical path to getting releases out. The cache restore keys were also a bit lax, meaning that we might accidentally have invalidated files in the cache by changing what we wanted to test, but the restore keys wouldn't have changed.
The cache files would fail to restore as a side-effect of ed79d42 (gh-11526); QPY was moved to be on the tail end of the lint run, rather than in a test run. This meant that it was no longer run as part of the push event when updating
mainor one of thestable/*branches. In Azure (and GitHub Actions), the "cache" action accesses a scoped cache, not a universal one for the repository 12. Approximately, base branches each have their own scope, and PR events open a new scope that is a child of the target branch, the default branch, and the source branch, if appropriate. A cache task can read from any of its parent scopes, but write events go to the most local scope. This means that we haven't been writing to long-standing caches for some time now. PRs would typically miss the cache on the first attempt, hit their cache for updates, then miss again once entering the merge queue.The fix for this is to run the QPY job on branch-update events as well. The post-job cache action will then write out to a reachable cache for all following events.
Cross-symengine tests
We previously were just running a single test with differing versions of symengine between the loading and generation of the QPY files. This refactors the QPY
run_tests.shscript to run a full pairwise matrix of compatibility tests, to increase the coverage.Details and comments
This is a CI change, so the chance I got it right first time is approximately zero. Just need a PR to start testing it. We'll need to update the branch-protection rules if we decide to merge this.
This is an automatic backport of pull request #13273 done by [Mergify](https://mergify.com).
Footnotes
https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/caching-dependencies-to-speed-up-workflows#restrictions-for-accessing-a-cache ↩
https://learn.microsoft.com/en-us/azure/devops/pipelines/release/caching?view=azure-devops#cache-isolation-and-security ↩