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 #12780

Merged
merged 3 commits into from
Jul 29, 2024

Conversation

mtreinish
Copy link
Member

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

@mtreinish mtreinish added Changelog: New Feature Include in the "Added" section of the changelog mod: transpiler Issues and PRs related to Transpiler labels Jul 17, 2024
@mtreinish mtreinish added this to the 1.2.0 milestone Jul 17, 2024
@mtreinish mtreinish requested a review from a team as a code owner July 17, 2024 21:28
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core

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.
@coveralls
Copy link

coveralls commented Jul 17, 2024

Pull Request Test Coverage Report for Build 10081309595

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 18 of 22 (81.82%) changed or added relevant lines in 2 files are covered.
  • 694 unchanged lines in 33 files lost coverage.
  • Overall coverage decreased (-0.01%) to 89.905%

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/circuit/library/standard_gates/swap.py 1 98.18%
qiskit/circuit/duration.py 1 70.27%
crates/accelerate/src/synthesis/permutation/utils.rs 1 99.02%
qiskit/transpiler/passes/layout/sabre_pre_layout.py 1 98.88%
qiskit/circuit/random/utils.py 1 94.87%
qiskit/transpiler/passes/routing/commuting_2q_gate_routing/commuting_2q_gate_router.py 1 99.12%
qiskit/transpiler/preset_passmanagers/builtin_plugins.py 2 95.77%
qiskit/transpiler/passes/scheduling/alap.py 2 96.72%
qiskit/transpiler/passes/scheduling/asap.py 2 96.92%
crates/accelerate/src/convert_2q_block_matrix.rs 2 94.0%
Totals Coverage Status
Change from base Build 9976077291: -0.01%
Covered Lines: 65869
Relevant Lines: 73265

💛 - Coveralls

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

LGTM, not that I'll ever have the chance to see the effects of the option in my laptop 😝. I just left a few suggestions on the reno.

mtreinish and others added 2 commits July 18, 2024 07:23
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
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.
@mtreinish mtreinish requested a review from eliarbel July 25, 2024 15:21
Copy link
Contributor

@eliarbel eliarbel left a comment

Choose a reason for hiding this comment

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

LGTM

@mtreinish mtreinish requested a review from ElePT July 29, 2024 11:33
Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

LGTM too

@ElePT ElePT added this pull request to the merge queue Jul 29, 2024
@ElePT ElePT added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Jul 29, 2024
Merged via the queue into Qiskit:main with commit f8ac2ad Jul 29, 2024
15 checks passed
mergify bot pushed a commit that referenced this pull request Jul 29, 2024
* 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)
github-merge-queue bot pushed a commit that referenced this pull request Jul 29, 2024
* 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)

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Procatv pushed a commit to Procatv/qiskit-terra-catherines that referenced this pull request Aug 1, 2024
* 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>
mtreinish added a commit to mtreinish/qiskit-documentation that referenced this pull request Aug 15, 2024
In Qiskit 1.2.0 a new env variable was added to control the threading
behavior of the routing and layout passes in the preset pass managers.
You can see the details of this new option in the 1.2.0 release notes or
in the PR Qiskit/qiskit#12780. This commit adds the documentation for
this feature to the local configuration guide.
github-merge-queue bot pushed a commit to Qiskit/documentation that referenced this pull request Aug 16, 2024
In Qiskit 1.2.0 a new env variable was added to control the threading
behavior of the routing and layout passes in the preset pass managers.
You can see the details of this new option in the 1.2.0 release notes or
in the PR Qiskit/qiskit#12780. This commit adds the documentation for
this feature to the local configuration guide.

---------

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Co-authored-by: abbycross <across@us.ibm.com>
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 stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants