Port ApplyLayout to Rust

[jakelishman]

Summary

This adds and uses the Rust-space paths for ApplyLayout. The Rust interface is deliberately forwards looking; it uses the new Rust-space TranspileLayout and all the surrounding infrastructure to begin the process of transitioning our passes to move the TranspileLayout to an inherent field on DAGCircuit, and updated by all relevant passes in Qiskit directly, rather than the ad-hoc, frequently temporarily out-of-sync collection of properties in the PropertySet.

The Rust-space version of ApplyLayout is split into the two forms: "apply a layout for the first time" (apply_layout) and "improve an already set layout" (update_layout). apply_layout can also handle the allocation of explicit ancillas automatically. The Python-space version of the class is not yet upgraded to allow this functionality, but that can be done in a follow-up.

Details and comments

The actual mechanics of porting ApplyLayout are mostly straightforward. The reason this PR is tricky is because the Rust-space ApplyLayout interface is forward-facing to the next iteration of Qiskit's compilation tracking. In other words, it's based around the Rust-space TranspileLayout, and sets up and then uses the Python-space paths for gentle migration from the Python-space TranspileLayout to the Rust-space one within the transpiler. This means we have to deal with new pathways through the compositions of initial_layout, virtual_permutation_layout, and final_layout.

Close #12262

edit: currently on hold because it's based on #14826, which needs reviewing first.

[jakelishman]

This PR should be ready now, it's just stacked on top of #14826.

[qiskit-bot]

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

• @Qiskit/terra-core

[coveralls]

Pull Request Test Coverage Report for Build 17236197815

Warning: 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.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 224 of 281 (79.72%) changed or added relevant lines in 10 files are covered.
• 278 unchanged lines in 10 files lost coverage.
• Overall coverage decreased (-0.02%) to 88.405%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/dag_circuit.rs	23	24	95.83%
crates/circuit/src/nlayout.rs	6	7	85.71%
crates/transpiler/src/transpile_layout.rs	23	29	79.31%
crates/transpiler/src/passes/apply_layout.rs	122	171	71.35%

Files with Coverage Reduction	New Missed Lines	%
crates/circuit/src/parameter/symbol_expr.rs	1	72.82%
qiskit/circuit/library/hamiltonian_gate.py	1	82.35%
qiskit/visualization/circuit/_utils.py	1	95.56%
crates/qasm2/src/lex.rs	4	91.75%
crates/circuit/src/operations.rs	5	85.35%
qiskit/circuit/library/pauli_evolution.py	5	95.1%
crates/transpiler/src/passes/basis_translator/compose_transforms.rs	23	85.35%
crates/circuit/src/dag_circuit.rs	25	84.88%
crates/circuit/src/parameter/parameter_expression.rs	103	82.81%
crates/transpiler/src/passes/basis_translator/mod.rs	110	80.59%

Totals
Change from base Build 17163252070:	-0.02%
Covered Lines:	90119
Relevant Lines:	101939

---

💛 - Coveralls

[jakelishman]

The current failure on that Windows test seems to be a true failure; on my machine I can reproduce with:

from qiskit.circuit.library import quantum_volume
from qiskit import transpile
from qiskit.providers.fake_provider import GenericBackendV2
from qiskit.transpiler import passes

BOGOTA_CMAP = [[0, 1], [1, 0], [1, 2], [2, 1], [2, 3], [3, 2], [3, 4], [4, 3]]
qcomp = GenericBackendV2(
    num_qubits=5,
    coupling_map=BOGOTA_CMAP,
    basis_gates=["id", "u1", "u2", "u3", "cx"],
    seed=42,
)

def attempt(seed):
    qv_circuit = quantum_volume(3, seed=seed)
    gates_in_basis_true_count = 0
    consolidate_blocks_count = 0

    def counting_callback_func(pass_, property_set, **_):
        nonlocal gates_in_basis_true_count
        nonlocal consolidate_blocks_count
        if isinstance(pass_, passes.GatesInBasis) and property_set["all_gates_in_basis"]:
            gates_in_basis_true_count += 1
        elif isinstance(pass_, passes.ConsolidateBlocks):
            consolidate_blocks_count += 1

    transpile(
        qv_circuit,
        backend=qcomp,
        optimization_level=3,
        callback=counting_callback_func,
        translation_method="synthesis",
        seed_transpiler=seed,
    )
    assert gates_in_basis_true_count + 2 == consolidate_blocks_count

attempt(3)

so I'll look into it.

[jakelishman]

Ok, the bug is arguably in VF2Layout. If the incoming circuit has a registerless virtual qubit that has no operations on it, VF2Layout doesn't assign it to any particular physical qubit. We used to not notice that because we never asked the layout for all qubits - we only ever looked up the ones that have nodes on them.

This causes an error on main:

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

qc = QuantumCircuit([Qubit() for _ in range(3)])
qc.cx(0, 1)

transpile(qc, coupling_map=CouplingMap.from_line(3), basis_gates=["sx", "rz", "cx"]).layout.initial_index_layout()

because the initial_layout object has an "ancilla" in place of one of the proper virtual qubits.

