nucleic / kiwi

Efficient C++ implementation of the Cassowary constraint solving algorithm
https://kiwisolver.readthedocs.io/en/latest/
Other
679 stars 88 forks source link

MNT: Enable free threading support for Python #186

Open greglucas opened 1 week ago

greglucas commented 1 week ago

This declares the module as supporting nogil Python releases. I have tested it locally and it works as expected. Without this I get a warning that kiwisolver doesn't support the nogil free threading, without this it runs as expected and passes the tests.

Some links for information on what I am updating here.

Update to the module definition: https://py-free-threading.github.io/porting/#__tabbed_1_1

CI testing: Added installation from the deadsnakes setup-python version, which I think will only work on the ubuntu runner: https://py-free-threading.github.io/ci/

cibuildwheel support to build the free threaded wheels: https://cibuildwheel.pypa.io/en/stable/options/#free-threaded-support

MatthieuDartiailh commented 1 week ago

Thanks for starting the discussion.

The C++ implementation is not thread safe for performance reason and I need to think about what to do on the Python side. Given the intended use a documentation update may be sufficient. However I would be happy to get some user feedback about it.

greglucas commented 1 week ago

There is some good discussion here https://py-free-threading.github.io/porting/ discussing several options for interacting with extension libraries and advice for how to approach a lot of different situations depending on where the thread-unsafe operations come in to play. I am coming from the Python matplotlib package which has kiwisolver as a dependency and I don't think a 2x decrease in speed would be too bad for us, but I don't really know 🤷 I think we may have users that will try to update a figure per thread or something like that, which would mean a separate solver and variables being accessed per thread. I doubt there would be much interaction of one thread updating the solver state on another thread if that helps at all.

tacaswell commented 6 days ago

is kiwi maintaining some global state that is thread-unsafe or are a given set of constraints unsafe to work on from multiple threads?

sccolbert commented 6 days ago

I don't recall using any global state (but it's been a while). I would definitely say it's not safe to work on the same set of constraints or solver from multiple threads. We 100% don't lock around anything.

On the Python binding side of things, there may be even more subtleties with reference counting, etc.

On Wed, Sep 11, 2024 at 11:14 AM Thomas A Caswell @.***> wrote:

is kiwi maintaining some global state that is thread-unsafe or are a given set of constraints unsafe to work on from multiple threads?

— Reply to this email directly, view it on GitHub https://github.com/nucleic/kiwi/pull/186#issuecomment-2344097978, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABBQSMN3MJLQ2ILTYRCGLLZWBT5BAVCNFSM6AAAAABN2FFIMKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBUGA4TOOJXHA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

MatthieuDartiailh commented 6 days ago

The main thing that I know is not thread safe is the use of reference counted pointer for variable in the C++ code. So, the creation/destruction of variables may require some sort of protection, and it may also apply to other solver manipulations.

tacaswell commented 6 days ago

I suspect the safest thing would be to have package global lock that is acquired when ever kiwi crosses from Python -> c++ and release when it returns (which effectively replicates the GIL). It can be scoped down later and if done on the c++ side can probably be omitted when built in non-freethreading mode.

sccolbert commented 6 days ago

That should work. I dont think any of the C++ code is re-entrant.

On Wed, Sep 11, 2024 at 14:10 Thomas A Caswell @.***> wrote:

I suspect the safest thing would be to have package global lock that is acquired when ever kiwi crosses from Python -> c++ and release when it returns (which effectively replicates the GIL). It can be scoped down later and if done on the c++ side can probably be omitted when built in non-freethreading mode.

— Reply to this email directly, view it on GitHub https://github.com/nucleic/kiwi/pull/186#issuecomment-2344484843, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABBQSKOZJFUCCYALF66W4LZWCIRRAVCNFSM6AAAAABN2FFIMKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBUGQ4DIOBUGM . You are receiving this because you commented.Message ID: @.***>