-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add StagedPassManager class for pass manager with defined stages #6403
Conversation
This commit adds a new PassManager subclass, FullPassManager. This class is used to have a PassManager with a defined structure and stages for the normal transpile workflow. The preset pass managers are then updated to be FullPassManager objects they conform to the fixed structure. Having a class with defined phases gives us flexibility in the future for making the transpiler pluggable with external plugins (similar to what's done in PR Qiskit#6124) and also have backend hook points before or after different phases of the transpile. Fixes Qiskit#5978
@ajavadia I've updated the preset passmanagers to abstract away the common code. There is still a bit of duplication in the layout and routing pass creation, but I opted to leave separate copies in each level mostly because the control flow logic (for layout) is different at each level and the parameters for some of the passes are different at each optimization level. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a routine for scheduling? This is kind of post_optimization
but seems like we are going to add many of them for pulse gate experiment (scheduled circuit, alignment, dynamical decoupling, pulse gate #6759)
I apologize for the delay I missed your comment here somehow. I've added a new scheduling stage to the end of the |
Thanks Matthew. This looks great. How easily we can update signature of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the added structure this brings (and the prospect of helping users and providers hook into the transpilation process). I have some mild reservations about the implementation as a PassManager
subclass in that it seems less flexible than adding support for labeled stages in the PassManager
(along the lines of #5069 ) both for future development of the presets and for other outside use cases. (It may be that a subclass is the interface we want to achieve this structure without needing #5069 .)
My only other concern is with regards to readability of the presets (similar to #3437 ). With the definition of stages (and corresponding functions to build them), it should be easier for newcomers to understand how the preset pass managers are built, and what will be run when, but we should collect some opinions here. (For me, consistency is key, but others may feel differently.)
There's also a failing QPY backwards compatibility test, but I'm not sure if it's related to these changes: https://dev.azure.com/qiskit-ci/qiskit-terra/_build/results?buildId=31290&view=logs&j=8fcf860c-8d5a-5d3a-546e-59924dc71ae1&t=b5052aa0-c7a8-5238-40d6-4558d53c7300&l=117
I like the direction this is heading, thanks for taking this on.
layout.append(_given_layout) | ||
layout.append(_choose_layout, condition=_choose_layout_condition) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason not to include these in generate_embed_passmanager
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC it's because the condition differs between the optimization levels. See my comment below (which I meant to put here not there, but it kind of works for either). Basically I was trying to keep things in common isolated to just the pieces which are shared completely between each optimization level. That way you can got to a specific optimization level's code and see what is shared and what is unique.
translation=translation, | ||
pre_optimization=pre_opt, | ||
scheduling=sched, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a step in the right direction, but I think this leaves the preset pass managers in a state that's somewhat harder to read and reason about, because some of the stages are generated elsewhere and some (and their control flow) are generated here.
Could we push all the control flow in to the generate_*
functions (so that every full pass manager has all the same stages, just some may be no-ops depending on the options or backend) with either different functions or arguments for the different optimization levels?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I did it this way because the control flow and logic around layout was different between the optimization levels. Like 0 is:
if initial_layout is not None:
layout = initial_layout
else:
layout = tivial_layout()
but level 1 is:
if initial_layout is not None:
layout = initial_layout
elif trivial_layout().is_perfect():
layout = trivial_layout()
else:
layout = DenseLayout()
but 2 and 3 are:
if initial_layout is not None:
layout = initial_layout
elif trivial_layout().is_perfect():
layout = trivial_layout()
elif csplayout.converges():
layout = csplayout()
else:
layout = DenseLayout()
(where the parameters for csp differ)
I was trying to keep things in common isolated to just the pieces which are shared completely between each optimization level. That way you can got to a specific optimization level's code and see what is shared and what is unique.
Co-authored-by: Kevin Krsulich <kevin@krsulich.net>
Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
…/common.py Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's get this merged! Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments and bugs :)
or (attr.startswith("pre_") and attr[4:] not in self.stages) | ||
or (attr.startswith("post_") and attr[5:] not in self.stages) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be this?
or (attr.startswith("pre_") and attr[4:] not in self.stages) | |
or (attr.startswith("post_") and attr[5:] not in self.stages) | |
or (attr.startswith("pre_") and attr[4:] in self.stages) | |
or (attr.startswith("post_") and attr[5:] in self.stages) |
def _update_passmanager(self): | ||
self._pass_sets = [] | ||
for stage in self.stages: | ||
# Add pre-stage PM | ||
pre_stage_pm = getattr(self, "pre_" + stage, None) | ||
if pre_stage_pm is not None: | ||
self._pass_sets.extend(pre_stage_pm._pass_sets) | ||
# Add stage PM | ||
stage_pm = getattr(self, stage, None) | ||
if stage_pm is not None: | ||
self._pass_sets.extend(stage_pm._pass_sets) | ||
# Add post-stage PM | ||
post_stage_pm = getattr(self, "post_" + stage, None) | ||
if post_stage_pm is not None: | ||
self._pass_sets.extend(post_stage_pm._pass_sets) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would happen if I assign a StagedPassManager
as a stage of itself (i.e. recursively)? Wouldn't _update_passmanager()
become unstable?
def remove(self, index: int) -> None: | ||
raise NotImplementedError | ||
|
||
def __getitem(self, index): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def __getitem(self, index): | |
def __getitem__(self, index): |
self._update_passmanager() | ||
return super()._create_running_passmanager() | ||
|
||
def passes(self) -> List[Dict[str, BasePass]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it make sense for this to be a @property
?
For some time (I expect since Qiskit#6403 but maybe before) we've been running the optimization loop for optimization level 3 twice for no reason. This is clearly a mistake as it adds a single extra run of all the optimization passes since we've already reached a steady state in depth and size by this point. This is just a waste of time and was unintentional. This commit removes the unintended duplication.
For some time (I expect since #6403 but maybe before) we've been running the optimization loop for optimization level 3 twice for no reason. This is clearly a mistake as it adds a single extra run of all the optimization passes since we've already reached a steady state in depth and size by this point. This is just a waste of time and was unintentional. This commit removes the unintended duplication. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Summary
This commit adds a new
PassManager
subclass,StragedPassManager
. This classis used to have a
PassManager
with a defined structure and stages. For example,when running the normal
transpile()
workflow. The preset pass managers are then updatedto be
StragedPassManager
objects they conform to the fixed structure. Havinga class with defined phases gives us flexibility in the future for making
the transpiler pluggable with external plugins (similar to what's done in
PR #6124) and also have backend hook points before or after different
phases of the transpile.
Details and comments
Fixes #5978