Rewrite singleton handling including `SingletonInstruction` #11014

jakelishman · 2023-10-13T09:46:58Z

Summary

This is a large rewrite of the singleton gate handling, building off the work done across the library to make the initial implementation work.

There are two main purposes to this commit:

Make SingletonInstruction available in addition to SingletonGate, and have these be easy to maintain in conjunction, not need to duplicate overrides between them, and not require inheritors to do any nasty multiple inheritance or the like.
Fix regressions in the construction time of SingletonGate instances.

In the end, this turned into a fairly complete rewrite that completely switches out the method of defining the classes; it transpires that the previous way of having the "immutable overrides" on all instances of the classes was the main source of slowdown, with __setattr__ being a large problem. The previous method was far easier from an implementation perspective, but the runtime costs ended up being too high.

This new form takes a vastly different strategy, explained in detail in qiskit/circuit/singleton.py. The gist is that we instead make the singleton instance a subclass of the class object we're creating, and only it contains the overrides to make itself immutable. We get around the instantiation problem (__init__ isn't special and doesn't skip the forbidden __setattr__) by constructing an instance of the base class, and then dynamically switching in the overrides afterwards. Since the overrides and the dynamic singleton type of the instance are designed to be co-operative (including in __slots__ layout), this can be done safely.

Details and comments

Fix #10867
Close #11004
Close #10986 (obsolete after this PR - the overhead is so small it's not worth the extra logic).

Both this and the previous state of singleton gates are currently having less effect on idiomatically constructed circuits than is ideal, because methods such as QuantumCircuit.x always pass label as a kwarg, which suppresses use of the singletons. This PR does not yet attempt to change that; that would be a follow-up. In principle, it requires a system that _SingletonMeta.__call__ can use to determine if arguments are their defaults or not - this could be done explicitly, or potentially by introspection of the base __init__ method during the class creation.

Some microbenchmarks to demonstrate the performance characteristics (since our asv suite doesn't really seem to be giving useful results at the moment). The four rows are defined in this script in order:

from qiskit.circuit import QuantumCircuit
from qiskit.circuit.library import XGate

immutable = QuantumCircuit(127)
for q in immutable.qubits:
    for _ in [None]*1_000:
        immutable.append(XGate(), [q], [])

mutable = QuantumCircuit(127)
for q in mutable.qubits:
    for _ in [None]*1_000:
        mutable.x(q)


%timeit XGate()
%timeit XGate(label="x")
%timeit immutable.copy()
%timeit mutable.copy()

The three points of comparison are the 0.25.2 tag, main at this commit's parent (7a550ad) and this PR.

Version	X() /µs	X(label) /µs	copy /ms	copy mut /ms
0.25.2	1.46(9)	1.56(1)	505(4)	506(6)
main	7.23(3)	7.91(3)	128(1)	1520(11)
PR	0.116(1)	1.94(1)	115(2)	502(7)

qiskit-bot · 2023-10-13T09:47:03Z

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

@enavarro51
@Cryoris
@Qiskit/terra-core
@ajavadia
@mtreinish
@nkanazawa1989

qiskit/circuit/singleton.py

jakelishman · 2023-10-13T09:56:18Z

cc: @wshanks, since you'd found some problems with our initial implementation.

coveralls · 2023-10-13T10:25:43Z

Pull Request Test Coverage Report for Build 6511459959

133 of 139 (95.68%) changed or added relevant lines in 24 files are covered.
11 unchanged lines in 4 files lost coverage.
Overall coverage decreased (-0.02%) to 87.032%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/circuit/controlledgate.py	3	4	75.0%
qiskit/transpiler/passes/optimization/template_matching/template_substitution.py	5	6	83.33%
qiskit/circuit/singleton.py	59	63	93.65%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/expr.rs	1	93.76%
crates/qasm2/src/lex.rs	2	91.16%
qiskit/extensions/quantum_initializer/squ.py	2	84.15%
crates/qasm2/src/parse.rs	6	97.13%

Totals
Change from base Build 6508678248:	-0.02%
Covered Lines:	74332
Relevant Lines:	85408

💛 - Coveralls

This is a large rewrite of the singleton gate handling, building off the work done across the library to make the initial implementation work. There are two main purposes to this commit: * Make `SingletonInstruction` available in addition to `SingletonGate`, and have these be easy to maintain in conjunction, not need to duplicate overrides between them, and not require inheritors to do any nasty multiple inheritance or the like. * Fix regressions in the construction time of `SingletonGate` instances. In the end, this turned into a fairly complete rewrite that completely switches out the method of defining the classes; it transpires that the previous way of having the "immutable overrides" on _all_ instances of the classes was the main source of slowdown, with `__setattr__` being a large problem. The previous method was far easier from an implementation perspective, but the runtime costs ended up being too high. This new form takes a vastly different strategy, explained in detail in `qiskit/circuit/singleton.py`. The gist is that we instead make the singleton instance a _subclass_ of the class object we're creating, and only it contains the overrides to make itself immutable. We get around the instantiation problem (`__init__` isn't special and doesn't skip the forbidden `__setattr__`) by constructing an instance of the _base_ class, and then dynamically switching in the overrides afterwards. Since the overrides and the dynamic singleton type of the instance are designed to be co-operative (including in `__slots__` layout), this can be done safely.

jakelishman · 2023-10-13T10:37:06Z

QPY test failures are fixed by the unrelated #11015.

mtreinish

Overall this looks great to me. It's a very clever solution (although I'm mildly concerned it's too clever and it'll be confusing to maintain in a couple years time, but the very detailed comments you left inline should help greatly with that) to solve the problems we had with the original SingletonGate implementation and it should make it simpler for us to expand the singleton pattern for other circuit object types in the future like: #10898 but also any other follow-ons. I just had a few comments inline mostly around documentation and comments.

qiskit/circuit/instruction.py

mtreinish · 2023-10-13T11:45:47Z

qiskit/circuit/library/standard_gates/dcx.py

@@ -48,14 +48,9 @@ class DCXGate(SingletonGate):
            \end{pmatrix}
    """

-    def __init__(self, label=None, duration=None, unit=None, _condition=None):
+    def __init__(self, label=None, *, duration=None, unit="dt"):


I agree making these keyword only makes sense, we should have done this in #10314. It'd be good to leave a comment on #11013 for @hunterkemeny to adopt this signature in that PR too.

Oh, this actually was a bit of a mistake that I changed what you originally had - I was just writing it on autopilot, mechanically and manually undoing the mistake I made where I'd removed all the label and duration arguments. But yeah, since we've got the opportunity, let's just do it.

releasenotes/notes/singletons-83782de8bd062cbc.yaml

qiskit/circuit/singleton.py

mtreinish · 2023-10-13T12:20:10Z

qiskit/circuit/singleton.py

+    __slots__ = ()
+
+    def c_if(self, classical, val):
+        return self.to_mutable().c_if(classical, val)


Oh, this is much neater and would have worked fine for the old class structure if we did out = self.to_mutable();out.condition = (...). I really should have done a pass to clean up all the overrides after adding the to_mutable() method.

Yeah - I think there's an instance in your ControlledGate PR where it could do a similar thing as well (or maybe it's a case that does obj = obj.to_mutable(); obj.condition = ... instead of obj = obj.c_if(...) - I forgot).

qiskit/circuit/singleton.py

wshanks · 2023-10-13T16:35:14Z

Very cool. I have never seen metaclasses used in practice before 🙂

I am curious about the benchmarks. Was SingletonGate primarily benchmarked for memory when it was proposed? I am surprised the 4x overhead was not noticed before. Maybe it was masked by the 4x speed up in copying?

jakelishman · 2023-10-13T16:42:55Z

It was primarily benchmarked for memory not speed, but also when you're doing QuantumCircuit.append, there's a bunch of overhead related to checking the circuit invariants, resolving qubit specifiers, etc, which is longer than the construction time of the objects themselves.

Matt did notice that QuantumCircuit.copy performance wasn't as good as we hoped, but there were a few factors that masked us from seeing the full effects:

the considered circuits had a mix of singletons and non-singletons, and copy is improved for singletons even on main, but that's offset by the slowdown in copies of the mutable gates
idiomatic circuit construction like QuantumCircuit.x doesn't actually create singletons on main or in this PR (but I'm preparing another PR with a bit more machinery that will make that work as expected)
we were prepared to accept some cost in circuit construction in order to get the memory savings, and we skewed too lenient because the memory savings are what we expected, and that's a big deal for us.

jakelishman · 2023-10-13T16:44:17Z

Also, our speed benchmarks in asv really don't seem to be turning up much - we need to devote a fair amount of time to looking at those and working out how we can make them more useful. At the very least, they're very out-of-date and don't even necessarily have idiomatic Terra usage.

mtreinish

This LGTM, thanks for the updates. I think this is ready to merge I just had two tiny inline nits with suggestions on the docs (but I worry one will fail lint, so we'll see).

qiskit/circuit/singleton.py

Co-authored-by: Matthew Treinish <[email protected]>

mtreinish

LGTM, thanks for the quick update. I think this is ready to go now.

jakelishman added performance Changelog: New Feature mod: circuit labels Oct 13, 2023

jakelishman added this to the 0.45.0 milestone Oct 13, 2023

jakelishman requested review from nonhermitian, a team, alexanderivrii and ShellyGarion as code owners October 13, 2023 09:46

jakelishman commented Oct 13, 2023

View reviewed changes

qiskit/circuit/singleton.py Show resolved Hide resolved

jakelishman assigned kevinhartman and mtreinish Oct 13, 2023

jakelishman force-pushed the singleton-instruction branch from 5a7c1d2 to 4cf52f2 Compare October 13, 2023 10:29

jakelishman added the priority: high label Oct 13, 2023

mtreinish reviewed Oct 13, 2023

View reviewed changes

jakelishman added 3 commits October 13, 2023 17:32

Make singleton documentation public

145decf

Add tests of new singleton behaviour

1274fe7

Merge remote-tracking branch 'ibm/main' into singleton-instruction

e1cde2f

jakelishman requested a review from mtreinish October 13, 2023 16:34

jakelishman mentioned this pull request Oct 13, 2023

Add duration and unit kwargs to __init__ for all standard gates #11013

Merged

mtreinish reviewed Oct 13, 2023

View reviewed changes

qiskit/circuit/singleton.py Outdated Show resolved Hide resolved

qiskit/circuit/singleton.py Outdated Show resolved Hide resolved

Fix typos

f3c6a43

Co-authored-by: Matthew Treinish <[email protected]>

mtreinish approved these changes Oct 13, 2023

View reviewed changes

mtreinish enabled auto-merge October 13, 2023 17:53

mtreinish added this pull request to the merge queue Oct 13, 2023

Merged via the queue into Qiskit:main with commit bcf5ce4 Oct 13, 2023

jakelishman deleted the singleton-instruction branch October 13, 2023 20:25

mtreinish mentioned this pull request Oct 13, 2023

Singleton parameterless controlled gates #10898

Merged

2 tasks

jakelishman mentioned this pull request Oct 18, 2023

Add singleton lookup-key logic #11040

Merged

kt474 mentioned this pull request Oct 20, 2023

Update qpy qiskit 0.45 Qiskit/qiskit-ibm-provider#736

Merged

alexanderivrii mentioned this pull request Jan 9, 2024

Fix qpy support for Cliffords #11495

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite singleton handling including `SingletonInstruction` #11014

Rewrite singleton handling including `SingletonInstruction` #11014

jakelishman commented Oct 13, 2023

qiskit-bot commented Oct 13, 2023

jakelishman commented Oct 13, 2023

coveralls commented Oct 13, 2023 •

edited

Loading

jakelishman commented Oct 13, 2023

mtreinish left a comment

mtreinish Oct 13, 2023

jakelishman Oct 13, 2023

mtreinish Oct 13, 2023

jakelishman Oct 13, 2023

wshanks commented Oct 13, 2023

jakelishman commented Oct 13, 2023

jakelishman commented Oct 13, 2023

mtreinish left a comment

mtreinish left a comment

Rewrite singleton handling including SingletonInstruction #11014

Rewrite singleton handling including SingletonInstruction #11014

Conversation

jakelishman commented Oct 13, 2023

Summary

Details and comments

qiskit-bot commented Oct 13, 2023

jakelishman commented Oct 13, 2023

coveralls commented Oct 13, 2023 • edited Loading

Pull Request Test Coverage Report for Build 6511459959

💛 - Coveralls

jakelishman commented Oct 13, 2023

mtreinish left a comment

Choose a reason for hiding this comment

mtreinish Oct 13, 2023

Choose a reason for hiding this comment

jakelishman Oct 13, 2023

Choose a reason for hiding this comment

mtreinish Oct 13, 2023

Choose a reason for hiding this comment

jakelishman Oct 13, 2023

Choose a reason for hiding this comment

wshanks commented Oct 13, 2023

jakelishman commented Oct 13, 2023

jakelishman commented Oct 13, 2023

mtreinish left a comment

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

Rewrite singleton handling including `SingletonInstruction` #11014

Rewrite singleton handling including `SingletonInstruction` #11014

coveralls commented Oct 13, 2023 •

edited

Loading