Change data type of QuantumCircuit.data elements
#8093
Merged
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
jakelishman
requested review from
a team,
ikkoham,
t-imamichi,
nonhermitian,
nkanazawa1989,
eggerdj,
DanPuzzuoli,
jyu00,
manoelmarques and
woodsp-ibm
as code owners
|
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:
|
This introduces a new, lightweight class `CircuitInstruction` the encapsulates the previous 3-tuple that made up the items of `QuantumCircuit.data`. It includes a legacy API that makes the object appear to be the old 3-tuple when used as if it were that tuple, and the relatively small changeset for this commit is an approximate indication that these should mostly be suitable. There will be significant performance penalties for accessing the legacy interface, however, due to some internal type conversions, so it is a matter of priority to convert internal usage to the new dotted-attribute access form. The only places necessary to change across Terra were those where the data elements were compared by referential equality to expected values (such as in the control-flow builders), and places that were invalidly accessing the private attribute `QuantumCircuit._data`. All these are updated. `ParameterTable` should in the future be changed to operate over this entire context, but that was beyond the scope of the initial commit.
jakelishman
force-pushed
the
instruction-data
branch
from
94d523e to
05219d7
Compare
|
For completeness, I also ran the Shor's algorithms tests that were the main problem in #7020. Here is the slowest of those on and now with this PR: That's about a 10% regression on those, as opposed to the 10-20x regression seen before. |
jakelishman
added
priority: high
Changelog: API Change
This previous commit is the minimal set of changes needed for the entire test suite to pass, but large parts of Terra and the test suite were still using the backwards-compatibility shims when dealing with `QuantumCircuit.data`. This changes over every single usage within Terra to use the new dotted-access form, rather than the tuple indexing or iteration that previously existed. The internal types of the `qargs` and `cargs` in `DAGDepNode` and `DAGOpNode` are also changed to be `tuple` now; previously they were semi-undefined, and in some cases were not consistent, but not caught in the test suite. Since changing the type of the `qubits` parameter in `CircuitInstruction` to be tuple (unless using the backwards-compatibility shims), this caused a lot of failures where the value was being assigned directly. Normalising to `tuple` has the same benefit as it does in the circuit instruction - less copying overhead, and no worrying about multiple writable references to the same object. By far the largest set of changes are in the test suite; we do a lot of comparing things for exact equality, and since the type is now changed from `list` to `tuple`, all the tests had to be updated. This isn't expected to have much impact on user code, but changing the type to an immutable one has large benefits for us when copying these data structures.
jakelishman
force-pushed
the
instruction-data
branch
from
05219d7 to
fb0fb32
Compare
|
Given the lint failure, I'm guessing I've left a dodgy type hint in the |
Some uses of the legacy interface related to directly setting `QuantumCircuit.data` had slipped through the cracks. A bug in `astroid` caused its inference to fail, and think that these settings made the scalar type of `QuantumCircuit.data` the old 3-tuple after it saw these settings. Since there's a property setter for `data`, that's a bug in `astroid`, but it still turned up some useful additional cases that were useful to correct.
|
I've updated this with some of Kevin's comments, and should have fixed all the linter failures. The linter was actually failing due to an inference bug in |
|
@kevinhartman, @mtreinish: fixed merge conflicts, and addressed actionable comments. |
mtreinish
previously approved these changes
Jun 21, 2022
pylint isn't actually correct in this case. `tuple(<list comp>)` is more efficient than `tuple(<generator>)` (certainly as of Python 3.10), because Python still needs to use a temporary vector-like expanding collection while consuming the iterable so despite appearances there's no memory benefit to the generator, and list comprehensions are translated into faster bytecode than general generators.
mtreinish
approved these changes
Jun 21, 2022
rathishcholarajan
added a commit
to rathishcholarajan/qiskit-ibm-runtime
that referenced
this pull request
Jul 5, 2022
Co-authored-by: Jake Lishman <[email protected]> Co-authored-by: Jake Lishman <[email protected]>
rathishcholarajan
added a commit
to Qiskit/qiskit-ibm-runtime
that referenced
this pull request
Jul 5, 2022
* Copy Qiskit/qiskit#8055 Co-authored-by: Matthew Treinish <[email protected]> * Copy qpy changes from Qiskit/qiskit#8093 Co-authored-by: Jake Lishman <[email protected]> Co-authored-by: Jake Lishman <[email protected]> * Copy Qiskit/qiskit#8200 Co-authored-by: Matthew Treinish <[email protected]> * Copy of Qiskit/qiskit#7300 Co-authored-by: Matthew Treinish <[email protected]> Co-authored-by: Naoki Kanazawa <[email protected]> * Copy of Qiskit/qiskit#8235 Co-authored-by: Matthew Treinish <[email protected]> Co-authored-by: Matthew Treinish <[email protected]> Co-authored-by: Jake Lishman <[email protected]> Co-authored-by: Jake Lishman <[email protected]> Co-authored-by: Naoki Kanazawa <[email protected]>
jakelishman
added a commit
to jakelishman/qiskit-terra
that referenced
this pull request
Jul 12, 2023
This removes (as best as our test suite touches) all uses of the implicit legacy tuple form of `CircuitInstruction` that have crept in since ea02667 (Qiskitgh-8093). The changes are largely just mechanical over to the new form.
ElePT
pushed a commit
to ElePT/qiskit-algorithms
that referenced
this pull request
Jul 27, 2023
* Convert QuantumCircuit.data items to CircuitInstruction This introduces a new, lightweight class `CircuitInstruction` the encapsulates the previous 3-tuple that made up the items of `QuantumCircuit.data`. It includes a legacy API that makes the object appear to be the old 3-tuple when used as if it were that tuple, and the relatively small changeset for this commit is an approximate indication that these should mostly be suitable. There will be significant performance penalties for accessing the legacy interface, however, due to some internal type conversions, so it is a matter of priority to convert internal usage to the new dotted-attribute access form. The only places necessary to change across Terra were those where the data elements were compared by referential equality to expected values (such as in the control-flow builders), and places that were invalidly accessing the private attribute `QuantumCircuit._data`. All these are updated. `ParameterTable` should in the future be changed to operate over this entire context, but that was beyond the scope of the initial commit. * Convert all usage of data tuple to CircuitInstruction This previous commit is the minimal set of changes needed for the entire test suite to pass, but large parts of Terra and the test suite were still using the backwards-compatibility shims when dealing with `QuantumCircuit.data`. This changes over every single usage within Terra to use the new dotted-access form, rather than the tuple indexing or iteration that previously existed. The internal types of the `qargs` and `cargs` in `DAGDepNode` and `DAGOpNode` are also changed to be `tuple` now; previously they were semi-undefined, and in some cases were not consistent, but not caught in the test suite. Since changing the type of the `qubits` parameter in `CircuitInstruction` to be tuple (unless using the backwards-compatibility shims), this caused a lot of failures where the value was being assigned directly. Normalising to `tuple` has the same benefit as it does in the circuit instruction - less copying overhead, and no worrying about multiple writable references to the same object. By far the largest set of changes are in the test suite; we do a lot of comparing things for exact equality, and since the type is now changed from `list` to `tuple`, all the tests had to be updated. This isn't expected to have much impact on user code, but changing the type to an immutable one has large benefits for us when copying these data structures. * Correct bad version in comment * Simplify unnecessary if/else * Update remnant legacy interface uses Some uses of the legacy interface related to directly setting `QuantumCircuit.data` had slipped through the cracks. A bug in `astroid` caused its inference to fail, and think that these settings made the scalar type of `QuantumCircuit.data` the old 3-tuple after it saw these settings. Since there's a property setter for `data`, that's a bug in `astroid`, but it still turned up some useful additional cases that were useful to correct. * Remove accidental blank line * Use QuantumCircuit.copy_empty_like in Instruction constructors * Correct global phase in Instruction.inverse * Shut pylint up about consider-using-generator pylint isn't actually correct in this case. `tuple(<list comp>)` is more efficient than `tuple(<generator>)` (certainly as of Python 3.10), because Python still needs to use a temporary vector-like expanding collection while consuming the iterable so despite appearances there's no memory benefit to the generator, and list comprehensions are translated into faster bytecode than general generators. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
jakelishman
added a commit
to jakelishman/qiskit-terra
that referenced
this pull request
Jun 21, 2024
This has been the legacy path since `CircuitInstruction` was added in Qiskitgh-8093. It's more performant to use the attribute-access patterns, and with more of the internals moving to Rust and potentially needing more use of additional class methods and attributes, we need to start shifting people away from the old form.
jakelishman
added a commit
to jakelishman/qiskit-terra
that referenced
this pull request
Jun 21, 2024
This has been the legacy path since `CircuitInstruction` was added in Qiskitgh-8093. It's more performant to use the attribute-access patterns, and with more of the internals moving to Rust and potentially needing more use of additional class methods and attributes, we need to start shifting people away from the old form.
jakelishman
added a commit
to jakelishman/qiskit-terra
that referenced
this pull request
Jun 21, 2024
This has been the legacy path since `CircuitInstruction` was added in Qiskitgh-8093. It's more performant to use the attribute-access patterns, and with more of the internals moving to Rust and potentially needing more use of additional class methods and attributes, we need to start shifting people away from the old form.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 24, 2024
This has been the legacy path since `CircuitInstruction` was added in gh-8093. It's more performant to use the attribute-access patterns, and with more of the internals moving to Rust and potentially needing more use of additional class methods and attributes, we need to start shifting people away from the old form.
Procatv
pushed a commit
to Procatv/qiskit-terra-catherines
that referenced
this pull request
Aug 1, 2024
This has been the legacy path since `CircuitInstruction` was added in Qiskitgh-8093. It's more performant to use the attribute-access patterns, and with more of the internals moving to Rust and potentially needing more use of additional class methods and attributes, we need to start shifting people away from the old form.
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
These two commits change the data type of the elements of
QuantumCircuit.datafrom an ad-hoc 3-tuple(Instruction, List[Qubit], List[Clbit])to an encapsulating namedtuple-like class calledCircuitInstructionwith three attributes:operation: currently anInstruction, but will presumably beOperationsoon.qubits:Tuple[Qubit, ...]clbits:Tuple[Clbit, ...]The first commit, b554a71, is the minimal set of changes needed to convert the data type, including backwards-compatibility shims. I suggest reviewers look at this first. The test suite passes (without modification) after this commit, which is a test of backwards compatibility.
The second commit, fb0fb32, changes every single usage of this tuple in Terra and its test suite over to using the new attribute-access pattern. This is almost entirely mechanical, but there is a slight backwards-compatibility break here. Previously, the data types of
qargsandcargsinDAGOpNodeandDAGDepNodewere not specified, but in practice were lists most (but not all) of the time. After this commit, they are now tuples in 100% of Terra code. This should have little-to-no effect on downstream code, because everything would have broken if anybody actually attempted to mutate one of these lists. The only likely possibilities are if people are trying to add the values to a list (Terra very occasionally tried this), or if they are comparing for equality with an exact list.Details and comments
See #7624 for a lot of the justification for why we want to change from an ad-hoc tuple to an extensible class as the backing scalar type. See also #7020 for a previous attempt at this. #7020 did not make the second commit in this PR, which is why this PR does not show the same performance regressions as that one.
I ran the test suites from Experiments and Nature against this, with no failures, suggesting the backwards compatibility shims in
CircuitInstructionwork pretty well, and the changes toDAGOpNodedon't have an effect on downstream code.The reason to use tuples not lists in the new instruction is to reduce memory usage on copy (such as from circuit to DAG); tuples can be bound directly, whereas lists need to be reallocated. They are also directly usable as dictionary keys (which we sometimes do), and we have no need to mutate them.
The new
CircuitInstructionclass is not immutable itself, although I don't propose we use that in any current usages. I anticipate we may want to use mutability when more items are in it (such as simple binding of parameters), especially if the collection underlyingQuantumCircuitchanges from alistto a DAG at any point. This may not be necessary, though.For all the end-to-end benchmarks, this appears to have no effect on the timings, after the second commit (there are regressions after only the first). I rebased these commits to tweak some bits after I ran the benchmark suite, but comparing this PR to
main, the slowdowns I found were:There is a ~20% performance regression on the few microbenchmarks where the construction time for the
CircuitInstructionoperations is the bottleneck (e.g. copying very narrow, very deep circuits), but actual transpilation workloads don't suffer. I feel like this is something we may need to accept, given the arguments in #7624.