Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Use singletons for standard library unparameterized, non-controlled g…
…ates (#10314) * Use singletons for standard library unparameterized, non-controlled gates This commit adds a new class SingletonGate which is a Gate subclass that reuses a single instance by default for all instances of a particular class. This greatly reduces the memory overhead and significant improves the construction speed for making multiple instances of the same gate. The tradeoff is in the flexibility of use because it precludes having any potentially mutable state in the shared instance. This is a large change to the data model of qiskit because it previously could be assumed that any gate instance was unique and there weren't any unintended side effects from modifying it in isolation (for example doing XGate().label = 'foo' wouldn't potentially break other code). To limit the impact around this instances of SingletonGate do not allow mutation of an existing instance. This can (and likely will) cause unexpected issues as usage of the class is released. Specifically what used to be valid will now raise an exception because it is a shared instance. This is evident from the code modifications necessary to most of the Qiskit code base to enable working with instances of SingletonGates. The knock on effects of this downstream are likely significant and managing how we roll this feature out is going to be equally if not more important than the feature itself. This is why I'm not personally convinced we want to do all this commit includes in a single release. I've opened this as a pull request primarily to start the conversation on how we want to do the roll out to try and minimize and notify downstream users of the potential breakage to avoid issues. The primary issue I have is this doesn't really follow the Qiskit deprecation policy as there is no user facing notification or documentation of this pending change and code that worked in the previously release will not work in the release with this feature. For some aspects of this change (primarily the setters on gate attributes) this can easily be handled by deprecating it in planned singleton standard library gates and waiting the necessary amount of time. But the more fundamental data model changes are hard to announce ahead of time. We can have a release note about it coming in the future but it will end up being very abstract and users will not necessarily be able to act on it ahead of time without concrete examples to test with. This was an issue for me in developing this feature as I couldn't anticipate where API breakages would occur until I switched over all the standard library gates, and there still might be others. Due to the large performance gains this offers and also in the interest of testing the API implications of using singleton gates the unparameterized and non-controlled gates available in qiskit.circuit.library.standard_gates are all updated to be subclasses of singleton gates. In aggregate this is causing construction to be roughly 6x faster and building circuits comprised solely of these gates consume 1/4th the memory as before. But it also exposed a large number of internal changes we needed to make to the wider circuit, QPY, qasm2, dagcircuit, transpiler, converters, and test modules to support working with singleton gates. Besides this there are a couple seemingly unrelated API changes in this PR and it is caused by inconsistencies in the Instruction/Gate API that were preventing this from working. The first which is the ECRGate class was missing a label kwarg in the parent. Similarly all Gate classes and subclasses were missing duration and unit kwargs on their constructors. These are necessary to be able to use these fields on singletons because we need an interface to construct an instance that has the state set so we avoid the use of the global shared instance. In the release notes I labeled these as bugfixes, because in all the cases the parent clases were exposing these interfaces and it primarily is an oversight that they were missing in these places. But personally this does seem more like something we'd normally document as a feature rather than a bugfix. A follow up PR will add a SingletonControlledGate class which will be similar to SingletonGate but will offer two singleton instance based on the value of ctrl_state (and also handle nested labels and other nested mutable state in the base gate). We can then update the standard library gates like CXGate, and CHGate to also be singletons. The ctrl state attribute is primarily why these gates were not included in this commit. * Fix Python 3.8 compatibility There are some differences in how the inspect stdlib module behaves between python 3.8 and newer versions of python. This was causing divergence in the test and qpy behavior where inspect was used to determine different aspects of a gate (either whether label was supported as an arg or find the number of free parameters). This commit fixes this by adjusting the code to handle both newer versions of inspect as well as older ones. * Simplify qpy deserialization label handling * Remove unused classmethod decorator * Fix lint * Fix timeline drawer on output of legacy DD pass * Fix doc build * Add methods to deal with mutability of singleton gates This commit adds two methods to the SingletonGate class, mutable and to_mutable. The mutable() method is a property method that returns whether a particular instance is mutable or a shared singleton instance. The second method to_mutable() returns a mutable copy of the gate. * Disallow custom attribute on singleton instances This commit adds a __setattr__ method to the singleton gate class to ensure that custom attributes are not settable for a shared singleton instance. It prevents addign a custom attribute if the instance is in singleton mode and will raise a NotImplementedError to avoid silently sharing state in the single shared instance. * Fix rebase error * Fix rebase issues * Fix module docstring * Add .mutable and .to_mutable() to Instruction To unify the access patterns between SingletonGates and other instructions this commit adds a common property mutable and method to_mutable() to check if any Instruction (not just singletons) are mutable or not and to get a mutable copy. For things that don't inherit from SingletonGate these are hard coded to `True` and to return a copy as by default every Instruction is mutable, only `SingletonGate` objects are different in this regard. * Unify handling of gates in scheduling passes Previously we were explicitly handling the SingletonGate class in the scheduling passes. But with the introduction of mutable and to_mutable() on Instruction we don't need to condition on gates being singleton or not and we can just handle them in the same manner as other instructions. This commit implements this change. * Remove unnecessary deepcopy in substitute_node_wtih_dag * Fix logic for SingletonGate.copy Co-authored-by: Jake Lishman <[email protected]> * Update Singleton Gate class docstring * Remove unused imports * Update release notes * Fix release note typos Co-authored-by: Jake Lishman <[email protected]> * Improve setattr performance * Fix deepcopy logic * Add check for kwargs up front * Fix docs typos * Add comment on to_mutable __init__ call * Fix lint * Handle positional initialization arguments in SingletonGate If there are any positional arguments set when initializing a new SingletonGate subclass those were not being handled correctly before. If there is a positional argument being set that would indicate at least a label is being set and indicates a mutable instance should be returned instead of the immutable singleton. This commit adds the missing check to the __new__ logic and also adds a test to verify the behavior is correct. --------- Co-authored-by: Jake Lishman <[email protected]> Co-authored-by: Jake Lishman <[email protected]>
- Loading branch information