python / cpython

The Python programming language
https://www.python.org
Other
63.1k stars 30.22k forks source link

typing docs are unclear on when `AsyncIterator` should be used in type hints as opposed to `AsyncGenerator` #112866

Open H0R5E opened 10 months ago

H0R5E commented 10 months ago

Bug report

Bug description:

Hi,

I have an issue with type checking for an aynchronous iterator that I break from for testing purposes. I call the aclose method on the iterator to clean it up after the break. This works fine in practice, but typing complains that the method doesn't exist. See the example below (the comment shows the pylance output).

import asyncio
from typing import AsyncIterator

async def ticker(delay: int, to: int) -> AsyncIterator[int]:
    for i in range(to):
        yield i
        await asyncio.sleep(delay)

async def use_ticker():

    aiter = ticker(1, 5)
    i = 0

    try:
        async for j in aiter:
            if i == 2: break
            print(j)
            i += 1
    finally:
        await aiter.aclose() # Cannot access member "aclose" for type "AsyncIterator[int]"

In collections.abc the aclose method is defined on an AsyncGenerator, but ticker, above, does not fit that model. aclose works on an AsyncIterator so I think it should be defined as a method in collections.abc also.

Thanks for your help,

Mat

CPython versions tested on:

3.9

Operating systems tested on:

Windows

AlexWaygood commented 10 months ago

What makes you think that AsyncIterators all have aclose methods? Here's an example of an async iterator that doesn't have an aclose method (using the asyncio REPL via python -m asyncio):

>>> class Foo:
...     def __init__(self):
...         self.iterable = iter(range(5))
...     def __aiter__(self):
...         return self
...     async def __anext__(self):
...         try:
...             return next(self.iterable)
...         except StopIteration:
...             raise StopAsyncIteration from None
...
>>> f = Foo()
>>> async for x in f:
...     print(x)
...
0
1
2
3
4
>>> f = Foo()
>>> it = aiter(f)
>>> it.aclose
Traceback (most recent call last):
  File "C:\Users\alexw\AppData\Local\Programs\Python\Python312\Lib\concurrent\futures\_base.py", line 449, in result
    return self.__get_result()
           ^^^^^^^^^^^^^^^^^^^
  File "C:\Users\alexw\AppData\Local\Programs\Python\Python312\Lib\concurrent\futures\_base.py", line 401, in __get_result
    raise self._exception
  File "C:\Users\alexw\AppData\Local\Programs\Python\Python312\Lib\asyncio\__main__.py", line 34, in callback
    coro = func()
           ^^^^^^
  File "<console>", line 1, in <module>
AttributeError: 'Foo' object has no attribute 'aclose'

The pylance error you're receiving seems like a true positive to me. If you need to be able to use the aclose method on the object returned from your ticker function, you need to annotate the return type of ticker as AsyncGenerator[int, Any] rather than AsyncIterator[int]. If you do that, pylance will no longer complain at you. Here's a link to the pyright playground (pyright is the type checker that pylance uses under the hood) that demonstrates this: https://pyright-play.net/?code=MQAgDgngTglg5gCwC4C4QGcmwMZILABQhMAtmAPZRIgCG6EAdtjOYQGZTkkhIRgwM4IUhSogAgvSYBxAKYNZUGkkoAaCQwiFtBOo2wgAJrLY8Y2ANaKAFMYA2NCGgFJ1K5wyQBKEAFoAfBJS2HIKSipQANou6uKaALoohCApIGyUwsIMIEqCstYqXkkEqaUgEDCydobCyWUpNADuNDDUekwsAHTodrKyYLZVjl46hO0GxqYAruiyAPpI5lZQ1kV1Keu0rYogALxmljYAjOoArCMlqTB7IAAMm5tYTpul42kZAFZZW0iKxfX1GCma67fYAJjQACMoLIaBYXgCwLBPNYPhcAWVrgBqfZHTZsAQ0Ox2Z6XepNFptbZQTo0bB2cizVYgUAAYRoDAY5Da2GwsnQ6BAJFkJEhOwARHSGbNxe8oDw%2BLIQOLJPoAJK-cKUaKeeLioA

Eclips4 commented 10 months ago

Hello! aclose method defined only on asynchronous generators, not asynchronous iterators. The same applies for common generators and iterators. In fact, your type hint should be AsyncGenerator[...] See docs for typing.AsyncGenerator here: https://docs.python.org/3.12/library/typing.html#typing.AsyncGenerator

H0R5E commented 10 months ago

What makes you think that AsyncIterators all have aclose methods?

Well, you know, I am just following the docs:

If your generator will only yield values, set the SendType to None:

...

Alternatively, annotate your generator as having a return type of either AsyncIterable[YieldType] or AsyncIterator[YieldType]

Thanks for your help, anyway.

AlexWaygood commented 10 months ago

Hmm. Well, perhaps we could improve the docs there! What the docs are trying to say is "if you're not doing anything with the async generator that's specific to it being an async generator, you can just use the simpler type hint". In this case you are doing something that only async generators have, as opposed to other async iterators -- you're using the aclose method.

Eclips4 commented 10 months ago

@H0R5E Would you like to send a PR with a docs update? ;)

H0R5E commented 10 months ago

@Eclips4, my proposal for the docs update would be to delete everything from "Alternatively" onwards. It seems to me that just typing these as AsyncGenerators would avoid confusion, given that the generator methods become available when you use the yield syntax.

If that's OK, would I submit a PR to all the relevant branches for each Python version?

JelleZijlstra commented 10 months ago

I don't think we should delete the sentence, as there are solid reasons to prefer these annotations (you are free to change your implementation to a different kind of iterable later, and you don't have to think about a send type).

A change should be submitted as a PR to the main branch only. After we merge the change, we'll use a bot to backport it to the active bugfix branches (3.11 and 3.12).

H0R5E commented 10 months ago

@JelleZijlstra, when you say "change your implementation to a different kind of iterable later," do you mean not use yield any more? If so, the documentation, as it stands, only gives examples using yield in an async function. Can you refactor it without it becoming a class? When you use yield, if it's going to become a magic AsyncGenerator, why encourage people to use a super type?

@Eclips4, I think the answer is 'no' then, as:

  1. I clearly don't know enough to describe the proper use of this case.
  2. This alias is deprecated anyway, so I'm not sure that it's worth updating
  3. The linked documentation in collections.abc is a one-liner

Thanks again for your help.

Mat

JelleZijlstra commented 10 months ago

If you make a library, you could decide that the API you're exposing is an async iterable. An async iterable could be implemented as an async generator, but if you say in your type annotations that it's definitely an async generator, you guarantee to your users that you'll always keep returning an async generator, which means you can't change later to some other kind of async iterable. As an analogy, you could have an API that currently returns a list, but you might want to annotate it as Iterable so that you're free later to change it to return say a set or a generator.

graingert commented 6 months ago

@JelleZijlstra I don't think this applies to async generators because at runtime it's very important that the generator be aclose()ed deterministically otherwise you get a ResourceWarning when running on trio. See also https://docs.python.org/3/library/contextlib.html#contextlib.aclosing