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

Workaround symengine serialization payload incompatibility #13251

Merged
merged 8 commits into from
Oct 2, 2024
+145 −10
Merged

Workaround symengine serialization payload incompatibility #13251

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
994be82
Workaround symengine serialization payload incompatibility
mtreinish Oct 1, 2024
ea69a45
Handle schedules too
mtreinish Oct 1, 2024
7aecf1a
Remove unused imports
mtreinish Oct 1, 2024
e089d5c
Gracefully handle failure to parse historical symengine files
mtreinish Oct 1, 2024
7aae984
Cap upper version of symengine in requirements list
mtreinish Oct 1, 2024
8486499
Fix release note upgrade section label
mtreinish Oct 2, 2024
9c7f1f3
Rewrite support documentation
jakelishman Oct 2, 2024
3946891
Fix mistakes in release note
jakelishman Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions qiskit/qpy/binary_io/value.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,29 @@ def _read_parameter_expression_v3(file_obj, vectors, use_symengine):

payload = file_obj.read(data.expr_size)
if use_symengine:
# This is a horrible hack to workaround the symengine version checking
# it's deserialization does. There were no changes to the serialization
# format between 0.11 and 0.13 but the deserializer checks that it can't
# load across a major or minor version boundary. This works around it
# by just lying about the generating version.
jakelishman marked this conversation as resolved.
Show resolved Hide resolved
symengine_version = symengine.__version__.split(".")
major = payload[2]
minor = payload[3]
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.

payload = bytearray(payload)
payload[3] = minor_version
payload = bytes(payload)
expr_ = load_basic(payload)
else:
from sympy.parsing.sympy_parser import parse_expr
Expand Down
13 changes: 13 additions & 0 deletions releasenotes/notes/fix-qpy-symengine-compat-858970a9a1d6bc14.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
---
fixes:
- |
Fixed an issue with :func:`.qpy.load` when loading a QPY payload that
contains a :class:`.ParameterExpression` that was generated when symengine
0.11 was installed and trying to load it in an environment where symengine
0.13 (the latest symengine release as of this qiskit release) or vice versa.
Previously, an error would have been raised by symengine around this version
mismatch. This has been worked around for these two specific symengine versions
but if you're trying to use different versions of symengine and there is a
mismatch with this version of Qiskit this might not work. You will need to
install Qiskit >1.3.0 to fix this mismatch issue more broadly for any potential
future version of symengine.
Loading