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

Pin symengine in backwards compatibility tests #13885

Merged
merged 4 commits into from
Feb 19, 2025

Conversation

ElePT
Copy link
Contributor

@ElePT ElePT commented Feb 19, 2025

Summary

The recent symengine 1.14 release breaks the qpy backwards compatibility tests in the stable/1.4 branch with a qiskit.qpy.exceptions.QpyError: 'Incompatible symengine version 0.14 used to generate the QPY payload' error.

The symengine version was already pinned to <1.14 in the following locations:

  • requirements.txt
  • test/qpy_compat/run_tests.sh

The symengine version is not directly modified in test/qpy_compat/process_version.sh, but seems to pull the version constraints from test/qpy_compat/qpy_test_constraints.txt, which didn't include symengine.

For this reason, this PR pins symengine to <1.14 in test/qpy_compat/qpy_test_constraints.txt.

Details and comments

The main branch doesn't seem to share this issue even though the inspected files look the same. There might be an additional constraint in main that I am missing, but for the moment decided to open this PR directly against 1.4. [Edit: I think that the tests in main are used the cached qpy files and not generating them from scratch. The bugfix should still probably be ported to 2.0 and 1.3]

@ElePT ElePT added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Feb 19, 2025
@ElePT ElePT marked this pull request as ready for review February 19, 2025 12:39
@ElePT ElePT requested a review from a team as a code owner February 19, 2025 12:39
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core
  • @mtreinish
  • @nkanazawa1989

@ElePT ElePT added this to the 1.4.0 milestone Feb 19, 2025
@ElePT ElePT added bug Something isn't working Changelog: None Do not include in changelog and removed Changelog: Bugfix Include in the "Fixed" section of the changelog labels Feb 19, 2025
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

This was definitely an oversight when we added the fixes for the symengine version dependency in 1.2.4. We fixed compatibility between symengine 0.13.0 and 0.11.0 and added the version cap to prevent this on new releases, and tested the range of supported symengine versions to handle the interop question. But forgot about future releases when installing historical qiskit releases. This should correct that oversight. I think your comment about it being related to the cache makes sense. I'm not sure why we're not hitting this on main though, because there are open PRs that should be invalidating the cache, but that doesn't change this PR as it is clearly correct.

@mtreinish mtreinish enabled auto-merge February 19, 2025 13:45
@ElePT
Copy link
Contributor Author

ElePT commented Feb 19, 2025

@Mergifyio backport stable/1.3 main

Copy link
Contributor

mergify bot commented Feb 19, 2025

backport stable/1.3 main

✅ Backports have been created

@jakelishman
Copy link
Member

I'm not sure why we're not hitting this on main though, because there are open PRs that should be invalidating the cache, but that doesn't change this PR as it is clearly correct.

For future posterity: the reason the existing PRs that invalidated the cache on main haven't yet failed is because each PR has its own cache context, which starts of as inheriting from the base branch. So new PRs against main inherit its cache, unless they invalidate it. Once they have invalidated it, they have their own cache, so subsequent non-invalidating pushes on a PR won't cause a new QPY rebuild until the cache is invalidated again. Each open PR that invalidates the QPY cache did so before the release of symengine 0.14, and hasn't broken it since, which is why it hasn't appeared yet.

@mtreinish mtreinish added this pull request to the merge queue Feb 19, 2025
Merged via the queue into Qiskit:stable/1.4 with commit 8c06fd2 Feb 19, 2025
18 checks passed
mergify bot pushed a commit that referenced this pull request Feb 19, 2025
* Add symengine constraint in qpy backwards compat tests

* Add comments

* Only pin upper limit

* Update comment

(cherry picked from commit 8c06fd2)
mergify bot pushed a commit that referenced this pull request Feb 19, 2025
* Add symengine constraint in qpy backwards compat tests

* Add comments

* Only pin upper limit

* Update comment

(cherry picked from commit 8c06fd2)
github-merge-queue bot pushed a commit that referenced this pull request Feb 19, 2025
* Add symengine constraint in qpy backwards compat tests

* Add comments

* Only pin upper limit

* Update comment

(cherry picked from commit 8c06fd2)

Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
github-merge-queue bot pushed a commit that referenced this pull request Feb 19, 2025
* Add symengine constraint in qpy backwards compat tests

* Add comments

* Only pin upper limit

* Update comment

(cherry picked from commit 8c06fd2)

Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Changelog: None Do not include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants