Skip to content

MAINT: declare that NumPy's C extensions support running without the GIL #26913

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 12, 2024
Merged

MAINT: declare that NumPy's C extensions support running without the GIL #26913

merged 3 commits into from
Jul 12, 2024

Conversation

ngoldbaum
Copy link
Member

@ngoldbaum ngoldbaum commented Jul 11, 2024 •
edited by seberg
Loading

If I apply this PR and build NumPy using cython/cython#6242 and the following diff:

diff --git a/numpy/random/_examples/cython/meson.build b/numpy/random/_examples/cython/meson.build
index 1ad754c536..e582847ec6 100644
--- a/numpy/random/_examples/cython/meson.build
+++ b/numpy/random/_examples/cython/meson.build
@@ -27,6 +27,7 @@ py3.extension_module(
     install: false,
     include_directories: [npy_include_path],
     dependencies: [npyrandom_lib, npymath_lib],
+    cython_args: ['-Xfreethreading_compatible=True'],
 )
 py3.extension_module(
     'extending',
@@ -34,13 +35,15 @@ py3.extension_module(
     install: false,
     include_directories: [npy_include_path],
     dependencies: [npyrandom_lib, npymath_lib],
+    cython_args: ['-Xfreethreading_compatible=True'],
 )
 py3.extension_module(
     'extending_cpp',
     'extending_distributions.pyx',
     install: false,
     override_options : ['cython_language=cpp'],
-    cython_args: ['--module-name', 'extending_cpp'],
+    cython_args: ['--module-name', 'extending_cpp',
+                  '-Xfreethreading_compatible=True'],
     include_directories: [npy_include_path],
     dependencies: [npyrandom_lib, npymath_lib],
 )
diff --git a/numpy/random/meson.build b/numpy/random/meson.build
index 1c90fb5866..15772eec79 100644
--- a/numpy/random/meson.build
+++ b/numpy/random/meson.build
@@ -83,6 +83,7 @@ foreach gen: random_pyx_sources
     link_with: gen[3],
     install: true,
     subdir: 'numpy/random',
+    cython_args: ['-Xfreethreading_compatible=True'],
   )
 endforeach

Then I'm able to get the full numpy unit tests to pass with the GIL disabled without setting PYTHON_GIL=0.

I'm sending this in separate from the cython changes to ease review.

The one thing I'm not clear about is whether the f2py change needs to be a new configuration option for f2py. NumPy itself doesn't use f2py, I needed to make the changes to get the f2py tests to pass without raising a warning.

@ngoldbaum ngoldbaum mentioned this pull request Jul 11, 2024
Closed
16 tasks
@seberg
Copy link
Member

seberg commented Jul 11, 2024

Nice that we reached this milestone! I guess at some point we need to add some docs for the support, also because I think it needs a warning somewhere that object arrays shouldn't be fully thread-safe (unless they are through some magic I don't understand).

The one thing I'm not clear about is whether the f2py change needs to be a new configuration option for f2py

Not 100% sure, but it seems right to just add. But is f2y really safe already? I am worried f2py modules may need locking similar to lapack-lite (there are a lot of static) or did you already audit it?

@ngoldbaum
Copy link
Member Author

Odd, I wonder why the asan tests failed. Let's see if it's just a flakey test...

I guess at some point we need to add some docs for the support, also because I think it needs a warning somewhere that object arrays shouldn't be fully thread-safe (unless they are through some magic I don't understand).

I agree docs should come before 2.1 comes out. I'll try to add a section somewhere.

I haven't yet had a chance to explore your object array suggestion, will cover that there.

Not 100% sure, but it seems right to just add. But is f2y really safe already? I am worried f2py modules may need locking similar to lapack-lite (there are a lot of static) or did you already audit it?

This is a good point, I seriously doubt it would be.

Another thing I can do is mark the RuntimeWarning in the f2py test as an expected warning in the free-threaded build.

@seberg
Copy link
Member

seberg commented Jul 11, 2024

Odd, I wonder why the asan tests failed. Let's see if it's just a flakey test...

Also failed on my PR, likely something happened...

Another thing I can do is mark the RuntimeWarning in the f2py test as an expected warning in the free-threaded build.

Sounds like a reasonable path for now?

@WarrenWeckesser
Copy link
Member

Odd, I wonder why the asan tests failed. Let's see if it's just a flakey test...

Also failed on my PR, likely something happened...

Also here: #26908. The log shows an error in a string operation: ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602004702c0f ...

@ngoldbaum
Copy link
Member Author

I added some filters to the relevant f2py tests, although there might be more that need it once #26917 is fixed.

@rgommers rgommers added this to the 2.1.0 release milestone Jul 12, 2024
@rgommers rgommers added the 39 - free-threading PRs and issues related to support for free-threading CPython (a.k.a. no-GIL, PEP 703) label Jul 12, 2024
rgommers
rgommers reviewed Jul 12, 2024
View reviewed changes
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

The last commit needs a tweak - filterwarnings call has a syntax error (should be a raw string it looks like). Then this LGTM - it should probably be merged at the same time as gh-26780.

Re f2py: whether an extension module is thread-safe doesn't only depend on the wrapper code that f2py generates (I suspect that that is more or less fine already), but primarily on the Fortran code that it's wrapping. Hence, f2py needs a CLI option just like Cython to declare a module safe. I suggest that we do that in a separate PR (since it's a slightly larger change than what's in here), and name it --freethreading-compatible just like Cython did.

@seberg
Copy link
Member

seberg commented Jul 12, 2024 •
edited
Loading

OK, if f2py already turns out safe, great! In practice the fortran code seemed to be more in the boat of ufuncs where we just opt-in users to be thread safe based on the fact that most release the GIL anyway.

In the case of f2py, there is some mechanism here for that, which seems to be a marker of the fortran function itself:

def isthreadsafe(rout):
    return 'f2pyenhancements' in rout and \
           'threadsafe' in rout['f2pyenhancements']

Although, Python callbacks may or may not add some nuance to the issue (I would think they are fine, unless f2py has some issues in the code it generates, though).

I don't think f2py can work with Python objects really? Now there is one problem that when we do not see the threadsafe we don't know whether we need a global lock or whether a per-function or per-module lock is sufficient.

So ideally it would seem that when all wrapped functions are marked threadsafe, you just create a thread-safe module.
When that is not the case, the question is whether you add a per-module, per-function, or even global lock.

So --freethreading-compatible seems like the right pick maybe guessing at per module safety, although you could add an option of module/per-function in theory.
One could potentially guess at per-module locking being good, but not sure it's worthwhile since I suspect a lot of functions fall into the threadsafe category anyway.

@rgommers
Copy link
Member

So ideally it would seem that when all wrapped functions are marked threadsafe, you just create a thread-safe module.
When that is not the case, the question is whether you add a per-module, per-function, or even global lock.

That seems reasonable. The CLI option could have a default of auto with that behavior (with true/false the other allowed options).

I do note that this is not 100% foolproof - e.g., from SciPy's arpack.pyf which uses the threadsafe marker:

threadsafe   ! it's not really threadsafe, but we use a lock on the Python side, so keeping GIL is not needed

@seberg
Copy link
Member

seberg commented Jul 12, 2024

The default is foolproof if everything is marked as "threadsafe", if that isn't the case you could guess only.
As that comment says: The function is fully threadsafe from the f2py perspective. It just chooses to imlement a custom fine-grained lock on the python side, which it could replace with a fine-grained (module or function scope) f2py generated lock when that comes available.

@ngoldbaum
Copy link
Member Author

Last push fixes the broken test. Not sure why that didn't trigger on my local setup.

I'll go ahead and add an item for marking f2py modules as supporting the GIL to the tracking issue and do that in a followup.

rgommers
rgommers approved these changes Jul 12, 2024
View reviewed changes
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

CI is happy now (the two failures are both unrelated), and the last major pending PR to actually make the core numpy internals safe just went in too. So in this one goes.

@rgommers rgommers merged commit d01249c into numpy:main Jul 12, 2024
66 of 68 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgommers rgommers rgommers approved these changes

Assignees
No one assigned
Labels
03 - Maintenance 39 - free-threading PRs and issues related to support for free-threading CPython (a.k.a. no-GIL, PEP 703)
Projects
None yet
Milestone
2.1.0 release
Development

Successfully merging this pull request may close these issues.
4 participants
@ngoldbaum @seberg @WarrenWeckesser @rgommers