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
gh-94912: Added marker for non-standard coroutine function detection #99247
Conversation
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
I can look into the News item. |
@carltongibson happy to see you here! Feel free to ask any questions you have: I am happy to help with what I can :)
Python API has >>> def some(): ...
...
>>> some.__code__.replace.__doc__
'Return a copy of the code object with new values for the specified fields.'
>>> some.__code__.replace.__text_signature__
'($self, /, *, co_argcount=-1, co_posonlyargcount=-1,\n co_kwonlyargcount=-1, co_nlocals=-1, co_stacksize=-1,\n co_flags=-1, co_firstlineno=-1, co_code=None, co_consts=None,\n co_names=None, co_varnames=None, co_freevars=None,\n co_cellvars=None, co_filename=None, co_name=None,\n co_linetable=None)' But, I don't know anything about its performance, sorry. |
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
Hi @sobolevn 1�7 thanks for your pointer very helpful. I've pushed two commits, making the tests pass:
I wanted to push that to see if I could get feedback. Thanks.
Certainly in our usage, this marker would be applied once at startup I think. If one were doing this in a hot-loop, I'd think the just use |
Lib/test/test_inspect.py
Outdated
# Second case: a class with an async def __call__. | ||
# - instance is awaitable. | ||
class Cl: | ||
async def __call__(self): |
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.
In the future don't forget to test objects with .__func__
set, because of how _has_code_flag
works :)
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.
I haven't followed exactly what you mean here. Sorry.
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.
Oh, sorry. I've meant that objects like staticmethod
and classmethod
have __func__
property set, where the original function is stored. inspect
generally supports this pattern and it should be tested in this case as well :)
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.
@sobolevn 1�7 OK, let me have a play and see. Thanks!
OK, I'm marking Ready for Review, since this would resolve our need from the discussion in #99247 to be able to continue to identify sync functions returning awaitables, without calling them to see that. The behaviour is as I'd expect.
Thanks! |
Let me know when you both feel this is ready to go in. |
Thanks @gvanrossum. I'm going to add the extra test cases, and then (from my POV) it's a Any changes? question.
That's being made an alias to the Thanks for the guidance both. |
Hi @sobolevn and @gvanrossum 1�7 thanks for your patience. I've added the additional tests, and adjusted the docs as suggested. I can rebase/squash/etc as you need, but I think it's ready for you to look at. (Happy to adjust as you think!) For my POV this would allow removal of the //cc @andrewgodwin. |
Thank you! I hope your first PR to CPython was a pleasant experience
I have just a single main concern: that inspect.iscoroutinefunction
no longer does what is documented.
Citing:
Return True if the object is a coroutine function (a function defined with an async def syntax).
Now it can return True
for instances with async __call__
methods.
This might backfire for users that trust this check right now.
I suggest to (but, this is just my opinion, I don't know what others think of it):
markcoroutinefunction
is a great thing to have👍 - But, maybe we can delay making changes to
iscoroutinefunction
to a separate PR? I see other options like creating newisdefiningcoroutine
(or any similar name) function.
Or we can change the docs and list all special cases.
cc @andrewgodwin Refs: python/cpython#99247 This would be needed for Django 4.2, which would support PY312 on release of that.
Thanks @sobolevn, yes it's been fine. Good point on the docs. Whichever way we go, a small tweak there. Some thoughts to your concern: If I followed correctly, the issue is (only) with the Given there's no existing test covering this, I think it's similar to how support for I note the Starlette framework have the same detection for such a class instance in place. (On phone so don't have link) So it's not just Django that's needing this behaviour. Update: here's where they use it, defining an The name |
Misc/NEWS.d/next/Library/2022-11-17-10-02-18.gh-issue-94912.G2aa-E.rst
Outdated
Show resolved
Hide resolved
I have made the requested changes; please review again
|
Thanks for making the requested changes! @gvanrossum, @kumaraditya303: please review the changes made to this pull request. |
Lib/inspect.py
Outdated
func = getattr(obj, "__func__", obj) | ||
if getattr(func, "_is_coroutine", None) is _is_coroutine: | ||
return True | ||
|
||
if not isclass(obj) and callable(obj) and getattr(obj.__call__, "_is_coroutine", None) is _is_coroutine: | ||
return True |
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.
For the check, we need to check plain functions and the __func__
cases, and the __call__
implementation for class instances. (Tests cover that: given that functions have a __call__
we need to be careful not to just check the one case.)
I looked at various ways of combining this into a single pass, but none that I found were particularly clear. Happy to take a suggestion, but I don't see this as being performance sensitive.
The implementation here is more sophisticated that the older asyncio
one 1�7
https://github.com/python/cpython/blob/3.11/Lib/asyncio/coroutines.py#L17-L24
1�7 but (as per the added test cases) various extra (seemingly legitimate) cases are now covered. (So
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.
I'm thinking about this, but being distracted by other stuff. I'll try to get to it later this week.
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.
Hey @gvanrossum 1�7 thanks.
I thought one maybe neater re-write looks like this:
diff --git a/Lib/inspect.py b/Lib/inspect.py
index 883d0b02bf..3b9bd14a38 100644
--- a/Lib/inspect.py
+++ b/Lib/inspect.py
@@ -410,12 +410,13 @@ def iscoroutinefunction(obj):
Coroutine functions are normally defined with "async def" syntax, but may
be marked via markcoroutinefunction.
"""
- func = getattr(obj, "__func__", obj)
- if getattr(func, "_is_coroutine", None) is _is_coroutine:
- return True
-
- if not isclass(obj) and callable(obj) and getattr(obj.__call__, "_is_coroutine", None) is _is_coroutine:
- return True
+ if not isclass(obj) and callable(obj):
+ # Test both the function and the __call__ implementation for the
+ # _is_coroutine marker.
+ f = getattr(getattr(obj, "__func__", obj), "_is_coroutine", None)
+ c = getattr(obj.__call__, "_is_coroutine", None)
+ if f is _is_coroutine or c is _is_coroutine:
+ return True
return _has_code_flag(obj, CO_COROUTINE) or (
not isclass(obj) and callable(obj) and _has_code_flag(obj.__call__, CO_COROUTINE)
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.
So perhaps a confusing aspect is that everything that's callable has a __call__
method. Also the presence of __func__
indicates it's a class or instance method.
At least, those things seem to be obfuscating what's going on a bit.
Also, looking at the code for _has_code_flag()
, I wonder if we don't need the same logic for checking the _is_coroutine
flag.
What would happen if we wrote a helper function like this?
def _has_coroutine_mark(f):
while ismethod(f):
f = f.__func__
f = functools._unwrap_partial(f)
if not (isfunction(f) or _signature_is_functionlike(f)):
return False
return getattr(f, "_is_coroutine", None) is _is_coroutine
(FWIW I wonder if the variable _is_coroutine
shouldn't be renamed _is_coroutine_marker
.)
Now, I'm not entirely sure why you're testing for __call__
, when none of the other is<something>function()
predicates test for it. Maybe things would become simpler if we removed that feature?
We could then define our function like this:
def iscoroutinefunction(obj):
return _has_code_flag(obj, CO_COROUTINE) or _has_coroutine_mark(obj)
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.
Now, I'm not entirely sure why you're testing for call
OK, so the issue is we have classes like this:
class Cl:
async def __call__(self):
pass
... where we need instances to be identified as async, and iscoroutinefunction
doesn't (currently) pick it up.
Note that this is an actual, already-out-there, need. e.g. the Starlette framework has this exact check to work around this limitation. asgiref
works around this limitation in its usage by marking the instance itself with the _is_coroutine
marker.
Either way, I think it is desirable to return True
for class instance of classes such as Cl
with an async def __call__()
So that's the reason for adding the additional ... _has_code_flag(obj.__call__, ...)
check.
In this PR, given that we were adding @staticmethod
and @classmethod
and so on, it seemed that we also should cover this kind of case:
class Cl2:
@inspect.markcoroutinefunction
def __call__(self):
pass
Which leads to needing similar in the _is_coroutine_marker
block.
The latter case is somewhat hypothetical... 1�7 it comes from trying to cover all the cases here, rather than from real-use℄1�7. To that extent I'd be happy to drop it, but I guess if we do there's an issue in X months time, saying "I need to do this".
I don't know the correct thing to say. (?)
(FWIW I wonder if the variable _is_coroutine shouldn't be renamed _is_coroutine_marker.)
Yep OK. +1
What would happen if we wrote a helper function like this?
Let me have a read.
Thanks so much for your thought and input on this.
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.
Hum, I still feel that you're sneaking in a new feature under the radar. :-) What this PR is supposed to fix is just to add the ability to mark a sync function as "morally async". IOW the behavior of
class C:
@inspect.markcoroutinefunction
def __call__(self): ...
print(inspect.iscoroutinefunction(C())
should be the same as
class C:
async def __call__(self): ...
print(inspect.iscoroutinefunction(C())
and since the latter prints False
, I don't think this PR has any business changing the output to True
.
If you think the functionality used by Starlette deserves to be included in inspect, you should probably open a new issue for that first so it can be discussed properly. But I personally think this is overreaching (however, I am repeating myself).
Have you considered adding def _has_coroutine_mark(f):
as I suggested?
(Sorry to be such a pain. But once we let this in there's no way back, so I prefer to have the minimal necessary functionality only. Other core devs may differ though.)
|
Name | Link |
---|---|
5ffba32 | |
https://app.netlify.com/sites/python-cpython-preview/deploys/6398396366b7310008247965 | |
https://deploy-preview-99247--python-cpython-preview.netlify.app | |
To edit notification comments on pull requests, go to your Netlify site settings.
Hi @gvanrossum —1�7I've updated as per your advice. Please let me know what you think.
Absolutely, I understand that.
That's slightly frustrating (
Repass over why I think `__call__` support is needed 1�7The issue is that The two missing cases are:
If we proceed here without handling the Beyond those added here, there are no tests for the classes defining
So, yes, we're going round in circles. Ultimately, I have to go with your decision (no problem). If you're not convinced then there's little left to say. Please advise if you think I should re-add Thanks for your patience and guidance on this PR, and all round work beyond that. |
Refs: python/cpython#99247 This would be needed for Django 4.2, which would support PY312 on release of that.
Refs: python/cpython#99247 This would be needed for Django 4.2, which would support PY312 on release of that.
@gvanrossum I think this is now as you want it. Please let me know if there's further to do here. Thanks. |
Thanks a lot for this improvement. Sorry it took us all a while to settle on the right way to do it. |
Thanks @gvanrossum. No worries! Better to be sure. Thanks again for your help! |
* main: pythongh-89727: Fix os.walk RecursionError on deep trees (python#99803) Docs: Don't upload CI artifacts (python#100330) pythongh-94912: Added marker for non-standard coroutine function detection (python#99247) Correct CVE-2020-10735 documentation (python#100306) pythongh-100272: Fix JSON serialization of OrderedDict (pythonGH-100273) pythongh-93649: Split tracemalloc tests from _testcapimodule.c (python#99551) Docs: Use `PY_VERSION_HEX` for version comparison (python#100179) pythongh-97909: Fix markup for `PyMethodDef` members (python#100089) pythongh-99240: Reset pointer to NULL when the pointed memory is freed in argument parsing (python#99890) pythongh-99240: Reset pointer to NULL when the pointed memory is freed in argument parsing (python#99890) pythonGH-98831: Add DECREF_INPUTS(), expanding to DECREF() each stack input (python#100205) pythongh-78707: deprecate passing >1 argument to `PurePath.[is_]relative_to()` (pythonGH-94469)
This is ref the discussion on #94912 to add an official way of marking callables as coroutine functions where they would not otherwise be detected.
cc. @gvanrossum @andrewgodwin I've just had time this afternoon to block off some first tests, and docs. I wanted to break ground so we keep it moving. It's very consciously drafts: please make suggestions of how you think it should go.
@gvanrossum Can I ask for a pointer on how I'm meant to set🤔
co_flags
for the_has_code_flag()
check?cpython/Lib/inspect.py
Lines 376 to 385 in c43714f
Thanks!
First PR on CPython, so likely it's wrong😊
% ./python.exe -m unittest test.test_inspect.TestPredicates
asyncio.iscoroutinefunction
a deprecated alias ofinspect.iscoroutinefunction
and removeasyncio.coroutines._is_coroutine
#94912 OK?asyncio.iscoroutinefunction
a deprecated alias ofinspect.iscoroutinefunction
and removeasyncio.coroutines._is_coroutine
#94912