Fix deadlock in Optimize1qGatesDecomposition

[mtreinish]

This commit fixes an oversight in the new multithreaded Optimize1qGatesDecomposition pass introduced in #15667 that could cause a deadlock with the Python GIL when using custom 1q gates. If there are custom gates defined in Python then we need Python to get the matrix of that custom gate. However, when running in a multithreaded context Python only allows one thread to interact with the interpreter at a time via the GIL. When operating in parallel the pass was not releasing the GIL from the parent thread running the pass. This meant that the children threads would wait to acquire the GIL to call Python but never be able to get the lock because the parent thread that spawned it never released the GIL.

This commit fixes that issue by adding a dedicate entrypoint for calling the pass via Python and having that entrypoint release the GIL prior to launching an parallel threads. It then reacquires the GIL before modifying the output DAG which might need Python for working with the custom gates. This mirrors the pattern used in the other parallel passes that work in a similar way such as UnitarySynthesis and TwoQubitPeepholeOptimization.

Fixes #16591

AI/LLM disclosure

• I didn't use LLM tooling, or only used it privately.
• I used the following tool to help write this PR description:
• I used the following tool to generate or modify code:

[qiskit-bot]

One or more of the following people are relevant to this code:

• @Qiskit/terra-core

[mtreinish]

I want to add another test that looks at doing a full path transpilation with each optimization level using a target involving only custom 1q and 2q gates. Just to make sure we have sufficient coverage of this path moving forward. I didn't have enough time to do it tonight, but if someone is feeling inspired they can feel free to push it up to my PR branch. Otherwise I'll circle back to it later.

[jakelishman]

Without having looked in any detail, a couple of questions:

1. For a backport, this looks like a very invasive change to a core pass. Is this smaller than it looks? Very approximately: is it possible to factor the process_run closure out in one commit, then do the other changes in a second, if not only for review ease to separate "code movement" from "code change" logic for the backport?
2. Is the backport going to be a nuisance, given this PR just rebased/merged in a non-backportable change?

[mtreinish]

Without having looked in any detail, a couple of questions:

1. For a backport, this _looks_ like a very invasive change to a core pass.  Is this smaller than it looks?  _Very_ approximately: is it possible to factor the `process_run` closure out in one commit, then do the other changes in a second, if not only for review ease to separate "code movement" from "code change" logic for the backport?

Functionally I'm not sure how reducible it will be. The crux of the issue is we need a Python-only path that is GIL aware and can manage the detach for only the parallel portion which requires some re-organization, both to explicitly split Python path from the non-Python path, and then also split the parallel and serial parts. I tried to minimize the code migration which resulted in a bit more duplication than is strictly necessary (thinking mostly the serial path).

That being said with ParameterVector in Rust now we might be able to get away without the re-attach for the serial mutation portion of the pass. In which case the change can be made smaller it'd be basically something like:

#[pyfunction]
pub fn py_run_pass(py, ...) {
   py.detach(|| {
       run_pass()
   })
}

pub fn run_pass() {...}

but given the panic bug we had in 2.4 I'm not 100% confident in doing that here, especially for a backport fix. That seems riskier to me, especially since we have places that can still do Py<_>::clone(), than the code churn this current form causes.

I can split this up into two PRs if you want, it realistically won't make the annoying to code movement part any easier to review. The second change that fixes the bug would be basically what I outlined above as the "minimal" riskier change once the reorg is done.

2. Is the backport going to be a nuisance, given this PR just rebased/merged in a non-backportable change?

I'm not worried about the backport mechanics. The rebase diff for that changes was just:

-     let sequence = unitary_to_gate_sequence_inner(
-         aview2(&operator),
-         target_basis_set,
-         qubit.index(),
-         None,
-         true,
-         None,
-     );
-     let Some(sequence) = sequence else {
+     let Some((sequence, new_error)) = resynthesize_run_min_error(aview2(&operator), target_basis_set, qubit, target) else {
 +        return Ok(None);
 +    };
-     let new_error = compute_error_from_target_one_qubit_sequence(&sequence, qubit, target);
 +

so reversing that it will apply cleanly on stable/2.5.

[jakelishman]

Oh yeah, I wasn't meaning we need 2 PRs necessarily. The point of splitting wasn't to make the code motion easier to review, it was to make the bit that actually changes the logic easier to review, because at the moment it's harder to pick that out of the diff. That's based on assumption that this was done by a simple move of the process_run closure out of the function with no changes, followed by a change in logic to add the two separate functions. If that's not the case, then the point's irrelevant.

For backport: that's good, thanks.

[mtreinish]

I split out the process_runs code move into #16595 as suggested. The diff for this PR becomes just this now: mtreinish/qiskit-core@split-out-process-runs...mtreinish:qiskit-core:fix-deadlock-o1qgd I've marked this as on hold until #16595 merges.

[mtreinish]

Now that #16595 has merged I've rebased this on main and removed the on hold label.