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

Parallelizing scheduling of multiple circuits via parallel_map. #8110

Merged
merged 10 commits into from
Jun 8, 2022

Conversation

upsideon
Copy link
Contributor

Summary

Fixes #2695 by using parallel_map instead of a list comprehension when scheduling multiple circuits to run on a backend.

Details and comments

A new file was added for unit tests corresponding to qiskit/compiler/scheduler.py, as such a file did not exist previously. The tests confirm that scheduling continues to operate as intended following these changes. Additional tests were added to cover code paths that raise exceptions.

@upsideon upsideon requested a review from a team as a code owner May 26, 2022 20:24
@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: New Feature Include in the "Added" section of the changelog label May 26, 2022
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

Thanks for taking on this task. Overall the code is straightforward enoguh and LGTM, just a few small inline comments in the test. Also before we merge this can you add a feature release note documenting that schedule() will execute in parallel for >1 circuit now so that we document this change. You can find the details on adding release notes

test/python/compiler/test_scheduler.py Outdated Show resolved Hide resolved
test/python/compiler/test_scheduler.py Outdated Show resolved Hide resolved
test/python/compiler/test_scheduler.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented May 26, 2022

Pull Request Test Coverage Report for Build 2461276100

  • 19 of 19 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 84.427%

Totals Coverage Status
Change from base Build 2460921856: 0.01%
Covered Lines: 54686
Relevant Lines: 64773

💛 - Coveralls

@upsideon upsideon requested a review from mtreinish May 31, 2022 19:24
@upsideon
Copy link
Contributor Author

upsideon commented Jun 6, 2022

@mtreinish Would you be willing to give these changes another review? Thank you!

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, this is looking better, just a couple of small inline comments mainly around the new tests and how you're monkey patch sys.

qiskit/test/base.py Outdated Show resolved Hide resolved
test/python/tools/test_parallel.py Outdated Show resolved Hide resolved
test/python/compiler/test_scheduler.py Outdated Show resolved Hide resolved
@upsideon
Copy link
Contributor Author

upsideon commented Jun 7, 2022

Thank you for the additional review, @mtreinish! I've addressed your most recent set of comments.

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for making the updates

@mergify mergify bot merged commit fb201bf into Qiskit:main Jun 8, 2022
@upsideon upsideon deleted the fixes-2695-scheduling-parallelization branch June 8, 2022 14:27
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimization: parallelize scheduler.schedule over circuits
4 participants