-
-
Notifications
You must be signed in to change notification settings - Fork 11.6k
CI, TST: Enable parallel threads testing in macOS CI job #30005
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
Conversation
|
Ugh, it looks like there are issues we'd need to fix in pytest-run-parallel before we can add it to the requirements file. Let's delete it from there for now and then explicitly install pytest-run-parallel in the parallel CI job. |
|
I opened scientific-python/spin#302 to track upstreaming the changes we're making to the |
|
Ping @rgommers - I always appreciate your take for CI changes and you have a lot of context about |
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.
LGTM! I think you also need to take HypothesisWorks/hypothesis#4562 (comment) into account. I also think that hypothesis issue can be closed.
I pinged Ralf to take a look since this changes the CI configuration.
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.
Thanks @bwhitt7 and @ngoldbaum. The CI and test suite changes themselves look good to me. I did test on Linux also with a much higher thread count (-p 17 and -p 21) and ran the full test suite several times, and found one failure (twice):
___________________________________ ERROR at call of TestRegression.test_openblas_threading ___________________________________
self = <numpy.linalg.tests.test_regression.TestRegression object at 0x5f98707d150>
def test_openblas_threading(self):
# gh-27036
# Test whether matrix multiplication involving a large matrix always
# gives the same (correct) answer
x = np.arange(500000, dtype=np.float64)
src = np.vstack((x, -10 * x)).T
matrix = np.array([[0, 1], [1, 0]])
expected = np.vstack((-10 * x, x)).T # src @ matrix
for i in range(200):
result = src @ matrix
mismatches = (~np.isclose(result, expected)).sum()
if mismatches != 0:
> assert False, ("unexpected result from matmul, "
"probably due to OpenBLAS threading issues")
E AssertionError: unexpected result from matmul, probably due to OpenBLAS threading issues
E assert False
expected = array([[-0.00000e+00, 0.00000e+00],
[-1.00000e+01, 1.00000e+00],
[-2.00000e+01, 2.00000e+00],
...99997e+06, 4.99997e+05],
[-4.99998e+06, 4.99998e+05],
[-4.99999e+06, 4.99999e+05]], shape=(500000, 2))
i = 1
matrix = array([[0, 1],
[1, 0]])
mismatches = np.int64(12)
result = array([[ 0.00000e+00, 0.00000e+00],
[-1.00000e+01, 1.00000e+00],
[-2.00000e+01, 2.00000e+00],
...99997e+06, 4.99997e+05],
[-4.99998e+06, 4.99998e+05],
[-4.99999e+06, 4.99999e+05]], shape=(500000, 2))
self = <numpy.linalg.tests.test_regression.TestRegression object at 0x5f98707d150>
src = array([[ 0.00000e+00, -0.00000e+00],
[ 1.00000e+00, -1.00000e+01],
[ 2.00000e+00, -2.00000e+01],
...99997e+05, -4.99997e+06],
[ 4.99998e+05, -4.99998e+06],
[ 4.99999e+05, -4.99999e+06]], shape=(500000, 2))
x = array([0.00000e+00, 1.00000e+00, 2.00000e+00, ..., 4.99997e+05,
4.99998e+05, 4.99999e+05], shape=(500000,))
numpy/linalg/tests/test_regression.py:180: AssertionError
PARALLEL FAILED numpy/linalg/tests/test_regression.py::TestRegression::test_openblas_threading - AssertionError: unexpected result from matmul, probably due to OpenBLAS threading issues
I think that one should be marked as thread_unsafe. There are a max number of memory buffers OpenBLAS can use, so testing with a high level of parallelism (resulting in massive oversubscription) is not robust. That's not really relevant for real-world usage, so it's fine to just mark it as thread-unsafe and move on.
With that one change, I think this can be merged.
|
@rgommers I've seen that failure several times. |
|
@rgommers Thank you for testing this! Marked the test. |
|
Great - in it went! Nice work:) |
Yeah I think I've seen it before as well; it's just way easier to trigger when running multiple calls in parallel. I have a feeling we will be revisiting that particular test some more later (just not related to free-threading / parallel testing). |
Getting to the final stretch of getting the test suite running in parallel threads! This fixes #29552, which describes more about this project I've been working on.
This PR introduces a few changes:
pytest-run-parallelin a CI job. This takes the place of one of the macOS Accelerate runs, specifically the fast test run withmacos-14and Python version3.14t-dev. This is so the CI runs don't take too much longer and the same number of jobs are active as before.spin testcommand to make running parallel threads easier. Users can now usespin test -p 4to run the tests under parallel threads, compared to the previous method of usingspin test -- --parallel-threads=4spin testchange. I was unable to build the docs locally, so please let me know if the syntax is incorrect.With the change to the CI jobs, me and @ngoldbaum will try and keep an eye on any failing tests that come up related to this PR, so that we can update our thread-unsafe markers and/or fix any failing tests. I also plan on updating more of the documentation soon to add more guidance on how to write thread-safe tests.