tornadoweb / tornado

Tornado is a Python web framework and asynchronous networking library, originally developed at FriendFeed.
http://www.tornadoweb.org/
Apache License 2.0
21.76k stars 5.51k forks source link

BLD: do not try to use limited API in freethreading builds #3434

Open tacaswell opened 3 weeks ago

tacaswell commented 3 weeks ago

This follows the recommendation at:

https://docs.python.org/3/howto/free-threading-extensions.html#limited-c-api-and-stable-abi

bdarnell commented 3 weeks ago

This seems annoyingly invasive for a feature that's still in its experimental phase - I would have expected setuptools to turn the py_limited_api options to no-ops as appropriate so you don't have to roll out new versions of every package with a binary wheel. (or conversely, for the stable ABI options to be passed in from the outside from cibuildwheel instead of baked into setup.py)

And it looks like this isn't even enough - we'd also need to call PyUnstable_Module_SetGIL (and deal with the compatibility break when its name changes)?

I would say this is something we shouldn't do until we have CI testing for it, but I also don't want to deal with early-adopter issues there either, since Tornado is by design a single-threaded framework.

tacaswell commented 3 weeks ago

With freethreading they added a new abi tag (cp313t) for wheels that are compatible with the freethreaded builds of CPython (so no existing wheels will install on it and wheels that work with freethreading won't install anywhere else).

This change makes it so that tornado will build at all with a freethreading build of CPython, but (intentionally) does not claim that it is thread safe (if CPython sees an extension module that does not mark itself as safe it will turn the GIL back on for you (which you can then in turn force back off with an ENV)). It seems quite reasonable to me not build and distribute the cp313t wheels and to leave that flag as "not-freethread-safe", but if you include this patch it will at least be installable by people who want to try.

I tried to add a CI job that runs under a freethread build (I do not really understand tox, but I think if GHA provides a freethreaded build of py313 it will "just work"?).


I think it is reasonable for setuptools to not auto-drop the stable ABI request (that seems like a pretty critical bit of user input to "helpfully" ignore!) and opting into the stable API seems like something the package should do internally (as you should know if you plan to use only the stable API). Having a way from outside to force a build process to opt-out does seem reasonable, but I am not aware of anything like that existing .

bdarnell commented 3 weeks ago

I think it is reasonable for setuptools to not auto-drop the stable ABI request (that seems like a pretty critical bit of user input to "helpfully" ignore!) and opting into the stable API seems like something the package should do internally (as you should know if you plan to use only the stable API). Having a way from outside to force a build process to opt-out does seem reasonable, but I am not aware of anything like that existing .

Right. What I'm asking for is for this to be more unified and declarative. I write my extension with the limited API (version 3.9). No one knows this but me so I should declare it in my setup metadata somewhere. As a consequence of this declaration, I want several things to happen:

(This extension is 70 lines of C and has changed 7 times in its decade-long history. I'd hoped opting into the limited API would mean it would no longer produce more churn it its build support than in the extension itself)

tacaswell commented 3 weeks ago

cibuildwheels docs seem to say "you must convince your build backend to do this" https://cibuildwheel.pypa.io/en/stable/faq/#abi3 as they just invoke what ever build backend the projects setup in setup.py/pyproject.toml. It is probably not reasonable to expect setuptools/meson/scikit-build/... to all respect the same cli flags or ENV to control if extensions are complied with the right build options.


I'm not the right person to ask for an explanation or justification of wheels and extension modules, but will refer you to https://pypackaging-native.github.io for a good summary of some of the ongoing discussions.


I adapted this from Matplotlib's tests without fully understanding it, needs a bit more config copied over.

tacaswell commented 3 weeks ago

sigh, I'm apparently very nerd sniped on this.

bdarnell commented 3 weeks ago

I suggest removing -full from the tox_env to reduce the number of yaks you have to shave at once. ;-)

tacaswell commented 3 weeks ago

I am seeing local warnings like:

 [W 241028 22:16:13 iostream:779] error on read: [SYS] unknown error (_ssl.c:2591)

but I also see those when using system Python so I hope that is something weird with the openssl from Arch....

tacaswell commented 3 weeks ago

Would you like me to squash / simplify the history?

ngoldbaum commented 1 week ago

Hi! I'm working on ecosystem compatibility for free-threaded Python and I ran across this PR. Hoping to clarify things.

Maybe @henryiii can give some additional context about cibuildwheel and Py_LIMITED_API. It might be slightly nicer for maintainers if cibuildwheel passed -U Py_LIMITED_API at build time for cp313t, since it definitely won't work. That might be pretty surprising though, especially if someone expected to only get the stable ABI wheel.

We're hoping to work on defining a new stable ABI that can work with the free-threaded build for 3.14, so hopefully you'll only need to deal with generating a version-specific wheel for 3.13.