QPY invalid payload generation via compose() or QuantumCircuit.control()

[mtreinish]

Environment

• Qiskit Terra version: 0.23.2
• Python version: 3.10
• Operating system: Linux

What is happening?

Running compose() with inplace=True from an input generated with QuantumCircuit.control() leads to a circuit that when qpy serialized is not load-able. This points to an internal state that doesn't match the actual data of the instruction object. I expect the mismatch is caused by the number of arguments or qubits the gate reported in qc which is incorrect

The specific failure in this case is:

Traceback (most recent call last):
  File "/tmp/test_qpy_roundtrip.py", line 17, in <module>
    new_qc = load(fd)[0]
  File "/tmp/foo/lib/python3.10/site-packages/qiskit/qpy/interface.py", line 269, in load
    loader(
  File "/tmp/foo/lib/python3.10/site-packages/qiskit/qpy/binary_io/circuits.py", line 905, in read_circuit
    _read_instruction(file_obj, circ, out_registers, custom_operations, version, vectors)
  File "/tmp/foo/lib/python3.10/site-packages/qiskit/qpy/binary_io/circuits.py", line 161, in _read_instruction
    struct.unpack(
struct.error: unpack requires a buffer of 33 bytes

How can we reproduce the issue?

import io

from qiskit.circuit.random import random_circuit
import numpy as np
from qiskit import QuantumCircuit
from qiskit.qpy import dump, load

qc0 = random_circuit(2, 2, seed=1).decompose(reps=1)
qc1 = random_circuit(2, 2, seed=1).decompose(reps=1)
qc = QuantumCircuit(3)
qc.compose(qc0.control(1), [0,1,2], inplace=True)
qc.compose(qc1.control(1), [0,1,2], inplace=True)

with io.BytesIO() as fd:
    dump(qc, fd)
    fd.seek(0)
    new_qc = load(fd)[0]

assert qc == new_qc

What should happen?

This should not error during the load() call

Any suggestions?

I believe something about the compose call is corrupting the internal state of the circuit which is leading to a QPY payload that has a mismatch between a size

[mtreinish]

For anyone experiencing this you can work around this failure by changing:

qc.compose(qc0.control(1), [0,1,2], inplace=True)
qc.compose(qc1.control(1), [0,1,2], inplace=True)

to

qc.append(qc0.control(1), [0,1,2])
qc.append(qc1.control(1), [0,1,2])

[jakelishman]

I think something's a bit odd about QuantumCircuit.control: it constructs a gate, from the circuit, controls that, and adds the resulting gate to a circuit. That means that when you compose it onto a circuit, it's still a wrapped custom instruction rather than having been properly inlined.

If I change QuantumCircuit.control to just return circuit_to_gate(self).control(...).definition, the QPY stuff works fine and that code makes more sense. That said, it's not necessarily the correct fix for here (unless there's an actual bug in QuantumCircuit.control), because the current form should still be producing a valid circuit that roundtrips through QPY.

[mtreinish]

Hmm, that seems odd qpy shouldn't care about it being wrapped in a custom instruction. I wonder if it's the same bug as #8941 where the controlled gates are ending up with the same names and that's causing issues.

[jakelishman]

I'm suspicious that there's a mistake in the recursive handling, when there's multiple custom instructions that all contain other custom instructions. I'm fairly confident the issue happens during the QPY dump, not the read. I instrumented write_circuit with a print(circuit.name), and it shows an asymmetry between how it handles the two gates - it shows that it touches qc.data[1].operation._definition and qc.data[1].operation.base_gate.definition, but only one of those two things for qc.data[0].

[NicoRenaud]

Hi all, thanks for looking into this issue !

The workaround proposed above (with inplace=False) will create two empty circuits though right ?

From what I could understand, when qpy dumps the base circuit (here: https://github.com/Qiskit/qiskit-terra/blob/16f6adb310719619f5cc07d314a95f12d6ea07c4/qiskit/qpy/binary_io/circuits.py#L649) something might be going wrong with the format. So when reading the .qpy file, the reader manages to read the first base circuit but fails at the second.

Thanks again !

[mtreinish]

The key to the workaround I suggested in #9746 (comment) is that it's using the append() method instead of compose() and that is always done in place. So taking the OP code it returns a circuit that looks like:

     ┌───────────────┐┌───────────────┐
q_0: ┤0              ├┤0              ├
     │               ││               │
q_1: ┤1 c_circuit-88 ├┤1 c_circuit-91 ├
     │               ││               │
q_2: ┤2              ├┤2              ├
     └───────────────┘└───────────────┘

which decomposed looks like:

q_0: ───────■──────────────■───────
     ┌──────┴──────┐┌──────┴──────┐
q_1: ┤0            ├┤0            ├
     │  circuit-88 ││  circuit-91 │
q_2: ┤1            ├┤1            ├
     └─────────────┘└─────────────┘

and another layer deeper is:

q_0: ──■───■──────────■───■─────────■───■──────────■───■──────────■───■───────»
     ┌─┴─┐ │P(-π/4)   │   │         │   │        ┌─┴─┐ │P(-π/4)   │   │       »
q_1: ┤ X ├─■──────────■───┼─────────■───┼────────┤ X ├─■──────────■───┼───────»
     └─┬─┘          ┌─┴─┐ │P(π/4) ┌─┴─┐ │P(-π/4) └─┬─┘          ┌─┴─┐ │P(π/4) »
q_2: ──■────────────┤ X ├─■───────┤ X ├─■──────────■────────────┤ X ├─■───────»
                    └───┘         └───┘                         └───┘         »
«                    
«q_0: ──■───■────────
«       │   │        
«q_1: ──■───┼────────
«     ┌─┴─┐ │P(-π/4) 
«q_2: ┤ X ├─■────────
«     └───┘          

[jakelishman]

The workaround uses QuantumCircuit.append not QuantumCircuit.compose - append is in place as well. The difference is that append adds things as a single instruction, whereas compose "inlines" the given circuit into the existing one. It so happens that QuantumCircuit.control does some pretty weird stuff internally, so you probably wouldn't spot the difference in this case, but for most things, it's important for abstract optimisations in the transpiler (overuse of compose is akin to prematurely inlining functions in a classical language).

I don't believe there's anything wrong with the QPY format itself, but I am suspicious that the recursive handling of custom gates (of which this is an example) might be skipping one of the necessary circuits when there's several compound custom gates in succession.

[NicoRenaud]

Oh I didn't notice the compose to append switch sorry ! Indeed that's a good fix !

[jakelishman]

Interestingly, as of 0.25.0, QPY now throws an error during the re-load, which might suggest where the underlying bug was:

---------------------------------------------------------------------------
error                                     Traceback (most recent call last)
<ipython-input-1-2524f28d100e> in <module>
     15     dump(qc, fd)
     16     fd.seek(0)
---> 17     new_qc = load(fd)[0]
     18

~/code/qiskit/terra/qiskit/qpy/interface.py in load(file_obj, metadata_deserializer)
    267     for _ in range(data.num_programs):
    268         programs.append(
--> 269             loader(
    270                 file_obj,
    271                 data.qpy_version,

~/code/qiskit/terra/qiskit/qpy/binary_io/circuits.py in read_circuit(file_obj, version, metadata_deserializer)
   1087     custom_operations = _read_custom_operations(file_obj, version, vectors)
   1088     for _instruction in range(num_instructions):
-> 1089         _read_instruction(file_obj, circ, out_registers, custom_operations, version, vectors)
   1090
   1091     # Read calibrations

~/code/qiskit/terra/qiskit/qpy/binary_io/circuits.py in _read_instruction(file_obj, circuit, registers, custom_operations, version, vectors)
    176     else:
    177         instruction = formats.CIRCUIT_INSTRUCTION_V2._make(
--> 178             struct.unpack(
    179                 formats.CIRCUIT_INSTRUCTION_V2_PACK,
    180                 file_obj.read(formats.CIRCUIT_INSTRUCTION_V2_SIZE),

error: unpack requires a buffer of 33 bytes

(or alternatively I broke something with #10392)

[jakelishman]

Oh no wait, sorry, I totally forgot what the top comment says - the error's the exact same. That's what I get for trying to come back to an issue after a couple of months and not taking the time to re-read everything properly.

[jakelishman]

I've got it: it's a bug in outputting the custom operations for a circuit that contains more than one; the breadth-first search through the new operations wasn't actually going breadth-first, it was only going through the new custom operations that appear from the last gate in the outer circuit because it was overwriting gates from before.

It's not actually anything to do with compose or control, it's solely about having multiple custom definitions that contain custom definitions of their own. I'll provide the fix a little later today. edit: actually, it so shakes out that there's no real problems unless the gate is controlled - it is something to do with handling of ControlledGate.