Skip to content

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

Closed
nkanazawa1989 wants to merge 1 commit into
Qiskit:mainfrom
nkanazawa1989:upgrade/builtin-pms
Closed

Update built-in pass managers (followup of #11448)#11494
nkanazawa1989 wants to merge 1 commit into
Qiskit:mainfrom
nkanazawa1989:upgrade/builtin-pms

Conversation

@nkanazawa1989
Copy link
Copy Markdown
Contributor

Summary

This PR is a followup of #11448 to keep logical change minimum. This PR overhauls built-in pass managers. PassManager.append pattern is almost removed from them since this is no longer necessary.

Details and comments

PassManager can be instantiated with predefined pass sequence even though they include conditional or do-while controllers as follows

pm = PassManager(
    ConditionalController(
        DoWhileController(
            [my_pass1, my_pass2, my_pass3], 
            do_while=callable2
        ), 
        condition=callable1
    )
)

The append pattern may be necessary when the pipeline changes with some condition (conditional branching):

if some_condition:
  pm.append(some_passes)
else:
  pm.append(some_other_passes)

In principle this can be also updated by introducing something like IfElseController. Then PassManager.append can be also dropped like flow controllers. However this is not the scope of this PR.

In addition, some inefficiency is removed with this PR. In the current code, following pattern is often used

pm_stage += some_other_pm

this implicitly builds new instance instead of mutating the original instance, thus it always comes with extra memory overhead (the original instance is discarded by GC though).

@nkanazawa1989 nkanazawa1989 requested a review from a team as a code owner January 5, 2024 02:10
@qiskit-bot
Copy link
Copy Markdown
Collaborator

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

  • @Qiskit/terra-core

@nkanazawa1989 nkanazawa1989 added this to the 1.0.0 milestone Jan 5, 2024
@nkanazawa1989 nkanazawa1989 added the mod: transpiler Issues and PRs related to Transpiler label Jan 5, 2024
@nkanazawa1989
Copy link
Copy Markdown
Contributor Author

Blocked by #11448

@coveralls
Copy link
Copy Markdown

coveralls commented Jan 5, 2024

Pull Request Test Coverage Report for Build 7580069842

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.001%) to 89.495%

Totals Coverage Status
Change from base Build 7576041329: -0.001%
Covered Lines: 59411
Relevant Lines: 66385

💛 - Coveralls

@jakelishman
Copy link
Copy Markdown
Member

Sorry I didn't get to this sooner. Since it's just an internal code refactor, it's eligible for 1.1 and I'll defer it til then.

@jakelishman jakelishman modified the milestones: 1.0.0, 1.1.0 Feb 1, 2024
@jakelishman jakelishman modified the milestones: 1.1.0, 1.2.0 Apr 18, 2024
@ElePT ElePT modified the milestones: 1.2.0, 1.3.0, 1.4.0 Jul 23, 2024
@ElePT
Copy link
Copy Markdown
Contributor

ElePT commented Feb 13, 2025

Thanks a lot for opening this PR @nkanazawa1989, unfortunately, I believe that the code base has diverged quite a bit from the original state that was considered in this PR. I will close it for backlog managing purposes.

@ElePT ElePT closed this Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mod: transpiler Issues and PRs related to Transpiler

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants