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

Optimization stage as a plugin. #10581

Merged
merged 8 commits into from
Aug 22, 2023
Merged

Conversation

raynelfss
Copy link
Contributor

@raynelfss raynelfss commented Aug 7, 2023

Summary

Fixes #9457.
These commits aim to add the Optimization stage of transpile as a PassManager plugin, with the purpose of re-packaging current transpilation stages to work the same way external plugins do.

Details and comments

  • Open to review.

@raynelfss raynelfss requested a review from a team as a code owner August 7, 2023 21:10
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Aug 7, 2023
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

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

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Aug 8, 2023

Pull Request Test Coverage Report for Build 5825260943

  • 52 of 52 (100.0%) changed or added relevant lines in 5 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.02%) to 87.261%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 3 91.39%
Totals Coverage Status
Change from base Build 5825072876: 0.02%
Covered Lines: 74263
Relevant Lines: 85104

💛 - Coveralls

@mtreinish mtreinish added Intern PR PR submitted by IBM Quantum interns and removed Community PR PRs from contributors that are not 'members' of the Qiskit repo labels Aug 9, 2023
@mtreinish mtreinish added Changelog: New Feature Include in the "Added" section of the changelog mod: transpiler Issues and PRs related to Transpiler labels Aug 14, 2023
@mtreinish
Copy link
Member

I think we should hold on this until #10621 merges. Then we can update the plugin construction to just call the plugin instead of needing the more involved logic.

This commit changes the plugin construction logic slightly to adjust
how the embedded translation stage is created. After Qiskit#10621 merged we
only need to use the plugin interface to get the translation stage pass
manager.
@mtreinish mtreinish self-assigned this Aug 22, 2023
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

This looks fine to me.

Comment on lines +485 to +488
if optimization_level == 3:
optimization.append(_minimum_point_check)
else:
optimization.append(_depth_check + _size_check)
Copy link
Member

Choose a reason for hiding this comment

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

Pre-existing, but I don't remember why we don't use the same check at all optimisation levels; there doesn't appear to be any discussion in #9612, but I know we talked about it more offline.

Copy link
Member

@mtreinish mtreinish Aug 22, 2023

Choose a reason for hiding this comment

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

It's a runtime thing. MinimumPoint adds a lot of overhead for the deep copying on potentially each iteration to do the rollback. We only added it to O3 in #9612 because the real need for the pass was coming from the numeric precision in UnitarySynthesis with the 2 qubit optimization causing the instability. For the optimizations we do in level 1 and 2 they're more stable (at least currently) and doing the fixed point comparison was reliable and significantly faster.

@jakelishman jakelishman added this pull request to the merge queue Aug 22, 2023
Merged via the queue into Qiskit:main with commit e7808e7 Aug 22, 2023
14 checks passed
@raynelfss raynelfss deleted the optimize_plugin branch August 22, 2023 19:41
SamD-1998 pushed a commit to SamD-1998/qiskit-terra that referenced this pull request Sep 7, 2023
* Initial: Optimization stage as a plugin.

* CI: Added fix to preset_passmanager test

* Simplify inner translation stage creation logic

This commit changes the plugin construction logic slightly to adjust
how the embedded translation stage is created. After Qiskit#10621 merged we
only need to use the plugin interface to get the translation stage pass
manager.

* Update documentation

---------

Co-authored-by: Matthew Treinish <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog Intern PR PR submitted by IBM Quantum interns mod: transpiler Issues and PRs related to Transpiler
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Create optimization stage (there is a different PM for each optimization level) plugins
5 participants