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

Workaround symengine serialization payload incompatibility #13251

Merged
merged 8 commits into from
Oct 2, 2024

Conversation

mtreinish
Copy link
Member

Summary

In QPY we rely on symengine's internal serialization to represent the internal symbolic expression stored inside a ParameterExpression object. However, this format is nominally symengine version specific and will raise an error if there is a mismatch between the version used to generate the payload and what is trying to read it. This became an issue in the recent symengine 0.13 release which started to raise an error when people installed it and tried to load QPY payloads across the versions. This makes the symengine serialization unsuitable for use in QPY because it's supposed to be independent of these kind of concerns, especially when QPY is used in a server-client model where you don't necessarily control the installed environment of symengine.

To correctly address this issue we'll need a new version of the QPY format that owns the serialization format of ParameterExpressions directly instead of relying on symengine which doesn't offer a compatibility guarantee on the format. However this won't be quick solution and users are encountering issues since the release of 0.13. This commit introduces a workaround for this specific instance of the mismatch. It turns out the payload format between 0.11 and 0.13 is completely unchanged except for the version number. So before passing the parameter expression payload to symengine for deserialization this commit checks the versions numbers are the same, if they're not it checks that we're dealing with 0.11 or 0.13, and if so it changes the version number in the payloads header appropriately. If the version number is outside those bounds it raises an exception because while this hack is known to be safe for translating between symengine 0.11 and 0.13, it's not possible to know for a future version whether the payload format changed or not.

Longer term we will need a proper fix in qpy version 13 that introduces a qiskit native serialization format for parameter expression instead of relying on symengine or sympy to do it for us.

Details and comments

In QPY we rely on symengine's internal serialization to represent the
internal symbolic expression stored inside a ParameterExpression object.
However, this format is nominally symengine version specific and will
raise an error if there is a mismatch between the version used to
generate the payload and what is trying to read it. This became an issue
in the recent symengine 0.13 release which started to raise an error
when people installed it and tried to load QPY payloads across the
versions. This makes the symengine serialization unsuitable for use in
QPY because it's supposed to be independent of these kind of concerns,
especially when QPY is used in a server-client model where you don't
necessarily control the installed environment of symengine.

To correctly address this issue we'll need a new version of the QPY
format that owns the serialization format of ParameterExpressions directly
instead of relying on symengine which doesn't offer a compatibility
guarantee on the format. However this won't be quick solution and users
are encountering issues since the release of 0.13. This commit
introduces a workaround for this specific instance of the mismatch. It
turns out the payload format between 0.11 and 0.13 is completely
unchanged except for the version number. So before passing the parameter
expression payload to symengine for deserialization this commit checks
the versions numbers are the same, if they're not it checks that we're
dealing with 0.11 or 0.13, and if so it changes the version number in
the payloads header appropriately. If the version number is outside
those bounds it raises an exception because while this hack is known to
be safe for translating between symengine 0.11 and 0.13, it's not
possible to know for a future version whether the payload format changed
or not.

Longer term we will need a proper fix in qpy version 13 that introduces
a qiskit native serialization format for parameter expression instead of
relying on symengine or sympy to do it for us.
@mtreinish mtreinish added priority: high 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 Oct 1, 2024
@mtreinish mtreinish added this to the 1.2.3 milestone Oct 1, 2024
@mtreinish mtreinish requested a review from a team as a code owner October 1, 2024 16:34
@qiskit-bot
Copy link
Collaborator

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

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

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.

I'm just pulling this locally to run a test myself, since this isn't something we're going to be able to test well in CI without adding a whole other test job, but in the meantime, two questions.

I'm fine with leaving the PR as-is if it works, though.

qiskit/qpy/binary_io/value.py Outdated Show resolved Hide resolved
Comment on lines 301 to 312
if int(symengine_version[1]) != minor:
if minor not in (11, 13):
raise exceptions.QpyError(
f"Incompatible symengine version {major}.{minor} used to generate the QPY "
"payload"
)
minor_version = int(symengine_version[1])
if minor_version not in (11, 13):
raise exceptions.QpyError(
f"Incompatible installed symengine version {symengine.__version__} to load "
"this QPY payload"
)
Copy link
Member

Choose a reason for hiding this comment

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

The ordering of this means that we can cope with generation and load from (say) a hypothetical symengine 0.14, but is it worth us risking that? We could put symengine>=0.11,<=0.13 in our requirements, potentially.

Not sure how I feel about that, just asking the question.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we, did I do the logic incorrectly? My intent was that if we have a version mismatch between the payload and the installed library and if either the payload or the installed version is not using 0.11 or 0.13 we'll raise the error.

There is an edge case here if the major version != 0, but I figured this was unlikely to be an issue (especially with a minor version of 11 or 13) in any reasonable timescale.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'm saying we can attempt to load if there's no payload mismatch and both were 0.14, but given we know there's problems, would we prefer just to forbid 0.14 from entering at all, rather than have it work iff there's a match?

Yeah, I saw the major version thing too, but was just assuming that we'll fix this before that happened.

Copy link
Member

Choose a reason for hiding this comment

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

(and by "yeah" I mean I'm agreeing with you - I think you did the logic right)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah ok, now I get what you're saying. My initial thought was if there is no mismatch I think we're fine as symengine should be totally fine if everything is the same version. I guess it's more a question of whether we think it's sufficiently a risk of foot gunning by generating a payload with 0.14.0 that will fail if someone is using any other version later

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I was kind of hoping you'd have a strong opinion about that last point so I'd be absolved of needing to develop one. In lieu of anything better, let's stick with the PR as written - upper bounds aren't ideal, and I don't particularly like introducing one within a patch release.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think that's what we're talking about. That's the only option really is either add a cap to the requirements file in 1.2.3, or we do it in the python code so that qpy.dump() fails if you have symenging > 0.14 installed. It might be prudent. I was hesitant to do that in this PR because I was worried about python 3.13 support. But since 0.13 has 3.13 support maybe it's worth it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I opted to add the cap in: 7aae984

Copy link
Member

Choose a reason for hiding this comment

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

For future posterity: Matt and I talked offline before this, and thought that pre-emptively capping is actually the lesser of the two evils. We don't like introducing an upper bound in a patch release, but we've had upper bounds on symengine before, and the hack we're doing to enable compatibility is pretty dangerous, since symengine's deserialisation code can hard crash on malformed payloads. It feels safer to try and restrict symengine 0.14 from entering (as best as we can), rather than hoping that nothing will break when it's released.

@coveralls
Copy link

coveralls commented Oct 1, 2024

Pull Request Test Coverage Report for Build 11142871106

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 11 of 23 (47.83%) changed or added relevant lines in 3 files are covered.
  • 31 unchanged lines in 6 files lost coverage.
  • Overall coverage decreased (-0.007%) to 88.861%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/qpy/common.py 9 21 42.86%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 94.02%
crates/accelerate/src/two_qubit_decompose.rs 1 91.45%
qiskit/circuit/library/blueprintcircuit.py 4 96.46%
qiskit/circuit/library/data_preparation/pauli_feature_map.py 6 91.8%
crates/qasm2/src/lex.rs 7 91.98%
crates/qasm2/src/parse.rs 12 97.15%
Totals Coverage Status
Change from base Build 11126578403: -0.007%
Covered Lines: 74149
Relevant Lines: 83444

💛 - Coveralls

@jakelishman
Copy link
Member

In local testing, I generated the QPY backwards compatibility tests using this branch, with Symengine 0.11 and Symengine 0.13, and then attempted to reload them with both. I found that the schedule code (qiskit/qpy/binary_io/schedules.py:109) has a similar loader, which is similarly not aware of multiple symengine versions and also needs a fix.

@jakelishman
Copy link
Member

Probably we can find a way to slightly unify those two code paths, since they looked pretty similar to me.

@mtreinish
Copy link
Member Author

mtreinish commented Oct 1, 2024

In local testing, I generated the QPY backwards compatibility tests using this branch, with Symengine 0.11 and Symengine 0.13, and then attempted to reload them with both. I found that the schedule code (qiskit/qpy/binary_io/schedules.py:109) has a similar loader, which is similarly not aware of multiple symengine versions and also needs a fix.

Fixed in: ea69a45

This commit updates the schedule serialization path too, as it was also
directly loading symengine expressions. The code handling the workaround
is extracted to a standalone function which is used in both spots now
instead of calling symengine directly.
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.

I tested this again locally with the same procedure as above, and verified that it works. The last question before I approve:

The use_symengine argument to QPY serialisation was introduced in #10820, released in Terra 0.45.0, which allowed symengine>=0.9. use_symengine=False was the default back then, but it was possible to set use_symengine=True, and if I set that during the QPY file generation, then this branch will fail to load the files. I can't just add 9 to the allowed minor versions we can load from, because it seems like between 0.9 and 0.11 the format did change, and we segfault symengine if we patch the version.

Are we prepared to accept this problem as "good enough", or do we want to look for anything deeper? Ideologically it's not nice to call it "good enough", but pragmatically I don't think I can see an alternative solution for us in the short term. Assuming we accept that issue, should we put a "known issues" release note in about it, or potentially document it in the QPY documentation about the backwards compatibility guarantees?

@jakelishman
Copy link
Member

To finish the thought - use_symengine=True became the default in 1.0.0, which also required symengine>=0.11.0, so we can at least say "using QPY with the defaults to load remains fully backwards compatible" (up to the JSON story, ofc, which has always explicitly warned about the backwards compat).

During the discovery on the fix in this PR we discovered that setting
the ``use_symengine`` flag from Qiskit 0.45.x and 0.46.x would result in
newer versions of Qiskit being unable to parse the QPY file for the same
issue as being addressed here. This commit expands the logic to account
for this and raise a useful warning. It also updates the release notes
and documentation to document these limitations.
Out of an abundance of caution this commit places a cap on the allowed
version of symengine users can install to be compatible with Qiskit.
Due to the symengine version dependence discovered in QPY around
serializing ParameterExpressions, we'll likely have a similar issue
when symengine 0.14.0 releases. Pre-emptively capping this means we
aren't going to be in this situation until we can confirm compatibility
with QPY serialization. The real solution for this will come in Qiskit#13252,
although as this behavior is embedded in QPY formats 10, 11, and 12 at
this point we'll have to handle this edge case moving forward regardless
of whether we introduce a better solution in 1.3.0 or not. Although
realistically in that case we will likely need to just document this as
a limitation when exporting QPY payloads with Qiskit 0.45.0 through
1.2.3 (and with the ``version`` flag set to >= 10 and < 13) and have
explicit error checking around the symengine version (which this PR
adds) when in that code path.
@mtreinish
Copy link
Member Author

Ok, I've updated the PR in: e089d5c to be more explicit about the symengine versions coming from 0.45.x and 0.46.x with use_symengine=True. I also added warnings to the docs and release notes about the known issues around this.

jakelishman
jakelishman previously approved these changes Oct 2, 2024
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@jakelishman jakelishman added this pull request to the merge queue Oct 2, 2024
Merged via the queue into Qiskit:main with commit fee9f77 Oct 2, 2024
15 checks passed
@mtreinish mtreinish deleted the hack-a-fix-symengine branch October 2, 2024 13:27
mergify bot pushed a commit that referenced this pull request Oct 2, 2024
* Workaround symengine serialization payload incompatibility

In QPY we rely on symengine's internal serialization to represent the
internal symbolic expression stored inside a ParameterExpression object.
However, this format is nominally symengine version specific and will
raise an error if there is a mismatch between the version used to
generate the payload and what is trying to read it. This became an issue
in the recent symengine 0.13 release which started to raise an error
when people installed it and tried to load QPY payloads across the
versions. This makes the symengine serialization unsuitable for use in
QPY because it's supposed to be independent of these kind of concerns,
especially when QPY is used in a server-client model where you don't
necessarily control the installed environment of symengine.

To correctly address this issue we'll need a new version of the QPY
format that owns the serialization format of ParameterExpressions directly
instead of relying on symengine which doesn't offer a compatibility
guarantee on the format. However this won't be quick solution and users
are encountering issues since the release of 0.13. This commit
introduces a workaround for this specific instance of the mismatch. It
turns out the payload format between 0.11 and 0.13 is completely
unchanged except for the version number. So before passing the parameter
expression payload to symengine for deserialization this commit checks
the versions numbers are the same, if they're not it checks that we're
dealing with 0.11 or 0.13, and if so it changes the version number in
the payloads header appropriately. If the version number is outside
those bounds it raises an exception because while this hack is known to
be safe for translating between symengine 0.11 and 0.13, it's not
possible to know for a future version whether the payload format changed
or not.

Longer term we will need a proper fix in qpy version 13 that introduces
a qiskit native serialization format for parameter expression instead of
relying on symengine or sympy to do it for us.

* Handle schedules too

This commit updates the schedule serialization path too, as it was also
directly loading symengine expressions. The code handling the workaround
is extracted to a standalone function which is used in both spots now
instead of calling symengine directly.

* Remove unused imports

* Gracefully handle failure to parse historical symengine files

During the discovery on the fix in this PR we discovered that setting
the ``use_symengine`` flag from Qiskit 0.45.x and 0.46.x would result in
newer versions of Qiskit being unable to parse the QPY file for the same
issue as being addressed here. This commit expands the logic to account
for this and raise a useful warning. It also updates the release notes
and documentation to document these limitations.

* Cap upper version of symengine in requirements list

Out of an abundance of caution this commit places a cap on the allowed
version of symengine users can install to be compatible with Qiskit.
Due to the symengine version dependence discovered in QPY around
serializing ParameterExpressions, we'll likely have a similar issue
when symengine 0.14.0 releases. Pre-emptively capping this means we
aren't going to be in this situation until we can confirm compatibility
with QPY serialization. The real solution for this will come in #13252,
although as this behavior is embedded in QPY formats 10, 11, and 12 at
this point we'll have to handle this edge case moving forward regardless
of whether we introduce a better solution in 1.3.0 or not. Although
realistically in that case we will likely need to just document this as
a limitation when exporting QPY payloads with Qiskit 0.45.0 through
1.2.3 (and with the ``version`` flag set to >= 10 and < 13) and have
explicit error checking around the symengine version (which this PR
adds) when in that code path.

* Fix release note upgrade section label

* Rewrite support documentation

* Fix mistakes in release note

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

---------

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: Jake Lishman <jake@binhbar.com>
(cherry picked from commit fee9f77)
github-merge-queue bot pushed a commit that referenced this pull request Oct 2, 2024
…13251) (#13255)

* Workaround symengine serialization payload incompatibility (#13251)

* Workaround symengine serialization payload incompatibility

In QPY we rely on symengine's internal serialization to represent the
internal symbolic expression stored inside a ParameterExpression object.
However, this format is nominally symengine version specific and will
raise an error if there is a mismatch between the version used to
generate the payload and what is trying to read it. This became an issue
in the recent symengine 0.13 release which started to raise an error
when people installed it and tried to load QPY payloads across the
versions. This makes the symengine serialization unsuitable for use in
QPY because it's supposed to be independent of these kind of concerns,
especially when QPY is used in a server-client model where you don't
necessarily control the installed environment of symengine.

To correctly address this issue we'll need a new version of the QPY
format that owns the serialization format of ParameterExpressions directly
instead of relying on symengine which doesn't offer a compatibility
guarantee on the format. However this won't be quick solution and users
are encountering issues since the release of 0.13. This commit
introduces a workaround for this specific instance of the mismatch. It
turns out the payload format between 0.11 and 0.13 is completely
unchanged except for the version number. So before passing the parameter
expression payload to symengine for deserialization this commit checks
the versions numbers are the same, if they're not it checks that we're
dealing with 0.11 or 0.13, and if so it changes the version number in
the payloads header appropriately. If the version number is outside
those bounds it raises an exception because while this hack is known to
be safe for translating between symengine 0.11 and 0.13, it's not
possible to know for a future version whether the payload format changed
or not.

Longer term we will need a proper fix in qpy version 13 that introduces
a qiskit native serialization format for parameter expression instead of
relying on symengine or sympy to do it for us.

* Handle schedules too

This commit updates the schedule serialization path too, as it was also
directly loading symengine expressions. The code handling the workaround
is extracted to a standalone function which is used in both spots now
instead of calling symengine directly.

* Remove unused imports

* Gracefully handle failure to parse historical symengine files

During the discovery on the fix in this PR we discovered that setting
the ``use_symengine`` flag from Qiskit 0.45.x and 0.46.x would result in
newer versions of Qiskit being unable to parse the QPY file for the same
issue as being addressed here. This commit expands the logic to account
for this and raise a useful warning. It also updates the release notes
and documentation to document these limitations.

* Cap upper version of symengine in requirements list

Out of an abundance of caution this commit places a cap on the allowed
version of symengine users can install to be compatible with Qiskit.
Due to the symengine version dependence discovered in QPY around
serializing ParameterExpressions, we'll likely have a similar issue
when symengine 0.14.0 releases. Pre-emptively capping this means we
aren't going to be in this situation until we can confirm compatibility
with QPY serialization. The real solution for this will come in #13252,
although as this behavior is embedded in QPY formats 10, 11, and 12 at
this point we'll have to handle this edge case moving forward regardless
of whether we introduce a better solution in 1.3.0 or not. Although
realistically in that case we will likely need to just document this as
a limitation when exporting QPY payloads with Qiskit 0.45.0 through
1.2.3 (and with the ``version`` flag set to >= 10 and < 13) and have
explicit error checking around the symengine version (which this PR
adds) when in that code path.

* Fix release note upgrade section label

* Rewrite support documentation

* Fix mistakes in release note

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

---------

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: Jake Lishman <jake@binhbar.com>
(cherry picked from commit fee9f77)

* Fix symengine typo

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Oct 2, 2024
This commit fixes a bug that slipped into the sole fix that made up
the now yanked 1.2.3 release, Qiskit#13251. The fix logic had a typo/mistake
when evaluating the major version in the symengine payload that meant
if you were trying to load a QPY payload generated with a different
symengine release it would always error. This only got through CI because
there was no test coverage of the edge case we were trying to fix. This
commit addresses both issues. It first fixes the typo in the QPY parsing
(which is a 2 character change) and then updates the QPY compat tests to
explicitly test using multiple symengine versions with QPY to make sure
we're exercising this code path moving forward (and validating this
fix).
github-merge-queue bot pushed a commit that referenced this pull request Oct 2, 2024
This commit fixes a bug that slipped into the sole fix that made up
the now yanked 1.2.3 release, #13251. The fix logic had a typo/mistake
when evaluating the major version in the symengine payload that meant
if you were trying to load a QPY payload generated with a different
symengine release it would always error. This only got through CI because
there was no test coverage of the edge case we were trying to fix. This
commit addresses both issues. It first fixes the typo in the QPY parsing
(which is a 2 character change) and then updates the QPY compat tests to
explicitly test using multiple symengine versions with QPY to make sure
we're exercising this code path moving forward (and validating this
fix).
mergify bot pushed a commit that referenced this pull request Oct 2, 2024
This commit fixes a bug that slipped into the sole fix that made up
the now yanked 1.2.3 release, #13251. The fix logic had a typo/mistake
when evaluating the major version in the symengine payload that meant
if you were trying to load a QPY payload generated with a different
symengine release it would always error. This only got through CI because
there was no test coverage of the edge case we were trying to fix. This
commit addresses both issues. It first fixes the typo in the QPY parsing
(which is a 2 character change) and then updates the QPY compat tests to
explicitly test using multiple symengine versions with QPY to make sure
we're exercising this code path moving forward (and validating this
fix).

(cherry picked from commit e5ec413)

# Conflicts:
#	qiskit/qpy/__init__.py
ElePT pushed a commit to ElePT/qiskit that referenced this pull request Oct 3, 2024
)

* Workaround symengine serialization payload incompatibility

In QPY we rely on symengine's internal serialization to represent the
internal symbolic expression stored inside a ParameterExpression object.
However, this format is nominally symengine version specific and will
raise an error if there is a mismatch between the version used to
generate the payload and what is trying to read it. This became an issue
in the recent symengine 0.13 release which started to raise an error
when people installed it and tried to load QPY payloads across the
versions. This makes the symengine serialization unsuitable for use in
QPY because it's supposed to be independent of these kind of concerns,
especially when QPY is used in a server-client model where you don't
necessarily control the installed environment of symengine.

To correctly address this issue we'll need a new version of the QPY
format that owns the serialization format of ParameterExpressions directly
instead of relying on symengine which doesn't offer a compatibility
guarantee on the format. However this won't be quick solution and users
are encountering issues since the release of 0.13. This commit
introduces a workaround for this specific instance of the mismatch. It
turns out the payload format between 0.11 and 0.13 is completely
unchanged except for the version number. So before passing the parameter
expression payload to symengine for deserialization this commit checks
the versions numbers are the same, if they're not it checks that we're
dealing with 0.11 or 0.13, and if so it changes the version number in
the payloads header appropriately. If the version number is outside
those bounds it raises an exception because while this hack is known to
be safe for translating between symengine 0.11 and 0.13, it's not
possible to know for a future version whether the payload format changed
or not.

Longer term we will need a proper fix in qpy version 13 that introduces
a qiskit native serialization format for parameter expression instead of
relying on symengine or sympy to do it for us.

* Handle schedules too

This commit updates the schedule serialization path too, as it was also
directly loading symengine expressions. The code handling the workaround
is extracted to a standalone function which is used in both spots now
instead of calling symengine directly.

* Remove unused imports

* Gracefully handle failure to parse historical symengine files

During the discovery on the fix in this PR we discovered that setting
the ``use_symengine`` flag from Qiskit 0.45.x and 0.46.x would result in
newer versions of Qiskit being unable to parse the QPY file for the same
issue as being addressed here. This commit expands the logic to account
for this and raise a useful warning. It also updates the release notes
and documentation to document these limitations.

* Cap upper version of symengine in requirements list

Out of an abundance of caution this commit places a cap on the allowed
version of symengine users can install to be compatible with Qiskit.
Due to the symengine version dependence discovered in QPY around
serializing ParameterExpressions, we'll likely have a similar issue
when symengine 0.14.0 releases. Pre-emptively capping this means we
aren't going to be in this situation until we can confirm compatibility
with QPY serialization. The real solution for this will come in Qiskit#13252,
although as this behavior is embedded in QPY formats 10, 11, and 12 at
this point we'll have to handle this edge case moving forward regardless
of whether we introduce a better solution in 1.3.0 or not. Although
realistically in that case we will likely need to just document this as
a limitation when exporting QPY payloads with Qiskit 0.45.0 through
1.2.3 (and with the ``version`` flag set to >= 10 and < 13) and have
explicit error checking around the symengine version (which this PR
adds) when in that code path.

* Fix release note upgrade section label

* Rewrite support documentation

* Fix mistakes in release note

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

---------

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: Jake Lishman <jake@binhbar.com>
ElePT pushed a commit to ElePT/qiskit that referenced this pull request Oct 3, 2024
This commit fixes a bug that slipped into the sole fix that made up
the now yanked 1.2.3 release, Qiskit#13251. The fix logic had a typo/mistake
when evaluating the major version in the symengine payload that meant
if you were trying to load a QPY payload generated with a different
symengine release it would always error. This only got through CI because
there was no test coverage of the edge case we were trying to fix. This
commit addresses both issues. It first fixes the typo in the QPY parsing
(which is a 2 character change) and then updates the QPY compat tests to
explicitly test using multiple symengine versions with QPY to make sure
we're exercising this code path moving forward (and validating this
fix).
github-merge-queue bot pushed a commit that referenced this pull request Oct 3, 2024
…13260)

* Fix bug in QPY symengine payload version handling (#13259)

This commit fixes a bug that slipped into the sole fix that made up
the now yanked 1.2.3 release, #13251. The fix logic had a typo/mistake
when evaluating the major version in the symengine payload that meant
if you were trying to load a QPY payload generated with a different
symengine release it would always error. This only got through CI because
there was no test coverage of the edge case we were trying to fix. This
commit addresses both issues. It first fixes the typo in the QPY parsing
(which is a 2 character change) and then updates the QPY compat tests to
explicitly test using multiple symengine versions with QPY to make sure
we're exercising this code path moving forward (and validating this
fix).

(cherry picked from commit e5ec413)

# Conflicts:
#	qiskit/qpy/__init__.py

* Fix merge artifact

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Julien Gacon <jules.gacon@googlemail.com>
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 priority: high 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.

4 participants