Handle custom operations with overlapping names in QPY

qiskit/qpy/__init__.py

@@ -162,6 +162,16 @@
 by ``num_circuits`` in the file header). There is no padding between the
 circuits in the data.
 
+.. _qpy_version_11:
+
+Version 11
+==========
+
+Version 11 is identical to Version 10 except that for names in the CUSTOM_INSTRUCTION blocks
+have a suffix of the form ``"_{uuid_hex}"`` where ``uuid_hex`` is a uuid
+hexadecimal string such as returned by :attr:`.UUID.hex`. For example:
+``"b3ecab5b4d6a4eb6bc2b2dbf18d83e1e"``.
+
 .. _qpy_version_10:
 
 Version 10

qiskit/qpy/binary_io/circuits.py

@@ -373,6 +373,9 @@ def _parse_custom_operation(
         ) = custom_operations[gate_name]
     else:
         type_str, num_qubits, num_clbits, definition = custom_operations[gate_name]
+    # Strip the trailing "_{uuid}" from the gate name if the version >=11
+    if version >= 11:
+        gate_name = "_".join(gate_name.split("_")[:-1])
     type_key = type_keys.CircuitInstruction(type_str)
 
     if type_key == type_keys.CircuitInstruction.INSTRUCTION:
@@ -572,7 +575,7 @@ def _dumps_instruction_parameter(param, index_map, use_symengine):
 
 
 # pylint: disable=too-many-boolean-expressions
-def _write_instruction(file_obj, instruction, custom_operations, index_map, use_symengine):
+def _write_instruction(file_obj, instruction, custom_operations, index_map, use_symengine, version):
     if isinstance(instruction.operation, Instruction):
         gate_class_name = instruction.operation.base_class.__name__
     else:
@@ -591,15 +594,17 @@ def _write_instruction(file_obj, instruction, custom_operations, index_map, use_
         or isinstance(instruction.operation, library.BlueprintCircuit)
     ):
         gate_class_name = instruction.operation.name
+        if version >= 11:
+            # Assign a uuid to each instance of a custom operation
+            gate_class_name = f"{gate_class_name}_{uuid.uuid4().hex}"
         # ucr*_dg gates can have different numbers of parameters,
         # the uuid is appended to avoid storing a single definition
         # in circuits with multiple ucr*_dg gates.
-        if instruction.operation.name in ["ucrx_dg", "ucry_dg", "ucrz_dg"]:
-            gate_class_name += "_" + str(uuid.uuid4())
+        elif instruction.operation.name in {"ucrx_dg", "ucry_dg", "ucrz_dg"}:
+            gate_class_name = f"{gate_class_name}_{uuid.uuid4()}"
 
-        if gate_class_name not in custom_operations:
-            custom_operations[gate_class_name] = instruction.operation
-            custom_operations_list.append(gate_class_name)
+        custom_operations[gate_class_name] = instruction.operation
+        custom_operations_list.append(gate_class_name)
 
     elif gate_class_name == "ControlledGate":
         # controlled gates can have the same name but different parameter
@@ -724,7 +729,7 @@ def _write_elem(buffer, op):
     file_obj.write(synth_data)
 
 
-def _write_custom_operation(file_obj, name, operation, custom_operations, use_symengine):
+def _write_custom_operation(file_obj, name, operation, custom_operations, use_symengine, version):
     type_key = type_keys.CircuitInstruction.assign(operation)
     has_definition = False
     size = 0
@@ -768,6 +773,7 @@ def _write_custom_operation(file_obj, name, operation, custom_operations, use_sy
                 custom_operations,
                 {},
                 use_symengine,
+                version,
             )
             base_gate_raw = base_gate_buffer.getvalue()
     name_raw = name.encode(common.ENCODE)
@@ -1002,7 +1008,9 @@ def _read_layout_v2(file_obj, circuit):
         circuit._layout._output_qubit_list = circuit.qubits
 
 
