Pass manager refactoring: add passmanager module

[nkanazawa1989]

Summary

This PR adds new qiskit.passmanager module and reorganizes qiskit.transpiler. New module implements a generic pass manager which is not sensitive to object types (input, output, IR) under transformation, and only provides a management for pass execution.

1 of 2 PRs. Pass manager internals will be refactored in the follow up.

Details and comments

The internal logic is almost as-is. One visible change is in qiskit.passmanager.passrunner. This provides BasePassRunner which is a superclass of RunningPassManager. New base class implements

• BasePassRunner._to_passmanager_ir
• BasePassRunner._to_target
• BasePassRunner._run_base_pass

which are all abstract methods. These are type sensitive and intended to be implemented by a subclass for particular data types, e.g. for existing pass manager, input: QuantumCircuit, passmanager_ir: DAGCircuit, output: QuantumCircuit. We plan to introduce another compiler for the pulse module.

The new methods

• BasePassRunner._run_pass_generic

implements data type agnostic flow control. This is a common machinery for every subclass.

[qiskit-bot]

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

• @Eric-Arellano
• @HuangJunye
• @javabster
• @Qiskit/terra-core

[coveralls]

Pull Request Test Coverage Report for Build 5352619334

• 389 of 413 (94.19%) changed or added relevant lines in 13 files are covered.
• 21 unchanged lines in 6 files lost coverage.
• Overall coverage increased (+0.003%) to 85.947%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/passmanager/base_pass.py	40	41	97.56%
qiskit/transpiler/runningpassmanager.py	55	57	96.49%
qiskit/passmanager/passmanager.py	89	95	93.68%
qiskit/passmanager/passrunner.py	76	83	91.57%
qiskit/passmanager/flow_controllers.py	63	71	88.73%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/expr.rs	1	93.76%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	1	90.39%
crates/qasm2/src/lex.rs	2	90.89%
qiskit/extensions/quantum_initializer/squ.py	2	80.0%
crates/accelerate/src/vf2_layout.rs	3	94.74%
crates/qasm2/src/parse.rs	12	96.65%

Totals
Change from base Build 5334907613:	0.003%
Covered Lines:	71473
Relevant Lines:	83159

---

💛 - Coveralls

[mtreinish]

Sorry for the delay in reviewing this. I took a quick first higher level pass over the PR. I really like the direction this is taking towards making the pass manager more general so we can use it more broadly as transformation recipe. I have a few inline comments so far. My biggest concern is how this is managing the backwards compatibiltiy of things that are moved unchanged from qiskit.transpiler -> qiskit.passmanager I feel like there are a couple of missing places where we should have redirects to ensure things still work from transpiler as is while we're splitting out the passmanager module.

[mtreinish]

Is the use of ABC here to prevent directly instantiating an instance of BasePassManager? I didn't see any abstract methods defined in the class to define an interface otherwise.

[nkanazawa1989]

Actually a developer needs to attach BasePassManager.PASS_RUNNER to make it work. In this base class BasePassRunner is just linked for convenience of type hint, but the base class doesn't provide IR conversion logic. One could also do abstract class property, but I don't like class property.

Alternatively, we can keep BasePassManager._create_running_passmanager method and make it abstract. However, writing and overriding this method just for difference of pass runner type seems awkward (we can remove this method in the followup PR #10127 ).

The another place where the pass runner type is necessary is run method. We need to dispatch the _run_single_circuit when input is a single entry, but we cannot safely count entry number because input program is usually iterable, e.g. QuantumCircuit and ScheduleBlock. We can get expected input type through above class attribute, i.e.

    if isinstance(programs, self.PASS_RUNNER.IN_PROGRAM_TYPE):
        # run single runner call
    # otherwise we can assume programs are sequence

[mtreinish]

This LGTM now, thanks for the updates and sorry for the slow turnaround. I left a few comments about release notes but we can just adjust those as part of the release prep.

I was starting to review the code for the pass manager itself and was going to ask a lot of questions about specifics and things that looked really weird to me until I realized most of that was pre-existing and just migrated code. I'll save those reviews for the next PR in the series that refactors the internals

[mtreinish]

Ok I guess an upgrade note is fine for this. I was more just trying to figure out whether this was a call to action for users catching TranspilerError before or whether we should document it somewhere else. The only other thought I had was maybe as an other note but this seems like a better place.

[jlapeyre]

I left a couple of suggestions on documentation that should be reviewed.

[jlapeyre]

My best recollection is that preserves has not been useful in practice. In fact, as far as I can see, it is not used at all in terra. In contrast, requires is used a bit. If it were not for backward compatibility, I imagine preserves would be removed. So I question whether it is a good idea to include this as a feature in the more abstract pass manager. Is it possible to define this attribute instead in BasePass(CommonBasePass) below?

[mtreinish]

I think for this PR it's ok to preserve preserves as this is strictly for migrating the common code that already exists into a reusable module. There is a second PR #10127 built on top of this which aims to clean things up. I think we can discuss specifics like that there. I think it would be easier to view this PR as just a code migration for the ease of review. We're not locked into the api here until the final release anyway so there is time to change it post merge

[nkanazawa1989]

Thanks @jlapeyre , I agree with you. I also observed that options dictionary is carried around passes,
https://github.com/Qiskit/qiskit-terra/blob/924702e9ddb3f6f0124bc8acb285ac3d1da12aff/qiskit/transpiler/runningpassmanager.py#L153
but eventually no one use this information (at least in Terra passes). However, I cannot decide what should be removed. I also want to cleanup and drop un-used features. Let's discuss carefully in #10127 . Once this is merged I'll rebase the PR for easier review.

[jlapeyre]

Sure it's ok, as long as it doesn't fall through the cracks. Btw. I think one or two people wrote or told me that preserves was an idea copied from llvm without knowing whether it would be useful.

[jlapeyre]

This means we have both passmanager.BasePass and transpiler.basepasses.BasePass. This will likely cause a little confusion when searching for things in the source tree. But, probably a good IDE would help. Also, in cases like this, people argue that this is exactly why namespaces exist. I think leaving this as you have it might not be a bad choice, but i'ts worth considering changing it. If I had a good substitute name, I'd suggest it. I think using passmanager.CommonBasePass might make the passmanager module too verbose.

[nkanazawa1989]

That's fair point. Since we have another cleanup PR #10127 , we can also change the class name in that PR if we come up with better substitute name. I would prefer changing the transpiler.basepasses.BasePass to CircuitBasePass and adding an alias of BasePass = CircuitBasePass. Although this is a public class, the base class is immediately subclassed by transform and analysis pass. I assume impact to users is limited.

(edit)

In current implementation probably we can also assume BasePass is agnostic to IR because .run is abstractmethod and pass subclass must implement functionality based on expected IR. Adding typecheck to BasePass is probably overkill. In this case we can just

from qiskit.passmanager.base_pass import BasePass

but we also need to move __call__ to passmanager.BasePass.

[jlapeyre]

CircuitBasePass . yes this is a good idea.

[jlapeyre]

See also a previous PR #9163 that was closed in favor of this one.

[nkanazawa1989]

Thanks for the review. I just updated document in ba6e698 and rebased this PR. @jlapeyre Let's discuss BasePass naming and removal of unused feature in the followup PR #10127 .

[nkanazawa1989]

Thanks @jlapeyre for approval. After discussion I came up with another name GenericPass. The passmanager.base_pass.GenericPass (BasePass) doesn't implement any feature specific to a particular IR / transformation, and this make me think this is sort of generic class.

Generic classes encapsulate operations that are not specific to a particular data type. The most common use for generic classes is with collections like linked lists, hash tables, stacks, queues, trees, and so on. Operations such as adding and removing items from the collection are performed in basically the same way regardless of the type of data being stored.

https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/generics/generic-classes

[mtreinish]

LGTM, thanks for all the updates. Lets continue the review of the specific internals of the passmanager in #10127