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

ENH: spatial: serialize concurrent calls to QHull #20619

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

andfoy
Copy link
Contributor

@andfoy andfoy commented Apr 30, 2024

Reference issue

Closes gh-8856

What does this implement/fix?

This PR introduces a Mutex mechanism to serialize QHull calls, thus preventing potential segfaults or undefined behaviour, since it is not thread-safe. See http://www.qhull.org/html/qh-code.htm#reentrant

Additional information

Depends on #20611

@github-actions github-actions bot added scipy.spatial Cython Issues with the internal Cython code base labels Apr 30, 2024
@andfoy andfoy changed the title Lock qhull ENH: Serialize concurrent calls to QHull Apr 30, 2024
Copy link

@ngoldbaum ngoldbaum left a comment

Choose a reason for hiding this comment

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

Too bad about the code churn but putting the lock cleanup in a finally block does make sense.

scipy/spatial/_qhull.pyx Show resolved Hide resolved
scipy/spatial/_qhull.pyx Outdated Show resolved Hide resolved
@ngoldbaum
Copy link

Probably worth benchmarking singlethreaded performance since acquiring an uncontested PyThread_type_lock does have some overhead.

@rgommers
Copy link
Member

rgommers commented May 1, 2024

Great, looks like this fixes a long-standing crash in LinearNDInterpolator! The one failure is memory allocation in the 32-bit job:

numpy.core._exceptions._ArrayMemoryError: Unable to allocate 1.49 GiB for an array with shape (6250000, 32) and data type float64

Could you skip this test on 32-bit platforms and when not enough memory is available @andfoy? Search the code base for _IS_32BIT and check_free_memory to see the patterns for how.

Also, could you add @pytest.mark.slow? This test takes a while to run, so we don't need to do so in every CI job.

@rgommers rgommers added the no-GIL Items related to supporting free-threaded builds of CPython label May 1, 2024
@andfoy
Copy link
Contributor Author

andfoy commented May 1, 2024

Thanks for the pointers! @ngoldbaum @rgommers @ev-br, I'll make sure to fix the tests and some minor lock acquisition details

@rgommers
Copy link
Member

rgommers commented May 3, 2024

The diff with whitespace hidden looks quite clean now. Probably about ready to go once the merge conflict is resolved?

@rgommers rgommers mentioned this pull request May 3, 2024
10 tasks
@lucascolley lucascolley changed the title ENH: Serialize concurrent calls to QHull ENH: spatial: serialize concurrent calls to QHull May 5, 2024
@andfoy andfoy marked this pull request as ready for review May 6, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cython Issues with the internal Cython code base no-GIL Items related to supporting free-threaded builds of CPython scipy.spatial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LinearNDInterpolator not thread safe
5 participants