Add control-flow builder interface

[jakelishman]

Summary

This adds a builder interface for control-flow operations on
QuantumCircuit (such as ForLoopOp, IfElseOp, and WhileLoopOp).
The interface uses the same circuit methods, but they are now overloaded
so that if the body parameter is not given, they return a context
manager. Entering one of these context managers pushes a scope into the
circuit, and captures all gate calls (and other scopes) and the
resources these use, and builds up the relevant operation at the end.
For example, you can now do:

    qc = QuantumCircuit(2, 2)
    with qc.for_loop(None, range(5)) as i:
        qc.rx(i * math.pi / 4, 0)

This will produce a ForLoopOp on qc, which knows that qubit 0 is the
only resource used within the loop body. These context managers can be
nested, and will correctly determine their widths. You can use
break_loop and continue_loop within a context, and it will expand to
be the correct width for its containing loop, even if it is nested in
further if_test blocks.

The if_test context manager provides a chained manager which, if
desired, can be used to create an else block, such as by

    qreg = QuantumRegister(2)
    creg = ClassicalRegister(2)
    qc = QuantumCircuit(qreg, creg)
    qc.h(0)
    qc.cx(0, 1)
    qc.measure(0, 0)
    with qc.if_test((creg, 0)) as else_:
        qc.x(1)
    with else_:
        qc.z(1)

The manager will ensure that the if and else bodies are defined over
the same set of resources (the user does not need to do this).

This commit also ensures that instances of ParameterExpression added
to a circuit inside all control flow instructions will correctly
propagate up to the top-level circuit.

Details and comments

The length of this PR is a little misleading. Of the nearly 4000 modifications, 2326 of them are adding new tests (these new tests take about ~1 sec to run in total), and much of the rest is comments and documentation. The actual implementation is about 600 logical lines of code.

The most important new objects added are:

• A new class, ControlFlowBuilderBlock, in qiskit/circuit/controlflow/builder.py, which is a lightweight scope manager, for tracking instructions and resources used inside a classical scope.
• Four new context managers, ForLoopContext, WhileLoopContext, IfContext and ElseContext, each in the same file as their associated Op version from Add Instruction subclasses for control flow. #7123. These are public in the sense that users will use the instances, but users should not instantiate them, nor need to call any of their methods.
• QuantumCircuit.if_test, .for_loop and .while_loop (but not .if_else) are overloaded so that if a body is not supplied, they return the context managers instead (ElseContext is only created when an IfContext object is entered).
• QuantumCircuit gained a private _control_flow_scopes per-instance stack (list), which is managed by the context managers, as C++-ish "friends" of QuantumCircuit.*

The builder interface roughly works as follows:

• On entry to a context manager, a scope is pushed onto the circuit stack. QuantumCircuit.append is updated to tail-call either QuantumCircuit._append or ControlFlowBuilderBlock.append as appropriate for the stack state.
• While in a context manager, the scope (ControlFlowBuilderBlock) tracks resources that are used by circuit methods (including QuantumCircuit.append) or InstructionSet.c_if (using the callback mechanism introduced in Fix resource resolution in InstructionSet.c_if #7255).
• On exit of a context manager, the builder block is built into a full QuantumCircuit instance, using all the qubits and clbits that were used during the block. The context manager then creates the ForLoopOp (or whatever), and appends it to the circuit. It should only block the resources that the block actually uses, and no more.
• break_loop, continue_loop, and any if_test blocks inside loops have unknown widths at the time they are appended. This is handled by a lightweight, private InstructionPlaceholder class which originally claims to use zero qubits and clbits. On ControlFlowBuilderBlock.build, these placeholder instructions are built into concrete versions, since at this point the width is known.
• The "if" and "else" context managers create a placeholder on exit if they are contained within a loop. This is in case they themselves contain a placeholder, in which case their width will need to expand as well.** This has some subtleties, and a large portion of the testing code is for edge cases around this that I found while writing the managers.

Further, the "else" context manager extends the "if" statement from a previous manager:

• The IfContext manager returns an ElseContext on entry, so the convention is

  with qc.if_test(cond) as else_:
      ...
  with else_:
      ...

• On entry, the "else" context verifies that the last instruction in the circuit (scope) was referentially the instruction it expects. In this case, it pops it off the circuit.
• In proceeds in the same manner as all the other context managers with the scoping.
• On exit, it adds bits to ensure that the true_body and the new false_body circuits have the same widths, taking care that the order of the bits is the same in both.

The context managers all check the exception status on exit, and make the circuit safe by popping off their incomplete states if the contexts are left via exception. The "else" manager re-instates the previous "if" instruction in this case, and resets itself so the user can attempt to re-enter it if required.

---

*: If absolutely desired, I can add a layer of indirection so that they are provided an "API" object which is populated by QuantumCircuit, to ensure that they can only perform the actions they need to. I thought this may make the code harder to read for now, though.

**: I elected not to put in logic to produce a concrete instance if they don't contain placeholders. IfContext could never return a concrete instance anyway (because on exit, it doesn't know what will happen in a possible ElseContext after it), and I don't think there's any benefit to this; it adds more complexity for no performance gain.

