Fix __module__ attribute for shared types
#6845
Conversation
Cython/Utility/Coroutine.c
Outdated
| @@ -1771,7 +1771,7 @@ static PyObject *__Pyx_CoroutineAwait_no_new(PyTypeObject *type, PyObject *args, | |||
| // We are applying this to all Python versions (hence the commented out version guard) | |||
| // to make the behaviour explicit. | |||
| // #if CYTHON_USE_TYPE_SPECS | |||
| static PyObject *__Pyx_CoroutineAwait_reduce_ex(__pyx_CoroutineAwaitObject *self, PyObject *arg) { | |||
| static PyObject *__Pyx_CoroutineTypes_reduce_ex(PyObject *self, PyObject *arg) { | |||
There was a problem hiding this comment.
For reasons I don't understand, changing the definition of __module__ magically made coroutines pickleable under protocols 1 and 2. Therefore I just explicitly banned it using the same function as CoroutineAwait uses.
It can't be a descriptor because that messes up `type(func).__module__`. I decided not to put it in `__dict__` because that involves instantiating `__dict__` for every instance (which we've previously decided not to do) and also deciding on the behaviour when `__dict__` is replaced. Therefore the solution seems to be to add some special casing to tp_getattro and tp_setattro. Fixes cython#6841.
tests/run/cyfunction.pyx
Outdated
| """ | ||
| >>> test_module.__module__ | ||
| 'cyfunction' | ||
| >>> type(test_module).__module__.startswith("_cython") |
There was a problem hiding this comment.
This seems to be a bit fragile locally, because for some reason I always end up with an older cached cyfunction implementation.
It works when I manually import the module alone but not when I use runtests. I'm not sure what the CI will end up doing, but I'm fairly sure this is a caching problem and will go away when the shared lib version changes.
|
I'm always wary of changing attribute access away from PyObject_GenericGetAttr due to CPython's internal optimisations.
Definitely not a change to merge at this point, the pickle changes seem risky.
|
Agree
I don't think it's as bad as it seems - the non-pickleability of coroutines cython/tests/run/test_coroutines_pep492.pyx Line 2385 in b086383
Yes. Probably worth waiting to see how many people the bug really affects for now |
Cython/Utility/Coroutine.c
Outdated
| @@ -1771,7 +1771,7 @@ static PyObject *__Pyx_CoroutineAwait_no_new(PyTypeObject *type, PyObject *args, | |||
| // We are applying this to all Python versions (hence the commented out version guard) | |||
| // to make the behaviour explicit. | |||
| // #if CYTHON_USE_TYPE_SPECS | |||
| static PyObject *__Pyx_CoroutineAwait_reduce_ex(__pyx_CoroutineAwaitObject *self, PyObject *arg) { | |||
| static PyObject *__Pyx_CoroutineTypes_reduce_ex(PyObject *self, PyObject *arg) { | |||
There was a problem hiding this comment.
Now that we're renaming this function, I think it's worth stating in the function name that this is not a pickle implementation but something that intentionally prevents pickling.
| static PyObject *__Pyx_CoroutineTypes_reduce_ex(PyObject *self, PyObject *arg) { | |
| static PyObject *__Pyx_Coroutine_fail_reduce_ex(PyObject *self, PyObject *arg) { |
| {Py_tp_getattro, (void *) PyObject_GenericGetAttr}, | ||
| {Py_tp_getattro, (void *)__pyx_Coroutine_getattro}, | ||
| {Py_tp_setattro, (void *)__pyx_Coroutine_setattro}, |
There was a problem hiding this comment.
Could you run a quick micro-benchmark to see if this has any visible impact on the time it takes to look up and call .send() from Python code (e.g. from within asyncio)? Especially with all the attribute lookup optimisations in Py3.1[34]? Just so that we have a number.
There was a problem hiding this comment.
Will do (although it'll probably have to wait a few days).
There's also a Limited API 3.8 breakage that needs fixing for this PR
There was a problem hiding this comment.
Just calling .send() on a generator looks like it ends up about 3x slower. That seems fairly bad so I'm going to try to implement an alternative approach instead.
|
I've proposed an alternative version with metaclasses which hopefully avoids the performance cost of this (at the cost of a little more code). |
|
I still think #6882 is better, but this is working as well as it can be |
I think they're only getting enabled because doctest can now see the functions.
This seems the better way to do it because it doesn't impose any runtime cost to attribute lookup. It makes all of the shared types that have a `__module__` attribute inherit from a metaclass that reports the module correctly. Supercedes cython#6845; fixes cython#6841
It can't be a descriptor because that messes up
type(func).__module__. I decided not to put it in__dict__because that involves instantiating__dict__for every instance (which we've previously decided not to do) and also deciding on the behaviour when__dict__is replaced.Therefore the solution seems to be to add some special casing to tp_getattro and tp_setattro.
Fixes #6841.