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

Feature/free threaded #2260

Merged
merged 1 commit into from
Nov 30, 2024
Merged

Feature/free threaded #2260

merged 1 commit into from
Nov 30, 2024

Conversation

serge-sans-paille
Copy link
Owner

No description provided.

@serge-sans-paille serge-sans-paille force-pushed the feature/free-threaded branch 6 times, most recently from 34bd810 to 9a0fd6b Compare November 27, 2024 10:16
@serge-sans-paille
Copy link
Owner Author

@rgommers does that look good to you?

@rgommers
Copy link
Contributor

That was fast! CI and C++ code look good, will test with SciPy to make sure it works.

Main question I have is about the lack of a --freethreading-compatible CLI flag to pythran, to make this opt-in (as I suggested in gh-2258). I'm fairly sure that for SciPy we don't have Pythran kernels that are thread-unsafe, but it is possible to write for example a function that modifies its input arguments, right? That kind of mutation tends to be implicitly protected by the GIL, and without it may have race conditions. That is the reason that Cython and F2py make adding Py_MOD_GIL_NOT_USED in generated C code opt-in, so the user can think about this.

@rgommers
Copy link
Contributor

The C++ code does work as advertised in SciPy. And testing this allowed me to get past the Pythran modules and find the next place to go fix the same thing, so this testing was already quite helpful.

@rgommers
Copy link
Contributor

Just stumbled across https://pythran.readthedocs.io/en/latest/MANUAL.html#thread-safety in the user manual. Is that still up to date, and if so should the generated code default to the behavior enabled by the -DTHREAD_SAFE_REF_COUNT flag under a free-threaded interpreter?

@rgommers
Copy link
Contributor

I think I actually like that the added flag in the .cfg file defaults to true. That makes it quite easy to start using this, and the user with odd code that turns out to be unsafe can still opt out.

@serge-sans-paille
Copy link
Owner Author

serge-sans-paille commented Nov 27, 2024 via email

@rgommers
Copy link
Contributor

Okay, testing done - all happy! Perhaps this still needs an entry in the docs, so the new option shows up at https://pythran.readthedocs.io/en/latest/MANUAL.html#customizing-your-pythranrc? Other than that it LGTM. 🚀

This is the default, and can be changed through
--config backend.freethreading_compatible=false
or the equivalent pythranrc entry.

Fix #2258
@paugier
Copy link
Contributor

paugier commented Nov 29, 2024

but it is possible to write for example a function that modifies its input arguments

Just a question about this remark. It seems to me that it is quite common for efficient numerical kernel to modify inplace numpy arrays (a very simple example here).

Will I have to call pythran with --config backend.freethreading_compatible=false in this case ?

@rgommers
Copy link
Contributor

@paugier only if vector_product in your example is a public function in a package, because then you have to guard against the user sharing the input array over multiple threads when they use the threading module in their own code. Such code is rare though, and typically already buggy (there's a reason you have a WARNING comment there). If it's within your own code base and you don't use threading yourself, that code should be safe.

@serge-sans-paille serge-sans-paille merged commit 4479f5a into master Nov 30, 2024
21 checks passed
@rgommers
Copy link
Contributor

rgommers commented Dec 2, 2024

Thanks again @serge-sans-paille! Do you have any plans for a new release with this included? For the first RC for SciPy 1.15.0 I can point at master, but ideally we'd have this for the final release in January. (if not possible, not the end of the world though)

@rgommers
Copy link
Contributor

@serge-sans-paille friendly ping on the question above. Also, will this go into 0.18.0 or can it be part of 0.17.1?

@serge-sans-paille
Copy link
Owner Author

Can be part of a 0.17.1. Do you think we can sleak in #2257 as well?

@rgommers
Copy link
Contributor

Great! I think I can get something done for gh-2257. Let me at least try to get a PR up this week that works with SciPy; if review shows it's nontrivial to get it right then we probably should aim for 0.18.0 instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants