-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Refactor parallelism utilities for public API #12412
base: main
Are you sure you want to change the base?
Conversation
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 11633906056Details
💛 - Coveralls |
d6d1da9
to
0bdea9c
Compare
Now rebased over #12410. |
decision is dependent on how many CPUs are available to Qiskit, what the :mod:`multiprocessing` | ||
start method is, how many processes were requested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean sort of, we don't explicitly check what multiprocessing is set to (ie we ignore if the user explicitly sets this to spawn), but the value for PARALLEL_DEFAULT
is based on what the OS default start method is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think I was being more aspirational than correct here. We should do the check based on the multiprocessing
start method if that's what we care about, but we don't. I can change the wording.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should just do this, it seems totally in scope to update the logic in the function to call multiprocessing.get_start_method()
and get rid of the OS based logic here for 1.2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should just do this, it seems totally in scope to update the logic in the function to call multiprocessing.get_start_method() and get rid of the OS based logic here for 1.2.
@mtreinish is this still something we ought to do prior to merging this for 1.3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we still intended to do this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this, which then of course broke a bunch of weird other assumptions we were making thirty miles away in the library, so I ended up really rejigging the public API of the parallelism utilities so we didn't need to make internal assumptions across file boundaries.
I'm not entirely sold on the way I wrote the multiprocessing
test (see the commit message) - I was trying to match the spirit of the previous assumption-heavy code, but I'm not certain we couldn't do a shade better.
decision is dependent on how many CPUs are available to Qiskit, what the :mod:`multiprocessing` | ||
start method is, how many processes were requested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should just do this, it seems totally in scope to update the logic in the function to call multiprocessing.get_start_method() and get rid of the OS based logic here for 1.2.
@mtreinish is this still something we ought to do prior to merging this for 1.3?
0bdea9c
to
e4fea41
Compare
should_run_in_parallel
to public API`should_run_in_parallel` was added in a stable manner to enable backport to the 1.1 series, but from 1.3 onwards, we want this to be part of the public interface so that others can rely on it too. As part of this, the parallelisation configuration was made more robust and controllable with context managers. This is convenient beyond just for users - it makes it far easier to control the parallelism during the test suite runs. Several instances where different parts of Qiskit and its test suite reached into deep internals of the parallelism utilities and made significant assumptions about the internal logic are refactored to use public interfaces to achieve what they wanted to. The multiprocessing detection is changed from making OS-based assumptions about what Python does to simply querying the module for its configuration. This makes it more robust to changes in Python's handling (especially important since 3.14 will change the default start method on Unix). In the future, we may want to change to making these assumptions only if the user hasn't configured the `multiprocessing` start method themselves.
e4fea41
to
96dbd6e
Compare
I've force-pushed a major new commit that properly refactored a bunch of the parallelism utilities to better support I also added a |
Given how major the changes I made here were, and that we're after the feature freeze deadline, I'm fine if we choose to leave this for Qiskit 2.0. |
I agree, given the scope of the changes and that we're only one day out from rc1 lets defer this to 2.0. We can move forward with it pretty soon after rc1 though. |
Summary
should_run_in_parallel
was added in a stable manner to enable backport to the 1.1 series, but from 1.3 onwards, we want this to be part of the public interface so that others can rely on it too.As part of this, the parallelisation configuration was made more robust and controllable with context managers. This is convenient beyond just for users - it makes it far easier to control the parallelism during the test suite runs. Several instances where different parts of Qiskit and its test suite reached into deep internals of the parallelism utilities and made significant assumptions about the internal logic are refactored to use public interfaces to achieve what they wanted to.
The multiprocessing detection is changed from making OS-based assumptions about what Python does to simply querying the module for its configuration. This makes it more robust to changes in Python's handling (especially important since 3.14 will change the default start method on Unix). In the future, we may want to change to making these assumptions only if the user hasn't configured the
multiprocessing
start method themselves.Details and comments
This was elided from #12410 to make that PR backwards compatible. This PR exposes the feature as part of the public API, so will be new for 1.2.
Depends on #12410.