QPY serialization fails for circuits with IfElse instructions

[kdk]

Environment

• Qiskit Terra version: 0.20.0.dev0+01913f4
• Python version: 3.7
• Operating system: Mac OSX 11.6.1

What is happening?

QPY serialization fails when attempting to serialize a circuit containing an IfElseOp.

How can we reproduce the issue?

import qiskit as qk

qc = qk.QuantumCircuit(2, 2)
qc.h(0)
qc.measure(0, 0)

with qc.if_test((qc.clbits[0], True)):
    qc.x(1)
    
qc.measure(1,1)

from qiskit.circuit import qpy_serialization as qpy
import tempfile

with tempfile.TemporaryFile() as fp:
    qpy.dump(qc, fp)
    out = qpy.load(fp)

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
/var/folders/97/vkqk0hs17r5dnlc8dv5nmfxh0000gn/T/ipykernel_34451/736355381.py in <module>
      3 
      4 with tempfile.TemporaryFile() as fp:
----> 5     qpy.dump(qc, fp)
      6     out = qpy.load(fp)

~/q/qiskit-terra/qiskit/circuit/qpy_serialization.py in dump(circuits, file_obj)
   1423     file_obj.write(header)
   1424     for circuit in circuits:
-> 1425         _write_circuit(file_obj, circuit)
   1426 
   1427 

~/q/qiskit-terra/qiskit/circuit/qpy_serialization.py in _write_circuit(file_obj, circuit)
   1493     index_map["c"] = clbit_indices
   1494     for instruction in circuit.data:
-> 1495         _write_instruction(instruction_buffer, instruction, custom_instructions, index_map)
   1496     file_obj.write(struct.pack(CUSTOM_DEFINITION_HEADER_PACK, len(custom_instructions)))
   1497 

~/q/qiskit-terra/qiskit/circuit/qpy_serialization.py in _write_instruction(file_obj, instruction_tuple, custom_instructions, index_map)
   1223         else:
   1224             raise TypeError(
-> 1225                 f"Invalid parameter type {instruction_tuple[0]} for gate {type(param)},"
   1226             )
   1227         instruction_param_raw = struct.pack(INSTRUCTION_PARAM_PACK, type_key.encode("utf8"), size)

TypeError: Invalid parameter type Instruction(name='if_else', num_qubits=1, num_clbits=1, params=[<qiskit.circuit.quantumcircuit.QuantumCircuit object at 0x13c510750>, None]) for gate <class 'qiskit.circuit.quantumcircuit.QuantumCircuit'>,

What should happen?

The circuit should be able to be serialized and de-serialized.

Any suggestions?

No response

[mtreinish]

So there are 2 things we need to fix here. The first is to add qiskit.circuit.controlflow to the module lookup path for terra objects here: https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/circuit/qpy_serialization.py#L1122-L1125 and https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/circuit/qpy_serialization.py#L976-L983 so that qpy know about the control flow instructions. Otherwise it will try to treat the control flow instruction objects as custom instructions and make them instances of Instruction instead of the actual control flow object. The other piece of this is to allow circuits as a valid parameter type for instructions, here: https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/circuit/qpy_serialization.py#L1183-L1222 and https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/circuit/qpy_serialization.py#L934-L956 and we can just use https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/circuit/qpy_serialization.py#L1428 with a BytesIO buffer for the payload.

The only potential (mostly because I'm not as familiar with the control flow instructions) complication I can think of is if IfElseOp(**params) doesn't work we'll need to add a custom handling for it here: https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/circuit/qpy_serialization.py#L961-L991 in the deserializer. But, if we need more information from the object than what is serialized by default we'll probably need to define a custom serialization format for it (similar to what we did for the PauliEvolutionGate in #7374) so that we have all the necessary information in the qpy data to reconstruct the operation on the deserialization side.

[jakelishman]

IfElseOp(**params) will fail, because the first positional argument is the condition, which is stored separately on Instruction, not in params. The same goes for WhileLoopOp, but ForLoopOp is in that form.

I guess we need to add an arbitrary serialisation hook here. I think we want to avoid any possible arbitrary _de_serialisation hooks, so we could ask for a method that will return a 2-tuple (args, kwargs) that can be passed to the initialiser to reconstruct the state of the object, and then recursively serialise those?

[mtreinish]

Well we've had to do deserialization conditions before for instruction types which behave differently for weird reasons. Barrier and Initialize are the only two currently. We also serialize the condition attribute currently. So assuming the condition attribute is the same tuple of (register/bit, value) that other instructions have we wouldn't need to change anything on the serialization side and we can just add conditions like we do for barrier and initialize today. I think this might be preferable just because it's a smaller change for a backport and doesn't require defining a new type for the custom instruction table.

I think for the ever growing qpy v4 todo list (which already includes a global parameter table and pulse support) in 0.20.0 allowing an instruction class to define it's own (args, kwargs) for the constructor with something like a __qpy_args__() method for serialization is a good thing to do.

[jakelishman]

Having the condition is fine, the problem is that the constructor of IfElseOp and WhileLoopOp take condition as their first positional argument, and don't put condition in the params attribute. The condition is the same tuple, but you do something like

my_if = IfElseOp((creg, 1), true_body, false_body)
assert len(my_if.params) == 2
assert my_if.params[0] is true_body
assert my_if.params[1] is false_body

We can do it, but it'll need special handling for IfElseOp and WhileLoopOp when reconstructing the instances, to pass the condition positionally first (they error if there's no condition).

Also, can qpy currently serialise a parameter of None? IfElseOp can have None as its second parameter if there isn't a false body.

[mtreinish]

Right, I just meant we have a place already for this kind of thing where we have to do that kind of custom deserialization construction. Since we had to do it for Barrier (which does Barrier(num_qubits) which isn't a parameter) and Initialize (which does Initialize(params) instead of Initialize(*params). So for 0.19.4 I think if we do something like:

if gate_name in {"IfElseOp", "WhileLoopOp"}:
    gate_class(condition, *params)

around here: https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/circuit/qpy_serialization.py#L986-L992 that fits in with an existing pattern and won't be too intrusive and seems like the simple path for a backport fix.

As for None, that's a good call we don't support that yet either. That's trivial to add as a supported param type, we can add that at the same time we do QuantumCircuit