Gates with custom pulses fail to transpile with the new provider

[yaelbh]

Environment

• Qiskit Terra version: 0.41.1
• Python version: 3.9.12
• Operating system: Linux

What is happening?

Defining a new gate, and attaching pulses to it, triggers an error during transpilation. The same code works well with the old provider.

How can we reproduce the issue?

from qiskit import QuantumCircuit, pulse, transpile, IBMQ
from qiskit.circuit import Gate
from qiskit.pulse import InstructionScheduleMap

from qiskit_ibm_provider import IBMProvider


provider = IBMProvider()
# provider = IBMQ.load_account()

backend = provider.backend.ibmq_lima

amp = 0.1
sigma = 10
num_samples = 128
gauss = pulse.library.Gaussian(num_samples, amp, sigma, name="Parametric Gauss")
with pulse.build(backend, default_alignment="sequential") as gatepulse:
    with pulse.align_left():
        pulse.play(gauss, pulse.control_channels(0, 1)[0])

inst_map = InstructionScheduleMap()
inst_map.add("newgate", [0, 1], gatepulse)

newgate = Gate("newgate", 2, [])

circ = QuantumCircuit(2)
circ.append(newgate, [0, 1])

circ = transpile(circ, backend, inst_map=inst_map, basis_gates=["newgate"])

Output:

