Remove list argument broadcasting and simplify transpile()

[mtreinish]

Summary

This commit updates the transpile() function to no longer support broadcast of lists of arguments. This functionality was deprecated in the 0.23.0 release. As part of this removal the internals of the transpile() function are simplified so we don't need to handle broadcasting, building preset pass managers, parallel dispatch, etc anymore as this functionality (without broadcasting) already exists through the transpiler API. Besides greatly simplifying the transpile() code and using more aspects of the public APIs that exist in the qiskit.transpiler module, this commit also should fix the overhead we have around parallel execution due to the complexity of supporting broadcasting. This overhead was partially addressed before in #7789 which leveraged shared memory to minimize the serialization time necessary for IPC but by using PassManager.run() internally now all of that overhead is removed as the initial fork will have all the necessary context in each process from the start.

Three seemingly unrelated changes made here were necessary to support our current transpile() API without relying on custom pass manager construction.

The first is the handling of layout from intlist. The current Layout class is dependent on a circuit because it maps Qubit
objects to a physical qubit index. Ideally the layout structure would just map virtual indices to physical indices (see #8060
for a similar issue, also it's worth noting this is how the internal NLayout and QPY represent layout), but because of the
existing API the construction of a Layout is dependent on a circuit. For the initial_layout argument when running with
multiple circuits to avoid the need to broadcasting the layout construction for supported input types that need the circuit
to lookup the Qubit objects the SetLayout pass now supports taking in an int list and will construct a Layout object at run
time. This effectively defers the Layout object creation for initial_layout to run time so it can be built as a function of the
circuit as the API demands. (this was handled separately in #10344)

The second is the FakeBackend class used in some tests was constructing invalid backends in some cases. This wasn't caught in the previous structure because the backends were not actually being parsed by transpile() previously which masked this issue. This commit fixes that issue because PassManagerConfig.from_backend() was failing because of the invalid backend construction.

The third issue is a new _skip_target private argument to generate_preset_pass_manager() and PassManagerConfig. This was necessary to recreate the behavior of transpile() when a user provides a BackendV2 and either basis_gates or coupling_map arguments. In general the internals of the transpiler treat a target as higher priority because it has more complete and restrictive constraints than the basis_gates/coupling map objects. However, for transpile() if a backendv2 is passed in for backend paired with coupling_map and/or basis_gates the expected workflow is that the basis_gates and coupling_map arguments take priority and override the equivalent attributes from the backend. To facilitate this we need to block pulling the target from the backend This should only be needed for a short period of time as when #9256 is implemented we'll just build a single target from the arguments as needed.

Details and comments

Fixes #7741
Fix #7883

TODO:

• Fix last failing tests (dt handling for scheduling)

[qiskit-bot]

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

• @Qiskit/terra-core

[coveralls]

Pull Request Test Coverage Report for Build 5587396079

• 76 of 81 (93.83%) changed or added relevant lines in 4 files are covered.
• 10 unchanged lines in 4 files lost coverage.
• Overall coverage increased (+0.03%) to 86.054%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/compiler/transpiler.py	71	76	93.42%

Files with Coverage Reduction	New Missed Lines	%
qiskit/compiler/transpiler.py	1	92.89%
qiskit/transpiler/instruction_durations.py	1	92.96%
crates/qasm2/src/lex.rs	2	91.39%
crates/qasm2/src/parse.rs	6	97.58%

Totals
Change from base Build 5582955280:	0.03%
Covered Lines:	72302
Relevant Lines:	84019

---

💛 - Coveralls

[danielleodigie]

From what I can see, it LGTM, other than that failing test I guess. I think it's a Linux test, and I can't recreate the issue on my end.

[jakelishman]

The test failure was a flakiness in the CI suite that I've made #10439 to address.

[jakelishman]

This looks a jillion times better than the old code. Given that this PR claims to close #7741, do you have approximate timings for the type of thing in that issue?

[jakelishman]

Is the new error message significantly worse?

[mtreinish]

It was when I modified this test, but @1ucian0 added a real error message in SetLayout as part of #10344 so it'll say:

The length of the layout is different than the size of the circuit 1024 <> 2

now. (I glossed over <> in my review we should change that to actually say 1024 qubits in the layout != 2 qubits in the circuit or something more explicit instead)

[jakelishman]

or just stick from __future__ import barry_as_FLUFL at the top of the file lol

[jakelishman]

output_name still broadcasts in _parse_output_name, but that at least doesn't get passed to the PassManager, so I guess we're ok with keeping that? I honestly had no idea we even had that option.

[mtreinish]

Yeah it shouldn't matter for this, it felt like if we're taking an output name argument it needs to support broadcasting. Since if you did transpile([qc1, qc2, qc3, qc4], output_name='foo') I'm not sure what the expected behavior for that would be (or why we'd force it on people). Since the broadcasting doesn't effect the PassManager object or the parallel dispatch I figured it was fine to keep it.

[jakelishman]

I'm fine with keeping it broadcasting, since it doesn't affect the PassManager. If the option wasn't already in transpile, I wouldn't have been in favour of adding it, haha - turns out that the initial use-case was to distinguish the same circuit transpiled with broadcast arguments (#2745).

[jakelishman]

To me, this seems to be indicating a problem with the passes that interpret this data; they should read it from the circuit during their run methods. It's the same problem that we had with SetLayout until just recently.

Ideally we'd fix that before this PR, but there's no time before 0.25 now. Can we check with the people who might be using these options within IBM that they're not going to be hamstrung by the decay to serial transpile, at least?

[mtreinish]

It is definitely a problem. My concern with doing the same fix as with SetLayout was I didn't think it was used in a single place like the input layout and that InstructionDurations would have to be updated in an instance variable in multiple passes. So I went with this approach instead to avoid doing this in multiple places.

@nkanazawa1989 or @itoko do you have any idea/thoughts here regarding the potential impact of serializing the execution with a custom instruction durations or dt input? I expect you've got the best handle on the impact of setting custom instruction durations (or dt) on the argument inputs to transpile().

My thinking is for most users it won't be noticeable because my feeling is most of them are on macOS anyway and we disable multiprocessing by default on macOS.

[nkanazawa1989]

I agree with Jake. Ideally we should fix passes to check circuit calibrations directly without updating the instruction durations here. In addition, duration values are already reported in the InstructionProperties, and we no longer need to create this object.

Just reusing the same custom instruction durations object is likely problematic. For example, we have two set of circuits generated by different experiments, and we may want to batch them together in a single job. When one is with the custom cal and the other is a reference with no custom cal, reusing the same durations object may impact the scheduling of second experiment circuits. When durations object is created for each circuit groups (this is impossible because circuits don't have context, because it's just a list, and thus we need to prepare durations object for each entry), we can avoid this kind of problem.

I prefer removing this option in future since duration values may conflict with one in the Target.

[itoko]

I think the potential impact of serializing the execution with a custom instruction durations or dt input would be little (at least for noisy simulation users). I didn't know until now that instruction durations were being used for pulse gates. I've been thinking InstructionDurations is for custom gates without calibrations used for noisy simulation and pulse gates are separately handled here. @naoki, do you think the case you mentioned above is covered by the code in the link? If so, maybe we could drop the updates with circuits in _parse_instruction_durations.

[mtreinish]

Given the comments I'm going to go ahead and add this to merge queue. I think we can revisit the exact structure of how the passes deal with instruction durations in a followup as that isn't blocked by this change, and the risk of this becoming a performance issue is low. I do wonder if an implementation of #9256 would alleviate some of those concerns. Because then the passes will only get a target and we're not using InstructionDurations anywhere (except for the plugin interface). But I'll convert this comment into an issue for 0.26 and we can track it there.

[jakelishman]

Noooo, my first non-trivial merged PR to Terra D: (#6814, haha)

[mtreinish]

This looks a jillion times better than the old code. Given that this PR claims to close #7741, do you have approximate timings for the type of thing in that issue?

I don't have the timings handy. I think that I tried to run the recreate from the issue when I wrote this a month ago and I was having trouble reproducing the issue locally. I'll give it a try again in a bit and see if I can get real numbers.

[jakelishman]

I'm happy with this and ready to merge, pending an answer from one of the potentially affected people about #10291 (comment). In general, I think that's probably a trade-off we're going to need to make; the parallel overhead for pulse jobs was already (I think?) high enough that parallel transpile probably wasn't helping a great amount already, and the behaviour is still achievable with explicit parallelisation with built-in Python tools.

I'll hold off on tagging for merge until there's a resolution in that comment thread.