-
Notifications
You must be signed in to change notification settings - Fork 141
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
_write_parameter_expression
takes version parameter
#1674
Conversation
Pull Request Test Coverage Report for Build 9119300455Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's probably the calling convention for it now, yeah, but that's very much not a public API function, and we cannot make guarantees that it'll even continue to exist in the future, given that we may well need to refactor the QPY export to be faster.
serializer=_write_parameter_expression, | ||
compress=False, | ||
use_symengine=bool(optionals.HAS_SYMENGINE), | ||
version=QPY_VERSION, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version wants to be compatible (less than) whatever version of QPY you'll be using on the backend to deserialise - QPY_VERSION
will always be the max the local version of Qiskit can handle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the use of QPY_VERSION
incorrect here then? I don't believe the server side has updated to qiskit 1.1 yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should repeat the logic in other places around the version kwarg and keep the version there the same as in other places. It might makes sense to extract that to a constant like RUNTIME_QPY_VERSION = 11
or something.
if (_TERRA_VERSION[0] == 1 and _TERRA_VERSION[1] >= 1) or _TERRA_VERSION[0] > 1:
kwargs["version"] = 11
From the Slack discussion I think we can just drop the support for |
Closing this PR, i'll open a separate one |
Summary
@jakelishman
version
was added as a new argument here which I believe caused our unit tests against Qiskit/main to start failing. Is this the correct way to handleversion
from the provider?Details and comments