Skip to content
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

Fix types.coroutine to accept objects from Cython #68504

Closed
1st1 opened this issue May 28, 2015 · 14 comments
Closed

Fix types.coroutine to accept objects from Cython #68504

1st1 opened this issue May 28, 2015 · 14 comments
Assignees
Labels
stdlib Python modules in the Lib dir

Comments

@1st1
Copy link
Member

1st1 commented May 28, 2015

BPO 24316
Nosy @gvanrossum, @ncoghlan, @scoder, @1st1
Dependencies
  • bpo-24315: collections.abc: Coroutine should be derived from Awaitable
  • Files
  • coroutine.patch
  • types_coroutine.patch
  • types_coroutine.patch
  • types_coroutine.patch
  • types_coroutine.patch
  • types_coroutine.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/1st1'
    closed_at = <Date 2015-05-29.20:21:09.464>
    created_at = <Date 2015-05-28.16:31:52.508>
    labels = ['library']
    title = 'Fix types.coroutine to accept objects from Cython'
    updated_at = <Date 2015-05-29.21:11:28.466>
    user = 'https://github.com/1st1'

    bugs.python.org fields:

    activity = <Date 2015-05-29.21:11:28.466>
    actor = 'yselivanov'
    assignee = 'yselivanov'
    closed = True
    closed_date = <Date 2015-05-29.20:21:09.464>
    closer = 'yselivanov'
    components = ['Library (Lib)']
    creation = <Date 2015-05-28.16:31:52.508>
    creator = 'yselivanov'
    dependencies = ['24315']
    files = ['39536', '39548', '39551', '39555', '39556', '39557']
    hgrepos = []
    issue_num = 24316
    keywords = ['patch']
    message_count = 14.0
    messages = ['244315', '244373', '244386', '244391', '244393', '244394', '244399', '244406', '244408', '244409', '244412', '244413', '244414', '244419']
    nosy_count = 5.0
    nosy_names = ['gvanrossum', 'ncoghlan', 'scoder', 'python-dev', 'yselivanov']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue24316'
    versions = ['Python 3.5', 'Python 3.6']

    @1st1
    Copy link
    Member Author

    1st1 commented May 28, 2015

    Stefan,

    This patch should solve the problem with types.coroutine accepting only pure python generator functions.

    The approach is, however, slightly different from what you've proposed. Instead of having a wrapper class (delegating .throw, .send etc to a wrapped object), we now simply check if the returned value of the wrapped function is an instance of collections.abc.Coroutine. bpo-24315 enables duck typing for coroutines, so if a cython-based coroutine implements all coroutine abstract methods, it will automatically pass types.coroutine.

    @1st1 1st1 self-assigned this May 28, 2015
    @1st1 1st1 added the stdlib Python modules in the Lib dir label May 28, 2015
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 29, 2015

    New changeset 7356f71fb0a4 by Yury Selivanov in branch '3.5':
    bpo-24316: Fix types.coroutine() to accept objects from Cython
    https://hg.python.org/cpython/rev/7356f71fb0a4

    New changeset 748c55375225 by Yury Selivanov in branch 'default':
    bpo-24316: Fix types.coroutine() to accept objects from Cython
    https://hg.python.org/cpython/rev/748c55375225

    @1st1 1st1 closed this as completed May 29, 2015
    @scoder
    Copy link
    Contributor

    scoder commented May 29, 2015

    I just noticed that I hadn't used the real "types.coroutine" in my Py3.5 tests when reporting back in bpo-24017. When I pass a Cython generator through it, I get

    """
    Traceback (most recent call last):
      File "tests/run/test_coroutines_pep492.pyx", line 245, in test_coroutines_pep492.CoroutineTest.test_func_5 (test_coroutines_pep492.c:13445)
        for el in bar():
      File "/opt/python3.5/lib/python3.5/types.py", line 197, in wrapped
        'non-coroutine: {!r}'.format(coro))
    TypeError: callable wrapped with types.coroutine() returned non-coroutine: <generator object at 0x7f178c458898>
    """

    This is actually obvious, given that the sole purpose of the decorator is to turn something that is a Generator and *not* a Coroutine into something that is a Coroutine, as a means for the user to say "but I know better". So checking for the return value being a Coroutine is wrong. Instead, it should check that it's a Generator and if it's not an Awaitable, wrap it as a self-returning Awaitable. That's more or less what my proposed implementation in bpo-24017 did:

      class types_coroutine(object):
        def __init__(self, gen):
            self._gen = gen
    
        class as_coroutine(object):
            def __init__(self, gen):
                self._gen = gen
                self.send = gen.send
                self.throw = gen.throw
                self.close = gen.close
    
            def __await__(self):
                return self._gen
    
        def __call__(self, *args, **kwargs):
            return self.as_coroutine(self._gen(*args, **kwargs))

    @1st1
    Copy link
    Member Author

    1st1 commented May 29, 2015

    I just noticed that I hadn't used the real "types.coroutine" in my Py3.5 tests when reporting back in bpo-24017.

    Please test thoroughly the attached patch.

    @scoder
    Copy link
    Contributor

    scoder commented May 29, 2015

    One failing test in "test_coroutines": test_func_5. The reason is that the GeneratorWrapper is not iterable (and there is no reason it shouldn't be, given that it wraps a Generator). That was my fault, I had already added an __iter__ method but didn't copy it in my previous message. Adding it as follows fixes the test for me:

        def __iter__(self):
            return self.__wrapped__

    Alternatively, "__iter__ = __await__" would do the same.

    @scoder
    Copy link
    Contributor

    scoder commented May 29, 2015

    BTW, it's not only for compiled generators but also for normal Python functions that construct Python generators internally and return them, or that delegate the generator creation in some way. With this change, it's enough to decorate the constructor function and not each of the potential generators that it returns.

    @1st1 1st1 reopened this May 29, 2015
    @1st1
    Copy link
    Member Author

    1st1 commented May 29, 2015

    Please test the attached patch.

    BTW, it's not only for compiled generators but also for normal Python functions that construct Python generators internally and return them

    You're right, that's why I used "primarily" word in that comment ;)

    types.coroutine() is only used by asyncio.coroutine() so far, and returning generator objects from "factory" functions isn't a very common pattern in asyncio.

    @1st1
    Copy link
    Member Author

    1st1 commented May 29, 2015

    Updated patch. Wrapper now proxies gi_code, gi_running and gi_frame

    @scoder
    Copy link
    Contributor

    scoder commented May 29, 2015

    Ok, now the problem with *this* patch is that __iter__ and __await__ are special methods that are being looked up on the type, not the instance. Similarly __next__, I think, as it also has its own (type) slot. But I think you're right that __next__ is also needed here.

    I'm attaching a patch that works for me.

    @1st1
    Copy link
    Member Author

    1st1 commented May 29, 2015

    I'm attaching a patch that works for me.

    Looks like we were working in parallel ;) I've incorporated your changes. Please look at the new patch (hopefully this one is final)

    @scoder
    Copy link
    Contributor

    scoder commented May 29, 2015

    Your latest patch works for me.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 29, 2015

    New changeset 8080b53342e8 by Yury Selivanov in branch '3.5':
    bpo-24316: Wrap gen objects returned from callables in types.coroutine
    https://hg.python.org/cpython/rev/8080b53342e8

    New changeset c0434ef75177 by Yury Selivanov in branch 'default':
    bpo-24316: Wrap gen objects returned from callables in types.coroutine
    https://hg.python.org/cpython/rev/c0434ef75177

    @1st1
    Copy link
    Member Author

    1st1 commented May 29, 2015

    Committed. Thanks, Stefan!

    @1st1 1st1 closed this as completed May 29, 2015
    @1st1
    Copy link
    Member Author

    1st1 commented May 29, 2015

    Stefan, please take a look at this issue bpo-24325 too.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants