Annotated operations #9846
Annotated operations #9846
Conversation
|
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:
|
|
Some of the tests that are failing due to making in At some prior point, I have changed quantum circuit's decompose to call However, Any advice? == Updates:
|
Pull Request Test Coverage Report for Build 5848761495
💛 - Coveralls |
There was a problem hiding this comment.
I've tried to review this only as a high-level overview of the concepts involved, rather than getting into the technical details of the code right now. Overall, I'm happy with the direction this looks like it's going in, and I think there's a lot to be gained with making these constructions lazy.
I think that this body of work might benefit from a text-based discussion on an RFC as well, especially including some illustrative examples of the types of tranpsiler pass we're attempting to enable with these operations, and which parts of circuit construction/synthesis that are currently eager that we're hoping to make lazier, and how we'll go about that using these objects.
| try: | ||
| # extract definition | ||
| definition = op.definition |
There was a problem hiding this comment.
This is likely going to have an impact on transpiler performance in general if it's in the preset pass managers, because it eagerly causes all objects in the circuit to be inspected for their definition, recursively all the way down the stack. I imagine we need to rethink things here: either we might need to consider limitations on what can be in a .definition field (not ideal), or we'll probably need to consider how the HLS passes will run or be called - we only need to call them when the definition is actually accessed, likely by the basis translation.
That might be an issue that's solved orthogonally to the annotated operations - as you pointed out, the problem of the HLS pass not recursing was pre-existing.
There was a problem hiding this comment.
This part of the code was adapted from UnrollCustomDefinitions, however I understand that UnrollCustomDefinitions runs at most once during transpilation, while in theory HighLevelSynthesis could be run multiple times (e.g., as part of the optimization loop, but this does not happen right now). I did add an argument recurse which would prevent examining any definitions. And yes HLS needs to support recursion (not doing so was an omission in the original implementation).
There was a problem hiding this comment.
Yeah, I'm worried about the performance of this too since we'll potentially be doing it on every node. We need to think a bit about our strategy here. Does it make sense to try and limit the scope here to just what is needed for annotated operations?
I'm thinking for a more general recursion fix we'll want to handle that on it's own because to handle it with minimal overhead, we'll likely want to discuss on it's own. Like I think we'll want a mix of strategy like what unroll custom definitions is doing with an equivalence library lookup and also checking op.definition to see if there are any known high level objects to synthesize in the definition.
There was a problem hiding this comment.
These are very good points. Based on this and other comments, in 1c71712 I am limiting the scope of this PR to avoid recursing into gates' definitions. That is, for now the annotated gates can only appear on top-level or as base gates of other annotated gates.
| # Currently, we depend on recursive synthesis producing either a QuantumCircuit or a Gate. | ||
| # If in the future we will want to allow HighLevelSynthesis to synthesize, say, | ||
| # a LinearFunction to a Clifford, we will need to rethink this. |
There was a problem hiding this comment.
I think this comment gets at a larger question with the organisation of "high-level synthesis" passes, and perhaps hints that there's really several different transpiler stages being rolled into one here. I don't have an answer for that.
|
@jakelishman, thanks for the high-level review and great questions that you've raised on the way. So the plan is to create an RFC and/or to have a discussion in one of qiskit-dev meetings and see how to go from there, correct? |
|
@jakelishman, as I've mentioned, I really like your suggestion to handle "annotated" operations via plugins, however there seems to be a small complication. Synthesis of annotated operations must be recursive: first the base gate must be synthesized (which really involves running |
|
Overall, the recursion being at that depth sounds generally ok to me, and largely what I expected; I don't think it's inherently a problem for a plugin to want to call more than just itself during a definition stage. I do agree that we'll want to take care with the interface to HLS stuff though, to make sure it's fairly straightforwards to use, and that instantiating a visitor isn't too heavy/expensive. |
There was a problem hiding this comment.
So I took a quick first pass over the PR and left a few inline comments inline. I think the basic structure outlined here is quite reasonable and looking good.
I think the biggest thing missing though is documentation around the new interface. The piece that I think will be exceedingly important is documenting the expectations for the AnnotatedOperation and the Modifiers api. For example if as a downstream user I wanted to create a new modifier, ReverseModifier that reverses the order of instructions in a definition how do I go about doing this, is this allowed? How does it integrate into the annotated operation and the corresponding synthesis. My guess from reading the code is it's not expected to allow any additional custom modifiers, but I think it's clear we document all the expectations of how we're defining the interface. I don't think there is necessarily a right or wrong answer, but we should just say how we want people to use the interface for annotated ops.
The other concern I have is around the performance overhead that Jake highlighted in HighLevelSynthesis around the recursion. I feel like we should separate out that bugfix for a separate PR so that the only recursion we do now is around annotated ops.UnrollCustomInstructions has a fair amount of overhead even for circuits that does not contain any custom instructions and that's even without doing anything recursive. (for HLS this constant overhead was recently fixed by #10788 which is what caused the merge conflict on this PR, so we should be careful to not try and re-introduce it). I think in my ideal scenario we'd merge UnrollCustomInstructions into HLS as a single pass, so HLS recurses if something is not in the plugin list, not in the target, and not in the equivalence lib, and adds a default recursive unroll in the absence of a plugin. But that is definitely a standalone PR.
Also, I think this needs a release note. :)
|
|
||
|
|
||
| class Modifier: | ||
| """Modifier class.""" |
There was a problem hiding this comment.
I would probably put something in the docstring here to document this is the base class that all modifiers for an annotated operation should inherit from.
| pass | ||
|
|
||
|
|
||
| @dataclasses.dataclass |
There was a problem hiding this comment.
Does this have any meaning for this class as there are no attributes?
There was a problem hiding this comment.
The __repr()__ function is a bit different: for instance print(InverseModifier()) outputs <qiskit.circuit.annotated_operation.InverseModifier object at 0x00000233A87C0160> without the decorator, and InverseModifier() with the decorator. This is not a big deal, but it would probably make sense to keep this consistent across different modifiers.
| """Control modifier: specifies that the operation is controlled by ``num_ctrl_qubits`` | ||
| and has control state ``ctrl_state``.""" | ||
|
|
||
| num_ctrl_qubits: int |
There was a problem hiding this comment.
?
| num_ctrl_qubits: int | |
| num_ctrl_qubits: int = 0 |
| try: | ||
| # extract definition | ||
| definition = op.definition |
There was a problem hiding this comment.
Yeah, I'm worried about the performance of this too since we'll potentially be doing it on every node. We need to think a bit about our strategy here. Does it make sense to try and limit the scope here to just what is needed for annotated operations?
I'm thinking for a more general recursion fix we'll want to handle that on it's own because to handle it with minimal overhead, we'll likely want to discuss on it's own. Like I think we'll want a mix of strategy like what unroll custom definitions is doing with an equivalence library lookup and also checking op.definition to see if there are any known high level objects to synthesize in the definition.
|
@mtreinish, many thanks for the review! I have tried to address your comments as follows:
|
Pull Request Test Coverage Report for Build 6404519711
💛 - Coveralls |
There was a problem hiding this comment.
This looks closer now, thanks for all the updates. I left a few inline comments and questions mostly about the docs. The only other thing I think would be good is to add a couple test cases to test/python/compiler/test_transpiler.py that tests the full path through transpile().
The other thought I had is this won't be serializable via qpy. But we can fix that in a follow up (I also just checked and Clifford has a similar issue).
| :class:`~.PowerModifier`. In the future, we are planning to make | ||
| the modifier interface extendable, accommodating for user-supplied | ||
| modifiers. |
There was a problem hiding this comment.
I'd probably remove this last sentence. I'm not sure the timetable we'd have for this. I'm actually fine leaving it locked down longer term, I just wanted to make sure we were documenting this as an expectation, because as a user I'd just come to this class and start trying to extend it.
There was a problem hiding this comment.
Agreed. Done in c77b810.
There was a problem hiding this comment.
I'm glad this is removed - I think any actual transpiler passes that involve user-extensible annotations would be extremely difficult to do well. Synthesising annotations is one thing, but optimisations and normalisation in the presence of them would I think be very difficult and risk compromising our ability to optimise control-gate annotations, etc.
| we are planning to add transpiler optimization passes that make use of | ||
| this higher-level representation, for instance removing a gate | ||
| that is immediately followed by its inverse. |
There was a problem hiding this comment.
| we are planning to add transpiler optimization passes that make use of | |
| this higher-level representation, for instance removing a gate | |
| that is immediately followed by its inverse. | |
| This enables writing transpiler optimization passes that make use of | |
| this higher-level representation, for instance removing a gate | |
| that is immediately followed by its inverse. |
There was a problem hiding this comment.
Done in c77b810
| base operation). However, an important difference is that the | ||
| circuit definition of an annotated operation is not constructed when | ||
| the operation is declared, and instead happens during transpilation, | ||
| specifically during :class:`~.HighLevelSynthesis` transpiler pass. |
There was a problem hiding this comment.
| specifically during :class:`~.HighLevelSynthesis` transpiler pass. | |
| specifically during the :class:`~.HighLevelSynthesis` transpiler pass. |
There was a problem hiding this comment.
Done in c77b810
| raise TranspilerError(f"HighLevelSynthesis was unable to synthesize {node.op}.") | ||
|
|
||
| if isinstance(decomposition, QuantumCircuit): | ||
| dag.substitute_node_with_dag(node, circuit_to_dag(decomposition)) |
There was a problem hiding this comment.
Are there any shared references here? I'm wondering if we can do copy_operations=False for the circuit to dag call here
There was a problem hiding this comment.
This is fixed in c77b810. In fact I started using copy_operations=False in the follow-up PR, but forgot to update it here as well.
|
|
||
| return dag | ||
|
|
||
| def _recursively_handle_op( |
There was a problem hiding this comment.
This name is future facing right, since there isn't any recursion currently :)
There was a problem hiding this comment.
You are right. Actually, even now it's recursive because synthesizing an annotated operation requires to first synthesize its "base operation" which might be another annotated operation. There are several tests to that extent.
| if not isinstance(decomposition, (QuantumCircuit, Operation)): | ||
| raise TranspilerError(f"HighLevelSynthesis was unable to synthesize {node.op}.") | ||
|
|
||
| if isinstance(decomposition, QuantumCircuit): |
There was a problem hiding this comment.
This is making me think in parallel we should have an option for a plugin to return DAGCircuits to avoid this conversion.
There was a problem hiding this comment.
I was thinking about this as well! Related to this, in the follow-up PR #10965 I have already allowed some of the internal functions like _recursively_handle_op to also return a DAGCircuit, otherwise there are too many useless back-and-forth conversions.
| In the future, we are also planning to make the modifier interface extendable, | ||
| accommodating for user-supplied modifiers. |
There was a problem hiding this comment.
I would remove this too because I'm not sure we're committing to this.
There was a problem hiding this comment.
Removed in c77b810.
|
Thanks @mtreinish. I have added "full transpile path" tests for circuit with annotated operations, with and without backend. |
| inverted and then controlled by 2 qubits. | ||
| """ | ||
| self.base_op = base_op | ||
| self.modifiers = modifiers if isinstance(modifiers, List) else [modifiers] |
There was a problem hiding this comment.
I didn't realize you could use the type classes from typing in isinstance checks like this. Looking at the cPython code it just wraps the underlying list type in an alias class and the __subclasscheck__ just compares against the inner list type if there given class isn't also an alias.
| for node in dag_op_nodes: | ||
| qubits = ( | ||
| [dag.find_bit(x).index for x in node.qargs] if self._use_qubit_indices else None | ||
| ) | ||
| decomposition = self._recursively_handle_op(node.op, qubits) |
There was a problem hiding this comment.
The question I have here is this means we're running the synthesis for every operation in the circuit right? Then if there is no synthesis decomposition here is the same as node.op meaning we're essentially doing dag.subsitute_node(node, node.op) for every op node in the circuit? I'm wondering if it makes more sense to have self._recursively_handle_op return None in this case and we just do:
| for node in dag_op_nodes: | |
| qubits = ( | |
| [dag.find_bit(x).index for x in node.qargs] if self._use_qubit_indices else None | |
| ) | |
| decomposition = self._recursively_handle_op(node.op, qubits) | |
| for node in dag_op_nodes: | |
| qubits = ( | |
| [dag.find_bit(x).index for x in node.qargs] if self._use_qubit_indices else None | |
| ) | |
| decomposition = self._recursively_handle_op(node.op, qubits) | |
| if decomposition is None: | |
| continue |
The other option I think would be to rebuild the dag from scratch (like we do in sabre and some other passes), as that's typically faster than doing in place substitution for every node. But in this case I expect the number of operations being synthesized are much less than those that aren't so just skipping the replacement should be sufficient.
There was a problem hiding this comment.
This is a very good point, and I was thinking of this in the follow-up PR #10965. The approach I took (also added here in ba42605) is to also return whether op was modified. It's slightly more convenient than returning None, for instance when we (recursively) synthesize the base gate of an annotated operation (which might itself be an annotated operation, hence the need for recursion) we don't need to check if the result is None and use base.op instead.
Interesting that in cases rebuilding the DAG circuit from scratch is faster (but I agree that we don't need this here since in practice the number of high-level-objects / annotated gates will be smaller than the number of ordinary gates).
| # Currently, we depend on recursive synthesis producing either a QuantumCircuit or a Gate. | ||
| # If in the future we will want to allow HighLevelSynthesis to synthesize, say, | ||
| # a LinearFunction to a Clifford, we will need to rethink this. | ||
| if not synthesized_op or not isinstance(synthesized_op, (QuantumCircuit, Gate)): |
There was a problem hiding this comment.
Could you use the bool second tuple element return from _self.recursively_handle_op here? But it doesn't really matter this should work fine.
Summary
This PR is the first installment to supporting gate modifiers, please refer to the issue #6879 and the discussion therein.
Specifically, this adds the
AnnotatedOperationclass which consists of a base operator and an ordered list of modifiers (currently supported modifiers areInverseModifier,ControlModifierandPowerModifier).This also adds the ability to synthesize annotated operations during transpilation (reusing the already existing code for computing inverse/control/power).
This does not include optimization passes for circuits with annotated operations; these will be included in a follow-up PR.
Details and comments
Here are a few simple examples of defining an annotated operation:
Both op1 and op2 are semantically equivalent to an
SGate()which is first inverted and then controlled by 2 qubits.And here is an example of creating a circuit that contains a "lazily" controlled
Clifford:As we have discussed in #6879, the ability to synthesize annotated operations now appears as a part of
HighLevelSynthesistranspiler pass (and not as an independent pass as I have originally had it). The intuition is thatHighLevelSynthesisshould be responsible for synthesizing various more abstract operations, which include both things such asCliffordsorLinearFunctions(which can be synthesized using synthesis plugins) andAnnotatedOperations(which can be synthesized by first synthesizing the base operation and then synthesizing its inversion/control/power variant). One important change that I had to make to support this is to makeHighLevelSynthesisrecursive, which was an oversight anyway, since we may well have a circuit that contains a gate whosedefinitioninvolves a gate whosedefinitionincludes aClifford.However this also leads to a few test failures, and I need an advice on how to best resolve that (will post a follow-up).