-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support control flow in ConsolidateBlocks #10355
Support control flow in ConsolidateBlocks #10355
Conversation
One or more of the the following people are requested to review this:
|
This comment was marked as resolved.
This comment was marked as resolved.
1f3c217
to
413aa02
Compare
Intermittent test failure. It happens locally with the test suite as well. But not if I run the test as a standalone script. Probably an RNG seed... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I like this approach as a quick path to getting it control flow aware. I had a couple questions inline. I also think adding some tests similar to the coverage in #9143 would be good here too just to sanity check the behavior around control flow a bit more thoroughly.
new_consolidate_blocks = self.__class__( | ||
force_consolidate=self.force_consolidate, | ||
approximation_degree=self._approximation_degree, | ||
target=self.target, | ||
decomposer=self.decomposer, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same question in #9143 but I'm wondering is there a reason we really need a new copy of the pass here? If we could just use new_consolidate_blocks = self
here it would simplify the code a bit (and we wouldn't need a new decomposer
argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess is that this was the safest thing to when testing #9143. Originally it was constructed for each block (or maybe each control flow op). @jakelishman suggested lifting it out of the innermost loop. I followed that here. I guess the intended semantics of this pass (or a pass in general) is that they should be immutable ? I don't see an obvious violation for ConsolidateBlocks
. I can just enable it and run tests. Might be a good idea to add a test to try to break it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With commit 7a34941 a new copy of the pass is no longer made. The original is reused.
basis_gates (List(str)): Basis gates from which to choose a KAK gate. | ||
approximation_degree (float): a float between [0.0, 1.0]. Lower approximates more. | ||
target (Target): The target object for the compilation target backend | ||
target (Target): The target object for the compilation target backend. | ||
decomposer: A 2q gate decomposer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have a bit more detail on what this argument is or mark it as private. As a user I'm not clear what the input type needs to be here or what the constraints are on the input type.
That being said, I'm not clear on why this argument is needed. I can see the argument for making this an input longer term so that users with custom decomposers could use that here. But in actuality the decomposer isn't really used for much in this pass, it just checks the number of supported qubits and the two qubit gate used in the kak decomposition (so as to not resynthesize a block unnecessarily).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The argument decomposer
was introduced just for supporting control flow ops. I added it to avoid constructing a new decomposer. I don't know of any other use for it. I agree it makes sense not to expose it. We could construct a new decomposer using __new__
. That would be inelegant, but would avoid introducing the new argument.
But if we can reuse the same instance of ConsolidateBlocks
, then the whole question is moot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This argument was removed in 3da4990
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also removed any mention of the decomposer in bf0a1f4. btw. the decomposer is needed for
https://github.com/Qiskit/qiskit-terra/blob/e2674cebcc9cf2cd7333f7fe645a833be27bc972/qiskit/quantum_info/synthesis/two_qubit_decompose.py#L1409-L1411
EDIT: I added some and they are being reviewed and modified. @mtreinish Are you thinking specifically tests like: crossing between a block and its parent env, and crossing between blocks? |
Before, we used the builder interface for an ifelse op. Some details of the circuit built, in particular the mapping of wires is not deterministic. We could have used canonicalize_control_flow. But instead we construct the IfElseOp manually. This removes the complexity of the builder interface from this test.
* Before, we created an new ConsolidationPass when descending into control flow blocks. With this commit, we use the existing pass. * Add some tests for cases where consolidation should not happen.
This was added in a previous commit in the series of commits for this PR. The code has been redesigned so that this argument is no longer necessary.
Added tests in 7a34941. |
def test_descent_into_control_flow(self): | ||
"""Test consolidation in blocks when control flow op is the same as at top level.""" | ||
qc = QuantumCircuit(2, 1) | ||
u2gate1 = U2Gate(-1.2, np.pi) | ||
u2gate2 = U2Gate(-3.4, np.pi) | ||
qc.append(u2gate1, [0]) | ||
qc.append(u2gate2, [1]) | ||
qc.cx(0, 1) | ||
qc.cx(1, 0) | ||
|
||
pass_manager = PassManager() | ||
pass_manager.append(Collect2qBlocks()) | ||
pass_manager.append(ConsolidateBlocks(force_consolidate=True)) | ||
result_top = pass_manager.run(qc) | ||
|
||
qc_control_flow = QuantumCircuit(2, 1) | ||
qc_block = copy.deepcopy(qc) | ||
|
||
qc_block = QuantumCircuit(qc.qubits, qc.clbits) | ||
qc_block.append(u2gate1, [0]) | ||
qc_block.append(u2gate2, [1]) | ||
qc_block.cx(0, 1) | ||
qc_block.cx(1, 0) | ||
|
||
ifop = IfElseOp((qc.clbits[0], False), qc_block, None) | ||
qc_control_flow.append(ifop, qc.qubits, qc.clbits) | ||
|
||
pass_manager = PassManager() | ||
pass_manager.append(Collect2qBlocks()) | ||
pass_manager.append(ConsolidateBlocks(force_consolidate=True)) | ||
result_block = pass_manager.run(qc_control_flow) | ||
gate_top = result_top[0].operation | ||
gate_block = result_block[0].operation.blocks[0][0].operation | ||
self.assertEqual(gate_top, gate_block) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with this test is not inherently the builder interface, it's because the test compares the exact matrix representation of an inner-scope gate with an outer-scope gate without resolving the bit binding of the inner scope. That's a vital part of resolving control-flow scopes.
It would be good to have tests that explicitly use the builder interface to ensure that there is no problem with bit binding throughout, because that's how users will build control-flow circuits.
Canonicalizing the control-flow structure after the pass has run (which is the only place the test canonicalisation ought to be done, because the tested functionality should work whether or not the form is canonicalised) would not fix this test failure, because the problem is fundamentally that the test was requiring that A(0, 1)
had the same matrix form as A(1, 0)
. Canonicalisation after the pass has run wouldn't change that the operator had been computed on (1, 0) (and suitably transposed) rather than (0, 1). Instead, the test could check the qubit binding, and swap the 2q operator if necessary:
test_matrix = ...
if <qubit-binding flips order>:
test_matrix = swap @ test_matrix @ swap
# or
test_matrix = test_matrix.reshape(2, 2, 2, 2).transpose(1, 0, 3, 2).reshape(4, 4)
Fwiw, the pass is returning a correct answer in the flipped case from what I saw. It's just the test that's wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the builder interface the only interface? I mean, is constructing the object directly considered an internal detail? Is bit binding documented anywhere?
If there is a simple interface that is part of the API, it makes sense to me to use this when testing if possible. Unless it is much more difficult than the builder interface.... Also I recall that the builder interface was supposed to be more provisional than the lower-level design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't really about the builder interface, it's about bit-binding that can also be done with the low-level interface. It's a really important part of the Terra data model that things that interact with scopes (which includes Instruction.definition
fields and matrices) need to know how to bind the CircuitInstruction.qubits
to their counterparts in the inner scope. This existed before the control-flow instructions as well, it's just that it most frequently comes up with them.
The builder interface for control-flow isn't any more provisional than any part of Terra.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is "bit binding"? I don't find anything like that phrase in terra. I really need some documentation or reference to code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked a bit. Looks as if upon construction some objects, such as control flow ops, don't do any mapping of qubits, nor do they make any assumptions about mapping. QuantumCircuit.to_gate
may do some kind of mapping, but I think not. And ConsolidateBlocks
implements mapping qubits between the parent circuit and the blocks at a low level, throughout the code. By low level, I mean qiskit has no tools for mapping qubits between objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By bit binding I mean the relationship between the ordering of CircuitInstruction.qubits
and how the operation
is applied. If you access operation.definition
, or in any way get a matrix representation or inner circuit from the operation
, you must ensure that you interpret the qubit ordering according to CircuitInstruction.qubits
. Your test is not doing that - you're taking a 2q matrix without regard to the ordering of the qubits, and consequently it's sometimes got a flipped basis if the block happened to be defined on 1, 0
instead of 0, 1
.
This isn't specific to control flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And Qiskit does this everywhere - this isn't something new, it's been a fundamental part of the data since the very first versions.
pass_manager = PassManager() | ||
if "run_list" in self.property_set: | ||
pass_manager.append(Collect1qRuns()) | ||
if "block_list" in self.property_set: | ||
pass_manager.append(Collect2qBlocks()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can likely make this PassManager
just once up in __init__
and retrieve it from self
each time, even during the recursion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. The cost should be negligible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After trying, I don't see an easy way to make this work. A ConsolidateBlocks
instance is constructed before the top-level RunningPassManager
injects the necessary information. The call PassManager
on my machine takes < 250ns. Constructing it and checking a dict
and appending passes takes 13us or 26us.
We could pass the required information when constructing ConsolidateBlocks
. That would allow us to do this more cleanly in the future. Maybe populate requires
based boolean-valued arguments. We'd support the status quo for a while.
In fact, doing this would be less fragile and better signal intent than the status quo. This:
is not as transparent as
ConsolidateBlocks(
collect_2q_blocks=True, basis_gates=basis_gates, target=target, approximation_degree=approximation_degree
),
UnitarySynthesis(
basis_gates,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing the required information to ConsolidateBlocks
would be my general choice so that the top level of the recursion and the recursive calls can all look more similar. At this stage in the 0.25 cycle, it might be better to leave what you've got and fix it in 0.45 instead so we've got more time.
releasenotes/notes/add-control-flow-to-consolidate-blocks-e013e28007170377.yaml
Outdated
Show resolved
Hide resolved
In the first version of the associated PR, we created a new pass when descending into control flow blocks. These lines were included to support that construction and are no longer needed.
Remove unused import in tests
…e28007170377.yaml Co-authored-by: Jake Lishman <jake@binhbar.com>
* Gates used in testing ConsolidateBlocks with control flow ops were copied from another test in the file. They complicate the test, and removing them does not weaken the test at all. * Factor some code within a test into a function
This has no effect on the test. But, previously, we used a clbit taken from an unrelated circuit. This might give the impression that there is some significance to this unexpected choice.
…t circuit Previously we used qubits and clbits from an unrelated circuit to specify which bits to apply an IfElse to when appending. Here, we use bits from the same circuit that we are appending the gate to. This does not change the test. But again, one might look for significance in the previous unusual choice. Qiskit considers two registers of the same length and the same name to be the same register. The previous behavior depended on this choice.
…circuit This has no effect on the test. As in the previous commits, the choice was unusual and might imply some significance. The more natural choice is to create new qubits by specifying the number.
These simplifications are similar to those made for the first test.
I greatly simplified all of the new tests. I have been unable to find a problem with resolving bit binding or interpreting matrices, etc. The tests never failed locally and have not been failing in CI for some time. This makes it difficult to discover what may be wrong with them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates here, and for the efforts to make the tests a lot clearer. The existing tests are still not quite exercising the possibility of the operations on inner blocks being in a different order based on how the operation is applied to qubits in the outer block. The pass already does have the correct behaviour, but we just weren't testing it.
I've written a test that illustrates what I mean - if we can add this, I'm happy to merge the PR. The part about the wire_map
for each instruction is the "binding" that I've been referring to each time. It's not specific to control-flow ops at all; the same binding concerns would be present when trying to compare the definition
of an applied gate as well, it's just that most people generally don't need to do that because the basis translator and unroller do it for them. Control-flow blocks are a type of nested scope that anyone handling always needs to recurse into though, so they're perhaps getting introduced to the concept of "binding" for the first time.
Co-authored-by: Jake Lishman <jake@binhbar.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me - thanks for sticking with it!
I was told there is an error in my tests, so I spent considerable time searching for it. But it looks like we are ready to merge without fixing these tests? Are they still wrong? Were they not wrong? EDIT: If I understand correctly, the things about binding come down to understanding that |
When qc.if_test((0, False), body.reverse_bits(), reversed(qc.qubits), qc.clbits)
qc.if_test((0, False), body.copy(), reversed(qc.qubits), qc.clbits) it transforms I think it's reasonable to include these tests.
|
The final form of the tests you've written isn't wrong (because of the exact form of the operations they construct to test against), but the initial form did have a problem, which is why they were showing flaky behaviour. The new test I added explicitly makes sure that we test the behaviour that was giving this PR flakiness before, so it additionally covers the case I was worried about. The tests you wrote are still very worthwhile, in addition to the one I wrote. |
Good. I'll compare the initial and final form of the tests. In any case, as far as the current state of the PR is concerned everything seems clear and correct to me. |
The original form of the tests was asserting that the same logical operation on outer qubits would always have precisely the same matrix form in an inner block, no matter which order the qubits were bound to the inner block. The control-flow builders do not guarantee that this order is consistent between constructions, which is well within the data model, and so the test was flaky. My new test explicitly constructs the same 2q operation with both permutations of how the inner block might be ordered relative to the outer block, and both permutations of how the block is applied to the outer two qubits. That means that we're testing that the pass produces correct output, no matter what the binding order is. |
So, for instance, the control-flow builder might flip both Since people will typically use the builder, it makes sense to include tests that take this into account. But testing without the builder, something closer to a unit test, is also a good thing to do. |
Yeah, that's it exactly. |
* Support control flow in ConsolidateBlocks * Add release note for support control flow in ConsolidateBlocks * Move imports from inside function to top level * Make construction of ConsolidateBlocks for control flow ops more efficient * Do IfElseOp test without builder interface Before, we used the builder interface for an ifelse op. Some details of the circuit built, in particular the mapping of wires is not deterministic. We could have used canonicalize_control_flow. But instead we construct the IfElseOp manually. This removes the complexity of the builder interface from this test. * Linting * Avoid cyclic import * Try to fix cyclic import (in lint tool only) * Reuse top-level consolidation pass and add tests * Before, we created an new ConsolidationPass when descending into control flow blocks. With this commit, we use the existing pass. * Add some tests for cases where consolidation should not happen. * Remove argument `decomposer` from constructor of ConsolidateBlocks This was added in a previous commit in the series of commits for this PR. The code has been redesigned so that this argument is no longer necessary. * Remove cruft accidentally left In the first version of the associated PR, we created a new pass when descending into control flow blocks. These lines were included to support that construction and are no longer needed. * Move function-level import to module level Remove unused import in tests * Write loop more concisely * Update releasenotes/notes/add-control-flow-to-consolidate-blocks-e013e28007170377.yaml Co-authored-by: Jake Lishman <jake@binhbar.com> * Use assertion in tests with better diagnostics * Remove reference to decomposer from docstring to ConsolidateBlocks The previous doc string was a bit imprecise. It also referred to a decomposer which although implied, is not meant to be accessible by the user. * Use more informative tests for ConsolidateBlocks with control flow * Simplify test in test_consolidate_blocks and factor * Gates used in testing ConsolidateBlocks with control flow ops were copied from another test in the file. They complicate the test, and removing them does not weaken the test at all. * Factor some code within a test into a function * Factor more code in test * Use clbit in circuit as test bit when appending IfElse This has no effect on the test. But, previously, we used a clbit taken from an unrelated circuit. This might give the impression that there is some significance to this unexpected choice. * In test, use bits in circuit as specifiers when appending gate to that circuit Previously we used qubits and clbits from an unrelated circuit to specify which bits to apply an IfElse to when appending. Here, we use bits from the same circuit that we are appending the gate to. This does not change the test. But again, one might look for significance in the previous unusual choice. Qiskit considers two registers of the same length and the same name to be the same register. The previous behavior depended on this choice. * In a test, don't use qubits from unrelated circuit when constructing circuit This has no effect on the test. As in the previous commits, the choice was unusual and might imply some significance. The more natural choice is to create new qubits by specifying the number. * Factor code in test to simplify test * Simplify remaining control flow op tests for ConsolidateBlocks These simplifications are similar to those made for the first test. * Run black * Update test/python/transpiler/test_consolidate_blocks.py Co-authored-by: Jake Lishman <jake@binhbar.com> --------- Co-authored-by: Jake Lishman <jake@binhbar.com>
Summary
This PR adds support for applying the
ConsolidateBlocks
compiler pass to the blocks in control flow operations.Note that two usages of "blocks" in Qiskit are in play here. In
ConsolidateBlocks
, "blocks" refers to collections of gates that can be resynthesized. On the other hand "block" also refers to the circuits that are run conditionally in control flow operations.Closes #9426 .
Note that
DAGCircuit.collect_runs
#9425 is no longer relevant if the present PR is merged.Details and comments
This PR follows the approach of @ewinston in
In this approach, during execution of the pass, we construct a new
PassMangager
and use this to operate on each block in each control flow operation. ThePassManager
runs the analysis passes on each of these blocks, and the information is communicated on theproperty_set
in the usual way. In particular, for any level of nesting, each of the analysis and transformation passes are run once for each control flow block at each level.Another approach, which is not taken here, would be to run the analysis passes once and the consolidation pass once. In this approach we would run the analysis passes recursively from the top level, recording the data for the top level and all lower level blocks at once on the
property_set
. Then the consolidation pass would recurse into the blocks making use of the data recorded in the previous step.Items