Movatterモバイル変換

makeasyncio.iscoroutinefunction a deprecated alias ofinspect.iscoroutinefunction and removeasyncio.coroutines._is_coroutine#94912

Closed

bedevere-bot added the awaiting review label

Copy link

bedevere-bot commentedNov 8, 2022

Most changes to Pythonrequire a NEWS entry.

Please add it using theblurb_it web app or theblurb command-line tool.

bedevere-bot added the testsTests in the Lib/test dir label

Copy link

ghost commentedNov 8, 2022•
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.

Copy link

ContributorAuthor

carltongibson commentedNov 8, 2022•
edited
Loading

~~CLA bot is giving an Heroku application error.~~ (Seems to have worked anyway 🕺)

I can look into the News item.

Copy link

Member

sobolevn commentedNov 8, 2022•
edited
Loading

@carltongibson happy to see you here! Feel free to ask any questions you have: I am happy to help with what I can :)

The simplest way of adding news is by usinghttps://blurb-it.herokuapp.com/
Running tests is better with./python.exe -m test -v test_inspect

co_flags for the _has_code_flag() check?

Python API has__code__.replace:

>>>defsome(): ......>>>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.

AlexWaygood added the topic-asyncio label

Copy link

bedevere-bot commentedNov 15, 2022

Most changes to Pythonrequire a NEWS entry.

Please add it using theblurb_it web app or theblurb command-line tool.

Copy link

ContributorAuthor

carltongibson commentedNov 15, 2022

Hi@sobolevn — thanks for your pointer very helpful.

I've pushed two commits, making the tests pass:

ab422f7 uses the__code__.replace() technique, and looks good. (I need to move this to a decorator — I'll work on this next.)
8c6429e adjusts the iscoroutinefunction() function to check the extra case of a class with anasync def __call__(). I went this way because I wasn't able to get the__code__ approach working. (AttributeError: type object 'Cl' has no attribute '__code__'. ...)

I wanted to push that to see if I could get feedback. Thanks.

I don't know anything about its performance

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 thejust useasync def response would become dominant. 🤔

sobolevn reviewed

Nov 15, 2022

Lib/inspect.py OutdatedShow resolvedHide resolved

Lib/test/test_inspect.py Outdated

		# Second case: a class with an async def __call__.
		# - instance is awaitable.
		class Cl:
		async def __call__(self):

Copy link

Member

sobolevnNov 15, 2022

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 :)

Copy link

ContributorAuthor

carltongibsonNov 15, 2022

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. 🤔

Copy link

Member

sobolevnNov 17, 2022

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 likestaticmethod andclassmethod 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 :)

Copy link

ContributorAuthor

carltongibsonNov 17, 2022

Choose a reason for hiding this comment

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

@sobolevn — OK, let me have a play and see. Thanks!

carltongibsonand others added5 commits

November 17, 2022 11:25

Added initial test and docs drafts.

4cf8e98

Make first case pass setting code flags.

6b8fa87

Adjust iscoroutinefunction() to look for async __call__.

fa22a16

📜🤖 Added by blurb_it.

513d358

Moved to decorator; extra tests.

397a975

carltongibson force-pushed thefix-issue-XXXXX branch from48eb2ec to397a975Compare

November 17, 2022 10:25

Copy link

ContributorAuthor

carltongibson commentedNov 17, 2022•
edited
Loading

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.

I'm quite sure that it's not perfect, but I need guidance on how you want it to be. Quite happy to make adjustments.
There were a couple of comments from@sobolevn that I didn't follow. Hopefully that additional tests clarify what's needed. (Just seengh-94912: Added marker for non-standard coroutine function detection #99247 (comment) — will experiment.)

Thanks!

carltongibson marked this pull request as ready for review

November 17, 2022 10:32

sobolevn reviewed

Nov 17, 2022

Copy link

Member

sobolevn left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Sidenote:asyncio.iscoroutinefunction will also be affected by this change.

Doc/library/inspect.rst OutdatedShow resolvedHide resolved

Lib/test/test_inspect.pyShow resolvedHide resolved

Copy link

Member

gvanrossum commentedNov 19, 2022

Let me know when you both feel this is ready to go in.

Copy link

ContributorAuthor

carltongibson commentedNov 19, 2022

Thanks@gvanrossum.

I'm going to add the extra test cases, and then (from my POV) it's aAny changes? question.

Sidenote: asyncio.iscoroutinefunction will also be affected by this change.

That's being made an alias to theinspect version as part ofgh-94912 IIUC — so are additional changes (doc maybe) needed as part ofthis PR? 🤔

Thanks for the guidance both.

carltongibson added2 commits

November 23, 2022 15:39

Added tests for lambda,@classmethod, and@staticmethodcases.

d156eab

Adjusted docs signature for review comment.

34859de

Copy link

ContributorAuthor

carltongibson commentedNov 23, 2022

Hi@sobolevn and@gvanrossum — 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 theasyncio.iscoroutinefunction (and the_is_coroutine marker attribute): we'd introduce a compatibility shim until 3.12 was the lowest supported version, but that's easily enough done.
Thanks again.

//cc@andrewgodwin.

sobolevn reviewed

Nov 23, 2022

Copy link

Member

sobolevn left a comment•
edited by gvanrossum
Loading

Choose a reason for hiding this comment

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

Thank you! I hope your first PR to CPython was a pleasant experience 😉

I have just a single main concern: thatinspect.iscoroutinefunction no longer does what is documented.

Citing:

Return True if the object is acoroutine function (a function defined with anasync def syntax).

Now it can returnTrue for instances with async__call__ methods.
This might backfire for users that trust this check right now.

I suggest to (but, this is justmy opinion, I don't know what others think of it):

markcoroutinefunction is a great thing to have 👍
But, maybe we can delay making changes toiscoroutinefunction 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.

carltongibson added a commit to django/asgiref that referenced this pull request

Nov 23, 2022

[DRAFT] Initial sketch of iscoroutinefunction shim for PY312

7f52d4a

cc@andrewgodwinRefs:python/cpython#99247This would be needed for Django 4.2, which would support PY312 on release of that.

Copy link

ContributorAuthor

carltongibson commentedNov 23, 2022•
edited
Loading

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 theCl2 example, with theasync def __call__.

Given there's no existing test covering this, I think it's similar to how support forpartial instances was added here in Python 3.8. It's a case we do want True for, and should add (and doc).

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 anis_async_callable helper, which is a good name for what we're about here. 🤔

The nameiscoroutinefunction gets stretched a little... I'm not sure a second function would carry its weight.

🤔

gvanrossum reviewed

Nov 23, 2022

Misc/NEWS.d/next/Library/2022-11-17-10-02-18.gh-issue-94912.G2aa-E.rst OutdatedShow resolvedHide resolved

Improve grammar in NEWS file

47a9fea

Copy link

bedevere-bot commentedDec 6, 2022

Thanks for making the requested changes!

@gvanrossum,@kumaraditya303: please review the changes made to this pull request.

bedevere-bot requested review fromgvanrossum andkumaraditya303

December 6, 2022 11:41

carltongibson commented

Dec 6, 2022

https://github.com/python/cpython/blob/3.11/Lib/asyncio/coroutines.py#L17-L24

Lib/inspect.py Outdated

Comment on lines 413 to 418

		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

Copy link

ContributorAuthor

carltongibsonDec 6, 2022•
edited
Loading

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 olderasyncio one…

… but (as per the added test cases) various extra (seemingly legitimate) cases are now covered. (So 👍 I guess)

Copy link

Member

gvanrossumDec 7, 2022

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.

Copy link

ContributorAuthor

carltongibsonDec 8, 2022

Choose a reason for hiding this comment

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

Hey@gvanrossum — thanks.

I thought one maybe neater re-write looks like this:

diff --git a/Lib/inspect.py b/Lib/inspect.pyindex 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)

Copy link

Member

gvanrossumDec 8, 2022

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 thateverything 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 otheris<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)

