Add constructor to build target from legacy transpiler model

[mtreinish]

Summary

This commit adds a new constructor method from_configuration() to the Target class. This can be used to construct a new Target object from the legacy input types: basis_gates, coupling_map, etc. This can then be used to provide a conversion layer in transpile() in support of migrating to using the target everywhere internally.

Details and comments

[qiskit-bot]

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

• @Qiskit/terra-core

[coveralls]

Pull Request Test Coverage Report for Build 4677945711

• 99 of 112 (88.39%) changed or added relevant lines in 1 file are covered.
• 243 unchanged lines in 10 files lost coverage.
• Overall coverage increased (+0.007%) to 85.437%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/transpiler/target.py	99	112	88.39%

Files with Coverage Reduction	New Missed Lines	%
qiskit/circuit/tools/pi_check.py	1	91.23%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	1	95.14%
qiskit/qasm/init.py	2	71.43%
qiskit/pulse/library/waveform.py	3	91.67%
qiskit/qpy/binary_io/schedules.py	18	86.36%
qiskit/quantum_info/operators/symplectic/clifford_circuits.py	19	90.87%
qiskit/qpy/type_keys.py	26	87.7%
qiskit/quantum_info/operators/symplectic/clifford.py	37	87.91%
qiskit/transpiler/passes/routing/bip_mapping.py	41	24.0%
qiskit/transpiler/passes/routing/algorithms/bip_model.py	95	16.88%

Totals
Change from base Build 4671582287:	0.007%
Covered Lines:	68049
Relevant Lines:	79648

---

💛 - Coveralls

[kdk]

A few comments after looking through test documentation and tests.

[kdk]

It looks like there's some overlap here with convert_to_target https://github.com/Qiskit/qiskit-terra/blob/ea984fda21b8cef1ab9b60c0bdbc1a1340263e6c/qiskit/providers/backend_compat.py#L33 . Would it make sense to consolidate the two (from_configuration looks to be more general), or do they serve different purposes?

[mtreinish]

This constructor is more general and takes slightly different information. convert_to_target is explicitly to go from a BackendV1's model objects (BackendConfiguration, BackendProperties, and PulseDefaults) while this is to go from the loose objects which are basically the input to transpile() and build a Target from those. There is definitely overlap as at the end of the day both the transpile() arguments and BackendV1 are expressing the same data (with roughly the same limitations) just in different formats. I think a good follow up to this might be to update convert_to_target to leverage this constructor instead of doing it manually. But I feel that's outside the scope of this PR (there is also potential overhead with doing that because we basically have to build a bunch of intermediate objects from the BackendConfiguration to match the input here).

[nkanazawa1989]

This method seems to me like encouraging end-user to build Target from these legacy data structure. Alternatively you can introduce private factory function in the transpile, so that end-user still cannot build Target with these transpiler args, which will be deprecated in future.

[mtreinish]

I think it's fine to still be user facing. For better or worse it'd be difficult to remove most of the legacy data structures considering how widely used they are. I think leaving this is as a public facing interface is fine. If we do decide to do something like deprecate InstructionDurations or something we can do that case by case basis here too.

[nkanazawa1989]

Thanks Matthew. Seems like the logic you introduced is different from the conventional V1 converter and new factory method is useful to migrate from transpiler args to Target. I added several comments, mainly about handling of instmap.

[nkanazawa1989]

This method seems to me like encouraging end-user to build Target from these legacy data structure. Alternatively you can introduce private factory function in the transpile, so that end-user still cannot build Target with these transpiler args, which will be deprecated in future.

[nkanazawa1989]

This ignores gates defined in the inst map. This must be something like union(set(basis_gates), set(inst_map.instructions)). You could also add gates in backend properties, but likely this is an edge case (basis_gates must be superset).

[nkanazawa1989]

This looks good to go now. I just need clarification of the policy for custom calibration handling. When we deal with inst map and custom pulse for publication, the most popular situation would be the following. A user may calibrate custom pulse for particular qubit or qubit pair, which is a part of the coupling map (because it's tough to manage chip-wide calibration without automation tool, though we have Qiskit Experiments calibrations now). What we used to do is

backend.configuration().basis_gates += ["my_gate"]
backend.defaults().instruction_schedule_map.add(
   "my_gate", (0, 1), my_gate_schedule
)

qc = QuantumCircuit(3)
qc.append(Gate("my_gate", 2, []), [0, 1])
qc.cx(1, 2)
qc.measure_all()

qc_t = transpile(
   qc, 
   backend,   # coupling_map is [[0,1], [1, 0], [1, 2], [2, 1], ...]
   initial_layout=[0, 1, 2],
)

with this my_gate only appears in the (0, 1) node, and the calibration is attached to the node. If we use another initial layout (or we do not provide), transpiled circuit may contain undefined my_gate and fails in the validation on provider. The problem here is coupling_map of [[0,1], [1, 0], [1, 2], [2, 1], ...] is agnostic to gate, and this limits the capability of experiment.

Proposed implementation in this PR preserves this behavior, although Target can support partial connectivity for a particular gate. In principle, you can write smarter logic to discover such partial connectivity based on defined calibrations. I'm fine if current implementation is intentional, especially from the backward compatibility point of view.

[mtreinish]

Yeah, that's a good question. For this I was opting mostly for backwards compatibility with how transpile() works today. The goal when I originally wrote this constructor was to have transpile() call this and then we only are working with Target moving forward. But I hadn't considered the needs of a custom calibration use case. My thinking for that use case though was that you could modify the target from the backend inplace either in the backend or as a copy. For example, something like:

backend.target.add_instruction(
    Gate(my_gate, 2, []),
    {(0, 1): InstructionProperties(duration=my_gate_schedule.duration(), calibration=my_gate_schedule)}
)

qc = QuantumCircuit(3)
qc.append(Gate("my_gate", 2, []), [0, 1])
qc.cx(1, 2)
qc.measure_all()

qc_t = transpile(
   qc, 
   backend,
   initial_layout=[0, 1, 2],
)

That should work fine. The only place this interface would be an issue for calibration would be if you did something like:

inst_map = backend.instruction_schedule_map
inst_map.add("my_gate", (0, 1), my_gate_schedule)
target = Target.from_configuration(
    backend.operation_names,
    backend.num_qubits,
    backend.coupling_map,
    inst_map=inst_map
)    

(or in terra 0.25.0 called transpile() like that)

But, my assumption was if you wanted to work at the individual argument level like that you'd just do this instead:

inst_map = backend.instruction_schedule_map
inst_map.add("my_gate", (0, 1), my_gate_schedule)
target = Target.from_configuration(
    backend.operation_names,
    backend.num_qubits,
    backend.coupling_map,
)
target.update_from_instruction_schedule_map(inst_map, {"my_gate": Gate(my_gate", 2, [])})    

But I want to make sure this works ok for how you were expecting calibrations to be used.

[nkanazawa1989]

Thanks for clarification. That makes sense. Anyways current transpiler arg model does't express partly calibrated gate very well, and a user who wants to deal with such calibration should move to Target. I think it's not mandatory to support full capability of Target (to express partly calibrated gate) with legacy transpiler input. We need to provide a good documentation to encourage users to use Target.

[nkanazawa1989]

@kdk do you want to do another review?

[mtreinish]

kdk do you want to do another review?

I'm just going to enqueue this for merging in the interest of time for the 0.24.0 release. kdk's on vacation this week but if there is any followup discussion on this we can revisit it in a follow up PR.