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

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

tacaswell
Copy link

@bdarnell
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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:

  • Give me a build-time error if I use a non-limited-API. For experimental builds like cp313t, I'm fine with this being a best-effort thing and enforcement of the limited API can be disabled if the headers don't have all the right ifdefs for this case. (Therefore I'd rather say py_limited_api="3.9" and have setuptools generate the #define when it applies rather than put that in setup.py).
  • When building wheels for distribution, use the stable ABI. It's unclear to me why (other than a historical accident) there is a separate control for this instead of being implied by the use of the limited API. If I say kwargs["options"] = {"bdist_wheel": {"py_limited_api": "cp39"}} in my setup.py then I agree that setuptools should not ignore this, but my point is that I shouldn't have to say that. Actually, now that I read that line more closely, isn't this something that cibuildwheel can/should be doing instead of putting it in my setup.py? It can't be automatic without a declarative form of py_limited_api in the extension but I could at least move this line to the cibuildwheel config where it wouldn't interfere with folks pip-installing into cp313t.

(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
Copy link
Author

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 tacaswell force-pushed the mnt/freethread_build branch from 9fe2d59 to 00f2b2b Compare October 29, 2024 01:39
@tacaswell
Copy link
Author

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

@bdarnell
Copy link
Member

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

This avoids warnings that tornado does not declare itself freethreading safe
which cause failures with cp313t as other errors are expected.
The GC collection rules have changed, tests may not be valid anymore.
@tacaswell
Copy link
Author

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
Copy link
Author

Would you like me to squash / simplify the history?

@ngoldbaum
Copy link

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.

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