Skip to content

Mark extensions as free-threading-compatible and build wheels #233

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

lysnikolaou
Copy link

Checklist

  • Pull request details were added to CHANGELOG.rst
  • Documentation was updated (if needed)

Hi there! 👋

I'm opening this PR to implement support for the free-threaded build. I checked that there's no global state and you're locking when compressor/decompressor objects are shared, so we should be good to go. Hopefully this helps. Let me know if I can help any other way!

@rhpvorderman
Copy link
Collaborator

There is global state, since there are no heap types. So this won't fly that easily I am affraid.

@lysnikolaou
Copy link
Author

Our approach up until now has been that static types are acceptable. Many other packages with static types (namely NumPy, SciPy and others) have shown that, in practice, static types are okay.

If you want, I could work on converting all the static types to heap types. However, since that's a bit more of an effort, I'd suggest we merge this and do that as a follow-up. aiohttp is currently blocked on isal not providing free-threaded wheels, so this is not just a theoretical issue.

What do you think?

@rhpvorderman
Copy link
Collaborator

Our approach up until now has been that static types are acceptable. Many other packages with static types (namely NumPy, SciPy and others) have shown that, in practice, static types are okay.

Okay, that is good to know coming from a core developer. Heap types make things a lot more difficult generally, so I have stuck with static types for this library. It is just something I do on the side. (It is in my work time, but I cannot dedicate huge chunks of time to it).

Given that this package is marked as "essential" for the python ecosystem, this change cannot be merged without any tests that assure within reason that it actually works.

aiohttp is currently blocked on isal not providing free-threaded wheels, so this is not just a theoretical issue.

Really, does every dependency have to support free-threading for free-threading to work? Why not just use builtin zlib/gzip in that case?

@lysnikolaou
Copy link
Author

Given that this package is marked as "essential" for the python ecosystem, this change cannot be merged without any tests that assure within reason that it actually works.

I agree with this. I can try to work on adding more testing, if you can give some guidance on what exactly it is that you wanna see more heavily tested. My initial, as it turns out incorrect, assumption was that the tests in test_igzip_threaded.py were sufficient.

Really, does every dependency have to support free-threading for free-threading to work? Why not just use builtin zlib/gzip in that case?

aiohttp depends on isal for testing that different zlib backends will work correctly, so it actually needs it for testing only. It's used heavily in their tests, so it seemed like implementing support for free-threading on isal would be easier. If that's not the case, we can try to not depend on isal under the free-threaded build in aiohttp. However, if you agree and you think there's a path forward for free-threading support in isal, it'd be preferrable to do it.

@rhpvorderman
Copy link
Collaborator

My initial, as it turns out incorrect, assumption was that the tests in test_igzip_threaded.py were sufficient.

I don't know what is sufficient because I haven't read up on free-threading yet. igzip_threaded makes sure each thread gets its own instantiation of an object so they can operate independently. There is explicit GIL dropping code in the methods of such objects.

I suppose with a proper free-threading thing the GIL never needs to be dropped, because it is not there and the GIL dropping code is simply empty. In that case, yes the igzip_threaded tests should be sufficient.

@rhpvorderman
Copy link
Collaborator

However, if you agree and you think there's a path forward for free-threading support in isal, it'd be preferrable to do it.

Well, it is sort if inevitable really. This library is used heavily in bioinformatics and those work loads do benefit a lot from threading. So this would needed to be done at some point anyway.

@lysnikolaou
Copy link
Author

lysnikolaou commented May 28, 2025

I suppose with a proper free-threading thing the GIL never needs to be dropped, because it is not there and the GIL dropping code is simply empty. In that case, yes the igzip_threaded tests should be sufficient.

If you're referring to this piece of code, this can stay as-is under the free-threaded build. Since the GIL is not there under the free-threaded build, this only attaches to/detaches from the CPython runtime. There's some improvements that can be made there, like using the critical section API introduced in 3.13+, but that can also be a follow-up issue.

@rhpvorderman
Copy link
Collaborator

If its "good enough" for now then sure. Let's merge it if the tests pass.

@ngoldbaum
Copy link

I suppose with a proper free-threading thing the GIL never needs to be dropped, because it is not there and the GIL dropping code is simply empty. In that case, yes the igzip_threaded tests should be sufficient.

FWIW this isn't quite the right mental model. Maybe read through this to understand how the C API in the GIL-enabled and free-threaded builds are actually identical, and in fact you still need to explicitly attach and detach from the runtime:

https://py-free-threading.github.io/porting-extensions/#working-with-the-free-threaded-cpython-interpreter-runtime

There are also lots of other suggestions in the guide I linked to for expanding support for free-threading. In particular, I'd try to think about adding explicitly multithreaded tests and perhaps running tests under ASAN or TSAN to be very sure there aren't any issues.

@rhpvorderman
Copy link
Collaborator

FWIW this isn't quite the right mental model. Maybe read through this to understand how the C API in the GIL-enabled and free-threaded builds are actually identical, and in fact you still need to explicitly attach and detach from the runtime:

Ah! So it is indeed as non-trivial as I initially suspected. Thanks for mentioning this. I will investigate and try to add free threading support at some point, but I will not do it at a whim.

@ngoldbaum
Copy link

ngoldbaum commented Jun 2, 2025

So it is indeed as non-trivial as I initially suspected.

Hopefully not so bad - at least if you were handling the GIL correctly on the GIL-enabled build. The point I was trying to make is that correct multithreaded native Python extensions look mostly the same on the GIL-enabled and free-threaded builds - you need to do exactly the same things to avoid hangs and deadlocks.

The main difference is that state can be simultaneously mutated from multiple threads, so new kinds of issues are possible on the free-threaded build. That said, you already have PyThread_type_lock mutexes on all the context objects you expose in Python, so your extension is already safe from that point of view. It might be possible to use a faster locking approach to avoid contention (if that even makes sense to do for compressor or decompressor handles?), but that's for future iterations. Both @lysnikolaou and myself are happy to chat about that.

I will investigate and try to add free threading support at some point, but I will not do it at a whim.

I think @lysnikolaou opened this because he'd like to work on it. Both of us are on a team at Quansight Labs that are working on ecosystem support for free-threading, and we have funding. Since python-isal is a dependency of aiohttp, we've identified it as one we need to work on to unblock ecosystem progress.

Of course we'd very much appreciate code review and your participation in discussions. That said, one of our overarching goals in this project is to not be a burden to maintainers and we'll endeavor to treat your time and attention seriously.

@rhpvorderman
Copy link
Collaborator

Alright. I am glad that you are willing to invest into this. I need to give this a proper review though and make sure I am up to speed on the free-threading stuff. This should be less work than implementing it myself, but I cnanot give an ETA as I am quite busy at the moment.

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