Copy link

ContributorAuthor

carltongibsonDec 9, 2022

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 forcall

OK, so the issue is we have classes like this:

        class Cl:            async def __call__(self):                pass

... where we needinstances to be identified as async, andiscoroutinefunction 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 returnTrue for class instance of classes such asCl with anasync 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... — it comes from trying to cover all the cases here, rather than from real-use™. 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.

Copy link

Member

gvanrossumDec 10, 2022

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 isjust 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 printsFalse, I don't thinkthis PR has any business changing the output toTrue.

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 addingdef _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.)

kumaraditya303 dismissed theirstale review

December 8, 2022 07:53

news entry added

Adjusted implementation.

3bc72e2

Copy link

netlifybot commentedDec 8, 2022•
edited
Loading

✅ Deploy Preview forpython-cpython-preview ready!

Name	Link
🔨 Latest commit	`5ffba32`
🔍 Latest deploy log	https://app.netlify.com/sites/python-cpython-preview/deploys/6398396366b7310008247965
😎 Deploy Preview	https://deploy-preview-99247--python-cpython-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to yourNetlify site settings.

carltongibson added2 commits

December 13, 2022 09:16

Renamed to _is_coroutine_marker.

c073cf7

Adjusted implementation and docs.

5ffba32

Copy link

ContributorAuthor

carltongibson commentedDec 13, 2022•
edited
Loading

Hi@gvanrossum — I've updated as per your advice. Please let me know what you think.

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...

Absolutely, I understand that.

...so it can be discussed properly...

That's slightly frustrating (🙂), as I thought that was exactly the conversation we'd had leading to your#99247 (comment) above:

I think it's better not to add a new function ...

Repass over why I think `__call__` support is needed…

The issue is thatinspect.iscoroutinefunction does not capture all the cases it needs to. Theasyncio version doesn't either, but this surfaces because the proposal to remove that version makes the deficiency (breakingly) worse.

The two missing cases are:

Sync functions that return coroutine objects.
Class instances withasync def __call__.

If we proceed here without handling the__call__ case we leave that broken (forcing clients to implement their own different shims for each framework) I can put that in a separate issue or PR or commit but as I'm reading you, you wouldn't accept that anyway?

Beyond those added here, there are no tests for the classes definingasync def __call__ methods withiscoroutinefunction. But as a consumer ofiscoroutinefunction I (clearly) wantTrue in such cases (and the hand-marked equivalents.) Fixing this seems no different that fixingfunctools.partial support in Python 3.8.

…however, I am repeating myself…

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__call__ support here, or open a new ticket, or leave it. (If you thinkleave it, that's totally fine. We can cope.)

Thanks for your patience and guidance on this PR, and all round work beyond that. 🎁

carltongibson added a commit to django/asgiref that referenced this pull request

Dec 13, 2022

Initial sketch of iscoroutinefunction shim for PY312

ef7d542

Refs:python/cpython#99247This would be needed for Django 4.2, which would support PY312 on release of that.

This was referencedDec 13, 2022

Added coroutine detection shims for Python 3.12django/asgiref#360

Merged

Refs #34118 -- Adopted asgiref coroutine detection shims.django/django#16380

Merged

kumaraditya303 removed their request for review

December 14, 2022 10:21

carltongibson added a commit to django/asgiref that referenced this pull request

Dec 14, 2022

Initial sketch of iscoroutinefunction shim for PY312

a663777

Refs:python/cpython#99247This would be needed for Django 4.2, which would support PY312 on release of that.

Copy link

ContributorAuthor

carltongibson commentedDec 15, 2022

@gvanrossum I think this is now as you want it. Please let me know if there's further to do here. Thanks.

gvanrossum approved these changes

Copy link

Member

gvanrossum left a comment

Choose a reason for hiding this comment

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

Perfect!

bedevere-bot added awaiting merge and removed awaiting change review labels

gvanrossum merged commit532aa4e intopython:main

bedevere-bot removed the awaiting merge label