-
-
Notifications
You must be signed in to change notification settings - Fork 677
Refactor atexit.pyx
#41021
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
base: develop
Are you sure you want to change the base?
Refactor atexit.pyx
#41021
Conversation
Refactor atexit callback handling for Python versions 3.14 and below.
|
Documentation preview for this PR (built with commit 2ec6f1b; changes) is ready! 🎉 |
|
I would prefer to simply do nothing (or throw a 'is deprecated/removed' exception) in |
I just tested the doctest in |
I also think it is useless in Python 3.13+. We can edit the build system to exclude this. If Python version bigger than 3.13. But we need to modify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the atexit.pyx module to support both Python ≤3.13 and Python 3.14+, which changed how atexit callbacks are stored internally. The refactoring provides a uniform interface for accessing atexit callbacks across different Python versions.
- Adds version-specific implementations for accessing atexit callbacks
- Introduces C functions that handle both the legacy C array format (Python ≤3.13) and the new PyList format (Python 3.14+)
- Updates the
_get_exithandlers()function to work with both storage formats while maintaining consistent behavior
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Remove distutils macro definition for Py_BUILD_CORE.
|
I thought that everything atexit-related in 3.14 is already in 3.13, no? |
Removed conditional compilation for atexit callback definition.
It rewrite in python 3.14 for supporting no gil. you can refer to this commit |
Yes, the open APIs is not changed. But it changes the internal implementation |
|
Maybe we can refactor It to support 3.14 immediately. Then we deprecate the atexit module. |
I suggest that we first do the refactor to support python3.14. Then we have one year to rewrite the |
|
Question: and also during runtime with Are we sure those will always agree (I don't think so). What do we do if they don't agree? Do we get a decent error then? Or do we get undefined behaviour? I'm mainly worried because the conditional code defines a dummy function that's supposed to be never called in one situation. |
Maybe I need to have assert in the dummy function. Or throwing an error. I have to write as this. Because >=3.14, we need to receive Python List, for python <=3.14, we need to receive C array. So I defined two functions. Maybe I should throw an error: The compile version and the running version is not consistent. The function should not arrive there. |
Thank you very much for your reviewing. Do you think I should do throwing an error in runtime I should throw error in the dummy function. |
Added error handling for unsupported Python versions in atexit functions.
|
Sorry, I don't know what's the best way to deal with cythonize time/compile time/run time discrepancies like the one that can arise here. I was hoping someone else would. Perhaps the cython people have a suggestion? Questions that may be relevant: does Python consider 3.13 and 3.14 to be binary compatible? They might because they may view the internal changes as not changing the publishes ABI. In that case we're on our own, because then I think you might end up being linked against a 3.14 runtime even if you compiled against a 3.13. If 3.13 and 3.14 are NOT considered binary compatible then we may not have to worry about this: then there should be other safeguards in place that prevent objects compiled for 3.13 to be dynamically linked at runtime to 3.14 and vice versa. Because there is also the subtle question that cythonization (making the c source from the cython) may not coincide with the actual compilation, it's also at least theoretically possible to have a mismatch there. The cython people may have good input about that. Question here: https://groups.google.com/g/cython-users/c/CjAn4sPDFK0/m/jeQLWu5QAgAJ |
I think the changes of python 3.14 is huge, the python group did a lot for the no gil support. I do not think the wheels in python 3.13 is also compatible in 3.14. But the python group does not say they are compatible or not in an official document. I suggest that we can merge it so we can support python 3.14 quickly. Then we have one year to discuss how to deal with the problem of |
Because the atexit module only has two open api, register and unregister. without internal api, we can not do the |
But for python 3.12, the doctest needs this module. we should support python 3.12 this year and next year. |
|
Yes, I think your support plan makes perfect sense. We just need to check if we're sufficiently defensive about version checks. Presently you define a "never called" |
I think it will directly throw Python error with my last commit. But how to run Python 3.14 wheel in Python 3.13? the Python abi is not compatible |
in fact, The origin atexit.pyx file also use if PY_VERSION_HEX to control the version |
If we can confirm that it does, then that would mean we don't have to worry about compile-time and runtime versions disagreeing. Note that python does have a Stable ABI: https://docs.python.org/3/c-api/stable.html#stable . Their API already has stronger compatibility guarantees. So they're definitely considering scenarios where code compiled for one version gets run with a different one. That's why I'm not so sure that it's impossible to encounter in the wild. |
Thank you very much. I will try to do this If runtime is not equal that. It will return importerror immediately. |
|
D Woods https://groups.google.com/g/cython-users/c/CjAn4sPDFK0/m/gl7_GcNVAgAJ actually thinks that compile-time/runtime version match is (softly) enforced through filenames, so normally we should not encounter a mismatch. So I'm now less worried about this issue. As a consequence I don't understand why Python is bothering with their "Stable ABI": if you're going to insist on version matches anyway, stability of the API is the only thing that matters (there are the micro versions, but there they already try to be ABI compatible). Sleep well. |
|
I'd guess Python ABI is very important for binary wheels. |
|
@nbruin I just have a question like this If I run this accidently, It will safely raise a Python error or It will get a segfault. Cython can safely handle this error?If this is Ok. I think maybe we do not need so complicated |
I think the cdef needs an And with that in place, I think the body could just be Cython should be able to figure out the right error setting and error-signalling return value then. Since these errors then get raised in |
I have refactor the
atexit.pyxsince for python 3.14, the changes ofatexitis so much for supporting the nogil version.Python<=3.13 Implementation:
atexitmodule stores callbacks in a C array of structs (atexit_callback).Python 3.14 Implementation:
atexitmodule was refactored to use a PythonPyListobject instead of a C array.PyList_Insert(state->callbacks, 0, callback)📝 Checklist
⌛ Dependencies
python/cpython@3b76682 python/cpython#127935