[WIP] Encapsulate instruction in args in "Instruction" class #7020
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
This commit introduces a a new class for representing an instruction with it's operands, Operation. An Operation object contains 3 fields an instruction, a list of Qubits for the qargs, and a list of Clbits for the cargs. The operation class is now used for all enties in the QuantumCircuit's data attribute list, instead of the previous tuple of the form (instruction, qargs, cargs). For backwards compatibility an Operation object also implements the sequence protocol and can be used as a tuple, although named attribute performance will be better (ie operation.instruction returns faster than operation[0], but both return the same object). Moving forward we should start update the tuple usage internally to use attribute access instead.
mtreinish
added
the
Changelog: New Feature
|
I'm thinking for performance we'll need to update the access patterns before merging this (which increases the scope of the PR quite a bit). The test execution seems to have slowed down a ton (mostly the shor's algorithm tests). |
|
For initial benchmarking I ran the quantum volume asv benchmarks: but the tests definitely show a regression in the shor's algorithm tests: the 10 slowest tests for me locally with this pr: vs running with main: now I'm doing a full asv benchmark to see what things look like in a more controlled setting. But I think we're going to want to update everything in the repo to use attr based access instead of index based before we merge this |
|
Yeah, there is definitely overhead for circuit operations, the full benchmark results with everything going through the tuple interface is: So I think the next step here is to update all the QuantumCircuit methods to use attribute access and see if that fixes the overhead from |
kdk
reviewed
Sep 15, 2021
There was a problem hiding this comment.
Thanks for taking this on. Agree this will be much needed as the circuit model evolves going forward.
I'm thinking for performance we'll need to update the access patterns before merging this (which increases the scope of the PR quite a bit). The test execution seems to have slowed down a ton (mostly the shor's algorithm tests).
...
So I think the next step here is to update all the QuantumCircuit methods to use attribute access and see if that fixes the overhead from getitem.
This definitely increases the scope of this PR. (If the performance impact really is that substantial, would we need to deprecate index-based access or otherwise help users know to migrate their code?)
Given that the existing access pattern is entirely index-based, would starting with a NamedTuple be a reasonable first step? (Alternately, using a tuple internally so that we could still have def __getitem__(self, idx): return self._tuple[idx])
| from collections.abc import Sequence | ||
|
|
||
|
|
||
| class Operation(Sequence): |
There was a problem hiding this comment.
The terminology we've used here has varied a bit, but it's probably worth thinking over before we nail this down.
Maybe someone with more compiler experience can weigh in, but I thought canonically an Instruction was the combination of an operation and its operands (qargs and cargs), whereas with this PR we'd have an Operation as the combination of an Instruction and its operands. Is there another name we want to use here, or are we set on Operation? (We do already have an InstructionSet which is ~largely unused but very similar in structure to what's here.)
There was a problem hiding this comment.
Thanks for taking this on. Agree this will be much needed as the circuit model evolves going forward.
I'm thinking for performance we'll need to update the access patterns before merging this (which increases the scope of the PR quite a bit). The test execution seems to have slowed down a ton (mostly the shor's algorithm tests).
...
So I think the next step here is to update all the QuantumCircuit methods to use attribute access and see if that fixes the overhead from getitem.This definitely increases the scope of this PR. (If the performance impact really is that substantial, would we need to deprecate index-based access or otherwise help users know to migrate their code?)
Given that the existing access pattern is entirely index-based, would starting with a
NamedTuplebe a reasonable first step? (Alternately, using a tuple internally so that we could still havedef __getitem__(self, idx): return self._tuple[idx])
Yeah, I was under the impression that slotted attribute access was the fastest return and that's what I was optimizing for and my previous experience with named tuple was it's named attribute access was slower. However, as of py 3.8 that no longer seems to be the case (with this pr: python/cpython#10495 ). I did a quick test with this pr applied on python 3.9:
from qiskit import QuantumCircuit
from collections import namedtuple
qc = QuantumCircuit(2)
qc.h(0)
Op = namedtuple('Op', ['instruction', 'qargs', 'cargs'])
op = qc.data[1]
op_tuple = tuple(op)
op_named_tuple = Op(*op_tuple)
With indexed access:
%timeit op[0]
75.5 ns ± 0.201 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
%timeit op_tuple[0]
22.6 ns ± 0.0474 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
%timeit op_named_tuple[0]
23 ns ± 0.0631 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
with attribute access:
%timeit op.instruction
28.1 ns ± 0.11 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
%timeit op_named_tuple.instruction
26.8 ns ± 0.298 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
So I'm thinking the best choice is a named tuple despite my earlier hesitation (which was clearly wrong). I'll have to see how things behave in py3.7 and 3.6 too (although this is the last release with 3.6 support) but I doubt it will be an issue and even if it causes a regression I think it'll be an ok tradeoff (or we can wait until after 0.19 to do this and then we'll only support 3.7, 3.8, 3.9, and 3.10).
There was a problem hiding this comment.
It just seemed like the logical choice to me since we're already using instruction to represent the opcode. The terminology here is a bit fuzzy, like I've heard instructions and operation (well really opcode) used interchangeable. If you think operation here would be too overloaded the other term I was thinking of was Statement (ie one line in assembly). But I'll defer to what others like I don't feel too strongly as long as we're not reusing a name already used (since deprecating Instruction as it's used today isn't really a viable path).
There was a problem hiding this comment.
Yeah I think Instruction is the most appropriate name to refer to the tuple, which is like one line of qasm. It's unfortunate it wasn't used like that since the beginning. But there are places in qiskit where it's used like that, e.g. InstructionScheduleMap.
I was thinking in this release we can deprecate Instruction as it is now, and call it something else (actually Operation is a good name for that: https://en.wikipedia.org/wiki/Quantum_operation). And then re-introduce Instruction in the next release to refer to the tuple (Operation, qubits, clbits). This would abide by our 3-month deprecation policy I think.
There was a problem hiding this comment.
I also prefer Instruction for this class, and Operation for the base class currently-known-as-Instruction.
There was a problem hiding this comment.
Regarding InstructionScheduleMap, is it possible to wrap instruction to return parameters as a dictionary Dict[str, ParameterValue]? This has been really problematic since parameters in a circuit instruction is order sensitive, and thus management of parameter in the mapper is not straightforward (because parameters in schedules are not order sensitive, i.e. this is always managed by dict). It seems to be good chance to address this issue.
https://github.com/Qiskit/qiskit-terra/blob/66edc9d651f863be7fd195c5c9b40c5f610b3e03/qiskit/scheduler/lowering.py#L159
There was a problem hiding this comment.
As a possible candidate to avoid the name collision around the re-use of Instruction, can we maybe re-use the InstructionSet class here (and then, at some point in the future, rename InstructionSet to Instruction)?
I just ran with 3.7 it's about what I thought: |
That looks good. The existing path stays about the same and the new patch is marginally slower (for py3.6 and 3.7). I'm still a bit surprised that the tests in |
levbishop
reviewed
Sep 20, 2021
Co-authored-by: Lev Bishop <18673315+levbishop@users.noreply.github.com>
kdk
changed the title
8 tasks
|
Since I just had cause to mention this PR in another issue: this PR is going to be obsoleted by the dynamic-circuits-related work on #7624 (currently visible on the I'll close this PR for clarity, but this encapsulation is still planned to happen. It's just delayed a little bit because of me being off to write my thesis. |
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 introduces a a new class for representing an instruction
with it's operands, Operation. An Operation object contains 3 fields an
instruction, a list of Qubits for the qargs, and a list of Clbits for
the cargs. The operation class is now used for all enties in the
QuantumCircuit's data attribute list, instead of the previous tuple of
the form (instruction, qargs, cargs). For backwards compatibility an
Operation object also implements the sequence protocol and can be used
as a tuple, although named attribute performance will be better (ie
operation.instructionreturns faster thanoperation[0], but both returnthe same object). Moving forward we should start update the tuple usage
internally to use attribute access instead.
Details and comments
TODO: