Skip to content

Don't define Py_LIMITED_API and don't re-enable GIL under free-threading #3482

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

Merged
merged 6 commits into from
May 1, 2025

Conversation

lysnikolaou
Copy link
Contributor

  • Defining the Py_LIMITED_API macro leads to build failures
  • Do not re-enable GIL when importing tornado.speedups since it's thread-safe.

@bdarnell
Copy link
Member

This needs a rebase on top of #3484 to pass CI.

I previously found the build to work when Py_LIMITED_API was defined (#3434 (comment)) as long as the py_limited_api argument was omitted. What exactly is the error when this is defined?

Even though the module is thread-safe, I'm reluctant to call PyUnstable_Module_SetGIL solely because of the name and the expectation that it will be removed/renamed in the future (while the Py_GIL_DISABLED macro will presumably not go away).

Because Tornado is a single-threaded framework, I'd prefer to avoid anything that implies future changes while free-threading is in its experimental phase. Minimizing churn here is more important to me than maximizing support for free-threading. (I'd be OK if the call to PyUnstable_Module_SetGIL were guarded by a #ifdef Py_UNSTABLE_GIL_DISABLED to ensure that the ifdef and the function had the same lifespan, so we'd fall back to GIL mode when the feature stabilized instead of breaking the build).

@lysnikolaou
Copy link
Contributor Author

What exactly is the error when this is defined?

On macOS, I'm getting the following build error (which I find expected since the Py_LIMITED_API macro is defined), when compiling:

      building 'tornado.speedups' extension
      creating build/temp.macosx-15.3-arm64-cpython-313t-pydebug/tornado
      clang -fno-strict-overflow -Wsign-compare -g -Og -Wall -O0 -DPy_LIMITED_API=0x03090000 -I/Users/lysnikolaou/repos/python/tornado/.venv-313t/include -I/Users/lysnikolaou/.pyenv/versions/3.13.3t-debug/include/python3.13td -c tornado/speedups.c -o build/temp.macosx-15.3-arm64-cpython-313t-pydebug/tornado/speedups.o
      In file included from tornado/speedups.c:2:
      /Users/lysnikolaou/.pyenv/versions/3.13.3t-debug/include/python3.13td/Python.h:51:4: error: "The limited API is not currently supported in the free-threaded build"
         51 | #  error "The limited API is not currently supported in the free-threaded build"
            |    ^

Even though the module is thread-safe, I'm reluctant to call PyUnstable_Module_SetGIL solely because of the name and the expectation that it will be removed/renamed in the future (while the Py_GIL_DISABLED macro will presumably not go away).

Agreed. Since the speedups module does not have any global or module state, I've implemented multi-phase init which is easy enough. This way we can use the Py_mod_gil slot, which is the long-term supported way.

@bdarnell
Copy link
Member

Hmm, that error is clear and has been there since 3.13.0b1. Did I make a mistake in my earlier testing and not actually run a free-threaded build?

The init changes still need some ifdefs because they use symbols introduced in python 3.12 and 3.13 but we support back to 3.9.

@lysnikolaou
Copy link
Contributor Author

Did I make a mistake in my earlier testing and not actually run a free-threaded build?

Sorry for not making it clear in my last comment, but I had to use TORNADO_EXTENSION=1 to get the error. Maybe that's the reason why this wasn't failing for you?

The init changes still need some ifdefs because they use symbols introduced in python 3.12 and 3.13 but we support back to 3.9.

Yup, missed that. On it.

Copy link
Member

@bdarnell bdarnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, the TORNADO_EXTENSION option probably wasn't set right in some of my tests (but I did have it sometimes because I did have at least some failures).

This got more complicated than anticipated but I think it's almost ready to go.

#if (!defined(Py_LIMITED_API) && PY_VERSION_HEX >= 0x030c0000) || Py_LIMITED_API >= 0x030c0000
{Py_mod_multiple_interpreters, Py_MOD_PER_INTERPRETER_GIL_SUPPORTED},
#endif
#if !defined(Py_LIMITED_API) && PY_VERSION_HEX >= 0x030d0000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these two #ifs not the same structure? The definitions are the same. I think the second one also needs a Py_LIMITED_API >= 0x030d0000 clause.

It would be simpler to do #ifdef Py_mod_gil, although I don't think it's mandated that these are #defines instead of global variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'd prefer to not go with #ifdef Py_mod_gil. These would probably not change, but it's still more stable to do a version check.

@bdarnell bdarnell added the build label Apr 29, 2025
@lysnikolaou
Copy link
Contributor Author

lysnikolaou commented Apr 30, 2025

Also, there's only wheels for the stable ABI on PyPI. Would you be open to uploading wheels for cp313t as well? If yes, I can work on it as well.

@bdarnell
Copy link
Member

bdarnell commented May 1, 2025

I'm open to uploading cp313t wheels to pypi if someone else does the work ;)

From my prior testing the blocker is that auditwheel was failing for unclear reasons so I assume the wheels aren't releasable until that's fixed (maybe just needs a version bump?) See

#CIBW_REPAIR_WHEEL_COMMAND: ""

@bdarnell
Copy link
Member

bdarnell commented May 1, 2025

Anyway, this looks good so I'm going to go ahead and merge it.

@bdarnell bdarnell merged commit d8e0d36 into tornadoweb:master May 1, 2025
15 checks passed
@lysnikolaou lysnikolaou deleted the free-threading branch May 2, 2025 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants