Legacy code removal in pass manager #11448

nkanazawa1989 · 2023-12-21T08:40:37Z

Summary

This PR is a followup of #10127 in which several methods and classes were deprecated. This PR removes deprecated code, and introduces new pattern to build pass pipeline.

Details and comments

The main goal of this PR is to remove following pattern to a build pass pipeline.

pm = PassManager()
pm.append([my_pass1, my_pass2, my_pass3], condition=callable1, do_while=callable2)

This pattern implicitly calls a factory function to create flow controller instances and wrap the passes my_pass* with them, by digesting the controller namespace defined in FlowController class attributes. This also internally tracks the group of passes appended together along with the specified controller names, with the extra overhead of memory footprint., i.e. PassManager._pass_sets.

With this PR, this append pattern is almost eliminated. Users now need to explicitly instantiate the controller class instead of using the syntactic sugar above. This drastically simplifies the factory logic, and the user can also precisely manage the building of nested controllers.

In addition to this, following classes and methods are also removed.

.append method of flow controller subclasses 937567a
RunningPassManager which is a subclass of FlowControllerLinear e02f6f5
Fenced objects a6c4916
max_iteration argument in append and replace 3b1161e

qiskit-bot · 2023-12-21T08:40:42Z

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

@enavarro51
@Qiskit/terra-core
@nkanazawa1989

This is necessary because kwargs is going to be removed from append method. Current pass list is generated based on the kwargs.

This also removes FlowController factory class

coveralls · 2023-12-21T09:12:35Z

Pull Request Test Coverage Report for Build 7417016020

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.08%) to 87.608%

Totals
Change from base Build 7282035075:	0.08%
Covered Lines:	59315
Relevant Lines:	67705

💛 - Coveralls

jakelishman

For the most part this looks sensible to me, though it's rather hard to verify that the preset passmanagers have stayed logically the same; it perhaps would have been easier to do the simple one-to-one replacements in this PR, and have a follow-up that does the refactoring of the logic, so the two can be checked separately. I tried to check them, but I'm not certain I'd have spotted a potential mistake.

Other than that, mostly just the additional question about transpiler.PassManager.passes - I'm not certain that keeping around the list format is a good idea, especially if it's in principal only for the visualisation. Exposing a tasks() method that returns a copy of the raw underlying list seems fine, though (maybe even on BasePassManager).

jakelishman · 2023-12-22T13:16:35Z

qiskit/passmanager/flow_controllers.py

-            raise KeyError("Flow controller not found: %s" % name)
-        del cls.registered_controllers[name]
-        cls.hierarchy.remove(name)
+FlowController = BaseController


Do we need to keep this name around now, or can we just rely on BaseController?

Not necessary. The alias is removed in 6e6d90e. I was just worried about deprecation of import path.

jakelishman · 2023-12-22T13:21:56Z

qiskit/transpiler/passmanager.py

+def _write_passes_recursive(tasks: BaseController | GenericPass | Sequence):
+    """Write passmanager passes list for visualization.
+
+    Conventionally, conditional flow controllers were initialized through .append() method
+    with keyword arguments.
+
+      pm = PassManager()
+      pm.append([PassA, PassB])
+      pm.append([PassC, PassD], condition=callable)
+      pm.append([PassE])


If this function is purely for visualisation, it should be in the pass_manager_visualization module, most likely. We should probably deprecate and remove PassManager.passes in favour of the flat PassManager.tasks, which I think already exists?

We shouldn't attempt to keep the recursive list structure around into the future; we already know that it's insufficient to represent even existing flow controllers, let alone future ones in the new structure.

Yeah this makes perfect sense. Actually drawer doesn't need to rely on this pass representation. This method and representation are removed in e3a991b. I also need to update #11449 to add deprecation for the method.

This reverts commit 4bcfcd6.

nkanazawa1989 · 2024-01-05T02:13:24Z

I reverted the commit for builtin passmanagers in 98c2ccf and added minimum change to them just to avoid deprecated code in 2f48717. In the followup PR #11494 builtin pass managers are refactored to eliminate use of the append pattern. This PR must be merged first, then I'll rebase the followup PR.

jakelishman

Thanks Naoki for making the modifications to this - it was 100x easier to check the changes to the preset pass managers, and it's clear that the vast majority of cases have 1:1 replacements that are strictly more powerful now.

jakelishman · 2024-01-10T13:53:33Z

releasenotes/notes/upgrade-pass-manager-98aa64edde67b5bb.yaml

+        )
+      )
+
+    Note that you can manage the pecking order of controllers when you want to nest them,


pecking order

I love this description!

jakelishman · 2024-01-10T13:55:39Z

test/python/transpiler/test_pass_scheduler.py



-class DoXTimesController(FlowController):
-    """A control-flow plugin for running a set of passes an X amount of times."""
-
-    def __init__(self, passes, options, do_x_times, **_):
-        super().__init__(options)
-        self.passes = passes
-        self.do_x_times = do_x_times
-
-    # pylint: disable=missing-function-docstring
-    def iter_tasks(self, metadata):
-        for _ in range(self.do_x_times(metadata.property_set)):
-            for pass_ in self.passes:
-                metadata = yield pass_
-
-


This is removed because it's tested as part of qiskit.passmanager elsewhere, I assume?

* Remove flow controller subclass .append * Update builtin pass managers to avoid append pattern * Remove running pass manager * Remove PropertySet and Fenced objects from transpile * Remove _pass_sets and let PassManager generate pass list on the fly This is necessary because kwargs is going to be removed from append method. Current pass list is generated based on the kwargs. * Remove kwargs from PassManager.append and .replace This also removes FlowController factory class * Remove max_iteration keyword from append and replace * Reno * minor: fix test and docs * Remove qiskit.passmanager.flow_controllers.FlowController * Remove .passes method from PassManager and StagedPassManager * Update reference image of pass manager drawer test * minor: fix drawer label generation * Revert "Update builtin pass managers to avoid append pattern" This reverts commit 4bcfcd6. * Minimum update of builtin pass managers

nkanazawa1989 added Changelog: API Change Include in the "Changed" section of the changelog mod: transpiler Issues and PRs related to Transpiler labels Dec 21, 2023

nkanazawa1989 added this to the 1.0.0 milestone Dec 21, 2023

nkanazawa1989 requested review from nonhermitian and a team as code owners December 21, 2023 08:40

nkanazawa1989 mentioned this pull request Dec 21, 2023

Legacy code deprecation in pass manager #11449

Merged

nkanazawa1989 added 8 commits December 21, 2023 17:44

Remove flow controller subclass .append

4c6fa41

Update builtin pass managers to avoid append pattern

4bcfcd6

Remove running pass manager

2ba369f

Remove PropertySet and Fenced objects from transpile

577946c

Remove _pass_sets and let PassManager generate pass list on the fly

890e3b7

This is necessary because kwargs is going to be removed from append method. Current pass list is generated based on the kwargs.

Remove kwargs from PassManager.append and .replace

2c1e295

This also removes FlowController factory class

Remove max_iteration keyword from append and replace

c7df061

Reno

7cf2358

nkanazawa1989 force-pushed the upgrade/pass_manager_cleanup branch from 778428e to 7cf2358 Compare December 21, 2023 08:49

nkanazawa1989 assigned jakelishman Dec 21, 2023

minor: fix test and docs

5a5a631

jakelishman reviewed Dec 22, 2023

View reviewed changes

nkanazawa1989 added 6 commits January 5, 2024 01:47

Remove qiskit.passmanager.flow_controllers.FlowController

6e6d90e

Remove .passes method from PassManager and StagedPassManager

e3a991b

Update reference image of pass manager drawer test

2ff6243

minor: fix drawer label generation

9062d52

Revert "Update builtin pass managers to avoid append pattern"

98c2ccf

This reverts commit 4bcfcd6.

Minimum update of builtin pass managers

2f48717

nkanazawa1989 mentioned this pull request Jan 5, 2024

Update built-in pass managers (followup of #11448) #11494

Open

jakelishman approved these changes Jan 10, 2024

View reviewed changes

jakelishman added this pull request to the merge queue Jan 10, 2024

jakelishman added the Changelog: Removal Include in the Removed section of the changelog label Jan 10, 2024

Merged via the queue into Qiskit:main with commit b8d21ae Jan 10, 2024
13 checks passed

mtreinish mentioned this pull request Jan 30, 2024

Cleanup internal usage of qiskit.passmanager in qiskit.transpiler to avoid warnings #11369

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Legacy code removal in pass manager #11448

Legacy code removal in pass manager #11448

nkanazawa1989 commented Dec 21, 2023 •

edited

Loading

qiskit-bot commented Dec 21, 2023

coveralls commented Dec 21, 2023 •

edited

Loading

jakelishman left a comment

jakelishman Dec 22, 2023

nkanazawa1989 Jan 4, 2024

jakelishman Dec 22, 2023

nkanazawa1989 Jan 4, 2024

nkanazawa1989 commented Jan 5, 2024

jakelishman left a comment

jakelishman Jan 10, 2024

jakelishman Jan 10, 2024

Legacy code removal in pass manager #11448

Legacy code removal in pass manager #11448

Conversation

nkanazawa1989 commented Dec 21, 2023 • edited Loading

Summary

Details and comments

qiskit-bot commented Dec 21, 2023

coveralls commented Dec 21, 2023 • edited Loading

Pull Request Test Coverage Report for Build 7417016020

Warning: This coverage report may be inaccurate.

💛 - Coveralls

jakelishman left a comment

Choose a reason for hiding this comment

jakelishman Dec 22, 2023

Choose a reason for hiding this comment

nkanazawa1989 Jan 4, 2024

Choose a reason for hiding this comment

jakelishman Dec 22, 2023

Choose a reason for hiding this comment

nkanazawa1989 Jan 4, 2024

Choose a reason for hiding this comment

nkanazawa1989 commented Jan 5, 2024

jakelishman left a comment

Choose a reason for hiding this comment

jakelishman Jan 10, 2024

Choose a reason for hiding this comment

jakelishman Jan 10, 2024

Choose a reason for hiding this comment

nkanazawa1989 commented Dec 21, 2023 •

edited

Loading

coveralls commented Dec 21, 2023 •

edited

Loading