Skip to content

ENH: add locking to python lapacklite interface #26687

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

Closed
wants to merge 1 commit into from

Conversation

ngoldbaum
Copy link
Member

Fixes #22509.

lapack-lite is not thread safe, with tons of static global state.

This PR adds a global lock to every call into the thread-unsafe low-level lapack-lite code. I'm sure it adds some overhead but we can ameliorate that in the future leveraging PyMutex. lapack-lite is thread-unsafe even in the standard python build, it just doesn't get hit very often, and usually in CI under odd build configurations.

With this change applied the scikit-image tests referred to in #24512, which still fail under lapacklite, pass. Ping @stefanv in case scikit-image is going out of its way to avoid lapacklite in tests.

I don't have a simple numpy-only reproducer that crashes without this change. Does anyone have a simple way to crash a non-thread-safe lapack so I can add a test?

I might make it harder to install lapack-lite in the free-threaded build as a followup.

@ngoldbaum ngoldbaum added the 39 - free-threading PRs and issues related to support for free-threading CPython (a.k.a. no-GIL, PEP 703) label Jun 13, 2024
@rgommers
Copy link
Member

Thanks Nathan!

I might make it harder to install lapack-lite in the free-threaded build as a followup.

xref #26157 (comment) where we discussed this. I'm fine with this deviation if it's still necessary after this PR. But if the locking fixes it, I'd prefer not to do this, since it will cause other problems when building numpy from source in downstream CI.

@seberg
Copy link
Member

seberg commented Jun 14, 2024

This doesn't look right, unfortunately. I don't know why we even have this lapack_lite module, it belongs deprecated (probably because it was a "private" version used before the linalg-umath existed).
Adding locking here fixes an issue in the free-threaded build for the lapack_lite module and I am happy if you just want to merge it, but that is pretty unimportant and doesn't close the issue.

The locks need to be either:

  • in the lapack_lite functions that get called here (and more), or
  • in umath_linalg.cpp and only if the lapack backend isn't threadsafe which is probably just lapack-lite.

My guess is that the second one is simpler.

@rgommers
Copy link
Member

IIRC I looked at completely removing lapack_lite recently, but it was complicated because of some direct usage in testing - so I left it alone. So I'd merge this as is. You're right though @seberg that adding locks in umath_linalg is also needed (and more important).

My guess is that the second one is simpler.

I think so too.

@seberg
Copy link
Member

seberg commented Jun 14, 2024

I think we can just delete (or deprecate) it, the only test that looks important is for xerbla and that:

Now, it would be nice to use our xerbla and give a good error for example in that issue! But it looks like it either doesn't work on all systems (e.g. due to mac dynamic linking resolution working differently) or probably never worked (or at least in a long time) for anything but lapack-lite, at least not in years...

@ngoldbaum
Copy link
Member Author

Thanks for the pointer, this is my first time touching the linalg module so the structure wasn't clear initially. I think I'll close this and do this in two followups. First, right now there isn't a way to tell from inside the source code what BLAS is being used, so I need to add some meson logic to generate a macro that is set when lapacklite is being used.

Next, I can add a new global static lock in the umath_linalg module and then acquire and release it in that module. I might end up putting the locking in the functions exposed to Python rather than around every BLAS call for the sake of simplicity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
01 - Enhancement 39 - free-threading PRs and issues related to support for free-threading CPython (a.k.a. no-GIL, PEP 703)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Lapack lite is not thread-safe (need to guard)
3 participants