bpo-34890: Make iscoroutinefunction, isgeneratorfunction and isasyncgenfunction work with functools.partial by pablogsal · Pull Request #9903 · python/cpython

pablogsal · 2018-10-15T22:14:04Z

I have added some extra test cases for isasyncgenfunction and isasyncgen in test_iscoroutine for symmetry, but I am not sure if that is the place for it. Tell me if I should reallocate these tests in other place.

https://bugs.python.org/issue34890

…work with partial

1st1 · 2018-10-15T22:18:19Z

Lib/inspect.py

                object.__code__.co_flags & CO_GENERATOR)
+    is_partial_generator = bool((isinstance(object, functools.partial) and
+                object.func.__code__.co_flags & CO_GENERATOR))
+    return is_generator_function or is_partial_generator


Wouldn't it be better to do this:

while isinstance(object, functools.partial): object = object.func

? The benefit would be support of nested partials and the code will probably look less ugly.

Also, while you're here, please rename object to obj or o.

pablogsal · 2018-10-15T22:25:00Z

@1st1 Done in 41f7f60. Thanks for the review :)

1st1 · 2018-10-15T22:34:05Z

Lib/inspect.py

    """
-    return bool((isfunction(object) or ismethod(object)) and
-                object.__code__.co_flags & CO_ASYNC_GENERATOR)
+    while isinstance(obj, functools.partial):


Why don't we add an _unwrap_partial helper. Or.. should we maybe use inspect.unwrap() too?

Actually, let's not use inspect.unwrap(). Decorating generators and coroutines isn't that common.

Do we still want the helper?

I have done the refactor in 3b841ac

1st1 · 2018-10-15T22:45:35Z

Lib/test/test_inspect.py

 import collections
 import datetime
 import functools
+from functools import partial


Please use module import (i.e. import functools).

Done in adb5db6. I did that originally to simplify the double functools.partial wrapping in the tests.

pablogsal · 2018-10-15T23:14:09Z

Hummmm, it seems that this changes the behaviour of coroutine in Lib/asyncio/coroutines.py:

def coroutine(func):
    """Decorator to mark coroutines.

    If the coroutine is not yielded from before it is destroyed,
    an error message is logged.
    """
    if inspect.iscoroutinefunction(func):
        # In Python 3.5 that's all we need to do for coroutines
        # defined with "async def".
        return func

    if inspect.isgeneratorfunction(func):
        coro = func
    else:
        @functools.wraps(func)
        def coro(*args, **kw):

this makes the function not being wrapped in CoroWrapper anymore.

@1st1 I don't know if this has a bit more impact than expected. Should I fix this by adding an extra check for partial in coroutine() (check commit 617688e to see what I mean), or should we approach this differently?

…eserve behaviour

1st1

Please remove new checks from asyncio.coroutine; AFAICT we don't need them.

1st1 · 2018-10-16T02:28:11Z

Lib/asyncio/coroutines.py

    """
-    if inspect.iscoroutinefunction(func):
+    if (inspect.iscoroutinefunction(func)
+            and not isinstance(func, functools.partial)):


We don't need extra checks in this function. Here, we check for an async def coroutine -- we don't need to wrap them in a CoroWrapper ever.

1st1 · 2018-10-16T02:29:29Z

Lib/asyncio/coroutines.py


-    if inspect.isgeneratorfunction(func):
+    if (inspect.isgeneratorfunction(func)
+            and not isinstance (func, functools.partial)):


We don't need this extra check either -- if func is a generator function, it means that we don't need to wrap it into a proxy to force it to return a generator.

bedevere-bot · 2018-10-16T02:30:07Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

pablogsal · 2018-10-16T08:10:26Z

@1st1 If I remove those checks then https://github.com/python/cpython/blob/master/Lib/test/test_asyncio/test_tasks.py#L425 will fail because the partials are not wrapped anymore and the __reprl__ will be different. I was not sure if this would be a problem or not (notice commit 617688e was just to show what I mean with "fix this by adding an extra check for partial in coroutine()" in my previous comment). Should I change the tests now that we don't need to wrap these (proposal of that change in commit caddd21)?

This is an example failure of that test for clarity:

======================================================================
FAIL: test_task_repr_partial_corowrapper (test.test_asyncio.test_tasks.PyTask_PyFuture_Tests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/pablogsal/github/cpython/Lib/test/test_asyncio/test_tasks.py", line 446, in test_task_repr_partial_corowrapper
    self.assertRegex(coro_repr, expected)
AssertionError: Regex didn't match: '<CoroWrapper \\w+.test_task_repr_partial_corowrapper\\.<locals>\\.func\\(1\\)\\(\\) running, ' not found in '<coroutine object BaseTaskTests.test_task_repr_partial_corowrapper.<locals>.func at 0x7f30c10b9788>'

----------------------------------------------------------------------

vstinner

I like the overall change.

@1st1: Would you mind to review this change?

vstinner · 2018-10-25T00:09:48Z

Lib/inspect.py

+def _unwrap_partial(func):
+    while isinstance(func, functools.partial):
+        func = func.func
+    return func


I suggest to move this helper function to Lib/functools.py.

vstinner · 2018-10-25T00:12:10Z

Misc/NEWS.d/next/Library/2018-10-15-23-10-41.bpo-34890.77E770.rst

@@ -0,0 +1,3 @@
+Make :func:`inspect.iscoroutinefunction`,
+:func:`inspect.isgeneratorfunction` and :func:`inspect.isasyncgenfunction`
+work with :func:`functools.partial`. Patch by Pablo Galindo.


Maybe also document the change with a ".. versionchanged:: 3.8" markup in https://docs.python.org/dev/library/inspect.html#inspect.isgeneratorfunction ?

vstinner · 2018-10-25T00:13:01Z

(Oh, @1st1 already started to review this change, good ;-))

vstinner

LGTM, but I would prefer to see a second review from a core (@1st1 maybe?).

1st1 · 2018-10-26T03:00:30Z

@1st1 If I remove those checks then https://github.com/python/cpython/blob/master/Lib/test/test_asyncio/test_tasks.py#L425 will fail because the partials are not wrapped anymore and the reprl will be different. I was not sure if this would be a problem or not

It's not a problem. Now that inspect.isgeneratorfunction works with functools.partial we no longer need to wrap partial objects in a proxy generator "just in case".

1st1

Sorry for the delayed review; I've been extremely busy last couple of weeks.

pablogsal · 2018-10-26T11:19:47Z

@1st1 Thank you very much for the review and the advice, Yuri! :)

jdemeyer · 2018-11-05T11:16:43Z

See also #6448 for a very related problem.

1st1 · 2019-03-29T17:03:07Z

Lib/functools.py

+# Helper functions
+
+def _unwrap_partial(func):
+    while isinstance(func, partial):


@pablogsal BTW this doesn't handle the case of partialmethod. Should we add that?

pablogsal self-assigned this Oct 15, 2018

pablogsal requested review from 1st1 and asvetlov October 15, 2018 22:14

the-knights-who-say-ni added the CLA signed label Oct 15, 2018

bedevere-bot added the awaiting merge label Oct 15, 2018

pablogsal changed the title ~~Make iscoroutinefunction, isgeneratorfunction and isasyncgenfunction work with functools.partial~~ bpo-34890: Make iscoroutinefunction, isgeneratorfunction and isasyncgenfunction work with functools.partial Oct 15, 2018

Make iscoroutinefunction, isgeneratorfunction and isasyncgenfunction …

633c497

…work with partial

pablogsal force-pushed the bpo34890 branch from ded4d70 to 633c497 Compare October 15, 2018 22:16

1st1 reviewed Oct 15, 2018

View reviewed changes

pablogsal force-pushed the bpo34890 branch from 3d14641 to da78465 Compare October 15, 2018 22:25

Simplify code, rename variables and add test for nested partials

41f7f60

pablogsal force-pushed the bpo34890 branch from da78465 to 41f7f60 Compare October 15, 2018 22:26

fixup! Simplify code, rename variables and add test for nested partials

b65500e

1st1 reviewed Oct 15, 2018

View reviewed changes

Add _unwrap_partial helper to reduce repetition

3b841ac

1st1 reviewed Oct 15, 2018

View reviewed changes

Use module import in tests

adb5db6

1st1 approved these changes Oct 15, 2018

View reviewed changes

Add extra check to Lib/asyncio/coroutines:coroutine for partial to pr…

617688e

…eserve behaviour

1st1 requested changes Oct 16, 2018

View reviewed changes

bedevere-bot removed the awaiting merge label Oct 16, 2018

bedevere-bot added the awaiting changes label Oct 16, 2018

Change test that checks __reprl__ of partial

caddd21

vstinner reviewed Oct 25, 2018

View reviewed changes

Refactor helper function and document changes

0585757

pablogsal requested a review from rhettinger as a code owner October 25, 2018 22:29

vstinner approved these changes Oct 25, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Oct 25, 2018

1st1 approved these changes Oct 26, 2018

View reviewed changes

pablogsal merged commit 7cd2543 into python:master Oct 26, 2018

bedevere-bot removed the awaiting merge label Oct 26, 2018

pablogsal deleted the bpo34890 branch October 26, 2018 11:19

1st1 reviewed Mar 29, 2019

View reviewed changes

Uh oh!

Comments

Conversation

pablogsal commented Oct 15, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1st1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Oct 16, 2018

Uh oh!

pablogsal commented Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Oct 25, 2018

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

1st1 commented Oct 26, 2018

Uh oh!

1st1 left a comment

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Oct 26, 2018

Uh oh!

jdemeyer commented Nov 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pablogsal commented Oct 15, 2018 •

edited by bedevere-bot

Loading

pablogsal commented Oct 15, 2018 •

edited

Loading

pablogsal commented Oct 15, 2018 •

edited

Loading

pablogsal commented Oct 16, 2018 •

edited

Loading

jdemeyer commented Nov 5, 2018 •

edited

Loading