Update transpiler pipeline to (only) use target internally

[ElePT]

Summary

This PR is a step towards the target-only transpiler pipeline. It manages to almost exclusively use the target input to communicate transpilation constraints. To do this, I designed a (very simple) "FakeTarget" (name tbd) designed for internal use that augments the Target data model supporting the use-case of coupling map provided with no basis gates, and modified the preset pass manager creation mechanisms to contemplate this new use-case.

The creation of FakeTarget is not documented as part the public API, as it's designed for internal use.

Details and comments

This PR addresses the following points from #9256:

• Update preset pass manager construction to only pass a Target to passes. Stage plugins initialization will need to fill out all the fields in the PassManagerConfig from the Target for backwards compatibility. We can stop doing this as we deprecate non-target constraint fields in PassManagerConfig.

• Deprecate anything we previously could represent that is lost in conversion to Target (or alternatively augment Target's data model to support these edge cases).

Note: There are 2 use cases where the Target is still skipped in the pass manager construction. Both are deprecated and will be removed in 2.0:

1. Whenever a custom basis gate is provided in basis_gates -> this use-case is now deprecated, the recommended alternative in these cases is to construct a Target user-side, which can be done through the Target.from_configuration method (there's a custom_name_mapping argument that allows to use custom gate definitions). Until the deprecation period is over, we still allow custom basis gates by skipping the target.

2. When instruction_durations are provided without a target or backend. The argument is deprecated, but we must support the use-case until the deprecation period is over.

[ElePT]

As of 9078df2, there are 347 failing tests that must be classified into one of these 2 use cases depending on the different input combinations:

1. User path we can support and must be updated to use target
2. User path we can't/ don't want to support and must be deprecated

More updates will follow.

======
Totals
======
Ran: 13302 tests in 180.7444 sec.
 - Passed: 12876
 - Skipped: 79
 - Expected Fail: 0
 - Unexpected Success: 0
 - Failed: 347
Sum of execute time for each test: 2002.1354 sec.

[ElePT]

Status after df78b1c:

======
Totals
======
Ran: 13562 tests in 234.3214 sec.
 - Passed: 13146
 - Skipped: 90
 - Expected Fail: 0
 - Unexpected Success: 0
 - Failed: 326
Sum of execute time for each test: 2640.2146 sec.

[ElePT]

Status after 8eea4cf

======
Totals
======
Ran: 16317 tests in 2518.9866 sec.
 - Passed: 16236
 - Skipped: 66
 - Expected Fail: 0
 - Unexpected Success: 0
 - Failed: 15
Sum of execute time for each test: 27668.6588 sec.

[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 13701403967

Details

• 71 of 80 (88.75%) changed or added relevant lines in 12 files are covered.
• 11 unchanged lines in 5 files lost coverage.
• Overall coverage increased (+0.01%) to 88.188%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/transpiler/preset_passmanagers/common.py	3	4	75.0%
qiskit/transpiler/preset_passmanagers/generate_preset_pass_manager.py	32	36	88.89%
qiskit/transpiler/target.py	16	20	80.0%

Files with Coverage Reduction	New Missed Lines	%
crates/accelerate/src/unitary_synthesis.rs	1	94.69%
qiskit/transpiler/preset_passmanagers/common.py	1	82.35%
qiskit/transpiler/passes/utils/gates_basis.py	2	88.24%
qiskit/transpiler/preset_passmanagers/builtin_plugins.py	2	94.65%
crates/qasm2/src/lex.rs	5	92.73%

Totals
Change from base Build 13698067235:	0.01%
Covered Lines:	71860
Relevant Lines:	81485

---

💛 - Coveralls

[eliarbel]

It seems to me that this PR is probably premature for 1.3. For one, consider the following difference compared to main:

qc = QuantumCircuit(2,2)
qc.cx(0,1)

pm = generate_preset_pass_manager(1, coupling_map=CouplingMap([[1,0]]))
tqc = pm.run(qc)
print(tqc)

The main branch gives this:

         ┌───┐┌───┐┌───┐
q_0 -> 0 ┤ H ├┤ X ├┤ H ├
         ├───┤└─┬─┘├───┤
q_1 -> 1 ┤ H ├──■──┤ H ├
         └───┘     └───┘
    c: 2/═══════════════

But on transpile-only-target-pipeline branch we get this:

q_0 -> 0 ──■──
         ┌─┴─┐
q_1 -> 1 ┤ X ├
         └───┘
    c: 2/═════ 

(GateDirection does not do what it's supposed to do here. See my line comments for more details).

The reason this is not caught by our unit testing is that we don't really have a way to directly compare transpilation results between versions AFAIK. Transpilation tests which do use pre-defined circuits as groundtruth (e.g. like in test_gate_direction.py) could have catch this, but I guess none really use generate_preset_pass_manager at all or
maybe use it but without just a coupling-map. I do see the FakeTarget instantiation branch in target.py reported as covered in the coverage report but probably not with those tests that compare output circuits and can flag a difference?

Anyway, once we have the above concern addressed, we should have explicit tests for the loose constraints and FakeTarget execution path to make sure all passes are included as before and we get the expected output circuits.

[ElePT]

Hi @eliarbel! Thanks for taking such a detailed look into the PR. I think you are raising some very good points. I definitely agree with avoiding the user exposure to the class, and I will work on an alternative not to generate the FakeTarget from Target.from_configuration. I also agree with the target is None issue, as well as the documentation, that should be easy to fix.

As for the implementation in Rust and use in GateDirection (and other passes), I wanted to share a few points:

1. This PR modifies the pass manager generation logic to skip passes that require basis gates, under the assumption that no basis gates means no synthesis/gate direction, etc. The current status in this branch is that, for the example you mentioned, the GateDirection pass is not run at all. Using a callback, this is how the pipeline looks like for the following branches (not saying this is correct, I am willing to revise it):

transpile-only-target-pipeline:
Passes: ['ContainsInstruction', 'UnitarySynthesis', 'HighLevelSynthesis', 'Unroll3qOrMore', 'InverseCancellation', 'SetLayout', 'TrivialLayout', 'CheckMap', 'FullAncillaAllocation', 'EnlargeWithAncilla', 'ApplyLayout', 'CheckMap', 'FilterOpNodes', 'HighLevelSynthesis', 'Depth', 'FixedPoint', 'Size', 'FixedPoint', 'InverseCancellation', 'GatesInBasis', 'HighLevelSynthesis', 'Depth', 'FixedPoint', 'Size', 'FixedPoint', 'ContainsInstruction']

Out:
q_0 -> 0 ──■──
         ┌─┴─┐
q_1 -> 1 ┤ X ├
         └───┘
    c: 2/═════

main:
Passes: ['ContainsInstruction', 'UnitarySynthesis', 'HighLevelSynthesis', 'Unroll3qOrMore', 'InverseCancellation', 'SetLayout', 'TrivialLayout', 'CheckMap', 'FullAncillaAllocation', 'EnlargeWithAncilla', 'ApplyLayout', 'CheckMap', 'FilterOpNodes', 'UnitarySynthesis', 'HighLevelSynthesis', 'BasisTranslator', 'CheckGateDirection', 'GateDirection', 'Depth', 'FixedPoint', 'Size', 'FixedPoint', 'Optimize1qGatesDecomposition', 'InverseCancellation', 'GatesInBasis', 'Depth', 'FixedPoint', 'Size', 'FixedPoint', 'ContainsInstruction']

Out:
         ┌───┐┌───┐┌───┐
q_0 -> 0 ┤ H ├┤ X ├┤ H ├
         ├───┤└─┬─┘├───┤
q_1 -> 1 ┤ H ├──■──┤ H ├
         └───┘     └───┘
    c: 2/═══════════════

2. So far, I thought I would be able to deal with this logic purely in Python space because, well, the coupling map doesn't exist in Rust, and that is all this FakeTarget contains (I acknowledge that the way it's constructed, being a subclass, this is not very well conveyed). The instruction_supported method, as you mentioned, is a bit of an exception, because there are truly no instructions in the fake target, so we have to come up with a convention. Our options here are to set it to True or False by default. And I believe that we can add a path in Rust space to support this use case where for an empty gate map, we return true or false. The issue that you pointed out is that some passes expect a true, while others expect a false. In the case of GateDirection with no basis gates, I am not sure what is the expected behavior. If we provide a coupling map exclusively, should we assume that all instructions in the circuit should follow the direction of the coupling map?

3. As for the addition of new unit tests for loose-constraints vs. target, I agree with further testing this case you brought up, and you are right in pointing out that we don't have enough sanity checks in the preset pm generation pipeline. However, this is also partially due to the large number of possible combinations that can be generated. I have a notable number of PRs dealing with target vs loose constraints and while things are a bit more standardized now, there has been historically very little understanding of the differences in behavior that come from different input combinations. In a nutshell, this could be a huge can of worms. I guess we now need to decide if we consider it important enough to block this PR until we have thorough tests everywhere, or whether we could find a middle ground where we add some key tests and follow up gradually with improvements in the testing base.

[eliarbel]

So first pass over the renewed PR surfaced this:

1. We need to remove the deprecated the instruction_durations and timing_constraints args from generate_preset_pass_manager. This is PR is a good place (really the last place :-) ) to do that.
2. Can we stop propagating coupling_map and basis_gates to PassManagerConfig already in this PR?

The first point is a blocker for this PR, the second one is a strong nice-to-have but I wouldn't consider as a blocker.

[eliarbel]

I went over the passes, thanks @ElePT for addressing the comments from my initial review!
I think it's a good start for the Target only flow that will facilitate cleaning the pass manager flows now, passing only target.

I've left suggestions and some questions in line.

[ElePT]

Thanks Eli! I still need to make sure tests pass (pretty sure there are still a few failures here and there after merging from main), and then I will make sure to apply your comments and update the reno with the input parameter removal from generate_preset_pass_manager.

[eliarbel]

This looks ready for me. Thanks @ElePT for the quick updates especially with the removals and 3Q gates and their related tests.

I've left a few tiny line comments for the docs; let's not block this PR over them but rather apply those in the final docs + reno updates before the final 2.0 release.

[eliarbel]

Suggested change

	(``basis_gates``, ``coupling_map``, or ``dt``).
	(``basis_gates``, ``coupling_map`` or ``dt``).

[eliarbel]

Suggested change

	``coupling_map``, and ``target``. If any of those other arguments
	``coupling_map`` and ``target``. If any of those other arguments

[eliarbel]

Suggested change

	" gates in the transpilation basis, use a custom `target` instance instead."
	" gates in the transpilation basis, use a custom `Target` instance instead."

[eliarbel]

That was a good catch! 👏

[eliarbel]

Suggested change

	In addition to this, the specification of custom basis gates through the ``basis`` gate argument of
	In addition to this, the specification of custom basis gates through the ``basis_gates`` argument of

[ElePT]

Sounds good! Thanks for your review. I can also open a follow-up PR for the docs changes during the rc period.