-def write_circuit(file_obj, circuit, metadata_serializer=None, use_symengine=False):
+def write_circuit(
+    file_obj, circuit, metadata_serializer=None, use_symengine=False, version=common.QPY_VERSION
+):
     """Write a single QuantumCircuit object in the file like object.
 
     Args:
@@ -1016,6 +1024,7 @@ def write_circuit(file_obj, circuit, metadata_serializer=None, use_symengine=Fal
             native mechanism. This is a faster serialization alternative, 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.
+        version (int): The QPY format version to use for serializing this circuit
     """
     metadata_raw = json.dumps(
         circuit.metadata, separators=(",", ":"), cls=metadata_serializer
@@ -1056,7 +1065,7 @@ def write_circuit(file_obj, circuit, metadata_serializer=None, use_symengine=Fal
     index_map["c"] = {bit: index for index, bit in enumerate(circuit.clbits)}
     for instruction in circuit.data:
         _write_instruction(
-            instruction_buffer, instruction, custom_operations, index_map, use_symengine
+            instruction_buffer, instruction, custom_operations, index_map, use_symengine, version
         )
 
     with io.BytesIO() as custom_operations_buffer:
@@ -1068,7 +1077,12 @@ def write_circuit(file_obj, circuit, metadata_serializer=None, use_symengine=Fal
                 operation = custom_operations[name]
                 new_custom_operations.extend(
                     _write_custom_operation(
-                        custom_operations_buffer, name, operation, custom_operations, use_symengine
+                        custom_operations_buffer,
+                        name,
+                        operation,
+                        custom_operations,
+                        use_symengine,
+                        version,
                     )
                 )
 

qiskit/qpy/binary_io/schedules.py

@@ -574,7 +574,9 @@ def read_schedule_block(file_obj, version, metadata_deserializer=None, use_symen
     return block
 
 
-def write_schedule_block(file_obj, block, metadata_serializer=None, use_symengine=False):
+def write_schedule_block(
+    file_obj, block, metadata_serializer=None, use_symengine=False, version=common.QPY_VERSION
+):  # pylint: disable=unused-argument
     """Write a single ScheduleBlock object in the file like object.
 
     Args:
@@ -588,6 +590,7 @@ def write_schedule_block(file_obj, block, metadata_serializer=None, use_symengin
             native mechanism. This is a faster serialization alternative, 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.
+        version (int): The QPY format version to use for serializing this circuit block
     Raises:
         TypeError: If any of the instructions is invalid data format.
     """

qiskit/qpy/common.py

@@ -20,7 +20,7 @@
 
 from qiskit.qpy import formats
 
-QPY_VERSION = 10
+QPY_VERSION = 11
 QPY_COMPATIBILITY_VERSION = 10
 ENCODE = "utf8"
 

qiskit/qpy/interface.py

@@ -199,7 +199,11 @@ def dump(
 
     for program in programs:
         writer(
-            file_obj, program, metadata_serializer=metadata_serializer, use_symengine=use_symengine
+            file_obj,
+            program,
+            metadata_serializer=metadata_serializer,
+            use_symengine=use_symengine,
+            version=version,
         )
 
 

releasenotes/notes/fix-qpy-custom-gates-name-conflict-5c4c7df3484f04e0.yaml

@@ -0,0 +1,19 @@
+---
+upgrade:
+  - |
+    The latest format version of QPY is now :ref:`qpy_version_11` and this
+    is what is emitted by default when running :func:`.qpy.dump`.
+fixes:
+  - |
+    Fixed an issue with the QPY serialization when a :class:`.QuantumCircuit`
+    contained multiple custom instructions instances that have the same
+    :attr:`~.Instruction.name` attribute. In QPY format versions before
+    :ref:`qpy_version_11` the QPY payload did not differentiate between
+    these instances and would only serialize the properties of the first
+    instance in a circuit. This could potentially cause an incorrect
+    deserialization if the other properties of the custom instruction were
+    different but the names were the same. This has been fixed in
+    QPY :ref:`qpy_version_11` so that each instance of a custom instruction
+    is serialized individually and there will no longer be a potential
+    conflict with overlapping names.
+    Fixes `#8941 <https://github.com/Qiskit/qiskit/issues/8941>`__

test/python/qasm2/test_structure.py

@@ -20,7 +20,6 @@
 import pickle
 import shutil
 import tempfile
-import unittest
 
 import ddt
 
@@ -657,8 +656,6 @@ def test_qpy_single_call_roundtrip(self):
             loaded = qpy.load(fptr)[0]
         self.assertEqual(loaded, qc)
 
-    # See https://github.com/Qiskit/qiskit-terra/issues/8941
-    @unittest.expectedFailure
     def test_qpy_double_call_roundtrip(self):
         program = """
             include "qelib1.inc";

test/python/qpy/test_circuit_load_from_qpy.py

@@ -17,7 +17,7 @@
 
 from ddt import ddt, data
 
-from qiskit.circuit import QuantumCircuit, QuantumRegister, Qubit
+from qiskit.circuit import QuantumCircuit, QuantumRegister, Qubit, Parameter, Gate
 from qiskit.providers.fake_provider import FakeHanoi, FakeSherbrooke
 from qiskit.exceptions import QiskitError
 from qiskit.qpy import dump, load, formats, QPY_COMPATIBILITY_VERSION
@@ -146,6 +146,45 @@ def test_empty_layout(self):
         qc._layout = TranspileLayout(None, None, None)
         self.assert_roundtrip_equal(qc)
 
+    def test_overlapping_definitions(self):
+        """Test serialization of custom gates with overlapping definitions."""
+
+        class MyParamGate(Gate):
+            """Custom gate class with a parameter."""
+
+            def __init__(self, phi):
+                super().__init__("my_gate", 1, [phi])
+
+            def _define(self):
+                qc = QuantumCircuit(1)
+                qc.rx(self.params[0], 0)
+                self.definition = qc
+
+        theta = Parameter("theta")
+        two_theta = 2 * theta
+
+        qc = QuantumCircuit(1)
+        qc.append(MyParamGate(1.1), [0])
+        qc.append(MyParamGate(1.2), [0])
+        qc.append(MyParamGate(3.14159), [0])
+        qc.append(MyParamGate(theta), [0])
+        qc.append(MyParamGate(two_theta), [0])
+        with io.BytesIO() as qpy_file:
+            dump(qc, qpy_file)
+            qpy_file.seek(0)
+            new_circ = load(qpy_file)[0]
+        # Custom gate classes are lowered to Gate to avoid arbitrary code
+        # execution on deserialization. To compare circuit equality we
+        # need to go instruction by instruction and check that they're
+        # equivalent instead of doing a circuit equality check
+        for new_inst, old_inst in zip(new_circ.data, qc.data):
+            new_gate = new_inst.operation
+            old_gate = old_inst.operation
+            self.assertIsInstance(new_gate, Gate)
+            self.assertEqual(new_gate.name, old_gate.name)
+            self.assertEqual(new_gate.params, old_gate.params)
+            self.assertEqual(new_gate.definition, old_gate.definition)
+
     @data(0, 1, 2, 3)
     def test_custom_register_name(self, opt_level):
         """Test layout preserved with custom register names."""
@@ -194,6 +233,56 @@ def test_no_register(self, opt_level):
         )
         self.assertEqual(tqc.layout.final_layout, new_circuit.layout.final_layout)
 
+
+class TestVersionArg(QpyCircuitTestCase):
+    """Test explicitly setting a qpy version in dump()."""
+
+    def test_custom_gate_name_overlap_persists_with_minimum_version(self):
+        """Assert the fix in version 11 doesn't get used if an older version is request."""
+
+        class MyParamGate(Gate):
+            """Custom gate class with a parameter."""
+
+            def __init__(self, phi):
+                super().__init__("my_gate", 1, [phi])
+
+            def _define(self):
+                qc = QuantumCircuit(1)
+                qc.rx(self.params[0], 0)
+                self.definition = qc
+
+        theta = Parameter("theta")
+        two_theta = 2 * theta
+
+        qc = QuantumCircuit(1)
+        qc.append(MyParamGate(1.1), [0])
+        qc.append(MyParamGate(1.2), [0])
+        qc.append(MyParamGate(3.14159), [0])
+        qc.append(MyParamGate(theta), [0])
+        qc.append(MyParamGate(two_theta), [0])
+        with io.BytesIO() as qpy_file:
+            dump(qc, qpy_file, version=10)
+            qpy_file.seek(0)
+            new_circ = load(qpy_file)[0]
+        # Custom gate classes are lowered to Gate to avoid arbitrary code
+        # execution on deserialization. To compare circuit equality we
+        # need to go instruction by instruction and check that they're
+        # equivalent instead of doing a circuit equality check
+        first_gate = None
+        for new_inst, old_inst in zip(new_circ.data, qc.data):
+            new_gate = new_inst.operation
+            old_gate = old_inst.operation
+            self.assertIsInstance(new_gate, Gate)
+            self.assertEqual(new_gate.name, old_gate.name)
+            self.assertEqual(new_gate.params, old_gate.params)
+            if first_gate is None:
+                first_gate = new_gate
+                continue
+            # This is incorrect behavior. This test is explicitly validating
+            # that the version kwarg being set to 10 causes the buggy behavior
+            # on that version of qpy
+            self.assertEqual(new_gate.definition, first_gate.definition)
+
     def test_invalid_version_value(self):
         """Assert we raise an error with an invalid version request."""
         qc = QuantumCircuit(2)