Skip to content
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

Disjoint handling in transpiler fails to rejoin barriers #11649

Closed
jakelishman opened this issue Jan 26, 2024 · 2 comments · Fixed by #11655
Closed

Disjoint handling in transpiler fails to rejoin barriers #11649

jakelishman opened this issue Jan 26, 2024 · 2 comments · Fixed by #11655
Assignees
Labels
bug Something isn't working mod: transpiler Issues and PRs related to Transpiler
Milestone

Comments

@jakelishman
Copy link
Member

Environment

  • Qiskit Terra version: main @ ce02f06
  • Python version: 3.10
  • Operating system: macOS

What is happening?

The preset passmangers for transpile are failing to correctly reconstruct barriers temporarily inserted during the compiler routing, which is leaking out invalid UUID objects inside the label field of those barriers.

How can we reproduce the issue?

Given a set up of:

from qiskit import transpile, QuantumCircuit
from qiskit.transpiler import CouplingMap

cm = CouplingMap([
    (0, 1),
    (1, 2),
    #
    (3, 4),
])
qc = QuantumCircuit(cm.size(), cm.size())
qc.cx(0, 1)
qc.cx(1, 2)
qc.cx(2, 0)
#
qc.cx(3, 4)
qc.measure(qc.qubits, qc.clbits)

out = transpile(qc, coupling_map=cm, seed_transpiler=0)

out contains Barrier instructions whose label attributes are not strings (they're UUID objects) and shouldn't exist at all, because the input circuit doesn't have any:

for i, ins in enumerate(out.data):
    if ins.operation.name == "barrier":
        print(i, repr(ins.operation.label))
        # Coerce to string so I can draw the circuit after.
        ins.operation.label = str(ins.operation.label)
8 UUID('31558989-c21a-4e5c-b014-01639295df7b')
10 UUID('31558989-c21a-4e5c-b014-01639295df7b')
12 UUID('31558989-c21a-4e5c-b014-01639295df7b')
14 UUID('31558989-c21a-4e5c-b014-01639295df7b')
15 UUID('31558989-c21a-4e5c-b014-01639295df7b')

I can't draw the circuit because the visualiser will explode, as will QPY (how this issue was detected). If I coerce those particular labels just to be string instances, the circuit looks like this:

                                                    ┌───┐                          ┌───┐                  31558989-c21a-4e5c-b014-01639295df7b ┌─┐
q_2 -> 0 ───────────────────────X───────────────────┤ H ├───────■──────────────────┤ H ├───────────────────────────────────░───────────────────┤M├───
                                │                   └───┘       │                  └───┘                                   ░                   └╥┘
                                │                   ┌───┐     ┌─┴─┐                ┌───┐                  31558989-c21a-4e5c-b014-01639295df7b  ║ ┌─┐
q_0 -> 1 ──■────────────────────X───────────────────┤ H ├──■──┤ X ├────────────────┤ H ├───────────────────────────────────░────────────────────╫─┤M├
           │                                        └───┘  │  └───┘                └───┘                                   ░                    ║ └╥┘
         ┌─┴─┐                ┌───┐                      ┌─┴─┐┌───┐ 31558989-c21a-4e5c-b014-01639295df7b                  ┌─┐                   ║  ║
q_1 -> 2 ┤ X ├────────────────┤ H ├──────────────────────┤ X ├┤ H ├──────────────────░────────────────────────────────────┤M├───────────────────╫──╫─
         └───┘                └───┘                      └───┘└───┘                  ░                                    └╥┘                   ║  ║
               31558989-c21a-4e5c-b014-01639295df7b  ┌─┐                                                                   ║                    ║  ║
q_3 -> 3 ──■────────────────────░────────────────────┤M├───────────────────────────────────────────────────────────────────╫────────────────────╫──╫─
           │                    ░                    └╥┘                                                                   ║                    ║  ║
         ┌─┴─┐ 31558989-c21a-4e5c-b014-01639295df7b   ║   ┌─┐                                                              ║                    ║  ║
q_4 -> 4 ┤ X ├──────────────────░─────────────────────╫───┤M├──────────────────────────────────────────────────────────────╫────────────────────╫──╫─
         └───┘                  ░                     ║   └╥┘                                                              ║                    ║  ║
    c: 5/═════════════════════════════════════════════╩════╩═══════════════════════════════════════════════════════════════╩════════════════════╩══╩═
                                                      3    4                                                               1                    0  2

so it's clear the temporary barrier we insert before measures isn't being recombined and then deleted.

What should happen?

  • The temporary barrier should have been recombined after the disjoint handling
  • The temporary barrier should have been removed before the circuit exited
  • We probably shouldn't use a literal UUID in the Barrier.label field since that's supposed to be constrained to be a string (everything except Barrier seems to enforce that, too).

Any suggestions?

No response

@jakelishman jakelishman added bug Something isn't working mod: transpiler Issues and PRs related to Transpiler labels Jan 26, 2024
@mtreinish mtreinish self-assigned this Jan 26, 2024
@mtreinish
Copy link
Member

So the root of the issue here is a shared state issue. When SabreLayout (or other layout passes too) calls qiskit.transpiler.passes.layout.disjoint_utils.run_pass_over_connected_components it passes the input dag directly and when there is >1 connected component the first thing as part of it does is split the barriers on the dag it receives in place to enable separating the circuit into it's connected components. This is modifying the input dag which when there is more > 1 connected component gets returned as is because we need to run routing separately. This is causing the barrier to leak out with the uuid label and that barrier isn't getting removed (from #10323) because the label changed.

There are two paths to fixing this, the first is just making a deepcopy of the dag so that there isn't a shared state when we split the barriers which will preserve the original input dag. The other option is to run disjoint_utils.combine_barriers(retain_uuid=False) before returning the input dag when there is more than one connected component to reverse the barrier splitting (this will also involve fixing the uuid piece to work with existing labels).

@jakelishman
Copy link
Member Author

My feeling is that running the barrier combiner with a limit to ensure it considers only the barriers split by the previous step is the cleaner approach, and the more reliable from a performance standpoint.

mtreinish added a commit to mtreinish/qiskit-core that referenced this issue 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
@1ucian0 1ucian0 added this to the 1.0.0 milestone Jan 27, 2024
github-merge-queue bot pushed a commit that referenced this issue 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
Labels
bug Something isn't working mod: transpiler Issues and PRs related to Transpiler
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants