Pass manager refactoring: cleanup internals #10127
Conversation
|
One or more of the the following people are requested to review this:
|
nkanazawa1989
added
mod: transpiler
This PR introduces two changes for * remove tight coupling of flow controller to pass runner instance, * remove pass normalization. PropertySet becomes context var so that flow controllers can be instantiated without coupling to a pass runner instance. BasePass becomes an iterable of length 1 to skip normalization. The decoupling of property set from pass runner allows pass manager to directly broadcast pass runner in the multithread, rather than distributing self and craete multiple pass runner in each thread.
nkanazawa1989
force-pushed
the
feature/passmanager-refactoring-pr2
branch
from
a039cbe
to
6505e20
Compare
This allows subclass to dispatch different pass runner type depending on the target code.
|
PR is rebased since #10124 is merged. Now this PR is ready for review. |
There was a problem hiding this comment.
I think the complexity with PropertySet is coming from a (pre-existing) poor distinction between the responsibilities of FlowController, PassManager and RunningPassManager, and very serious (pre-existing) confusion in FlowController about what it is and should be doing. The FuturePropertySet that's introduced here seems to me to be making things a bit worse in terms of logical architecture by coupling to stateful globals, even if those have some controllable context. Fundamentally, the state of the compilation (the PropertySet) we need to manipulate is local to an iteration through a series of passes, but moving that state manipulation into contextvars hides that while necessitating non-local getters to pull the local state from a global instance.
FlowController is currently, architecturally, reasoned about as if it were a pass, but I think everything might get a lot clearer if we normalise our architecture such that:
PassManagercontains exactly oneFlowController(aLinearFlowControllerin almost all cases) - thepassesfield can be a property that views into this, potentially.FlowControllergains a method.runthat takes in thePropertySetFlowControlleris responsible for running all passes and delegating to internalFlowControllersPassManager.runcreates the initial property set, and immediately passes it to the firstFlowController.runPassManager.runis responsible for moving properties from thePropertySetto the output format, like it currently is (though honestly, I'm not convinced that this shouldn't be the responsibility of the passes themselves).
That makes everything about the property set explicitly stateful, and the state is just passed from object to object. We don't need to fetch the state from contexts; FlowControllers will always have it for their conditions (no need for any of the partial stuff) because it's just directly given to their new run methods.
In this situation, I'd suggest basically completely rewriting the flow controllers and turning them into regular classes with a simple defined API, instead of what's currently in place.
qiskit/passmanager/base_pass.py
Outdated
| def __eq__(self, other): | ||
| try: | ||
| if len(other) == 1: | ||
| # Reference might be List[BasePass] which was conventional representation | ||
| # of a single base pass in the pass manager. | ||
| other = next(iter(other)) | ||
| except TypeError: | ||
| return False | ||
| return hash(self) == hash(other) |
There was a problem hiding this comment.
Do we actually use BasePass.__eq__? It seems really a pretty weird thing to have, to me. At any rate, hash(self) == hash(other) is not a correct implementation of equality haha - it would have been better to not define an __eq__ at all and use the default implementation (which boils down to x is y).
There was a problem hiding this comment.
Yes, it's actually used. Pass runner (currently in flow controller) manages already run passes with a set instance, and it prevents execution of duplicated passes by design. This equivalence is used for the set object. For example,
The actual log without equivalency check will be
run transformation pass PassA_TP_NR_NP
Pass: PassA_TP_NR_NP - 0.46515 (ms) <-- required
run transformation pass PassC_TP_RA_PA
Pass: PassC_TP_RA_PA - 0.05412 (ms)
run transformation pass PassA_TP_NR_NP
Pass: PassA_TP_NR_NP - 0.04673 (ms) <-- required
run transformation pass PassB_TP_RA_PA
Pass: PassB_TP_RA_PA - 0.04196 (ms)
...
Since both PassC_TP_RA_PA and PassB_TP_RA_PA require PassA_TP_NR_NP, which is independently instantiated and added to .requires. Dropping this violates ~30 unit tests, and may impact user's code by repeatedly executing the requires passes.
Strictly speaking this implementation is not really robust. This logic uses Python repr as a easy hash, however every Qiskit object doesn't properly implement the repr dunder (e.g. Target). Developer should have implemented the equality dunder for every pass, but it's too late now.
| **partial_controller: Partially evaluated callables representing | ||
| a condition to invoke flow controllers. This dictionary | ||
| must be keyed on the registered controller name. | ||
| """ | ||
| self._passes = passes | ||
| self.passes = FlowController.controller_factory(passes, options, **partial_controller) |
There was a problem hiding this comment.
I would dearly love to refactor this controller_factory and partial_controller stuff away - it's nearly impenetrable to understand, and I don't think it actually buys us anything at all. It'd be much cleaner to have FlowController be an abstract base class with the directly instantiable actual controllers deriving from it.
I don't know if you'd prefer that to be a different PR, though.
There was a problem hiding this comment.
I agree. I want to drop controller_factory (it becomes much readable though), and I don't think appending passes to pass manager with kwargs, e.g. .append(passes, condition=callable1, do_while=callable2), is really good interface as it introduces implicit and uncontrollable pass hierarchy through the FlowController class variable. User must explicitly instantiate controllers rather than relying on this uncontrollable factory method.
- Add OptimizerTask as a protocol for the pass and flow controller. These are in principle the same object that inputs and outputs IR with optimization. - A task gains execute method that takes IR and property set. This makes property set local to iteration of the passes. - Drop dependency on pass runner. Now pass manager has a single linear flow controller. - Pass manager gain responsibility of compiler frontend and backend as pass runner dependency is removed. This allows flow controllers to be still type agnostic. - Drop future property set, as this is no longer necessary because optimization task has the execute method explicitly taking the property set.
- Remove pass runner baseclass and replace RunningPassmanager baseclass with FlowControllerLiner - Implemented frontoend and backend functionality in transpiler Pass manager
This reverts commit fbd64d9. The follow on PR to this one Qiskit#10127 which is making internal refactors to the pass manager code now that's it a standalone module is still undergoing active review and the scope of the PR is sufficiently large that it likely won't be viable for the pending 0.25.0 release. This commit temporarily reverts Qiskit#10124 which was the first step of creating a module by porting the pass manager code in it's current form to a standalone module so that we're not committed to the API as part of the 0.25.0 release to give more time for Qiskit#10127 to finalize what the eventual `qiskit.passmanager` API will look like. This revert should itself be reverted after 0.25.0rc1 is tagged and the main branch opens up for 0.45.0 development. As this revert is not an indication that we did not want Qiskit#10124 it's just to avoid committing to the API prematurely.
…10454) This reverts commit fbd64d9. The follow on PR to this one #10127 which is making internal refactors to the pass manager code now that's it a standalone module is still undergoing active review and the scope of the PR is sufficiently large that it likely won't be viable for the pending 0.25.0 release. This commit temporarily reverts #10124 which was the first step of creating a module by porting the pass manager code in it's current form to a standalone module so that we're not committed to the API as part of the 0.25.0 release to give more time for #10127 to finalize what the eventual `qiskit.passmanager` API will look like. This revert should itself be reverted after 0.25.0rc1 is tagged and the main branch opens up for 0.45.0 development. As this revert is not an indication that we did not want #10124 it's just to avoid committing to the API prematurely.
…" (Qiskit#10454) This reverts commit fbd64d9. The follow on PR to this one Qiskit#10127 which is making internal refactors to the pass manager code now that's it a standalone module is still undergoing active review and the scope of the PR is sufficiently large that it likely won't be viable for the pending 0.25.0 release. This commit temporarily reverts Qiskit#10124 which was the first step of creating a module by porting the pass manager code in it's current form to a standalone module so that we're not committed to the API as part of the 0.25.0 release to give more time for Qiskit#10127 to finalize what the eventual `qiskit.passmanager` API will look like. This revert should itself be reverted after 0.25.0rc1 is tagged and the main branch opens up for 0.45.0 development. As this revert is not an indication that we did not want Qiskit#10124 it's just to avoid committing to the API prematurely.
- Move handling of required passes to generic pass itself. This makes optimizer tasks the composite pattern-like for more readability. - Readd MetaPass to transpiler BasePass as a metaclass which implements predefined equivalence check for all passes. This is necessary to take care of duplicated pass run, which is prohibited in circuit pass manager. - Add FlowControllerMeta that allows users to subclass FlowController, while delegating the pass control to BaseFlowController. - Readd count to pass manager callback - Add PassState that manages the state of execution including PropertySet. This is a portable namespace that can be shared across passes.
There was a problem hiding this comment.
This review just contains little, less consequential comments. I made nkanazawa1989#51 that implements a bunch of little documentation tweaks (corrected typos, Sphinx links, etc) rather than marking them inline here - I thought that'd be easier for both of us.
I said it before, but thank you for all the work on this PR and the refactoring so far! The code is definitely much easier to follow in your new form.
releasenotes/notes/add-passmanager-module-3ae30cff52cb83f1.yaml
Outdated
Show resolved
Hide resolved
Tweaks to documentation
- Add new container class PassmanagerMetadata - Rename propertyset with compilation_status - Provide metadata with the iter_tasks instead of property_set - Send metadata through generator - Turn metadata required, and let pass manager create metadata - Return metadata from execute along with the IR
… stacklevel to deprecation warning
|
Thanks @jakelishman for another round of review. I should have noticed |
This is primarily to avoid a potential conflict with the terminology `metadata` that's associated with the `input_program`, and because the object is the state object for a pass-manager execution. There is still a risk of confusion with `RunState`, but since that's a subcomponent of `PassManagerState`, it feels fair enough.
There was a problem hiding this comment.
I pushed up a commit to rename PassmanagerMetadata to PassManagerState, just because that terminology's a bit more inline with how we usually use the words "state" and "metadata" within the library (and the capitalisation was a bit off compared to what we normally do). I hope that's ok, I just wanted to get this approved without sending it back to you to have to do more work.
Thank you so much for this, and the immense effort that you've gone to to getting this over the line. It's really a fantastic refactor of some of the hardest-to-work-with code that was in Qiskit before now, and I'm really glad we had you working on it!
jakelishman
added
Changelog: Deprecation
|
I guess until the code path really gets deprecated it doesn't really matter, but I just wanted to note that while testing the 1.0 beta I tried looking at all warnings and noticed that the warning gets generated a lot internally by Qiskit. For instance, this code generates it: import warnings
from qiskit import QuantumCircuit, transpile
warnings.simplefilter("default", PendingDeprecationWarning)
qc = QuantumCircuit(1, 1)
qc.measure_all()
transpile(qc, optimization_level=0) |
Summary
This PR cleanups internals of the pass manager and flow controllers. 2 of 2 PRs.
Details and comments
This is the overhaul of current pass control machinery. The points of the overhaul are:
Conventionally flow controller also had a responsibility of single pass execution. In new implementation, a common base class
Taskdefines the execute method, and responsibility of pass execution is delegated to pass itself. In addition, the running pass manager (pass runner) is consolidated into the flow controller execute method. This drastically simplifies the internals of pass execution management.Conventionally pass execution is managed by a single pass runner instance, and thus instance variables can be used to manage the state of pass execution (e.g. property set, count, completed passes, etc). In new implementation, the pass execution is delegated into multiple flow controllers, and thus the state information is aggregated into single portable data structure
WorkflowStatuswhich is handed over passes through the execute method along with conventionalPropertySet.