Add singleton lookup-key logic

[jakelishman]

Summary

This adds overridable logic for singleton classes to define a "key" function, which takes the same arguments as its __init__ and returns a lookup key that is used as a comparison for returning singletons. This lets the standard singleton gates easily return the singleton instance even when label is explicitly set to the default, such as happens using the idiomatic QuantumCircuit methods like x.

This keying logic has a simple logical extension to allow more than one singleton to exist for a single base class. This functionality is not yet used by the Qiskit standard library, but can easily be used to make both closed- and open-controlled versions of gates singletons, or to allow specific hard-coded parameters for (e.g.) RZGate singletons.

The decision on which gates to extend this to can be done in follow-up commits later.

Details and comments

The most notable change here is that idiomatic circuit construction now produces singleton instances. For example, QuantumCircuit.x(0) previously would produce a mutable XGate because internally it constructed it as XGate(label=None) due to a default argument to label in the x method. This commit makes explicit defaults correctly resolve to the same singleton instance. As a side effect, this causes the base_gate in controlled gate subclasses to become a singleton if (as default) the _base_label is not overridden.

The key machinery logically immediately allows multiple singletons to exist for any given class, as they can now be simply distinguished. I didn't implement that in this commit for any of the standard-library operations, but in principle this allows SingletonControlledGate subclasses to make both open- and closed-control states singletons, and lays the groundwork for parametric gates like RZGate to have special parameter values that are singletons (though these will need an extension to the standard-library gate key function).

Using an extension to the timing script from #11014:

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

immutable = QuantumCircuit(127)
for q in immutable.qubits:
    for _ in [None]*1_000:
        immutable.append(XGate(), [q], [])
for (q1, q2) in zip(immutable.qubits[:-1], immutable.qubits[1:]):
    for _ in [None]*100:
         immutable.append(CXGate(), [q1, q2], [])

idiomatic = QuantumCircuit(127)
for q in idiomatic.qubits:
    for _ in [None]*1_000:
        idiomatic.x(q)
for (q1, q2) in zip(idiomatic.qubits[:-1], idiomatic.qubits[1:]):
    for _ in [None]*100:
         idiomatic.cx(q1, q2)


%timeit XGate()
%timeit XGate(label="x")
%timeit CXGate()
%timeit CXGate(ctrl_state=1)
%timeit CXGate(label="x")
%timeit immutable.copy()
%timeit idiomatic.copy()

Note that before this commit, idiomatic contained only mutable gate objects. The new timings compared to main at the parent b18faa3 are:

	main	PR
X() /µs	0.121(1)	0.122(1)
X(label) /µs	1.89(1)	2.10(4)
CX() /µs	0.120(1)	0.123(5)
CX(ctrl) /µs	9.4(1)	0.895(6)
CX(label) /µs	9.3(1)	5.55(6)
copy /ms	111(1)	113(1)
copy idiom /ms	504(10)	115(1)

To explain timing differences:

• X(label="x") is ~10% slower because of the additional indirection through the key lookup.
• CX(ctrl_state=1) is faster now because that ctrl_state is a default value. It's not quite as fast as CX() because the zero-arguments case gets a fast path so that the construction idioms for high-performance inner loops are as fast as possible (no key lookup).
• CX(label) is faster despite not returning a default because the .base_gate object of the CXGate now does get generated as a singleton, whereas before it didn't even though it was eligible to be because it constructs XGate(label=_base_label) where _base_label defaults to None.
• The "idiomatic" circuit copy time now matches the "fast-path" copy time because the idiomatic one now creates singleton instances.

[qiskit-bot]

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

• @Cryoris
• @Qiskit/terra-core
• @ajavadia

[jakelishman]

This test is removed because the base_gate now gets (correctly) created as a singleton instance, so in that case the deepcopy should keep the objects referentially identical. It's not a formal requirement for that base to be immutable, though, so I didn't flip the polarity of the test.

[mtreinish]

Overall I'm happy with this approach, it fixes the performance (both in runtime and memory) gaps that we still had around singletons usage. It builds on the SingletonControlledGate class to add shared instances of custom control state which I had abandoned in the original subclassing route for singletons because it wasn't critical and was difficult to manage. I'm a bit nervous about this change for rc1, but I think the aggregate performance wins might be worth it at this point, especially given our test coverage for this is pretty high because it effects most gates in the standard library.

[mtreinish]

I think generally this is the safer approach here, but I'm wondering where it's coming from. Are we doing follow up mutation on node.op somewhere, I'm just trying to grasph why CI didn't flag this in the earlier singleton PRs.

[jakelishman]

It's very hard to parse, but the next line after this highlighted one is node.name = ..., and DAGOpNode.name is a property that forwards sets to DAGOpNode.op.name, which is where the mutation happens.

[mtreinish]

Ah, I forgot about the forwarding we do from DAGOpNode, this makes sense then. Although how it passed unit tests I'm not sure, because this clearly wasn't singleton safe.

[jakelishman]

It's because in practice, all previous ControlledGate instances that would end up here would have been created by some __init__ that did something along the lines of

def __init__(self, ctrl_state=None, *, _base_label=None):
    base = XGate(label=_base_label)
    super().__init__("cx", 2, 0, [], num_ctrl_qubits=1, ctrl_state=ctrl_state, base_gate=base)

Before this PR, the base would be a mutable instance, because the constructor wouldn't be able to tell that the explicit setting of label was just the default.

[mtreinish]

Is the try/except here faster than the None default on get()? If I were using the try except pattern I would have just don cls._singleton_static_lookup.get[key] instead of calling get()

[jakelishman]

This isn't about catching the None (which would raise KeyError not TypeError), it's about catching the case that the key returned was unhashable. Since the key is highly likely to contain arbitrary user input, it's not fair for key implementors to manually validate that the user input is hashable, and at best they'd be duplicating work that we have to do here with the try-and-see approach anyway.

For example:

>>> {}.get([], None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unhashable type: 'list'

And the reason we need this, is say we're defining the key for CXGate, something like:

def key(label=None, ctrl_state=None):
    return (ctrl_state,) if label is None else None

now the user does something pathological like CXGate(ctrl_state=[1, 2, 3]). If we didn't try/catch, they'd get a crazy error message that really doesn't show the problem. With this form, we gracefully build a mutable instance for them, and let CXGate.__init__ handle the type error with its normal mechanisms.

[jakelishman]

Oh, unless I misunderstood you - since we're already doing try/except (which in Python 3.10+ has I think zero or very near zero cost), we could skip the Python-space function call and catch KeyError on the None instead? I don't know the speed of that. I can do that if there's an improvement. I feel a little weird about potentially trying to lookup the None return value, even though that's explicitly "don't do a lookup" - we're then relying hard on None never getting spuriously added to the lookup, or all instances that should be mutable will suddenly get created immutable with some random state.

[mtreinish]

Yeah, I was thinking to avoid the extra call overhead of get(), but I agree it'd probably be ugly enough and such a small difference we wouldn't be able to measure it that's it not worth it.

[mtreinish]

Is there a reason to not just use tuple(instruction._params). Like either way we'll error either way on mutation, is there something doing explicit type checking for ._params to be a list?

[jakelishman]

I'll run some tests with it, but my recollection is that there's hairy bits of opflow that do weird stuff like all_params = x.params + y.params, which implicitly requires that they're both the same C sequence type.

[mtreinish]

It's fine I guess, from a strict typing perspective we define the params attribute as a list explicitly so this is correct even if it's a bit less than desirable.

[jakelishman]

For posterity: after running a couple of tests we realised that there's a much bigger reason we'd forgotten about. Well-behaved code that wants to use gate.params for its own purposes (for example creating a new gate) often does gate.params.copy() to ensure it's got an owned version, and since tuple doesn't have copy method (it doesn't make sense on immutable collections), it would have failed everything.

[mtreinish]

Do you want to add a test for CXGate(ctrl_state=0) is CXGate(ctrl_state="0")? It's covered by the discrete test cases added above, but having coverage for our stdlib usage might be good too.

[jakelishman]

That would fail currently, because I didn't set CXGate to add an additional singleton that corresponds to the ctrl_state=0. I can make that change (etc for other 1q controls) if you want it, though.

[mtreinish]

Nah, it's fine it's been a long day and I just assumed it was there because the infrastructure is being added to the PR. But, that was part of my concern with this PR scope was expanding the singletons for the controlled gate control state, so it's good that it's not in the PR.

[jakelishman]

Yeah, that's the reason I didn't do it in this PR.

[jakelishman]

I'm fine if we want to leave this out of 0.45 on the basis of it being late - it's why I didn't pre-emptively tag it for the milestone - though in practice I think if we don't do this, then quite a lot of immutability gets really left on the table.

There's also a very little tweak (create_singleton -> create_default_singleton) that would technically be a public API change if we didn't merge this til after 0.45, and if that was the case, we'd be left with the API having a slightly awkward corner where create_singleton=False doesn't necessarily imply that no singletons are created, just that the one with zero arguments isn't created.

[coveralls]

Pull Request Test Coverage Report for Build 6567210762

• 134 of 138 (97.1%) changed or added relevant lines in 14 files are covered.
• 13 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+0.007%) to 86.949%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/circuit/singleton.py	88	92	95.65%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	1	91.41%
crates/qasm2/src/parse.rs	12	96.67%

Totals
Change from base Build 6563885537:	0.007%
Covered Lines:	73729
Relevant Lines:	84796

---

💛 - Coveralls

[mtreinish]

Ok, I think I'm happy with this for 0.45 after talking through the open discussions and updating the docstrings to use the proper colonial spelling. The key reasons I think we need to make an exception and include this for 0.45 are I think we'll definitely want this in the next release even if we waiting until after 0.45 and the minor API change this introduces to building singletons would still be necessary and just harder to do in 0.45, also the performance is convincing. If it wasn't for the API changes on the singleton constructors I'd be inclined to potentially do this for 0.45.0 post rc1.

The code here is definitely sufficiently hairy to follow because it plays deep with the python data model, but there are more then enough inline comments and docstrings to hopefully keep this maintainable longer term (at least there are for me). This was excellent work navigating the interface to make this possible, I think the end state here is more sustainable long term considering the next steps we're thinking about with singletons.