[mtreinish]

Overall this is looking good, I like the new logical flow it's much clearer especially in the Python code. I just have few inline comments and questions. The biggest one is maybe just because I haven't looked at vf2post in rust yet, but I am a bit lost in the typing expectations for the rust space caller of update_layout(). Mechanically I also am wondering about the choice of assert_eq! in several places, the panic message for that is very testing focused from what I remember.

[mtreinish]

I thought you didn't want these conversions implemented to make it explicit when converting between Qubit types, or maybe I'm misremembering. It feels like a year ago we talked about this, but I can't find the PR discussion that I'm vaguely remembering. Maybe it was just the direct virt->phys and vice versa that we were talking about back then.

[jakelishman]

It's quite possible I did think that / make that argument in the past. If so, I changed my mind a bit - I think I softened a lot on the From trait. I still think that From<VirtualQubit> for PhysicalQubit would be a mistake, but From<Qubit> for VirtualQubit is now (imo) fine - you're just saying "here's a qubit, I'm saying it's virtual". If I argued before against it, I guess it was on the basis of wanting to be explicit about having to write VirtualQubit, but if so, I definitely softened on that on the bases of a) it ends up being a bit too noisy and b) having the function/use-site have a type is already explicit/type-checked enough to catch the bugs, I think.

[mtreinish]

This does change the exact register names from Python. While I agree this is a more foolproof implementation of what the intent was in FullAncillaAllocation in Python it just called the equivalent of:

format!("ancilla_{}", QuantumRegister::anonymous_instance_count().fetch_add(1, Ordering::Relaxed)

While this will always use the lowest i value for ancilla_{i} that doesn't conflict with another name, it feels like it would have more overhead than just using the total number of created registers in the program like what python was doing. I guess the difference in a c context is that we may not be bumping the instance count for owned registers, was that your concern here?

[jakelishman]

The Python-space version is bugged - there's nothing stopping you from defining ancilla1 yourself and then FullAncillaAllocation will happily just attempt to re-use it and explode:

from qiskit.circuit import QuantumCircuit, QuantumRegister
from qiskit.transpiler import CouplingMap, PassManager, passes

cm = CouplingMap.from_line(4)
qc = QuantumCircuit(QuantumRegister(1, "ancilla"), QuantumRegister(1, "ancilla0"))
PassManager([
    passes.TrivialLayout(cm),
    passes.FullAncillaAllocation(cm),
    passes.EnlargeWithAncilla(),
]).run(qc)

will raise a "duplicate register error" (if it's the first thing you run in your Python session).

[jakelishman]

To speak to efficiency - I put the fast path in because 99.99% of the time, we're going to be going down that route, and it just outputs the regular name. If not, we already have larger performance problems with having tonnes of registers (iirc, QuantumCircuit.add_register is quadratic in register count),

We can't even have a test asserting anything about this, because I didn't have to change the test suite.

[mtreinish]

Was this needed for the property set handling differences in the pass? If so is there a backwards compat concern here?

[jakelishman]

I don't think there's a concern. There's two parts to the change, both around the fact that the test as previously written used BasePass.__call__, which was relatively recently changed to go via PassManager.run.

Now:

1. since PassManager.run sets up original_qubit_indices itself on entry, the ApplyLayout pass was getting the wrong data. The fact that the pass's existing property set isn't completely clobbered on entry to a pass is already pretty weird - the correct way of feeding data through BasePass.__call__ has always been to pass a property_set argument, which existed even before we swapped over the mechanism. The test passes before because ApplyLayout didn't used to notice if original_qubit_indices was incorrect.
2. If I use PassManager.run (implicitly), then we're not directly testing the output of ApplyLayout, because there's also a layout-normalisation step during the pass manager finalisation. So this makes the test more true to its original intention.
   Since PassManager.run sets up original_qubit_indices itself now, and this pass was relying on historical behaviour that you could manually write to the property_set stored in the pass before execution and nothing would touch it. But the switch of BasePass.__call__ a couple of versions ago already changed that behaviour.

[alexanderivrii]

This PR is very-very-very-very nice. (And it also shows how much easier and error-prone it is to work with the new Rust-space TranspileLayout instead of the Python-space one).

[alexanderivrii]

The users would be able to hit this panic if they call ApplyLayout twice in a row, right? Should this be then a transpiler error of some kind instead of a panic?

[jakelishman]

A Rust-space user can, but a Python-space user shouldn't be able to because the Python-space entry point shouldn't be able to construct a Rust-space TranspileLayout object that contains a set initial_layout. I can add a test that it's safe from Python space, though - that'd be a good addition.

[jakelishman]

Actually, the only real test I can add of this would probably have to assert nonsense behaviour of the ApplyLayout pass; I'd have to attempt to to apply a layout to a circuit that already has one, and rely on the fact that we can't tell if a DAGCircuit had a layout applied yet. The test would become invalidated when we move TranspileLayout onto the DAGCircuit, because it'd stop spuriously succeeding and should begin to throw an exception.

[mtreinish]

LGTM, thanks for the updates.