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

__aiter__ should return async iterator instead of awaitable #71430

Closed
1st1 opened this issue Jun 6, 2016 · 22 comments
Closed

__aiter__ should return async iterator instead of awaitable #71430

1st1 opened this issue Jun 6, 2016 · 22 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) release-blocker type-feature A feature request or enhancement

Comments

@1st1
Copy link
Member

1st1 commented Jun 6, 2016

BPO 27243
Nosy @gvanrossum, @brettcannon, @ncoghlan, @larryhastings, @ned-deily, @ambv, @vadmium, @1st1
Files
  • fix_aiter.patch
  • fix_aiter2.patch
  • fix_aiter3.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 2016-06-11.05:26:38.950>
    created_at = <Date 2016-06-06.19:21:36.418>
    labels = ['interpreter-core', 'type-feature', 'release-blocker']
    title = '__aiter__ should return async iterator instead of awaitable'
    updated_at = <Date 2016-11-08.20:15:49.312>
    user = 'https://github.com/1st1'

    bugs.python.org fields:

    activity = <Date 2016-11-08.20:15:49.312>
    actor = 'python-dev'
    assignee = 'yselivanov'
    closed = True
    closed_date = <Date 2016-06-11.05:26:38.950>
    closer = 'martin.panter'
    components = ['Interpreter Core']
    creation = <Date 2016-06-06.19:21:36.418>
    creator = 'yselivanov'
    dependencies = []
    files = ['43264', '43268', '43307']
    hgrepos = []
    issue_num = 27243
    keywords = ['patch']
    message_count = 22.0
    messages = ['267544', '267547', '267548', '267551', '267557', '267559', '267560', '267561', '267563', '267565', '267572', '267577', '267865', '268035', '268054', '268055', '268063', '268089', '268117', '268184', '268216', '280343']
    nosy_count = 9.0
    nosy_names = ['gvanrossum', 'brett.cannon', 'ncoghlan', 'larry', 'ned.deily', 'lukasz.langa', 'python-dev', 'martin.panter', 'yselivanov']
    pr_nums = []
    priority = 'release blocker'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue27243'
    versions = ['Python 3.5', 'Python 3.6']

    @1st1
    Copy link
    Member Author

    1st1 commented Jun 6, 2016

    There is a small flaw in PEP-492 design -- __aiter__ should not return an awaitable object that resolves to an asynchronous iterator. It should return an asynchronous iterator directly.

    Let me explain this by showing some examples.

    I've discovered this while working on a new asynchronous generators PEP. Let's pretend that we have them already: if we have a 'yield' expression in an 'async def' function, the function becomes an "asynchronous generator function":

       async def foo():
          await bar()
          yield 1
          await baz()
          yield 2

    foo -- is an asynchronous generator function

    foo() -- is an asynchronous generator

    If we iterate through "foo()", it will await on "bar()", yield "1", await on "baz()", and yield "2":

       >>> async for el in foo():
       ...     print(el)
       1
       2

    If we decide to have a class with an __aiter__ that is an async generator, we'd write something like this:

       class Foo:
          async def __aiter__(self):
              await bar()
              yield 1
              await baz()
              yield 2

    However, with the current PEP-492 design, the above code would be invalid! The interpreter expects __aiter__ to return a coroutine, not an async generator.

    I'm still working on the PEP for async generators, targeting CPython 3.6. And once it is ready, it might still be rejected or deferred. But in any case, this PEP-492 flaw has to be fixed now, in 3.5.2 (since PEP-492 is provisional).

    The attached patch fixes the __aiter__ in a backwards compatible way:

    1. ceval/GET_AITER opcode calls the __aiter__ method.

    2. If the returned object has an '__anext__' method, GET_AITER silently wraps it in an awaitable, which is equivalent to the following coroutine:

        async def wrapper(aiter_result):
            return aiter_result
    1. If the returned object does not have an '__anext__' method, a DeprecationWarning is raised.

    @1st1 1st1 self-assigned this Jun 6, 2016
    @1st1 1st1 added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Jun 6, 2016
    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jun 6, 2016

    While I agree this needs to be fixed, one key piece of documentation needed will be to cover how to write an __aiter__ method that does the right thing on both 3.5.1 and 3.5.2+ (and also avoids the deprecation warning in the latter case).

    (I've also added Larry to the cc list as release manager)

    @gvanrossum
    Copy link
    Member

    Would it be easier to handle for everyone if this did not vary between
    3.5.0/1 and 3.5.2, and instead was an incompatibility in 3.6? (That would
    still be allowed given 492's provisional status.)

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jun 6, 2016

    Since the old behaviour is only deprecated with Yury's changes, rather than disallowed entirely, I think it makes sense to provide a 3.5.x version that also supports the new behaviour.

    In addition to my docs comment above, the other thing I noticed in reviewing the patch is that the proposed tests don't currently check that we *don't* emit a deprecation warning for the newly permitted forward compatible cases that return an asynchronous iterator (rather than an awaitable) directly from __aiter__.

    There are definitely some tests that exercise that path, so we could rely on the general principle of "the test suite shouldn't emit deprecation warnings", but we could also add some tests that specifically check that no warning is emitted in that case.

    @larryhastings
    Copy link
    Contributor

    As RM my default position is naturally "don't change behavior in point releases". I'm willing to be overruled by the BDFL, less so by anybody else.

    @1st1
    Copy link
    Member Author

    1st1 commented Jun 6, 2016

    While I agree this needs to be fixed, one key piece of documentation needed will be to cover how to write an __aiter__ method that does the right thing on both 3.5.1 and 3.5.2+ (and also avoids the deprecation warning in the latter case).

    Oh, this is tough.

    How about we do this:

    1. In 3.5.2 we start to support new behaviour. We raise PendingDeprecationWarning. We update documentation and PEP-492. The recommended way to write 3.5 code is to keep returning awaitables from __aiter__. We can add a snippet of code for a compatibility decorator to the docs.

    2. In 3.6 we start to raise DeprecationWarning. The recommended way to write code for 3.6 & 3.7 is to return async iterators from __aiter__.

    3. In 3.7 we remove support of the old behaviour.

    It's a rather long & conservative process, but it will be

    @1st1
    Copy link
    Member Author

    1st1 commented Jun 6, 2016

    It's a rather long & conservative process, but it will be easier for people to migrate their code.

    @gvanrossum
    Copy link
    Member

    Unless Nick disagrees (or unless we can't figure out how to implement it) I
    think Yury's proposal makes sense. So if Larry is asking for my fiat, this
    is it.

    @larryhastings
    Copy link
    Contributor

    Okay.

    I'm hoping to not delay 3.5.2 RC1, and the schedule calls for me to tag the release Saturday afternoon. Can you guys get this solid and checked in before then?

    @1st1
    Copy link
    Member Author

    1st1 commented Jun 6, 2016

    Can you guys get this solid and checked in before then?

    Will do my best. I'll update the patch soon with some code comments and docs.

    @1st1
    Copy link
    Member Author

    1st1 commented Jun 7, 2016

    Updated patch (fix_aiter2.patch)

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jun 7, 2016

    Yury's proposal sounds good to me - I'll have time to do a proper review tomorrow (at a quick glance, my one suggestion is to move the if statement outside the example decorator, so the decorator is defined differently based on the Python version, rather than checking the version when called)

    @1st1
    Copy link
    Member Author

    1st1 commented Jun 8, 2016

    Nick,

    Please see the updated patch. Do you think it's ready to be merged? I want buildbots to have some time with it before RC.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jun 9, 2016

    +1 from me - my only comments were on the docs updates and one of the explanatory comments in the code.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 9, 2016

    New changeset 93ad47d63b87 by Yury Selivanov in branch '3.5':
    Issue bpo-27243: Fix __aiter__ protocol
    https://hg.python.org/cpython/rev/93ad47d63b87

    New changeset 9ff95c30a38e by Yury Selivanov in branch 'default':
    Merge 3.5 (issue bpo-27243)
    https://hg.python.org/cpython/rev/9ff95c30a38e

    @1st1
    Copy link
    Member Author

    1st1 commented Jun 9, 2016

    Thanks a lot, Nick! I've merged the patch.

    @1st1 1st1 closed this as completed Jun 9, 2016
    @1st1 1st1 added the type-feature A feature request or enhancement label Jun 9, 2016
    @1st1
    Copy link
    Member Author

    1st1 commented Jun 9, 2016

    I've also updated PEP-492: https://hg.python.org/peps/rev/fef4b9969b9d

    Please feel free to post to this issue if you think that I should have covered it differently or in more detail.

    @vadmium
    Copy link
    Member

    vadmium commented Jun 10, 2016

    Test suite emits a new warning, and fails under python -Werror:

    ======================================================================
    ERROR: test_readline (test.test_asyncio.test_pep492.StreamReaderTests)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/media/disk/home/proj/python/cpython/Lib/test/test_asyncio/test_pep492.py", line 89, in test_readline
        data = self.loop.run_until_complete(reader())
      File "/media/disk/home/proj/python/cpython/Lib/asyncio/base_events.py", line 387, in run_until_complete
        return future.result()
      File "/media/disk/home/proj/python/cpython/Lib/asyncio/futures.py", line 274, in result
        raise self._exception
      File "/media/disk/home/proj/python/cpython/Lib/asyncio/tasks.py", line 239, in _step
        result = coro.send(None)
      File "/media/disk/home/proj/python/cpython/Lib/test/test_asyncio/test_pep492.py", line 85, in reader
        async for line in stream:
    PendingDeprecationWarning: 'StreamReader' implements legacy __aiter__ protocol; __aiter__ should return an asynchronous iterator, not awaitable

    @vadmium vadmium reopened this Jun 10, 2016
    @1st1
    Copy link
    Member Author

    1st1 commented Jun 10, 2016

    This is because sys.version_info is 3.5.1 (not 3.5.2 yet) in the "3.5" branch. Once 3.5.2 RC is tagged the warning will disappear.

    https://github.com/python/cpython/blob/master/Lib/asyncio/streams.py#L693

    @vadmium
    Copy link
    Member

    vadmium commented Jun 11, 2016

    I didn’t realize, sorry for the noise

    @vadmium vadmium closed this as completed Jun 11, 2016
    @1st1
    Copy link
    Member Author

    1st1 commented Jun 11, 2016

    I didn’t realize, sorry for the noise

    Actually thanks for reporting this, Martin. I didn't realize that sys.version_info was 3.5.1 in 3.5 branch.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 8, 2016

    New changeset a0d50aad7b02 by Yury Selivanov in branch '3.6':
    Issue bpo-27243: Change PendingDeprecationWarning -> DeprecationWarning.
    https://hg.python.org/cpython/rev/a0d50aad7b02

    New changeset 9550f0d22d27 by Yury Selivanov in branch 'default':
    Merge 3.6 (issue bpo-27243)
    https://hg.python.org/cpython/rev/9550f0d22d27

    @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
    interpreter-core (Objects, Python, Grammar, and Parser dirs) release-blocker type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants