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

Deprecate lists for argument input on transpile() #8835

Merged
merged 7 commits into from
Oct 17, 2022

Conversation

mtreinish
Copy link
Member

Summary

Right now the support for the argument broadcasting with list inputs for various transpiler options on the transpile() function causes a significant performance overhead to support, primarily do to how we have to handle the multiple arguments across a parallel dispatch boundary. It also significantly increases the code complexity of the function to support more than one input for each argument (except circuits). The utility of doing this type of argument handling is quite limited since a similar result can be achieved with a for loop and would like be simpler for users to reason about. When weighing all these factors the best path forward is to just remove this functionality. This commit starts the process of removing this feature by marking it as deprecated. Once the deprecation cycle is complete we can greatly simplify the code in transpile and primarily replace it with a call to
generate_preset_pass_manager() and passmanager.run() (the only thing I think we'll have to handle out of band is faulty qubits defined in a BackendProperties for BackendV1).

Related to #7741

Details and comments

Right now the support for the argument broadcasting with list inputs for
various transpiler options on the transpile() function causes a
significant performance overhead to support, primarily do to how we have
to handle the multiple arguments across a parallel dispatch boundary. It
also significantly increases the code complexity of the function to
support more than one input for each argument (except circuits). The
utility of doing this type of argument handling is quite limited since a
similar result can be achieved with a for loop and would like be simpler
for users to reason about. When weighing all these factors the best path
forward is to just remove this functionality. This commit starts the
process of removing this feature by marking it as deprecated. Once the
deprecation cycle is complete we can greatly simplify the code in
transpile and primarily replace it with a call to
generate_preset_pass_manager() and passmanager.run() (the only thing I
think we'll have to handle out of band is faulty qubits defined in a
BackendProperties for BackendV1).

Related to Qiskit#7741
@mtreinish mtreinish requested a review from a team as a code owner October 4, 2022 16:33
@mtreinish mtreinish added this to the 0.23.0 milestone Oct 4, 2022
@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

@mtreinish mtreinish added the Changelog: Deprecation Include in "Deprecated" section of changelog label Oct 4, 2022
@coveralls
Copy link

coveralls commented Oct 4, 2022

Pull Request Test Coverage Report for Build 3267941290

  • 4 of 5 (80.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.007%) to 84.757%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/compiler/transpiler.py 4 5 80.0%
Totals Coverage Status
Change from base Build 3267684487: 0.007%
Covered Lines: 61915
Relevant Lines: 73050

💛 - Coveralls

Copy link
Contributor

@kevinhartman kevinhartman left a comment

Choose a reason for hiding this comment

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

LGTM, other than a few documentation-related requests.

qiskit/compiler/transpiler.py Outdated Show resolved Hide resolved
qiskit/compiler/transpiler.py Outdated Show resolved Hide resolved
kevinhartman
kevinhartman previously approved these changes Oct 17, 2022
@mergify mergify bot merged commit 696e53d into Qiskit:main Oct 17, 2022
@mtreinish mtreinish deleted the deprecate-transpile-broadcast branch October 17, 2022 21:45
ajavadia pushed a commit to ajavadia/qiskit that referenced this pull request Oct 18, 2022
* Deprecate lists for argument input on transpile()

Right now the support for the argument broadcasting with list inputs for
various transpiler options on the transpile() function causes a
significant performance overhead to support, primarily do to how we have
to handle the multiple arguments across a parallel dispatch boundary. It
also significantly increases the code complexity of the function to
support more than one input for each argument (except circuits). The
utility of doing this type of argument handling is quite limited since a
similar result can be achieved with a for loop and would like be simpler
for users to reason about. When weighing all these factors the best path
forward is to just remove this functionality. This commit starts the
process of removing this feature by marking it as deprecated. Once the
deprecation cycle is complete we can greatly simplify the code in
transpile and primarily replace it with a call to
generate_preset_pass_manager() and passmanager.run() (the only thing I
think we'll have to handle out of band is faulty qubits defined in a
BackendProperties for BackendV1).

Related to Qiskit#7741

* Update qiskit/compiler/transpiler.py

Co-authored-by: Kevin Hartman <[email protected]>

* Update qiskit/compiler/transpiler.py

Co-authored-by: Kevin Hartman <[email protected]>

* Fix release note typo

Co-authored-by: Kevin Hartman <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Deprecation Include in "Deprecated" section of changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants