Add interoperation between TranspileLayout and PropertySet#14826
Conversation
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 17153389326Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
This adds a constructor and a "destructor" to `TranspileLayout` to cleanly centralise how the current ad-hoc `PropertySet` properties are combined into a `TranspileLayout`, and lets them be written back out again. This lets passes in the middle of an execution pipeline construct a complete `TranspileLayout` structure, based on the current state. This is a step along the road to centralising `TranspileLayout` into a core part of the IR; we can now add methods to it, write transpiler passes against `TranspileLayout`, and start transitioning the logic, without breaking backwards compatibility. The idea is that passes will opt in to creating a `TranspileLayout`, mutate it, then write it back out again. In the future, we can have the pass manager itself manage legacy passes by extracting the `TranspileLayout` into the `PropertySet` before executing a legacy pass.
f3f7d63 to
66197c7
Compare
| # Throughout the rest of this, we will speak about index permutations as lists that mean: | ||
| # | ||
| # qubit `permutation[i]` goes to new index `i` | ||
| # | ||
| # or in alternative langauge, | ||
| # | ||
| # after the permutation, qubit `i` contains qubit `permutation[i]`. | ||
| # | ||
| # This is to match the convention that `PermutationGate` uses, but beware: it might not be | ||
| # the way you think about permutations (it's not my preferred convention---Jake). | ||
| # | ||
| # Now, we'll step through the tranpsilation process. At each point, we'll relate the | ||
| # objects we have back to a 3-tuple of abstract objects, which are applied in order: | ||
| # | ||
| # (relabelling, explicit instructions, implicit instructions) | ||
| # | ||
| # The "explicit instructions" are always just the DAG itself. The "relabelling" is | ||
| # generally associated with the "initial layout" and the metadata linking the original | ||
| # virtual qubit objects and their indices. The "implicit instructions" is where all the | ||
| # interesting stuff happens; at the moment, in Qiskit, we only track an implicit final | ||
| # permutation, though you could imagine a world where we allow a lot more things to be | ||
| # tracked, such as necessary classical post-processing steps. | ||
| # | ||
| # We will attempt to always have in hand an "undoing" permutation, such that doing | ||
| # `qc.append(PermutationGate(permutation), qc.qubits)` to the output of the transpiler, were | ||
| # it to terminate at any given step, would be enough to precisely recreate the unitary of | ||
| # the circuit (with due hand-waviness around measurements/resets), up to the qubit | ||
| # relabelling of the initial layout. |
There was a problem hiding this comment.
This discussion probably actually wants to get moved into proper documentation in qiskit.transpiler, and the formalism completed. Creating this documentation and the pedagogy of the rest of this function was actually one of the main points of the PR - it went hand-in-hand with creating the method itself, since I wanted to use both in the new Rust-y ApplyLayout, but I couldn't understand what everything meant.
There was a problem hiding this comment.
I find the above paragraph confusing. When you say "to the output of the transpiler", do you mean the circuit returned by the transpiler (without the output permutation, which is implicit), or the circuit with the output permutation (as above you argue that the transpiler really returns a triple that includes this output permutation as part of the implicit stuff)?
I presume you mean the circuit without the output permutation, and, under the assumption that the initial layout is trivial, appending the PermutationGate(permutation) gives us exactly the unitary operator of the original circuit (assuming it only consists of unitary operations). However I don't understand why this is called "undoing" rather than "doing". The way I think about this say after the ElidePermutationPass, is that we have moved the explicit permutation gate at the end of the circuit to an implicit object, so to get the original operator we need to add it back (and not to undo it again).
I am fine keeping the word "undo", but could you please rephrase the paragraph to avoid any potential confusion?
There was a problem hiding this comment.
I did a bit more of a rewrite in 150d4a7 - hopefully that's a bit clearer.
I presume you mean the circuit without the output permutation, and, under the assumption that the initial layout is trivial, appending the PermutationGate(permutation) gives us exactly the unitary operator of the original circuit.
Yes, but also in my world, you still do qc.append(PermutationGate(permutation), qc.qubits) first if you do have an initial layout, while the qubits are still the relabelled physical ones, and then you undo the initial-layout relabelling to get back to the original unitary matrix. The reason for it is that things that modify the final permutation shouldn't ever need to care what the original relabelling was (and they don't, in any current Qiskit pass). In my 3-tuple form, they only deal with the (_, explicit, implicit) part, not the first (relabelling, _, _) bit, which means there's less to sync up.
However I don't understand why this is called "undoing" rather than "doing". The way I think about this say after the ElidePermutationPass, is that we have moved the explicit permutation gate at the end of the circuit to an implicit object, so to get the original operator we need to add it back (and not to undo it again).
ElidePermutations is one of the few that reduces the number of swaps/permutations in the circuit explicitly at the moment. Imo most things (like routing passes, which almost always run) add swaps in that you'd need to "remove" to get back, which is why I used "undoing". To me, you "undo" the effects of an elision by re-adding them whereas "doing" them again wouldn't imply an inverse. But I tried to avoid using "undo" without greater context in the rewrite to help out.
|
Thanks Jake for starting this. The following message is more of a high-level discussion/questions rather than a review. First of all, I very-very much like the long-term vision described in this PR's summary. This is how I understand it (so please tell me if there are discrepancies between this and how you view it). Each of the transpiler passes takes a One immediate question: this PR adds the interop between PropertySet and python-space TranspileLayout, while #14778 aims to define a Rust-space TranspileLayout. Which of these two PRs should be merged first? (Thinking through the interop here may have consequences as to how the rust-space TranspileLayout will eventually look like). Currently, this PR replaces the Currently this is done at the very end of the run and in particular canonicalizes away the horrible "virtual_permutation_layout" that we have introduced in the past, and from what I can judge this should work correctly. In addition, based on the release notes it seems that converting the property set into a transpile layout and back should also work after every pass in the transpilation pipeline, but I am confused as whether this already happens. I do want this to be the case after this PR is merged. There are two things that confuse me.
|
Yeah, this is correct with some subtlety: the Then we have to choose the concrete parts. Obviously we have the DAG for "explicit". We currently have a In my abstract model, we can choose to represent the 3 items however we like. The actual choice of representation in any version of Qiskit is making a choice of the user API we offer, the backwards compatibility from the previous version, and the expressibility of the compiler. We don't have to keep The "implicit" bit at the end is perhaps the most subtle. We could store
Matt and I (unfortunately) don't think we can merge the Python and Rust-space
We can't really break backwards compatibility of this object, so for Qiskit 2.x, we're kind of stuck with the current state of affairs: if
Once we transition to |
|
The short version of it: This PR isn't intended to change any behaviour in Qiskit 2.x. It's just laying the groundwork of the conversion functions that will be necessary in the upgrade path to Qiskit 3.x. |
mtreinish
left a comment
There was a problem hiding this comment.
Overall this LGTM, it's really nice to have a clear description of all the moving pieces here and how it all fits together. I agree with your inline comment we should bubble this up to the transpile module docs to make it clear for people (especially as we now have docs for pass authors) I just had a few small questions inline but otherwise I think I"m happy to merge this.
alexanderivrii
left a comment
There was a problem hiding this comment.
This looks good to me as well. Thanks for adding the detailed description of what each field in the property set means, our assumptions about them, and how they interact which each other. I have asked to rephrase one of the paragraphs, since I believe there might be still room for confusion left.
One other question. This PR allows to "canonicalize" the property set by calling from_property_set followed by write_into_property_set. In practice, this PR calls these once at the very end of transpilation instead of _finalize_layouts, where I believe everything works perfectly. Hence I am happy to merge this as is. However, is it true that we can currently "canonicalize" the property set after every transpiler pass and we would still get correct results?
I am afraid this might not be true (for instance, would this work right after ElidePermutations or after one of the analysis passes the compute but do not apply the layout?), I would be happy to be wrong, but if not - can you please add an explicit warning to this extent?
| # Throughout the rest of this, we will speak about index permutations as lists that mean: | ||
| # | ||
| # qubit `permutation[i]` goes to new index `i` | ||
| # | ||
| # or in alternative langauge, | ||
| # | ||
| # after the permutation, qubit `i` contains qubit `permutation[i]`. | ||
| # | ||
| # This is to match the convention that `PermutationGate` uses, but beware: it might not be | ||
| # the way you think about permutations (it's not my preferred convention---Jake). | ||
| # | ||
| # Now, we'll step through the tranpsilation process. At each point, we'll relate the | ||
| # objects we have back to a 3-tuple of abstract objects, which are applied in order: | ||
| # | ||
| # (relabelling, explicit instructions, implicit instructions) | ||
| # | ||
| # The "explicit instructions" are always just the DAG itself. The "relabelling" is | ||
| # generally associated with the "initial layout" and the metadata linking the original | ||
| # virtual qubit objects and their indices. The "implicit instructions" is where all the | ||
| # interesting stuff happens; at the moment, in Qiskit, we only track an implicit final | ||
| # permutation, though you could imagine a world where we allow a lot more things to be | ||
| # tracked, such as necessary classical post-processing steps. | ||
| # | ||
| # We will attempt to always have in hand an "undoing" permutation, such that doing | ||
| # `qc.append(PermutationGate(permutation), qc.qubits)` to the output of the transpiler, were | ||
| # it to terminate at any given step, would be enough to precisely recreate the unitary of | ||
| # the circuit (with due hand-waviness around measurements/resets), up to the qubit | ||
| # relabelling of the initial layout. |
There was a problem hiding this comment.
I find the above paragraph confusing. When you say "to the output of the transpiler", do you mean the circuit returned by the transpiler (without the output permutation, which is implicit), or the circuit with the output permutation (as above you argue that the transpiler really returns a triple that includes this output permutation as part of the implicit stuff)?
I presume you mean the circuit without the output permutation, and, under the assumption that the initial layout is trivial, appending the PermutationGate(permutation) gives us exactly the unitary operator of the original circuit (assuming it only consists of unitary operations). However I don't understand why this is called "undoing" rather than "doing". The way I think about this say after the ElidePermutationPass, is that we have moved the explicit permutation gate at the end of the circuit to an implicit object, so to get the original operator we need to add it back (and not to undo it again).
I am fine keeping the word "undo", but could you please rephrase the paragraph to avoid any potential confusion?
|
After some experimenting, I can indeed see that "canonicalizing" the transpile layout right after the elide permutations pass leads to incorrect results. This is expected (due to the deficiency of our python-based transpile layout class) and the reason behind introducing the (arguably very ugly) |
|
Sasha: can you give a code example showing that this is wrong after |
|
Jake: The following code (with your PR applied) prints Adding to the |
|
Have you got #14919 included in your branch when you try that? |
|
Not explicitly (let me try). Update: yeah unfortunately the problem persists even with #14919 applied (I was pretty sure this would happen, but wanted to double-check). |
|
Thanks, I'll look into it - it might be because of the effective double-set of the initial layout (due to how we have to handle it with |
|
As to Sasha's comment above about the potential bug (#14826 (comment)): actually this is because our logic in the default layout plugin construction is pretty incompatible with There is a different problem with using the |
If a virtual-permutation setting pass uses `TranspileLayout.write_into_property_set` to normalise its virtual permutation into a final layout, we would have subsequently failed to detect the permutation if the initial layout was (validly) reset to `None`. This corrects the logic.
|
Ok, so to speak to Sasha's bug, there's about 3 things going on:
With all three things fixed, Sasha's example with the modified I think there'll be a tail of issues to work through as we transition the transpiler pipeline over to using |
mtreinish
left a comment
There was a problem hiding this comment.
This LGTM now, thanks for the updates.
…t#14826) * Add interoperation between `TranspileLayout` and `PropertySet` This adds a constructor and a "destructor" to `TranspileLayout` to cleanly centralise how the current ad-hoc `PropertySet` properties are combined into a `TranspileLayout`, and lets them be written back out again. This lets passes in the middle of an execution pipeline construct a complete `TranspileLayout` structure, based on the current state. This is a step along the road to centralising `TranspileLayout` into a core part of the IR; we can now add methods to it, write transpiler passes against `TranspileLayout`, and start transitioning the logic, without breaking backwards compatibility. The idea is that passes will opt in to creating a `TranspileLayout`, mutate it, then write it back out again. In the future, we can have the pass manager itself manage legacy passes by extracting the `TranspileLayout` into the `PropertySet` before executing a legacy pass. * Remove unused import * Be more commital about 3.0 * Make comment more explicit about state * Handle case of `final_layout` without `layout` If a virtual-permutation setting pass uses `TranspileLayout.write_into_property_set` to normalise its virtual permutation into a final layout, we would have subsequently failed to detect the permutation if the initial layout was (validly) reset to `None`. This corrects the logic.
…t#14826) * Add interoperation between `TranspileLayout` and `PropertySet` This adds a constructor and a "destructor" to `TranspileLayout` to cleanly centralise how the current ad-hoc `PropertySet` properties are combined into a `TranspileLayout`, and lets them be written back out again. This lets passes in the middle of an execution pipeline construct a complete `TranspileLayout` structure, based on the current state. This is a step along the road to centralising `TranspileLayout` into a core part of the IR; we can now add methods to it, write transpiler passes against `TranspileLayout`, and start transitioning the logic, without breaking backwards compatibility. The idea is that passes will opt in to creating a `TranspileLayout`, mutate it, then write it back out again. In the future, we can have the pass manager itself manage legacy passes by extracting the `TranspileLayout` into the `PropertySet` before executing a legacy pass. * Remove unused import * Be more commital about 3.0 * Make comment more explicit about state * Handle case of `final_layout` without `layout` If a virtual-permutation setting pass uses `TranspileLayout.write_into_property_set` to normalise its virtual permutation into a final layout, we would have subsequently failed to detect the permutation if the initial layout was (validly) reset to `None`. This corrects the logic.
Summary
This adds a constructor and a "destructor" to
TranspileLayoutto cleanly centralise how the current ad-hocPropertySetproperties are combined into aTranspileLayout, and lets them be written back out again. This lets passes in the middle of an execution pipeline construct a completeTranspileLayoutstructure, based on the current state.This is a step along the road to centralising
TranspileLayoutinto a core part of the IR; we can now add methods to it, write transpiler passes againstTranspileLayout, and start transitioning the logic, without breaking backwards compatibility. The idea is that passes will opt in to creating aTranspileLayout, mutate it, then write it back out again. In the future, we can have the pass manager itself manage legacy passes by extracting theTranspileLayoutinto thePropertySetbefore executing a legacy pass.Details and comments
Based on #14825.