Add version flag to qpy dump #11644
Add version flag to qpy dump #11644
Conversation
This commit adds a new keyword argument to the qpy.dump() function which is used to specify a compatibility version of QPY which is the latest version for the 0.45 and 0.46 version. This basically adds a compatibility mode for generating QPY files that returns a fixed QPY version that enables the 0.45 and 0.46 release to load the QPY files generated with the 1.0 release. The default output doesn't change and dump() will always return the latest QPY format version by default as this is the version that gets all the bugfixes and is kept up to date with the QuantumCircuit class. As of this commit the compatibility version and the latest version are the same, both at version 10. However, this flag is being added so that the interface exists for when we inevitably need to update the QPY version field for any release in the 1.x major version series. There are potential PRs in the 1.0 release series that will bump the latest QPY version to 11 which will change this and those PRs will need to factor this in when they change the format.
mtreinish
added
Changelog: New Feature
|
One or more of the the following people are requested to review this:
|
qiskit/qpy/interface.py
Outdated
| @@ -76,6 +78,7 @@ def dump( | |||
| file_obj: BinaryIO, | |||
| metadata_serializer: Optional[Type[JSONEncoder]] = None, | |||
| use_symengine: bool = True, | |||
| version: int | None = None, | |||
There was a problem hiding this comment.
Do you think it's worth setting this directly as version: int = 10, and us just changing that default on minor releases (if appropriate)? I'm not sure if allowing None buys us anything, and using the value directly makes it appear more obviously in things like documentation and help.
There was a problem hiding this comment.
I specifically want the default to always be the latest version. I'm fine with making the default explicit, but I would prefer version: int = common.QPY_VERSION so that we are always tracking it.
There was a problem hiding this comment.
I think the edge case I have in mind is if we have to release a new QPY format in a patch release, but we still want the default to be fixed for the minor release.
If we elect that bugs requiring a QPY format change aren't eligible for backport, then I agree with using the CURRENT_VERSION flag, otherwise I prefer a separate DEFAULT_VERSION attribute, but I don't feel too strongly.
There was a problem hiding this comment.
I think not allowing QPY format changes in bugfix releases makes the most sense for a few reasons. I hadn't considered decoupling the latest and default for a potential backport scenario that's an interesting idea. But I'm not sure even then we'd want to allow backports to patch releases because to me it increases the complexity of the backport and increases the risk of making the change.
There was a problem hiding this comment.
Yeah, in general I agree with this, it's just not hitherto been our practice. If we want to change the policy to this for 1.0+ I'm on board.
qiskit/qpy/interface.py
Outdated
| version: The QPY format version to emit. By default this defaults to | ||
| the latest supported format (which is currently 10), however for | ||
| compatibility reasons if you need to load the generated QPY payload with an older version | ||
| of Qiskit you can also select the minimum supported export version, which only can change | ||
| during a Qiskit major version release, to generate an older QPY format version. As of this | ||
| major release series the minimum supported export version is version 10. |
There was a problem hiding this comment.
If we changed the default to just be an int, we wouldn't need to comment on the "this is currently 10" bit, which I'm almost certain we'll forget to update next time we change it.
We might want to expose the minimum supported export version as a data attribute on the qiskit.qpy module (e.g. qiskit.qpy.MINIMUM_DUMP_VERSION = 10) so it can be queried programmatically and so we can just point to that in the docs and have that sentence autoupdate as well. (tbh, we probably want qiskit.qpy.CURRENT_VERSION as well)
There was a problem hiding this comment.
qpy.common.QPY_VERSION is the equivalent of what qiskit.qpy.CURRENT_VERSION would be. But we can rexport it and add a module attribute easily enough.
There was a problem hiding this comment.
Now version=10 shows up in the signature in the API docs, but I think it would be good to add QPY_VERSION and QPY_COMPATIBILITY_VERSION to the API docs as well, and possibly reference them instead of writing 10 in the docstring here.
Pull Request Test Coverage Report for Build 7716962098Warning: This coverage report may be inaccurate.We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
💛 - Coveralls |
Co-authored-by: Will Shanks <willshanks@us.ibm.com>
Summary
This commit adds a new keyword argument to the qpy.dump() function which is used to specify a compatibility version of QPY which is the latest version for the 0.45 and 0.46 version. This basically adds a compatibility mode for generating QPY files that returns a fixed QPY version that enables the 0.45 and 0.46 release to load the QPY files generated with the 1.0 release. The default output doesn't change and dump() will always return the latest QPY format version by default as this is the version that gets all the bugfixes and is kept up to date with the QuantumCircuit class.
As of this commit the compatibility version and the latest version are the same, both at version 10. However, this flag is being added so that the interface exists for when we inevitably need to update the QPY version field for any release in the 1.x major version series. There are potential PRs in the 1.0 release series that will bump the latest QPY version to 11 which will change this and those PRs will need to factor this in when they change the format.
Details and comments