Mark extensions as free-threading-compatible and build wheels

[lysnikolaou]

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]

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

[lysnikolaou]

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]

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]

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]

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]

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]

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]

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

[ngoldbaum]

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]

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]

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]

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.