Update transpiler pipeline to only use target

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

Details

• 66 of 68 (97.06%) changed or added relevant lines in 9 files are covered.
• 7 unchanged lines in 3 files lost coverage.
• Overall coverage increased (+0.003%) to 88.913%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/transpiler/target.py	18	20	90.0%

Files with Coverage Reduction	New Missed Lines	%
qiskit/transpiler/preset_passmanagers/builtin_plugins.py	1	95.56%
crates/accelerate/src/basis/basis_translator/mod.rs	1	87.29%
crates/qasm2/src/lex.rs	5	91.48%

Totals
Change from base Build 12771449472:	0.003%
Covered Lines:	79475
Relevant Lines:	89385

---

💛 - 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.

[eliarbel]

Maybe _PartialTarget is a better name? Also I would add to its docstring that this is an internal-only class and should not be used directly by the user. Though it's not in the API doc but still it wouldn't hurt to highlight.

[eliarbel]

When checking a FakeTarget instance in a boolean context, e.g. like:

target = Target.from_configuration(basis_gates=None, coupling_map=CouplingMap([[0,1]]))
if target:
    print("I'm alive")

The evaluation fails.

This would be problematic for all the passes that can work with partial target and do a check like self.target. Overriding __len__ can mitigate this.
Also in the level_x.. pass managers (e.g. level_1_pass_managers) functions, there are checks like this:

if (coupling_map and not coupling_map.is_symmetric) or (
        target is not None and target.get_non_global_operation_names(strict_direction=True)
    ):

target is not None will also always evaluate to False with FakeTarget.

[ElePT]

target is not None does evaluate to True because it's an explicit check for None, so at least we can confirm that the pass manager generation logic was working as expected (else it would have been a bigger issue). However, it's true that the output of __len__ is 0. I am wondering if it might make more sense not making FakeTarget a subclass of Target because it inherits all of these method that make little sense for the object we have.

[eliarbel]

You're right, if target is None is checking the reference itself. But if target: is the boolean context where __len__ is used. There are a couple places in the codebase in which this pattern in used so it's still a potential issue.

Regarding whether or not to inherit from Target - Ideally the best way going forward might be to support partial initialization of Target itself. So, for example, in the case of calling Target.from_configuration with only a coupling map, internally Target will end up containing dummy instructions that will make constructing a coupling map and other functionality like instruction_supported work as expected. This will probably require changing the Target internals. Or maybe FakeTarget can use Target.add_instruction using an internal only instruction which might require changes also on the Rust side.
@raynelfss

Still I think we'll want to have a subclass like FakeTarget or some other special factory anyway as a way to ensure partial target initialization is only supported for internal code callers.

[eliarbel]

Target.instruction_supported is implemented in Rust and used in gates_in_bases.rs and gate_direction.rs. If these get a FakeTarget from Python how would this overriden method is used?

Additionally, and even more important, returning True here by default is not a good solution for the GateDirection pass. We need to think about a way to solve this.

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

[ElePT]

After an offline discussion, we have concluded that:

1. We know how to fix the particular example raised by @eliarbel (the use of GateDirection and with the new fake target), by extracting the coupling map in python space and not providing the fake target to the Rust implementation.

2. However, this is just an example of the potential disruption this change could cause in user workflows. And while we don't make any promises on the specific transpiler output (in terms of what gates are used), we might be breaking user assumptions that could be perceived as a change in our interface. For this reason we have decided to leave this PR for 2.0 instead of merging it in a minor release.

The plan is to fix the individual issues raised in the review and expand the test base with some significant tests that could give us more assurance/ a better understanding of the differences in output this PR could cause.

The deprecation warnings and a few fixes are elegible for 1.3 so they have been singled out in #13394.