---

This also fixes #7280 - I found that issue while writing a test for the new interfaces, so I fixed it as part of this PR by making QuantumCircuit._update_parameter_table and QuantumCircuit._assign_parameters handle parameters that are themselves circuits. I didn't add a release note for this bugfix, because the bug was never released.

[coveralls]

Pull Request Test Coverage Report for Build 1531202224

• 432 of 448 (96.43%) changed or added relevant lines in 11 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.09%) to 83.06%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/circuit/controlflow/if_else.py	144	147	97.96%
qiskit/circuit/controlflow/builder.py	96	102	94.12%
qiskit/circuit/quantumcircuit.py	105	112	93.75%

Totals
Change from base Build 1530408033:	0.09%
Covered Lines:	51165
Relevant Lines:	61600

---

💛 - Coveralls

[kevinhartman]

Consider this just a first pass since there are some things here I'm not yet up to speed on. Specifically, I'm not sure I fully understand how the placeholder stuff works, or even what the semantics are of having additional instructions within a block following a break_loop or continue_loop. I would have expected that to be an error 😄.

Purely from an API perspective, I'm not sure I'm entirely sold on IfContext returning an ElseContext when entered. This seems somewhat unexpected to me on its own, but is a bit more uncomfortable IMO since it is expected to be used outside of the context that created it.

with qc.if_test(c) as else_:
    ...
with else_: # <-- uncomfortable
    ...

In most languages other than Python, I would expect else_ to be undefined here. In Python, I would expect accessing this object to be an error, since it would already be "closed", for some definition of closed.

As a user, I think I would be more comfortable with a DSL that worked like this:

with qc.if_test(c) as cond:
    with cond.body:
        ...
    with cond.else_body:
        ...

To improve readability, we could make the type of cond implement iterable, so we could do decomposition, or just make it a tuple or namedtuple:

with qc.if_test(c) as (if_, else_):
    with if_:
        ...
    with else_:
        ...

Perhaps the context managers of if_ and else_ here could be the same (e.g. BasicBlockContext) and the interesting stuff could all happen in the __exit__ of the root level IfElseContext. I'm not sure if that might simplify some of this code, but if it could, it might be especially nice to consider.

If the consensus is that nesting context managers like this would be a pain for users, I might still recommend that context managers returned by QuantumCircuit methods return a decomposable object when they're entered, since this would be more easily extensible, and feels less weird. I.e., the context manager returns some object that represents the IfContext when entered, and that thing provides a means to defining an else block, perhaps as well as some other things.

with qc.if_test(c) as (else_,):
    ...
with else_:
    ...

[jakelishman]

For the main comment: I'd agree that in a sanely scoped language else_ should be out-of-scope, but in Python it's a documented feature that's used in the standard library (for example the docs for unittest.assertWarns) and elsewhere (for example TensorFlow's GradientTape). I think for experienced Python developers, that's not unusual behaviour (though of course except Exception as e only defines e within the scope, because to hell with consistency, right), but it's also something we can use docs and tutorials to help with new users.

I'm not a huge fan of the "extra" context manager because it feels a bit more like a library convenience than a programmer one - it makes the circuit look as though you've entered two scopes to create an if, rather than just one, and it'd start to get very wordy if you needed to nest if branches.

For the thing about having the return value of if_test be something decomposable: that's already the case with the ElseContext object - we can attach arbitrary behaviour to it. You can enter the object directly because it's got an __enter__ on it, but you don't need to; it's also just a regular Python object. Originally I'd written that we might want to do "flat" elif with

with qc.if_test(...) as else_:
    ...
with else_.if_test(...) as else_:
    ...
with else_:
    ...

(the second as else_ could probably be omitted too) which would produce the same sort of thing as

with qc.if_test(...) as outer_else:
    ...
with outer_else:
    with qc.if_test(...) as inner_else:
        ...
    with inner_else:
        ...

which is along a similar vein. I think that the regular if/else construct is the majority of usage, though, and that we should optimise the syntax for that case first, but also open to other ideas.

[kevinhartman]

Overall, this LGTM. I have a few design-related suggestions for the internal implementation, but these wouldn't change the interface as you have it, so should you choose to investigate them, I believe it'd be fine to merge this PR as is for the release, and then do a follow-up PR with any corresponding refactorings.

I'm not a huge fan of the "extra" context manager because it feels a bit more like a library convenience than a programmer one.

Good point. If this pattern is used elsewhere and not out of place for Python programmers, I agree that it's better we go for whatever is easiest to use.

Design thoughts

From a design perspective, the current implementation is very flexible, but there are a lot of moving parts. Specifically, the separation between scopes and contexts. Currently, it seems like a concerted effort between construct-specific context classes and QuantumCircuit is necessary to manage scoping, which makes it difficult to understand control flow building without combing through a bunch of files. Correct me if I'm wrong, but it seems like there's a 1:1 relationship between entered contexts and scopes. I wonder if that means we can just use the scoping built into Python with statements, rather than keeping our own scope stack around.

With that in mind, I wonder if a more monolithic approach could work. Just spit-balling, but perhaps such a thing would look like:

• A single re-entrant context class (not in the threading sense), ControlFlowContext, which would be our monolith, tracking control flow state and pending instructions.
• To handle specific control flow operations, ControlFlowContext would use a state pattern, with state classes for each control flow instruction that share a common interface. This interface could define things like validations, how to determine the bits to use when built, what instruction to emit, what ControlFlowContext should return when entered, etc.
• QuantumCircuit would hold a single reference to ControlFlowContext (initialized with a special None state) and return it on calls to control flow instructions, after setting the corresponding state class on it.
• All instruction appends are routed through the ControlFlowContext, which calls the circuit append if in special None state, else appends to current scope.
• For an if_test, we'd still return the ControlFlowContext on entry. This would be the same handle you got by originally calling qc.if_test. The difference is the context would have advanced to the "else" state by the time you enter it again.

I'm sure this is overly-simplistic and would need more thought, but the main idea would be to 1) get rid of explicit scope tracking and just use the program stack and 2) encapsulate control flow building logic and state management into a single class so it's easily understandable and can be ripped out if we move away from this API in the future.

[jakelishman]

For the main design comment: if you wanted to, you could avoid a stack in this design in the same way. When you're using these interfaces, QuantumCircuit only needs to know the current object to send the output of append to, so instead of pushing and popping from a stack, the objects could equally just put a single reference to the current ControlFlowBuilderBlock in QuantumCircuit, stealing the existing object if one is already there. On exit, they just put the object back. That's the same way it'd be done in your form if you don't want an explicit stack, I think.

I chose to make it an explicit stack rather than a single object to make it clearer how the data structure works - I thought having the "steal and replace" pattern like that would actually be less clear, and it'd be much harder to debug what state you program is in in a crash, because all the state would be tied up in a bunch of different objects in a linked-list type structure, rather than a clear stack. It was also so that QuantumCircuit is more empowered to inspect partially-constructed scopes, which would be entirely opaque to it otherwise. I considered QuantumCircuit to be the arbiter on which of its methods should be allowed to be called at a given state during its construction, because I was worried that more of them would need to reject inputs if they were in partially-constructed loop scopes or things like that. In the end, that last worry turned out to be a bit misplaced (certainly for the time being).

I think perhaps the difference is that I considered QuantumCircuit to be in charge of things, which then manages a few helper classes to avoid it from becoming too large. You can absolutely quibble with this design. It meant that some amount of information has to pass between them. In yours, I think you're suggesting that the monolith is actually one point lower, at the level of data management - I originally avoided this because QuantumCircuit is still choosing between sending data to .data or .scopes, so I thought it wouldn't have been a clean split. I do buy some of your other reasons, though, which I hadn't thought of at the time. I think in your case you'll have less control-flow logic within QuantumCircuit, but you'll trade off by having more methods defined on your monolith; instead of QuantumCircuit.break_loop deciding what it's up to, it'd just be a thin wrapper around ControlFlowState.break_loop, so there's some duplication. I don't know where exactly, but I imagine there'll be cases we haven't thought of that would still need the two-way flow of information. It may well be less in your model, though.

I considered having IfContext and friends defined inside QuantumCircuit (which would avoid jumping between files), but the reason I didn't was because quantumcircuit.py is already 4,500 lines long, and getting longer. It's also easier to generate documentation for stuff in separate files - classes defined inside others can't be documented. I also thought that IfContext and friends were complex enough that using contextlib.contextmanager would be harder to read, especially with the error handling they do on exit.

[jakelishman]

Basically, I'm fine with your point 2, but I actively designed against your point 1 because I thought it'd make debugging and current-program-state inspection more difficult. We could swap to that form, but I'm not sure I see the benefit.

[kevinhartman]

LGTM. I'll add a follow-up comment shortly to continue our discussion 😄.

[kevinhartman]

That's the same way it'd be done in your form if you don't want an explicit stack, I think.

Yep, I think that's right. I agree that it could be helpful for debugging to keep track of the current context hierarchy as well. Thinking about it a bit more, I was more interested in envisioning a way we could manipulate scope tracking from a single place, which is more in line with point 2.

I considered having IfContext and friends defined inside QuantumCircuit

I think this is what I would naturally expect as well with QuantumCircuit being the arbiter, and since it's not the case, it means lots of friendly manipulation of QuantumCircuit by the context classes. This is what made me think to suggest defining some kind of manager class which instead owns all of context/scope tracking. In my proposal, this entity would both own the scope stack as well as perform manipulation of it, based on its current state and context enters/exits. I also figured why not just make this arbiter also the context manager that is entered and exited by the user for simplicity.

Sticking with the current design, each context class directly manipulates the scope stack, and needs to remember to call build when the context closes, and append to the real circuit. This is flexible, but it seems like there could be an abstraction built here to make these boilerplate things more automatic. Without changing the current design, I wonder if some sort of base class for the contexts could help. This would however still have the issue of friendly manipulation of the scope stack in QuantumCircuit since it'd still be the owner—it'd just be a bit more consolidated.

it'd just be a thin wrapper around ControlFlowState.break_loop, so there's some duplication.

I was envisioning handling this by using a state pattern, which (at least in my head) wouldn't add duplication. Maybe something like this from the perspective of QuantumCircuit:

class QuantumCircuit:
  def __init__():
    ...
    self.flow_context = FlowContext(append_to=self.data)
  ...
  def if_test():
    ...
    if true_body is None:
      # no need to specify anything about scoping here, i.e. in_loop,
      # since this would be managed internally by `flow_context`.
      self.flow_context.set_state(IfState(condition, label=label))
      return self.flow_context

I don't know where exactly, but I imagine there'll be cases we haven't thought of that would still need the two-way flow of information. It may well be less in your model, though.

Quite possibly! I'm really not sure. It's easy for me to comment from a safe distance on these things, but I'd need to put code to file to actually see what holds water. I'm making these suggestions only to give you things to consider from your vantage point as the implementer. The current design is fine with me if there don't seem to be any obvious benefits to changing it to you!

[jakelishman]

I think these are all good points - I also wasn't entirely satisfied with how much "friend" access I'd gave the context managers over QuantumCircuit. I mentioned up in the top comment I'd also considered making an "API" object where QuantumCircuit defines some callbacks that it gives to the contexts, so they're only allowed to take certain actions, and each can only be done in a certain order and a certain number of times, but I thought that would be too confusing.

I also was wary of messing too much with QuantumCircuit.data, particularly close to the release, because I think that would have a lot of knock-on effects.

I think we should keep talking about this, after the release as well, with a view to potentially changing some of the stuff to match what you're saying. I'd certainly be in favour of removing the friend access from the contexts, and if we can do the state machine in a neat way while still allowing convenient debugging and runtime inspection of the stack, I think I'd be in favour of it - it feels like the correct direction for managing control flow.

The duplication I had in mind was that I thought your monolith would basically end up with a break_loop operation of its own, which QuantumCircuit.break_loop() would call, so then we'd have a break_loop and continue_loop on both objects. You could also just have your appender inspect every object it's passed, I suppose, but I think of append as a lower-level "put exactly this object into the circuit", which is view I'd have to change. As a super minor point, I'm not a fan of constructing an object just to throw it away if we can avoid it (manual memory-management habits, lol).

[jakelishman]

Oh yeah, about the potential for a base class to handle build and append - I think this might actually add more complexity than it saves. It'd be fine for while, which is super simple, but the logic in if and else is rather more complicated around constructing the block, so it would need a lot of overriding, and for needs to decide whether to bind its parameter into the block after it's built but before it appends. I think the level of abstraction you'd need in the base class to represent all those different operations would be far more complex, especially since all it's really trying to "automate" is

body = block.build(qubits, clbits)
circuit.append(body, qubits, clbits)

[kevinhartman]

You could also just have your appender inspect every object it's passed [...]. As a super minor point, I'm not a fan of constructing an object just to throw it away if we can avoid it

Ah yeah, I suppose that would be necessary. I don't like constructing extra objects either, but it is nice in that we keep control-flow-based decisions out of QuantumCircuit instruction methods. Perhaps this would make it easier to turn control flow building support on and off through some kind of package import as well, but I'm not sure why that'd be useful, or if it'd be possible / good practice in Python.

I think this might actually add more complexity than it saves.

That's fair. I didn't have anything concrete in mind, but I was curious if there might be some way to pull context enter/exit stuff (i.e. scope stack manipulation, building, and instruction appending) up into a base class and then have data-oriented abstract methods for concrete classes to implement. It sounds like a clean separation might not exist, so let's just put a pin in it in case we come back to investigate the refactoring stuff from earlier in this thread.

Also, yay! Merged! :D