Fix PulseQobj converter name collision

[nkanazawa1989]

Summary

This PR fixes name collision bug (edge case) of pulse qobj converter (QobjToInstructionConverter).

Details and comments

In the pulse qobj model, instruction is managed by the unique string identifier, e.g. ShiftPhase: fc, in the transfer layer. Only exception is the Play instruction, because this instruction takes two types of operands, namely, parametric pulse and waveform. When the backend doesn't support a parametric pulse form, this is converted into waveform and actual waveform samples are stored in the .pulse_library field. Thus command .name of the play instruction is parametric_pulse or unique waveform name (such as CX_d10_u15, which is a foreign key to refer to the pulse library), rather than "play".

Currently, QobjToInstructionConverter manages these instructions with a dedicated method. This is sort of descriptor pattern, where dedicated method to parse each pulse library item (as well as other instructions) is dynamically generated. Note that this descriptor is directly attached to the class. Thus, when multiple converters are instantiated with different backends in the same thread, it could cause name collision when backend reports different waveforms under the same name. For example,

pulse_library_from_backend_x = [PulseLibraryItem(name="pulse123", samples=[0.1, 0.1, 0.1])]
converter_of_backend_x = QobjToInstructionConverter(pulse_library_from_backend_x, buffer=0)

pulse_library_from_backend_y = [PulseLibraryItem(name="pulse123", samples=[0.2, 0.2, 0.2])]
converter_of_backend_y = QobjToInstructionConverter(pulse_library_from_backend_y, buffer=0)

qobj1 = PulseQobjInstruction(name="pulse123", qubits=[0], t0=0, ch="d0")

sched_out_x = converter_of_backend_x(qobj1)
sched_out_y = converter_of_backend_y(qobj1)

sched_out_x and sched_out_y accidentally loads the same pulse from the pulse library of backend y (because this is the latest instance). Note that this doesn't happen in current Qiskit client, because converter is immediately discarded after PulseDefaults for a particular backend is instantiated. However, this mechanism doesn't stop developers to keep converter. Such converter is silently mutated when another backend is loaded.

In this PR, descriptor-like pattern is replaced with visitor pattern, and class level cache is removed.

New test test_instruction_name_collision is also added.

[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

[nkanazawa1989]

I confirmed new mechanism works properly with real backend.

from qiskit import QuantumCircuit, transpile, schedule

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

# chose qubit pair with DDCX,
# this is currently waveform instruction because of missing parametric pulse class.
# this is nice to debug pulse library reference mechanism.
bell_schedule = schedule(transpile(qc, backend, initial_layout=[3, 5]), backend)
job = backend.run(bell_schedule)
result = job.result()
result.get_counts()  # {'00': 1929, '01': 557, '10': 88, '11': 1426}

[coveralls]

Pull Request Test Coverage Report for Build 3853489076

• 133 of 187 (71.12%) changed or added relevant lines in 3 files are covered.
• 9 unchanged lines in 4 files lost coverage.
• Overall coverage decreased (-0.02%) to 84.584%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/qobj/converters/pulse_instruction.py	129	183	70.49%

Files with Coverage Reduction	New Missed Lines	%
qiskit/qobj/converters/pulse_instruction.py	1	78.4%
src/sabre_swap/layer.rs	2	98.95%
qiskit/pulse/library/waveform.py	3	91.49%
src/sabre_swap/mod.rs	3	98.84%

Totals
Change from base Build 3823792673:	-0.02%
Covered Lines:	64190
Relevant Lines:	75889

---

💛 - Coveralls

[mtreinish]

Overall this looks good to me, I didn't see anything that really stood out as an issue in reviewing this. However, I'm not the most familiar with this part of the code base. I've not really had any reason to work with pulse qobj converters directly before. I left some review comments inline on the release notes, but they're not really blocking (we can always fix that as part of the release prep PR)

[mtreinish]

I'm surprised pylint doesn't complain about this being camel case.

[nkanazawa1989]

Yes, this was what I wanted to change. Since now we can use method dispatch in terra, I updated the implementation of these method in f1ad415

[mtreinish]

LGTM, thanks for the quick update.