Traceback (most recent call last):
  File "/mnt/c/Users/143721756/wsl/balagan/szxbug.py", line 31, in <module>
    circ = transpile(circ, backend, inst_map=inst_map, basis_gates=["newgate"])
  File "/home/yaelbh/miniconda3/envs/env1/lib/python3.9/site-packages/qiskit/compiler/transpiler.py", line 381, in transpile
    _serial_transpile_circuit(
  File "/home/yaelbh/miniconda3/envs/env1/lib/python3.9/site-packages/qiskit/compiler/transpiler.py", line 474, in _serial_transpile_circuit
    result = pass_manager.run(circuit, callback=callback, output_name=output_name)
  File "/home/yaelbh/miniconda3/envs/env1/lib/python3.9/site-packages/qiskit/transpiler/passmanager.py", line 528, in run
    return super().run(circuits, output_name, callback)
  File "/home/yaelbh/miniconda3/envs/env1/lib/python3.9/site-packages/qiskit/transpiler/passmanager.py", line 228, in run
    return self._run_single_circuit(circuits, output_name, callback)
  File "/home/yaelbh/miniconda3/envs/env1/lib/python3.9/site-packages/qiskit/transpiler/passmanager.py", line 283, in _run_single_circuit
    result = running_passmanager.run(circuit, output_name=output_name, callback=callback)
  File "/home/yaelbh/miniconda3/envs/env1/lib/python3.9/site-packages/qiskit/transpiler/runningpassmanager.py", line 125, in run
    dag = self._do_pass(pass_, dag, passset.options)
  File "/home/yaelbh/miniconda3/envs/env1/lib/python3.9/site-packages/qiskit/transpiler/runningpassmanager.py", line 173, in _do_pass
    dag = self._run_this_pass(pass_, dag)
  File "/home/yaelbh/miniconda3/envs/env1/lib/python3.9/site-packages/qiskit/transpiler/runningpassmanager.py", line 202, in _run_this_pass
    new_dag = pass_.run(dag)
  File "/home/yaelbh/miniconda3/envs/env1/lib/python3.9/site-packages/qiskit/transpiler/passes/basis/unroll_custom_definitions.py", line 102, in run
    raise QiskitError(
qiskit.exceptions.QiskitError: "Cannot unroll the circuit to the given basis, ['newgate']. Instruction newgate not found in equivalence library and no rule found to expand."

What should happen?

Should work.

Any suggestions?

@haggaila says:
An exception happens in file qiskit/transpiler/passes/basis/unroll_custom_definitions.py, in the method UnrollCustomDefinitions.run. With the old provider, self._target is None upon entrance to the function, and everything gets properly initialized (in particular device_insts contains the added newgate). With the new IBM provider, self._target is initialized to something, resulting in an exception.

@nkanazawa1989 please share your observations.

[yaelbh]

I didn't check the main branch.

[nkanazawa1989]

It's known issue #9489. This will be fixed in next release.

[nkanazawa1989]

Sorry this is another issue from #9489. One must fix

https://github.com/Qiskit/qiskit-terra/blob/6829bb18cf791960896fe72b9be9611aac44155a/qiskit/transpiler/passes/basis/unroll_custom_definitions.py#L60-L62

this logic. With V2, target is always always provided from the backend and basis_gates is ignored. With #9489 probably this will be also fixed (target and operation_names property are updated with instmap), however, if you remove instmap (i.e. you may want to manually add calibration after transpile) it doesn't work.

[jakelishman]

you may want to manually add calibration after transpile

Maybe I misunderstood, but if the calibrations aren't attached during the transpilation, I would think it's correct behaviour for us to error out saying "we can't translate the circuit for this backend".

(to be clear: I agree that the rest of the issue stands regardless of that point)

[nkanazawa1989]

I'm fine with raising an error (to be on strict side). However, currently this workflow is supported with backend V1 (because basis_gates is context-less).

from qiskit.circuit import Gate, QuantumCircuit
from qiskit.pulse import Schedule
from qiskit import transpile

g = Gate("custom", 1, [])
qc = QuantumCircuit(1)
qc.append(g, [0])

qc_t = transpile(qc, basis_gates=["custom"])

sched1 = Schedule(name="sched1")
sched2 = Schedule(name="sched2")

qc_t1 = qc_t.copy()
qc_t1.add_calibration(g, (0,), sched1)

qc_t2 = qc_t.copy()
qc_t2.add_calibration(g, (0,), sched2)

[jakelishman]

Yeah, the equivalent in BackendV2 would be to add custom as an instruction supported on all single qubits to the given Target (potentially we can do this automatically if given basis_gates, but I wouldn't like to speak to Matthew's design plans). If that hasn't happened, then we're right to error out.

[mtreinish]

This is something that is coming from the mix of Target and basis_gates as an argument in transpile() the individual passes tend to favor Target as higher priority than basis_gates if they're given both because in general target contains a stronger set of constraints and is what should be used if the constructor was given both. The individual pass cannot know that the basis_gates argument was manually passed in and overrides the Target. In general transpile() should invalidate the target if both a backend and any hardware constraint arguments are provided. The quick fix here (for backport) would be to update transpile() to set target to None if coupling_map or basis_gates (or arguably backend_properties, instruction_durations, or inst_map although these typically don't override the supported operations or connectivity and are used opportunistically by most passes for extra information) are set.

The longer term fix, hopefully for 0.24, is to implement: #9256 and use a target for everything from transpile(). That path would mean that we always generate a target and in the case of transpile(qc, backend, basis_gates=[..]) we'd build a new Target from basis_gates instead of from the backend. But right now I'm blocked on review for #9263 and optionally #9255 (although it looks like there is a merge conflict issue there) so I haven't started implementing it yet.

In my ideal world we'd honestly just raise an error if a user specifies both a backend and basis gates or a coupling map because what the intent there is kind of ambiguous from my PoV. But,that's kind of hard behavior to walk back. Regardless transpile(qc, basis_gates=[..]) should always be valid and just means treat them as globally available and ideal.

[nkanazawa1989]

Dropping all arguments from transpile seems much cleaner but this requires user to learn the API of Target, which is really different from conventional transpiler args. Generating Target on the fly from the transpiler args sounds reasonable to me. My PR #9587 allows one to build target from the inst_map.

[mtreinish]

Well I wasn't proposing we drop the arguments, it was more it's not clear what a user's intent is if they specify transpile(qc, backend, basis_gates=['r', 'custom', 'ccx']) (especially if backend is a BackendV2) as a backend and a basis gate list are specifying overlapping information, but the backend's version is much more nuanced. My preference would be to raise an exception if someone does that. But that also wasn't a serious proposal either because of the backwards compatibility issues with doing that.

[mtreinish]

I pushed up #9789 for the short-term fix to this which can be backported for 0.23.3. Longer term I'd still like to get #9256 for 0.24.0.