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

Fix issue with ScheduleBlock qpy serialization and use_symengine #11261

Merged
merged 5 commits into from
Nov 18, 2023

Conversation

mtreinish
Copy link
Member

@mtreinish mtreinish commented Nov 16, 2023

Summary

This commit fixes an issue with the use_symengine flag on the qpy.dump() function. Previously in certain conditions a symengine expression that was listed in the qpy header as being encoded via symengine was incorrectly being serialized using sympy. This would cause a failure on deserialialization as the qpy payload was invalid and symengine could not parse a symengine representation.

Details and comments

This was originally caught during the development of #10902 as that switches the default of use_symengine to True as it makes symengine a hard requirement. This commit splits it out so the isolated fix can be backported to 0.45.1.

This commit fixes an issue with the use_symengine flag on the qpy.dump()
function. Previously in certain conditions a symengine expression that
was listed in the qpy header as being encoded via symengine was
incorrectly being serialized using sympy. This would cause a failure on
deserialialization as the qpy payload was invalid and symengine could
not parse a symengine representation.

This was originally caught during the development of Qiskit#10902 as that
switches the default of use_symengine to ``True`` as it makes symengine
a hard requirement. This commit splits it out so the isolated fix can be
backported to 0.45.1.
@mtreinish mtreinish added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: Bugfix Include in the "Fixed" section of the changelog labels Nov 16, 2023
@mtreinish mtreinish added this to the 0.45.1 milestone Nov 16, 2023
@mtreinish mtreinish requested a review from a team as a code owner November 16, 2023 22:57
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

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

@mtreinish mtreinish added the mod: qpy Related to QPY serialization label Nov 16, 2023
@coveralls
Copy link

coveralls commented Nov 16, 2023

Pull Request Test Coverage Report for Build 6909834098

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.02%) to 85.914%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 3 91.67%
Totals Coverage Status
Change from base Build 6897835680: 0.02%
Covered Lines: 65952
Relevant Lines: 76765

💛 - Coveralls

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Thanks for catching this oversight 😅 . It looks like the windows tests are complaining about symengine, would the change you made to pyproject.toml (skip-test) on #10902 fix that?

@mtreinish
Copy link
Member Author

Oh good call, yeah I need to add a skip decorator on the new test so it skips when HAS_SYMENGINE is false.

The skip-test in the pyproject.toml is about our cibuildwheel jobs which are used to publish precompiled binaries. #10902 makes symengine a hard requirement for all platforms and because symengine doesn't publish precompiled binaries for i686 and win32 we're downgrading our support of those platforms in that PR to tier 3 (meaning we don't/can't test the wheels we publish on those platforms). The skip-test config is just the job configuration for that change.

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Looks good to me other than the mistaken cross-ref.

@jakelishman jakelishman added this pull request to the merge queue Nov 17, 2023
Merged via the queue into Qiskit:main with commit 1dd4f54 Nov 18, 2023
14 checks passed
mergify bot pushed a commit that referenced this pull request Nov 18, 2023
)

* Fix issue with ScheduleBlock qpy serialization and use_symengine

This commit fixes an issue with the use_symengine flag on the qpy.dump()
function. Previously in certain conditions a symengine expression that
was listed in the qpy header as being encoded via symengine was
incorrectly being serialized using sympy. This would cause a failure on
deserialialization as the qpy payload was invalid and symengine could
not parse a symengine representation.

This was originally caught during the development of #10902 as that
switches the default of use_symengine to ``True`` as it makes symengine
a hard requirement. This commit splits it out so the isolated fix can be
backported to 0.45.1.

* Skip symengine test if symengine isn't installed

* Fix skip syntax

* Update releasenotes/notes/fix-schedule-qpy-use-symengine-05ae1dfab73e3ff8.yaml

Co-authored-by: Jake Lishman <[email protected]>

---------

Co-authored-by: Jake Lishman <[email protected]>
(cherry picked from commit 1dd4f54)
github-merge-queue bot pushed a commit that referenced this pull request Nov 18, 2023
) (#11275)

* Fix issue with ScheduleBlock qpy serialization and use_symengine

This commit fixes an issue with the use_symengine flag on the qpy.dump()
function. Previously in certain conditions a symengine expression that
was listed in the qpy header as being encoded via symengine was
incorrectly being serialized using sympy. This would cause a failure on
deserialialization as the qpy payload was invalid and symengine could
not parse a symengine representation.

This was originally caught during the development of #10902 as that
switches the default of use_symengine to ``True`` as it makes symengine
a hard requirement. This commit splits it out so the isolated fix can be
backported to 0.45.1.

* Skip symengine test if symengine isn't installed

* Fix skip syntax

* Update releasenotes/notes/fix-schedule-qpy-use-symengine-05ae1dfab73e3ff8.yaml

Co-authored-by: Jake Lishman <[email protected]>

---------

Co-authored-by: Jake Lishman <[email protected]>
(cherry picked from commit 1dd4f54)

Co-authored-by: Matthew Treinish <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog mod: qpy Related to QPY serialization stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants