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

Add version flag to qpy dump #11644

Merged
merged 9 commits into from
Jan 31, 2024
27 changes: 26 additions & 1 deletion qiskit/qpy/interface.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@

"""User interface of qpy serializer."""

from __future__ import annotations

from json import JSONEncoder, JSONDecoder
from typing import Union, List, BinaryIO, Type, Optional
from collections.abc import Iterable
Expand Down Expand Up @@ -76,6 +78,7 @@ def dump(
file_obj: BinaryIO,
metadata_serializer: Optional[Type[JSONEncoder]] = None,
use_symengine: bool = True,
version: int | None = None,
Copy link
Member

Choose a reason for hiding this comment

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

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.

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 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in: 2b90f67

Copy link
Member

Choose a reason for hiding this comment

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

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.

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 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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

):
"""Write QPY binary data to a file

Expand Down Expand Up @@ -126,9 +129,23 @@ def dump(
but not supported in all platforms. Please check that your target platform is supported
by the symengine library before setting this option, as it will be required by qpy to
deserialize the payload. For this reason, the option defaults to False.
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.
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in: 2b90f67

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 added it to the api docs here: 2d38739 and removed the use of 10 in the docstring here in: 600bb9c


.. note::

If specified with an older version of QPY the limitations and potential bugs stemming
from the QPY format at that version will persist. This should only be used if
compatibility with loading the payload with an older version of Qiskit is necessary.

Raises:
QpyError: When multiple data format is mixed in the output.
TypeError: When invalid data type is input.
ValueError: When an unsupported version number is passed in for the ``version`` argument
"""
if not isinstance(programs, Iterable):
programs = [programs]
Expand All @@ -153,13 +170,21 @@ def dump(
else:
raise TypeError(f"'{program_type}' is not supported data type.")

if version is None:
version = common.QPY_VERSION
elif version not in {10, common.QPY_VERSION}:
raise ValueError(
f"The specified QPY version {version} is not support for dumping with this version, "
f"of Qiskit. The only supported versions are 10 and {common.QPY_VERSION}"
)

version_match = VERSION_PATTERN_REGEX.search(__version__)
version_parts = [int(x) for x in version_match.group("release").split(".")]
encoding = type_keys.SymExprEncoding.assign(use_symengine)
header = struct.pack(
formats.FILE_HEADER_V10_PACK,
b"QISKIT",
common.QPY_VERSION,
version,
mtreinish marked this conversation as resolved.
Show resolved Hide resolved
version_parts[0],
version_parts[1],
version_parts[2],
Expand Down
15 changes: 15 additions & 0 deletions releasenotes/notes/add-qpy-version-flag-6bb1756e671fde55.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
---
features:
- |
Added a new flag, ``version``, to the :func:`.qpy.dump` function which
optionally takes an integer value for the :ref:`qpy_format` version to
emit from the dump function. This is useful if you need to generate a QPY
file that will be loaded by an older version of Qiskit. However, the
supported versions to emit are very limited, only the latest QPY version
(which is the default), and the compatibility QPY version
which is :ref:`qpy_version_10` (which was introduced in Qiskit 0.45.0).
The compatibility version will remain fixed for the the entire 1.x.y major
version release series. This does not change the backwards compatibility
guarantees of the QPY format when calling :func:`.qpy.load`, it just enables
users to emit an older version of QPY to maintain compatibility and
interoperability between the 0.x and 1.x release series.
jakelishman marked this conversation as resolved.
Show resolved Hide resolved
26 changes: 24 additions & 2 deletions test/python/qpy/test_circuit_load_from_qpy.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,23 @@
class QpyCircuitTestCase(QiskitTestCase):
"""QPY schedule testing platform."""

def assert_roundtrip_equal(self, circuit):
def assert_roundtrip_equal(self, circuit, version=None):
"""QPY roundtrip equal test."""
qpy_file = io.BytesIO()
dump(circuit, qpy_file)
dump(circuit, qpy_file, version=version)
qpy_file.seek(0)
new_circuit = load(qpy_file)[0]

self.assertEqual(circuit, new_circuit)
self.assertEqual(circuit.layout, new_circuit.layout)
if version is not None:
qpy_file.seek(0)
file_version = struct.unpack("!6sB", qpy_file.read(7))[1]
self.assertEqual(
version,
file_version,
f"Generated QPY file version {file_version} does not match request version {version}",
)


@ddt
Expand Down Expand Up @@ -185,3 +193,17 @@ def test_no_register(self, opt_level):
list(new_circuit.layout.input_qubit_mapping.values()),
)
self.assertEqual(tqc.layout.final_layout, new_circuit.layout.final_layout)

def test_invalid_version_value(self):
"""Assert we raise an error with an invalid version request."""
qc = QuantumCircuit(2)
with self.assertRaises(ValueError):
dump(qc, io.BytesIO(), version=3)

def test_version_10_roundtrip(self):
"""Test the version is set correctly when specified."""
qc = QuantumCircuit(2)
qc.h(0)
qc.cx(0, 1)
qc.measure_all()
self.assert_roundtrip_equal(qc, version=10)
mtreinish marked this conversation as resolved.
Show resolved Hide resolved