Use singletons for standard library unparameterized, non-controlled gates #10314
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ates 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.
mtreinish
added
priority: high
performance
Changelog: New Feature
|
One or more of the the following people are requested to review this:
|
|
If the giant commit message isn't clear enough, I'm not entirely convinced we should do all of this in 0.25.0. I opened up this PR primarily to show what the end state looks like and also to start a discussion around exactly how we roll this out to users in a controlled manner. Specifically what of this we want to include in 0.25.0 and how we plan to roll the rest of it out in subsequent releases? |
|
|
|
I ran a subset of the asv benchmarks quickly to get a feeling for the performance in some of our benchmarks. The benefits are more drastic for the more repeated instances of singleton gates we have to create, especially when there are copies. (the ~6x number I came up with in the commit message was for a 100 qubit circuit with a depth of 100 composed solely of singleton gates involving multiple copies). |
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.
Pull Request Test Coverage Report for Build 5751485044
💛 - Coveralls |
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.
|
If the singletons are meant to be immutable, doesn't it make sense to define In [1]: from qiskit.circuit.library import XGate
In [2]: x1 = XGate._instance
In [3]: x2 = XGate()
In [4]: id(x1) == id(x2)
Out[4]: True
In [5]: x1.a = 3
In [6]: x2.a
Out[6]: 3
In [7]: x3 = XGate()
In [8]: x3.a
Out[8]: 3I did some quick performance tests. |
Yeah we need to. We should add that to this PR before merging. The only open question is whether to do it via |
👍 I thought about trying to modify what you have here to use The |
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.
|
I added |
|
@mtreinish, to clarify: if I now write |
jakelishman
approved these changes
Sep 19, 2023
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Sep 20, 2023
This commit fixes an oversight in Qiskit#10314 where the handling of pickle wasn't done correctly. Because the SingletonGate class defines __new__ and based on the parameters to the gate.__class__() call determines whether we get a new mutable copy or a shared singleton immutable instance we need special handling in pickle. By default pickle will call __new__() without any arguments and then rely on __setstate__ to update the state in the new object. This works fine if the original instance was a singleton but in the case of mutable copies this will create a singleton object instead of a mutable copy. To fix this a __reduce__ method is added to ensure arguments get passed to __new__ forcing a mutable object to be created in deserialization. Then a __setstate__ method is defined to correctly update the mutable object post creation.
kevinhartman
added a commit
to kevinhartman/qiskit
that referenced
this pull request
Sep 25, 2023
…rolled gates (Qiskit#10314)" This reverts commit e62c86b.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 28, 2023
* Fix pickle handling for SingletonGate class This commit fixes an oversight in #10314 where the handling of pickle wasn't done correctly. Because the SingletonGate class defines __new__ and based on the parameters to the gate.__class__() call determines whether we get a new mutable copy or a shared singleton immutable instance we need special handling in pickle. By default pickle will call __new__() without any arguments and then rely on __setstate__ to update the state in the new object. This works fine if the original instance was a singleton but in the case of mutable copies this will create a singleton object instead of a mutable copy. To fix this a __reduce__ method is added to ensure arguments get passed to __new__ forcing a mutable object to be created in deserialization. Then a __setstate__ method is defined to correctly update the mutable object post creation. * Use __getnewargs_ex__ insetad of __reduce__ & __setstate__ This commit pivots the pickle interface methods used to implement __getnewargs_ex__ instead of the combination of __reduce__ and __setstate__. Realistically, all we need to do here is pass that we have mutable arguments to new to trigger it to create a separate object, the rest of pickle was working correctly. This makes the interface being used for pickle a lot clearer. * Improve assertion in immutable pickle test
|
@mtreinish Would it be possible to also make the Barrier instruction a singleton? It is not completely without parameters (the number of qubits), so perhaps we need multiple singletons. And similarly for the Clifford operations |
|
This PR is an early part of a complete effort to move most parameter-like state from the Barrier being variadic will be a bit weird, but likely we will be able to cache its different sizes, while the standard-library Clifford gates (but not the higher-level |
|
@eendebakpt That's a fair point I opened an issue to track that here: #10953 |
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Oct 6, 2023
This commit is a small optimization inside the Optimize1qGatesDecomposition pass. The last stage of the pass is taking a circuit sequence and using it to construct an equivalent 1q dag. To do this the pass iterates over the returned circuit sequence from the 1q synthesis routine and looks up the gate name in a mapping to gate classes, and creates a new object of that class with any angles provided. However, for XGate and SXGate there are no angles, and since Qiskit#10314 merged there is extra overhead with the repeated construction of these gate classes (see Qiskit#10867 for more details). Since these gates are now singletons since Qiskit#10314 it is safe to just reuse the same instance because calling XGate() will return that instance anyway. This commit updates the DAGCircuit construction to just reuse the same instance if the gate in circuit sequence is for x or sx.
rupeshknn
pushed a commit
to rupeshknn/qiskit
that referenced
this pull request
Oct 9, 2023
* Fix pickle handling for SingletonGate class This commit fixes an oversight in Qiskit#10314 where the handling of pickle wasn't done correctly. Because the SingletonGate class defines __new__ and based on the parameters to the gate.__class__() call determines whether we get a new mutable copy or a shared singleton immutable instance we need special handling in pickle. By default pickle will call __new__() without any arguments and then rely on __setstate__ to update the state in the new object. This works fine if the original instance was a singleton but in the case of mutable copies this will create a singleton object instead of a mutable copy. To fix this a __reduce__ method is added to ensure arguments get passed to __new__ forcing a mutable object to be created in deserialization. Then a __setstate__ method is defined to correctly update the mutable object post creation. * Use __getnewargs_ex__ insetad of __reduce__ & __setstate__ This commit pivots the pickle interface methods used to implement __getnewargs_ex__ instead of the combination of __reduce__ and __setstate__. Realistically, all we need to do here is pass that we have mutable arguments to new to trigger it to create a separate object, the rest of pickle was working correctly. This makes the interface being used for pickle a lot clearer. * Improve assertion in immutable pickle test
This was referenced Oct 13, 2023
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
May 1, 2024
The previous definition of a swap gate using ECR rz and sx was incorrect and also not as efficient as possible. This was missed because the tests were accidently broken since Qiskit#10314 which was fixed in the previous commit. This commit updates the definition to use one that is actually correct and also more efficient with fewer 1 qubit gates. Co-authored-by: Alexander Ivrii <alexi@il.ibm.com>
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 2, 2024
* Add equivalence library entry for swap to ECR or CZ This commit adds two new equivalence library entries to cover the conversion from a SWAP gate to either ecr or cz directly. These are common 2q basis gates and without these entries in the equivalence library the path found from a lookup ends up with a much less efficient translation. This commit adds the two new entries so that the BasisTranslator will use a more efficient decomposition from the start when targeting these basis. This will hopefully result in less work for the optimization stage as the output will already be optimal and not require simplification. Testing for this PR is handled automatically by the built-in testing harness in test_gate_definitions.py that evaluates all the entries in the standard equivalence library for unitary equivalence. * Add name to annotated gate circuit in qpy backwards compat tests * Fix equivalence library tests As fallout from the addition of SingletonGate and SingletonControlledGate we were accidentally not running large portions of the unit tests which validate the default session equivalence library. This test dynamically runs based on all members of the standard gate library by looking at all defined subclasses of Gate and ControlledGate. But with the introduction of SingletonGate and SingletonControlledGate all the unparameterized gates in the library were not being run through the tests. This commit fixes this to catch that the swap definition added in the previous commit on this PR branch used an incorrect definition of SwapGate using ECRGate. The definition will be fixed in a follow up PR. * Use a more efficient and actually correct circuit for ECR target The previous definition of a swap gate using ECR rz and sx was incorrect and also not as efficient as possible. This was missed because the tests were accidently broken since #10314 which was fixed in the previous commit. This commit updates the definition to use one that is actually correct and also more efficient with fewer 1 qubit gates. Co-authored-by: Alexander Ivrii <alexi@il.ibm.com> * Update ECR circuit diagram in comment * Simplify cz equivalent circuit * Simplify cz circuit even more Co-authored-by: Shelly Garion <46566946+ShellyGarion@users.noreply.github.com> Co-authored-by: Alexander Ivrii <alexi@il.ibm.com> --------- Co-authored-by: Alexander Ivrii <alexi@il.ibm.com> Co-authored-by: Shelly Garion <46566946+ShellyGarion@users.noreply.github.com>
ElePT
pushed a commit
to ElePT/qiskit
that referenced
this pull request
May 31, 2024
* Add equivalence library entry for swap to ECR or CZ This commit adds two new equivalence library entries to cover the conversion from a SWAP gate to either ecr or cz directly. These are common 2q basis gates and without these entries in the equivalence library the path found from a lookup ends up with a much less efficient translation. This commit adds the two new entries so that the BasisTranslator will use a more efficient decomposition from the start when targeting these basis. This will hopefully result in less work for the optimization stage as the output will already be optimal and not require simplification. Testing for this PR is handled automatically by the built-in testing harness in test_gate_definitions.py that evaluates all the entries in the standard equivalence library for unitary equivalence. * Add name to annotated gate circuit in qpy backwards compat tests * Fix equivalence library tests As fallout from the addition of SingletonGate and SingletonControlledGate we were accidentally not running large portions of the unit tests which validate the default session equivalence library. This test dynamically runs based on all members of the standard gate library by looking at all defined subclasses of Gate and ControlledGate. But with the introduction of SingletonGate and SingletonControlledGate all the unparameterized gates in the library were not being run through the tests. This commit fixes this to catch that the swap definition added in the previous commit on this PR branch used an incorrect definition of SwapGate using ECRGate. The definition will be fixed in a follow up PR. * Use a more efficient and actually correct circuit for ECR target The previous definition of a swap gate using ECR rz and sx was incorrect and also not as efficient as possible. This was missed because the tests were accidently broken since Qiskit#10314 which was fixed in the previous commit. This commit updates the definition to use one that is actually correct and also more efficient with fewer 1 qubit gates. Co-authored-by: Alexander Ivrii <alexi@il.ibm.com> * Update ECR circuit diagram in comment * Simplify cz equivalent circuit * Simplify cz circuit even more Co-authored-by: Shelly Garion <46566946+ShellyGarion@users.noreply.github.com> Co-authored-by: Alexander Ivrii <alexi@il.ibm.com> --------- Co-authored-by: Alexander Ivrii <alexi@il.ibm.com> Co-authored-by: Shelly Garion <46566946+ShellyGarion@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
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 and copying 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.
Details and comments
Related to: #5895 and #6991
Fixes: #3800