Handle an optional symengine in qpy #13917
Conversation
This commit changes the API for qpy.load() to make it explicit that symengine may not be installed in the future but is a requirement for deserializing certain payloads that were generated using symengine. We're expecting to drop symengine as a requirement with Qiskit#13278 which should improve Qiskit's installation story and also will facilitate adding a C api for circuits. But the qpy v10, v11, and v13 have a hard requirement on symengine (specifically 0.11.0 and 0.13.0) to be able to deserialize `ParameterExpression` if the symbolic encoding was set to use it. Since the function needs to support being able to deserialize these payloads symengine will need to be installed. This commit adds the document and release note to indicate this change in behavior.
mtreinish
added
documentation
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 13568873793Details
💛 - Coveralls |
This commit raises the qpy compatibility version to QPY format version 13. As the larger PR is planning for a world without symengine installed by default this becomes a problem for the generation side too. This becomes a blocker for using QPY < 13 in the future so this commit opts to just raise the minimum verison to get ahead of any potential issues.
mtreinish
changed the title
| @@ -209,52 +209,6 @@ def test_no_register(self, opt_level): | |||
| class TestVersionArg(QpyCircuitTestCase): | |||
| """Test explicitly setting a qpy version in dump().""" | |||
|
|
|||
| def test_custom_gate_name_overlap_persists_with_minimum_version(self): | |||
There was a problem hiding this comment.
This test was removed because it was explicitly testing the bug fixed in QPY 11 was present if we specifically set the QPY version to 10. Since the bug is still fixed in QPY 13, there isn't anything to test anymore since we can't generate a buggy payload.
There was a problem hiding this comment.
For most of this PR I'm happy, but I'm a bit concerned that we didn't issue deprecation warnings in Qiskit 1.4 about the withdrawal of support for writing QPY versions 10--12. How painful would it be to continue to support them, and document that the optional is required to support write out of those QPY versions with symengine/sympy?
| use_symengine: This flag is no longer used by QPY versions supported by this function and | ||
| will have no impact on the generated QPY payload except to set a field in a QPY v13 file | ||
| header which is unused. |
There was a problem hiding this comment.
It was potentially a mistake for us not to update symbolic_encoding in version 13 to be some typecode that's independent of sympy and symengine. I guess there's not much to be done about it now.
There was a problem hiding this comment.
Yeah, it was an oversight, we were a bit rushed trying to get QPY13 in for 1.3. It's something we could potentially fix in v14, but it's a bit tight timewise to slip that in to v14. But we also missed the deprecation of the kwarg here so we'd be stuck with an arg that has no impact either way.
There was a problem hiding this comment.
I'm not all that beaten up about the kwarg tbh, it's more the payload. Right now, with QPY 13 already having shipped with a meaningless field there, I'm not sure how much purpose there is in changing it in QPY 14.
The way this is implemented in the #13278 currently is that it basically stringifies the inner rust expression object, runs it through a regex find replace to update the syntax, then passes that string to sympy to create a new expression object. If the sympy object is then either used verbatim if sympy encoding is used or converted to a symengine expression and that is encoded. It seems to work for all our current tests that use an explicit version, but it seems overly fragile for a core piece of qiskit. FWIW, we did document this case in the stability guidelines and the dump docstring to be only guaranteed to work for the n-1 release, so I think we have a leg to stand on here. Obviously it would have been much better if we did warn about it in <= 1.4.0, but that ship has sailed. |
Summary
This commit changes the API for qpy.load() to make it explicit that symengine may not be installed in the future but is a requirement for deserializing certain payloads that were generated using symengine. We're expecting to drop symengine as a requirement with #13278 which should improve Qiskit's installation story and also will facilitate adding a C api for circuits. But the qpy v10, v11, and v13 have a hard requirement on symengine (specifically 0.11.0 and 0.13.0) to be able to deserialize
ParameterExpressionif the symbolic encoding was set to use it. Since the function needs to support being able to deserialize these payloads symengine will need to be installed. This commit adds the document and release note to indicate this change in behavior.Similarly,
qpy.dump()will have issues if symengine is not used anymore for the internal representation for QPY 10, 11, and 12 of an expression relies on symengine. So we won't necessarily be able to generate a symengine expression losslessly or without any overhead so trying to support this in a model without symengine installed isn't a tenable. This commit raises the minimum compatibility version so symengine (or sympy) isn't needed to generate the payload anymore.Details and comments