Add pass to filter ops and label inserted barriers #10323
Merged
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
|
One or more of the the following people are requested to review this:
|
This commit adds a new transpiler pass RemoveLabeledOps which is used to remove any op nodes that match a given label. This is paired with a new label option for BarrierBeforeFinalMeasurements. These are combined in the preset pass managers to ensure we're not always adding a barrier before output measurements in the output of the transpiler. Fixes Qiskit#10321
mtreinish
force-pushed
the
no-barrier-leakage
branch
from
ef070b1 to
caae56f
Compare
Pull Request Test Coverage Report for Build 5347967433
💛 - Coveralls |
jakelishman
reviewed
Jun 22, 2023
In the case when a label is set to trigger the removal of the labelled barrier the merge step would lose the context around which barrier instruction was transpiler inserted and which was user provided. To address this issue this commit skips the MergeAdjacentBarrier step so that the transpiler barrier is kept separate from any user inserted barriers which need to be preserved.
Pull Request Test Coverage Report for Build 6958650976
💛 - Coveralls |
This commit generalizes the new RemoveLabeledOps pass to be a generic node filtering pass that given a filter function it will remove any matching op nodes in the circuit.
mtreinish
changed the title
mtreinish
added
the
Changelog: New Feature
|
I've updated this PR to fix the bug with adjacent barriers and also generalized the filter pass to work for any filter function and not just those that match a given label. This should be ready for review now. |
mtreinish
changed the title
mtreinish
commented
Nov 22, 2023
jakelishman
previously approved these changes
Nov 22, 2023
There was a problem hiding this comment.
The new pass seems like a very useful utility pass for sure, and it definitely seems better for us to not leak out the internal detail of the barrier before the final measures.
Happy to approve/merge as-is, but I had a couple of questions that maybe want a change.
Comment on lines
254
to
260
| self.assertIsInstance(res, QuantumCircuit) | ||
| layout = res._layout.initial_layout | ||
| self.assertEqual( | ||
| [layout[q] for q in qc.qubits], [22, 7, 2, 12, 1, 5, 14, 4, 11, 0, 16, 15, 3, 10] | ||
| [layout[q] for q in qc.qubits], [10, 16, 8, 4, 11, 20, 15, 19, 18, 7, 12, 1, 2, 0] | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
Do we know why this changed? The actual barriers inserted before routing should have been the same, right, so it shouldn't have affected StochasticSwap? It's not really a problem, I just don't understand why it's changed.
There was a problem hiding this comment.
My assumption was because the input circuit has barriers in it that this was a side effect of not running merge adjacent barriers part of the barrier insertion. (this only changed with 29c1217 ). So, now we have multiple barriers followed by an all qubit barrier instead of it all getting merged into a single barrier. But I didn't look too closely at the internals of what was causing sabre to diverge internally.
This commit inverts the predicate usage to be consistent with Python's built in filter() function. Now if the predicate returns True the dag node is retained and if it returns false it is removed. This is also explicitly documented to make it clear how the pass is to be used.
jakelishman
approved these changes
Nov 22, 2023
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Nov 22, 2023
This commit updates a layout in the: `TestSabreLayout.test_layout_many_search_trials` test case which was recently changed in Qiskit#10323. The change in test ouput caused by Qiskit#10323 is what triggered the investigation into this bugfix and now that barriers are being treated correctly by sabre the layout doesn't change in the test case anymore and this is reverting the test assertion to use the original layout before Qiskit#10323 was merged.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Nov 23, 2023
* Avoid routing 2q barriers in SabreSwap This commit fixes an oversight in the sabre swap pass where a 2 qubit barrier would have been treated like a 2 qubit gate and swaps could potentially be inserted if the sabre algorithm thought it didn't have connectivity for the qargs to the barrier. However as barrier is just a compiler directive it is valid on all qubit pairs so this swap insertion was unnecessary, it was still valid but just not an optimal output. This commit fixes it by adding context to the sabre dag around whether a given node is a directive (and valid on all qubits) or not. * Update rust tests too * Skip directives in extended set generation too Co-authored-by: Kevin Hartman <[email protected]> * Add release note * Update test layout in test_layout_many_search_trials This commit updates a layout in the: `TestSabreLayout.test_layout_many_search_trials` test case which was recently changed in #10323. The change in test ouput caused by #10323 is what triggered the investigation into this bugfix and now that barriers are being treated correctly by sabre the layout doesn't change in the test case anymore and this is reverting the test assertion to use the original layout before #10323 was merged. --------- Co-authored-by: Kevin Hartman <[email protected]>
gluonhiggs
pushed a commit
to gluonhiggs/qiskit
that referenced
this pull request
Nov 23, 2023
* Avoid routing 2q barriers in SabreSwap This commit fixes an oversight in the sabre swap pass where a 2 qubit barrier would have been treated like a 2 qubit gate and swaps could potentially be inserted if the sabre algorithm thought it didn't have connectivity for the qargs to the barrier. However as barrier is just a compiler directive it is valid on all qubit pairs so this swap insertion was unnecessary, it was still valid but just not an optimal output. This commit fixes it by adding context to the sabre dag around whether a given node is a directive (and valid on all qubits) or not. * Update rust tests too * Skip directives in extended set generation too Co-authored-by: Kevin Hartman <[email protected]> * Add release note * Update test layout in test_layout_many_search_trials This commit updates a layout in the: `TestSabreLayout.test_layout_many_search_trials` test case which was recently changed in Qiskit#10323. The change in test ouput caused by Qiskit#10323 is what triggered the investigation into this bugfix and now that barriers are being treated correctly by sabre the layout doesn't change in the test case anymore and this is reverting the test assertion to use the original layout before Qiskit#10323 was merged. --------- Co-authored-by: Kevin Hartman <[email protected]>
FabianBrings
pushed a commit
to FabianBrings/qiskit
that referenced
this pull request
Nov 27, 2023
* Add pass to remove labeled ops and label inserted barriers This commit adds a new transpiler pass RemoveLabeledOps which is used to remove any op nodes that match a given label. This is paired with a new label option for BarrierBeforeFinalMeasurements. These are combined in the preset pass managers to ensure we're not always adding a barrier before output measurements in the output of the transpiler. Fixes Qiskit#10321 * Fix rebase issues * Handle pre-existing adjacent barriers In the case when a label is set to trigger the removal of the labelled barrier the merge step would lose the context around which barrier instruction was transpiler inserted and which was user provided. To address this issue this commit skips the MergeAdjacentBarrier step so that the transpiler barrier is kept separate from any user inserted barriers which need to be preserved. * Generalize RemoveLabeledOps to FilterOpNodes This commit generalizes the new RemoveLabeledOps pass to be a generic node filtering pass that given a filter function it will remove any matching op nodes in the circuit. * Add release note * Add tests for new pass * Add release note for new barrier pass kwarg * Fix type hint * Fix docs typo * Invert predicate usage This commit inverts the predicate usage to be consistent with Python's built in filter() function. Now if the predicate returns True the dag node is retained and if it returns false it is removed. This is also explicitly documented to make it clear how the pass is to be used. * Update pass manager drawer reference images
FabianBrings
pushed a commit
to FabianBrings/qiskit
that referenced
this pull request
Nov 27, 2023
* Avoid routing 2q barriers in SabreSwap This commit fixes an oversight in the sabre swap pass where a 2 qubit barrier would have been treated like a 2 qubit gate and swaps could potentially be inserted if the sabre algorithm thought it didn't have connectivity for the qargs to the barrier. However as barrier is just a compiler directive it is valid on all qubit pairs so this swap insertion was unnecessary, it was still valid but just not an optimal output. This commit fixes it by adding context to the sabre dag around whether a given node is a directive (and valid on all qubits) or not. * Update rust tests too * Skip directives in extended set generation too Co-authored-by: Kevin Hartman <[email protected]> * Add release note * Update test layout in test_layout_many_search_trials This commit updates a layout in the: `TestSabreLayout.test_layout_many_search_trials` test case which was recently changed in Qiskit#10323. The change in test ouput caused by Qiskit#10323 is what triggered the investigation into this bugfix and now that barriers are being treated correctly by sabre the layout doesn't change in the test case anymore and this is reverting the test assertion to use the original layout before Qiskit#10323 was merged. --------- Co-authored-by: Kevin Hartman <[email protected]>
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Jan 26, 2024
This commit fixes an issue in the disjoint layout processing caused by a shared reference from the input dag to a layout pass to the internal transformations the layout processing performs. As part of the disjoint layout processing the input dag needs to be split into smaller dags for each connected component in the circuit. To enable this any barriers present in the circuit need to be split up prior to analyzing the connected components in the dag. A multiqubit barrier doesn't represent a real connectivity requirement so they need to be split prior to analysis to ensure they don't factor into the connected component computation. To faciliate not losing the semantic meaning of the barrier for the layout analysis of each connected component, the barrier split is done by first taking an n-qubit barrier and converting it into n parallel 1q barriers with a uuid label applied to it. Then after we split the circuit into the separate connected components the uuid is used to identify any components of the same barrier and combine them together. There were two issues with this approach the first is that the splitting of the barriers was done on the input dag without copying first. This causes the input dag to be modified and because in most cases it's a shared instance which would leak out the barrier splitting if the input dag was returned verbatim from the pass, which in most cases it would be. This issue is fixed by ensuring that we re-combine any split barriers after we've split the dag into it's connected components. The second issue is the uuid label assignment would overwrite any existing labels on barriers. While setting labels on barriers is uncommon for users to do (but still completely valid) this is causing a bigger issues since Qiskit#10323 because the transpiler is assigning labels to barriers it creates internally so they can be removed before the circuit is returned. This is also fixed in this commit by appending a uuid to the existing label instead of overwriting it, so we're able to restore the original label when we recombine barriers. Fixes Qiskit#11649
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 30, 2024
* Fix split barriers leaking during disjoint layout processing This commit fixes an issue in the disjoint layout processing caused by a shared reference from the input dag to a layout pass to the internal transformations the layout processing performs. As part of the disjoint layout processing the input dag needs to be split into smaller dags for each connected component in the circuit. To enable this any barriers present in the circuit need to be split up prior to analyzing the connected components in the dag. A multiqubit barrier doesn't represent a real connectivity requirement so they need to be split prior to analysis to ensure they don't factor into the connected component computation. To faciliate not losing the semantic meaning of the barrier for the layout analysis of each connected component, the barrier split is done by first taking an n-qubit barrier and converting it into n parallel 1q barriers with a uuid label applied to it. Then after we split the circuit into the separate connected components the uuid is used to identify any components of the same barrier and combine them together. There were two issues with this approach the first is that the splitting of the barriers was done on the input dag without copying first. This causes the input dag to be modified and because in most cases it's a shared instance which would leak out the barrier splitting if the input dag was returned verbatim from the pass, which in most cases it would be. This issue is fixed by ensuring that we re-combine any split barriers after we've split the dag into it's connected components. The second issue is the uuid label assignment would overwrite any existing labels on barriers. While setting labels on barriers is uncommon for users to do (but still completely valid) this is causing a bigger issues since #10323 because the transpiler is assigning labels to barriers it creates internally so they can be removed before the circuit is returned. This is also fixed in this commit by appending a uuid to the existing label instead of overwriting it, so we're able to restore the original label when we recombine barriers. Fixes #11649 * Always use string uuid * Add release note
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 adds a new transpiler pass FilterOpNodes which is used to remove operations that match a given filter function. This is paired with a new label option for BarrierBeforeFinalMeasurements which assigns a label to the inserted barrier. These are combined in the preset pass managers to ensure we're not always adding a barrier before output measurements in the output of the transpiler.
Details and comments
Fixes #10321
Fixes #11093
TODO:
.measure_all()case) and likely fix