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

generate_preset_pass_manager generates transpiler error on efficientSU2 at 12 qubits #11784

Closed
nonhermitian opened this issue Feb 13, 2024 · 10 comments · Fixed by #11787
Closed
Labels
bug Something isn't working
Milestone

Comments

@nonhermitian
Copy link
Contributor

nonhermitian commented Feb 13, 2024

Environment

  • Qiskit version: 1.0.0rc1 or 0.45.3
  • Python version: 3.12
  • Operating system: Linux

What is happening?

Running benchmarks that were presented at Summit now error when using generate_preset_pass_manager.
It fails at 12 qubits with the error:

TranspilerError: 'Fewer qubits in the circuit (12) than the coupling map (127). Have you run a layout pass and then expanded your DAG with ancillas? See FullAncillaAllocation, EnlargeWithAncilla and ApplyLayout.'

Oddly enough it works for smaller numbers .

How can we reproduce the issue?

service = QiskitRuntimeService()
backend = service.get_backend('ibm_kyiv')

target = backend.target
pm = generate_preset_pass_manager(target=target, optimization_level=3)
    
qubits = list(range(2, 128))

qiskit_times = []
qiskit_depth = []
qiskit_2q = []

for N in qubits:
    print('Starting', N, 'qubits')
    qc = EfficientSU2(N, entanglement='circular')
    print('circuit building done')
    st = time.perf_counter()
    trans_qc = pm.run(qc)
    #trans_qc = transpile(qc, backend, optimization_level=3)
    fin = time.perf_counter()
    qiskit_times.append(fin-st)
    qiskit_depth.append(trans_qc.depth())
    qiskit_2q.append(trans_qc.count_ops().get('cx', 0))
    print('Qiskit', fin-st, qiskit_2q[-1])
    print()

What should happen?

It should work like it did in previous versions

Any suggestions?

No response

@nonhermitian nonhermitian added the bug Something isn't working label Feb 13, 2024
@jakelishman
Copy link
Member

Did this work with a previous version of Qiskit with generate_preset_pass_manager? If so, which version?

I thought we had an open issue about unifying the behaviour of transpile and generate_preset_pass_manager, but apparently not. There's a lot of rough edges right now, most of which were somewhat necessary for internal use when transpile broadcast its arguments, but since that was removed, now it's (imo) just an interface bug when the two don't align, like here.

@mtreinish
Copy link
Member

This isn't an interface bug about the people running with backends/targets and basis_gates. I expect this is a reused state bug, looking at this locally it runs the loop ~10 times and then fails when N == 12 because apparently the embedding step is not run in the pass manager. My assumption is that because the property set is getting reused between executions of pm some state is skipping apply layout. My assumption is it's because of N<12 having a vf2layout match and when we hit 12 there isn't one anymore it's triggering sabreswap to run and it was skipping the appropriate layout steps. But I need to confirm

Either way if my guess is correct, then the correct thing to do is clear out the property set when run() is called.

@mtreinish
Copy link
Member

I also can confirm that this exact script didn't fail in Qiskit 0.44.3. But it does fail with 1.0.0rc1

@jakelishman
Copy link
Member

Yeah, I wasn't thinking it was the same as Target/basis_gates - I was thinking it was going to be more related to discussions that have happened offline where we realised that generate_preset_pass_manager handles scheduling_method and timing_constraints differently to transpile.

Ah yeah, that does make sense, and it could easily explain it if the bug appeared in 0.45 - that's when the pass manager handling changed, and the way the task iterator is generated changed between those versions. The problem is that PassManager generates its property_set on __init__ (like RunningPassManager used to), not in _run_workflow, which is where it should do it do be consistent with the old form. Should be a straightforwards bugfix and backportable to 0.46.

@jakelishman jakelishman modified the milestones: 1.0.0, 0.46.1 Feb 13, 2024
@nonhermitian
Copy link
Contributor Author

Yeah, I was using whatever was available before summit. I think the RC of 0.45

@jakelishman
Copy link
Member

jakelishman commented Feb 13, 2024

This issue actually pops up an interesting question to me: PassManager.property_set has always fundamentally meant PassManager.run can't be re-entrant, which isn't naively isn't much of a problem because we wouldn't expect people to use threaded parallelism over it for all the standard Python reasons. However, for the same reason, the parallel-dispatch form of PassManager.run currently has no way of returning the property sets from the separate invocations, and there have been use-cases where we want to access analysis from within the transpilation after the run (I think some IBM-internal projects would like this, in particular).

Perhaps we ought to consider an expansion of the interface that allows returning both the final programs and the workflow states at some point? That would be a separate issue to this, of course.

@jakelishman
Copy link
Member

Also, presumably the reason that things fail at 12 qubits in this particular set up is because Kyiv is heavy-hex, the input circuit is a ring graph, and ring(12) is the first time that the input coupling dependency is and induced subgraph isomorphism of the Kyiv coupling constraints, so we take a different path through the layout stage (VF2Layout rather than SabreLayout), and that has an effect on the rest.

That's unrelated to the actual bug reported here, though it might be interesting to see why the embedding is failing if VF2Layout succeeds on re-runs when everything's fine if it re-uses Sabre. Possibly VF2Layout or some other component of our preset pipelines is more fragile in a way that could be a bug waiting to happen in a different form.

@mtreinish
Copy link
Member

My suspicion (although I haven't had time to confirm it yet) is that it's actually an interaction of VF2PostLayout and VF2Layout when apply_layout gets called it will have two layouts one for 12 qubits from vf2layout and one from vf2postlayout in the previous run and that is confusing things. The conditional logic around how we choose to skip passes is pretty intricate and makes a lot of assumptions about the workflow based on what gets left in the property set, so I could see being really fragile if you have a mix of leftover pieces in the property set from the previous runs

@jakelishman
Copy link
Member

As long as it's all about the PropertySet we're all good - that's state that should be explicitly be refreshed between full pass-manager invocations. My worry is that there's places where we have individual passes that store internal state within themselves that they don't refresh on calls to run, and they cause problems on re-runs. Hopefully it's just all in the PropertySet, though - that's what it should be.

@mtreinish
Copy link
Member

As a short term workaround for anyone hitting something like this until #11787 is merged and released you can modify the example code to do something like:

import time
from qiskit.transpiler.preset_passmanagers import generate_preset_pass_manager
from qiskit.circuit.library import EfficientSU2
from qiskit_ibm_runtime import QiskitRuntimeService
from qiskit.passmanager import PropertySet

service = QiskitRuntimeService()
backend = service.get_backend('ibm_kyiv')

target = backend.target
pm = generate_preset_pass_manager(target=target, optimization_level=3)
    
qubits = list(range(2, 128))

qiskit_times = []
qiskit_depth = []
qiskit_2q = []

for N in qubits:
    print('Starting', N, 'qubits')
    qc = EfficientSU2(N, entanglement='circular')
    print('circuit building done')
    st = time.perf_counter()
    trans_qc = pm.run(qc)
    fin = time.perf_counter()
    pm.property_set = PropertySet()
    qiskit_times.append(fin-st)
    qiskit_depth.append(trans_qc.depth())
    qiskit_2q.append(trans_qc.count_ops().get('cx', 0))
    print('Qiskit', fin-st, qiskit_2q[-1])
    print()

Running pm.property_set = PropertySet() will clear out the property set after running the pass manager so it's not getting reused between runs anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants