Skip to content
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

Pass manager refactoring: cleanup internals #10127

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
6505e20
Refactor internals of pass manager and flow controllers.
nkanazawa1989 May 18, 2023
3fb2464
Replace class attribute PASS_RUNNER with property method.
nkanazawa1989 Jun 24, 2023
830c279
Merge branch 'main' into feature/passmanager-refactoring-pr2
1ucian0 Jun 26, 2023
07fec44
Merge branch 'main' into feature/passmanager-refactoring-pr2
1ucian0 Jul 10, 2023
f0c96a1
Refactor passmanager module
nkanazawa1989 Jul 20, 2023
fdc1f53
Refactor transpiler passmanager
nkanazawa1989 Jul 20, 2023
5a03304
Rename module: base_pass -> base_optimization_tasks
nkanazawa1989 Jul 20, 2023
f77cf03
Backward compatibility fix
nkanazawa1989 Jul 20, 2023
8a46f2b
Update module documentation
nkanazawa1989 Aug 5, 2023
ac12c6d
Update release note
nkanazawa1989 Aug 5, 2023
19987f0
Merge branch 'feature/passmanager-refactoring-pr2' of github.com:nkan…
nkanazawa1989 Aug 5, 2023
687590e
lint fix
nkanazawa1989 Aug 5, 2023
a5c8417
OptimizationTask -> Task
nkanazawa1989 Sep 22, 2023
65fc13d
Typo fix FlowControllerLiner -> FlowControllerLinear
nkanazawa1989 Sep 22, 2023
5034cc4
Use RunState value. Set 0 to success case.
nkanazawa1989 Sep 22, 2023
305fa58
Separate property set from pass state. Now Task.execute method has tw…
nkanazawa1989 Oct 10, 2023
e526bff
Pending deprecate fenced_property_set and remove usage.
nkanazawa1989 Oct 10, 2023
2051412
Pending deprecate FencedObject baseclass and remove usage.
nkanazawa1989 Oct 13, 2023
6211a5d
Add some inline comments.
nkanazawa1989 Oct 10, 2023
a18dd91
Convert RunState to raw Enum
nkanazawa1989 Oct 10, 2023
e20949a
Change the policy for the use of multiple IRs. Just updated module do…
nkanazawa1989 Oct 10, 2023
5701447
Updated interface of base controller. Namely .append() and .passes ar…
nkanazawa1989 Oct 13, 2023
59837b3
Remove FlowControllerMeta and turn FlowController into a subclass of …
nkanazawa1989 Oct 13, 2023
ca7dba6
Remove dependency on controller_factory method and deprecate.
nkanazawa1989 Oct 13, 2023
2c11019
Update release note
nkanazawa1989 Oct 13, 2023
f3912e8
Merge branch 'main' of github.com:Qiskit/qiskit-terra into feature/pa…
nkanazawa1989 Oct 13, 2023
ae14aa7
Readd code change in #10835
nkanazawa1989 Oct 13, 2023
388d1dd
typehint fix
nkanazawa1989 Oct 13, 2023
5958f79
doc fix
nkanazawa1989 Oct 13, 2023
dd666e9
move FencedPropertySet class back to transpiler.fencedobjs
nkanazawa1989 Oct 13, 2023
f28cad9
Temporary: add lint ignore
nkanazawa1989 Oct 16, 2023
9346ce5
Fix example code in class doc
nkanazawa1989 Oct 16, 2023
c56bf7c
Tweaks to documentation
jakelishman Oct 16, 2023
e211d66
Merge pull request #51 from jakelishman/pr-refactor-docs
nkanazawa1989 Oct 17, 2023
ed9a8d7
Change baseclass of the MetaPass and revert f28cad9
nkanazawa1989 Oct 17, 2023
d889ac0
Update interface of execute and use generator feature.
nkanazawa1989 Oct 17, 2023
7fba347
Update deprecations
nkanazawa1989 Oct 17, 2023
b9348ae
Delay instantiation of FlowControllerLinear until execution of pass m…
nkanazawa1989 Oct 18, 2023
da6c810
Stop wrapping a list of tasks with flow controller linear.
nkanazawa1989 Oct 18, 2023
31fcf67
Remove flow_controller_conditions from base controller append and add…
nkanazawa1989 Oct 18, 2023
f0a547a
Misc updates
nkanazawa1989 Oct 18, 2023
5d9f16e
disable cyclic-import
nkanazawa1989 Oct 18, 2023
df0ee05
Remove typecheck
nkanazawa1989 Oct 18, 2023
d7cdf8f
Rename `PassmanagerMetadata` to `PassManagerState`
jakelishman Oct 18, 2023
f140148
Correct warning stacklevel
jakelishman Oct 19, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion qiskit/passmanager/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
:toctree: ../stubs/

PropertySet
FuturePropertySet

Exceptions
----------
Expand All @@ -102,5 +103,5 @@
from .passmanager import BasePassManager
from .flow_controllers import FlowController, ConditionalController, DoWhileController
from .base_pass import GenericPass
from .propertyset import PropertySet
from .propertyset import PropertySet, FuturePropertySet
from .exceptions import PassManagerError
17 changes: 17 additions & 0 deletions qiskit/passmanager/base_pass.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,24 @@ def __init__(self):
def __hash__(self):
return self._hash

def __iter__(self):
# To avoid normalization overhead.
# A pass must be scheduled in iterable format in the pass manager,
# which causes serious normalization overhead, since passes can be nested.
# This iterator allows the pass manager to treat BasePass and FlowController equally.
yield self

def __len__(self):
return 1

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@nkanazawa1989 nkanazawa1989 Aug 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

https://github.com/Qiskit/qiskit-terra/blob/28113b6a9fc1fd31bc211a73fe7a6908fffa1114/test/python/transpiler/test_pass_scheduler.py#L798-L819

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.


def name(self):
Expand Down
94 changes: 81 additions & 13 deletions qiskit/passmanager/flow_controllers.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,13 @@
"""Pass flow controllers to provide pass iterator conditioned on the property set."""
from __future__ import annotations
from collections import OrderedDict
from collections.abc import Sequence
from typing import Union, List
from collections.abc import Sequence, Callable
from functools import partial
from typing import List, Union, Any
import logging

from .base_pass import GenericPass
from .propertyset import FuturePropertySet
from .exceptions import PassManagerError

logger = logging.getLogger(__name__)
Expand All @@ -32,7 +34,21 @@ class FlowController:

registered_controllers = OrderedDict()

def __init__(self, passes, options, **partial_controller):
def __init__(
self,
passes: Sequence[GenericPass | "FlowController"],
nkanazawa1989 marked this conversation as resolved.
Show resolved Hide resolved
options: dict,
**partial_controller: Callable,
):
"""Create new flow controller.

Args:
passes: passes to add to the flow controller. This must be normalized.
options: PassManager options.
**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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self.options = options
Expand Down Expand Up @@ -71,6 +87,7 @@ def remove_flow_controller(cls, name):

Args:
name (string): Name of the controller to remove.

Raises:
KeyError: If the controller to remove was not registered.
"""
Expand All @@ -90,28 +107,59 @@ def controller_factory(
Args:
passes: passes to add to the flow controller.
options: PassManager options.
**partial_controller: Partially evaluated controller arguments in the form `{name:partial}`
**partial_controller: Partially evaluated callables representing
a condition to invoke flow controllers. This dictionary
must be keyed on the registered controller name.
Callable may only take property set, and if the property set is unbound,
the factory method implicitly binds :class:`.FuturePropertySet`.
When multiple controllers and conditions are provided, this recursively
initializes controllers according to the priority defined by
the key order of the :attr:`FlowController.registered_controllers`.

Raises:
PassManagerError: When partial_controller is not well-formed.

Returns:
FlowController: A FlowController instance.
"""
if not _is_passes_sequence(passes):
raise PassManagerError(
f"{passes.__class__} is not a valid BasePass of FlowController instance."
)
if None in partial_controller.values():
raise PassManagerError("The controller needs a condition.")

if partial_controller:
for registered_controller in cls.registered_controllers.keys():
if registered_controller in partial_controller:
return cls.registered_controllers[registered_controller](
passes, options, **partial_controller
)
raise PassManagerError("The controllers for %s are not registered" % partial_controller)

for key, value in partial_controller.items():
if callable(value) and not isinstance(value, partial):
partial_controller[key] = partial(value, FuturePropertySet())
for label, controller_cls in cls.registered_controllers.items():
if label in partial_controller:
return controller_cls(passes, options, **partial_controller)
raise PassManagerError(f"The controllers for {partial_controller} are not registered")
return FlowControllerLinear(passes, options)


def _is_passes_sequence(passes: Any) -> bool:
"""Check if input is valid pass sequence.

Valid pass sequence representation are:

* BasePass,
* FlowController,
* Sequence of BasePass or FlowController.

Note that nested sequence is not a valid pass sequence.
FlowController.passes must be validated in constructor.
"""
if isinstance(passes, (GenericPass, FlowController)):
return True
for inner_pass in passes:
if not isinstance(inner_pass, (GenericPass, FlowController)):
return False
return True
nkanazawa1989 marked this conversation as resolved.
Show resolved Hide resolved


class FlowControllerLinear(FlowController):
"""The basic controller runs the passes one after the other."""

Expand All @@ -123,7 +171,17 @@ def __init__(self, passes, options): # pylint: disable=super-init-not-called
class DoWhileController(FlowController):
"""Implements a set of passes in a do-while loop."""

def __init__(self, passes, options=None, do_while=None, **partial_controller):
def __init__(
self,
passes: PassSequence,
options: dict = None,
do_while: Callable = None,
**partial_controller: Callable,
):
if not callable(do_while):
raise PassManagerError("The flow controller parameter do_while is not callable.")
if not isinstance(do_while, partial):
do_while = partial(do_while, FuturePropertySet())
self.do_while = do_while
self.max_iteration = options["max_iteration"] if options else 1000
super().__init__(passes, options, **partial_controller)
Expand All @@ -141,7 +199,17 @@ def __iter__(self):
class ConditionalController(FlowController):
"""Implements a set of passes under a certain condition."""

def __init__(self, passes, options=None, condition=None, **partial_controller):
def __init__(
self,
passes: PassSequence,
options: dict = None,
condition: Callable = None,
**partial_controller: Callable,
):
if not callable(condition):
raise PassManagerError("The flow controller parameter condition is not callable.")
if not isinstance(condition, partial):
condition = partial(condition, FuturePropertySet())
self.condition = condition
super().__init__(passes, options, **partial_controller)

Expand Down
Loading