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

Add config option to leverage all cores for sabre (backport #12780) #12841

Merged
merged 1 commit into from
Jul 29, 2024

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Jul 29, 2024

Summary

By default when running sabre in parallel we use a fixed number of threads (depending on optimization level). This was a tradeoff made for having deterministic results across multiple systems with a fixed seed set. However when running qiskit on systems with a lot of CPUs available we're leaving potential performance on the table by not using all the available cores. This new flag lets users opt-in to running sabre with n trials for n CPUs to potentially get better output results from the transpiler, with minimal to no runtime overhead, at the cost of the results not necessarily being reproducible when run on a different computer.

Details and comments
This is an automatic backport of pull request #12780 done by Mergify.

* Add config option to leverage all cores for sabre

By default when running sabre in parallel we use a fixed number of
threads (depending on optimization level). This was a tradeoff made for
having deterministic results across multiple systems with a fixed seed
set. However when running qiskit on systems with a lot of CPUs
available we're leaving potential performance on the table by not using
all the available cores. This new flag lets users opt-in to running
sabre with n trials for n CPUs to potentially get better output results
from the transpiler, with minimal to no runtime overhead, at the cost of
the results not necessarily being reproducible when run on a different
computer.

* Apply suggestions from code review

Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>

* Rework logic to use the default if larger than CPU_COUNT

This commit refactors the logic added in the previous commit to a single
helper function. This reduces the code duplication and makes it easier
to work with. While doing this the logic has been updated so that when
the flag is set and the default number of trials is larger than the
CPU_COUNT we use the default. This means the logic when the flag is set
is to run `max(default_trials, CPU_COUNT)` which should better match
user expectations around the flag.

---------

Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
(cherry picked from commit f8ac2ad)
@mergify mergify bot requested a review from a team as a code owner July 29, 2024 13:43
@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 following people are relevant to this code:

  • @Qiskit/terra-core

@github-actions github-actions bot added the Changelog: New Feature Include in the "Added" section of the changelog label Jul 29, 2024
@github-actions github-actions bot added this to the 1.2.0 milestone Jul 29, 2024
@github-actions github-actions bot added the mod: transpiler Issues and PRs related to Transpiler label Jul 29, 2024
@mtreinish mtreinish enabled auto-merge July 29, 2024 13:58
@coveralls
Copy link

Pull Request Test Coverage Report for Build 10145508314

Details

  • 18 of 22 (81.82%) changed or added relevant lines in 2 files are covered.
  • 6 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.02%) to 89.96%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/user_config.py 2 3 66.67%
qiskit/transpiler/preset_passmanagers/builtin_plugins.py 16 19 84.21%
Files with Coverage Reduction New Missed Lines %
qiskit/transpiler/preset_passmanagers/builtin_plugins.py 2 95.77%
crates/qasm2/src/lex.rs 4 92.37%
Totals Coverage Status
Change from base Build 10144976788: 0.02%
Covered Lines: 66228
Relevant Lines: 73619

💛 - Coveralls

@mtreinish mtreinish added this pull request to the merge queue Jul 29, 2024
Merged via the queue into stable/1.2 with commit e29e7c0 Jul 29, 2024
18 checks passed
@mergify mergify bot deleted the mergify/bp/stable/1.2/pr-12780 branch July 29, 2024 17:04
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 mod: transpiler Issues and PRs related to Transpiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants