Fix QPY serialisation of ControlledGate with open controls #8571

jakelishman · 2022-08-17T17:48:10Z

Summary

Previously, an incorrect definition and name would be re-instated on
QPY deserialisation of a ControlledGate instance with open controls.
The base name would include the dynamic _o{ctrl_state} suffix, causing
the suffix to later be duplicated, and the definition would duplicate
the logic that added the open controls. This fixes both by stripping
the suffix on re-read before it is assigned, and serialising only the
"11...1" state definition, since this is what is required and stored by
ControlledGate.

Details and comments

Fix #8549.

This is technically a breaking change in the behaviour of QPY (it changes how something is serialised and deserialised), but it is not a format change. A file created with the previous serialisation behaviour will still load with this new logic, and an older Qiskit will be able to deserialise a circuit dumped with this logic, but in both cases, a ControlledGate with open controls will not be restored to the expected name/definition. There will be no exception, it'll just produce the wrong result, as it did before this patch.

Previously, an incorrect definition and name would be re-instated on QPY deserialisation of a `ControlledGate` instance with open controls. The base name would include the dynamic `_o{ctrl_state}` suffix, causing the suffix to later be duplicated, and the definition would duplicate the logic that added the open controls. This fixes both by stripping the suffix on re-read before it is assigned, and serialising only the "11...1" state definition, since this is what is required and stored by `ControlledGate`.

qiskit-bot · 2022-08-17T17:48:12Z

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

@Qiskit/terra-core
@mtreinish
@nkanazawa1989

coveralls · 2022-08-17T18:21:33Z

Pull Request Test Coverage Report for Build 2882009479

8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
5 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.003%) to 84.048%

Files with Coverage Reduction	New Missed Lines	%
qiskit/extensions/quantum_initializer/squ.py	2	79.78%
qiskit/pulse/library/waveform.py	3	89.36%

Totals
Change from base Build 2877911364:	-0.003%
Covered Lines:	56313
Relevant Lines:	67001

💛 - Coveralls

mtreinish

LGTM, do you want to add a backwards compat test for this case too? I'm thinking it might be a good idea since the upgrade path on this seems a bit nuanced so we probably don't want it to regress moving forward.

jakelishman

Yeah, the backwards compatibility test sounds good - added in this commit. There's already a test of ctrl_state=0, but unfortunately it's on a CX gate, which doesn't have the same issues because it's a known gate.

jakelishman · 2022-08-18T01:26:26Z

test/qpy_compat/test_qpy.py

@@ -525,6 +549,8 @@ def generate_circuits(version_str=None):
        output_circuits["controlled_gates.qpy"] = generate_controlled_gates()
        output_circuits["schedule_blocks.qpy"] = generate_schedule_blocks()
        output_circuits["pulse_gates.qpy"] = generate_calibrated_circuits()
+    if version_parts >= (0, 21, 2):


I've made this 0.21.2 assuming we might throw this in the backports, but if we're leaving it for 0.22.0, feel free to just modify this line.

Yeah, that's fine I probably would have just done > (0.21.1) to cover either case but this works fine as I was intending to tag this as stable backport potential

mtreinish

LGTM, thanks for adding the backwards compat test

* Fix QPY serialisation of ControlledGate with open controls Previously, an incorrect definition and name would be re-instated on QPY deserialisation of a `ControlledGate` instance with open controls. The base name would include the dynamic `_o{ctrl_state}` suffix, causing the suffix to later be duplicated, and the definition would duplicate the logic that added the open controls. This fixes both by stripping the suffix on re-read before it is assigned, and serialising only the "11...1" state definition, since this is what is required and stored by `ControlledGate`. * Add QPY backwards compatibility test Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit a0964c1)

…8578) * Fix QPY serialisation of ControlledGate with open controls Previously, an incorrect definition and name would be re-instated on QPY deserialisation of a `ControlledGate` instance with open controls. The base name would include the dynamic `_o{ctrl_state}` suffix, causing the suffix to later be duplicated, and the definition would duplicate the logic that added the open controls. This fixes both by stripping the suffix on re-read before it is assigned, and serialising only the "11...1" state definition, since this is what is required and stored by `ControlledGate`. * Add QPY backwards compatibility test Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit a0964c1) Co-authored-by: Jake Lishman <jake.lishman@ibm.com>

This commit fixes a bug in the QPY serialization of ControlledGate subclasses that defined custom _define() methods. The _define() method is the typical way to provide a custom definition in Gate classes. While ControlledGate class provides an alternative interface that handles custom control states at initialization, but the _define() interface is still valid even if it doesn't account for this. In Qiskit#8571 we updated the QPY serialization code to use the _definition() method directly to correctly handle the open control case. But this fix neglected the case where people were providing definitions via the mechanism from Gate. This commit fixes this assumption by ensuring we load the definition that comes from _define() which will fix the serialization of these custom subclasses. Fixes Qiskit#8794

This commit fixes a bug in the QPY serialization of ControlledGate subclasses that defined custom _define() methods. The _define() method is the typical way to provide a custom definition in Gate classes. While ControlledGate class provides an alternative interface that handles custom control states at initialization, but the _define() interface is still valid even if it doesn't account for this. In #8571 we updated the QPY serialization code to use the _definition() method directly to correctly handle the open control case. But this fix neglected the case where people were providing definitions via the mechanism from Gate. This commit fixes this assumption by ensuring we load the definition that comes from _define() which will fix the serialization of these custom subclasses. Fixes #8794 Co-authored-by: Jake Lishman <jake.lishman@ibm.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

This commit fixes a bug in the QPY serialization of ControlledGate subclasses that defined custom _define() methods. The _define() method is the typical way to provide a custom definition in Gate classes. While ControlledGate class provides an alternative interface that handles custom control states at initialization, but the _define() interface is still valid even if it doesn't account for this. In #8571 we updated the QPY serialization code to use the _definition() method directly to correctly handle the open control case. But this fix neglected the case where people were providing definitions via the mechanism from Gate. This commit fixes this assumption by ensuring we load the definition that comes from _define() which will fix the serialization of these custom subclasses. Fixes #8794 Co-authored-by: Jake Lishman <jake.lishman@ibm.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 3fb8939)

This commit fixes a bug in the QPY serialization of ControlledGate subclasses that defined custom _define() methods. The _define() method is the typical way to provide a custom definition in Gate classes. While ControlledGate class provides an alternative interface that handles custom control states at initialization, but the _define() interface is still valid even if it doesn't account for this. In Qiskit#8571 we updated the QPY serialization code to use the _definition() method directly to correctly handle the open control case. But this fix neglected the case where people were providing definitions via the mechanism from Gate. This commit fixes this assumption by ensuring we load the definition that comes from _define() which will fix the serialization of these custom subclasses. Fixes Qiskit#8794 Co-authored-by: Jake Lishman <jake.lishman@ibm.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

This commit fixes a bug in the QPY serialization of ControlledGate subclasses that defined custom _define() methods. The _define() method is the typical way to provide a custom definition in Gate classes. While ControlledGate class provides an alternative interface that handles custom control states at initialization, but the _define() interface is still valid even if it doesn't account for this. In #8571 we updated the QPY serialization code to use the _definition() method directly to correctly handle the open control case. But this fix neglected the case where people were providing definitions via the mechanism from Gate. This commit fixes this assumption by ensuring we load the definition that comes from _define() which will fix the serialization of these custom subclasses. Fixes #8794 Co-authored-by: Jake Lishman <jake.lishman@ibm.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 3fb8939) Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

jakelishman added Changelog: Bugfix Include in the "Fixed" section of the changelog mod: qpy Related to QPY serialization labels Aug 17, 2022

jakelishman requested review from a team and nkanazawa1989 as code owners August 17, 2022 17:48

jakelishman mentioned this pull request Aug 17, 2022

qpy changes names of ControlledGates with non-default ctrl_state #8549

Closed

mtreinish reviewed Aug 17, 2022

View reviewed changes

Add QPY backwards compatibility test

53d80f0

jakelishman commented Aug 18, 2022

View reviewed changes

mtreinish added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Aug 18, 2022

mtreinish approved these changes Aug 18, 2022

View reviewed changes

mtreinish added the automerge label Aug 18, 2022

Merge branch 'main' into fix-qpy-controlledgate-open-ctrl

211bf54

mergify bot merged commit a0964c1 into Qiskit:main Aug 18, 2022

mergify bot mentioned this pull request Aug 18, 2022

Fix QPY serialisation of ControlledGate with open controls (backport #8571) #8578

Merged

jakelishman deleted the fix-qpy-controlledgate-open-ctrl branch August 19, 2022 12:37

kt474 mentioned this pull request Sep 22, 2022

Qpy 0.21.2 updates Qiskit/qiskit-ibm-runtime#541

Merged

mtreinish mentioned this pull request Oct 17, 2022

'NoneType' object has no attribute 'metadata' when trying to serialize custom gate on qiskit-terra==0.21.2 when it used to work on qiskit-terra==0.21.1 #8794

Open

mtreinish mentioned this pull request Oct 17, 2022

Fix qpy custom ControlledGate with overloaded _define() #8927

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix QPY serialisation of ControlledGate with open controls #8571

Fix QPY serialisation of ControlledGate with open controls #8571

jakelishman commented Aug 17, 2022

qiskit-bot commented Aug 17, 2022

coveralls commented Aug 17, 2022 •

edited

Loading

mtreinish left a comment

jakelishman left a comment

jakelishman Aug 18, 2022

mtreinish Aug 18, 2022

mtreinish left a comment

Fix QPY serialisation of ControlledGate with open controls #8571

Fix QPY serialisation of ControlledGate with open controls #8571

Conversation

jakelishman commented Aug 17, 2022

Summary

Details and comments

qiskit-bot commented Aug 17, 2022

coveralls commented Aug 17, 2022 • edited Loading

Pull Request Test Coverage Report for Build 2882009479

💛 - Coveralls

mtreinish left a comment

Choose a reason for hiding this comment

jakelishman left a comment

Choose a reason for hiding this comment

jakelishman Aug 18, 2022

Choose a reason for hiding this comment

mtreinish Aug 18, 2022

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

coveralls commented Aug 17, 2022 •

edited

Loading