Skip to content

Change create_task type #6779

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

Merged
merged 3 commits into from
Feb 2, 2022
Merged

Change create_task type #6779

merged 3 commits into from
Feb 2, 2022

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Jan 2, 2022

I've manually tested that Awaitables are not supported:

import asyncio

class My:
    def __await__(self):
        async def inner():
            print('here')
        return inner

async def main():
  await asyncio.create_task(My())

asyncio.run(main())

Output:

Traceback (most recent call last):
  File "/Users/sobolev/Desktop/cpython/build/ex.py", line 15, in <module>
    asyncio.run(main())
  File "/Users/sobolev/.pyenv/versions/3.9.9/lib/python3.9/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "/Users/sobolev/.pyenv/versions/3.9.9/lib/python3.9/asyncio/base_events.py", line 642, in run_until_complete
    return future.result()
  File "/Users/sobolev/Desktop/cpython/build/ex.py", line 13, in main
    await asyncio.create_task(My())
  File "/Users/sobolev/.pyenv/versions/3.9.9/lib/python3.9/asyncio/tasks.py", line 361, in create_task
    task = loop.create_task(coro)
  File "/Users/sobolev/.pyenv/versions/3.9.9/lib/python3.9/asyncio/base_events.py", line 433, in create_task
    task = tasks.Task(coro, loop=self, name=name)
TypeError: a coroutine was expected, got <__main__.My object at 0x10c38fe20>

But, Generator objects are also supported:

import asyncio
def gen():
   i = yield
   print(1, i)  # prints: `1 None`

async def main():
  await asyncio.create_task(gen())

asyncio.run(main())

Closes #6770

else:
def create_task(coro: Generator[_TaskYieldType, None, _T] | Awaitable[_T]) -> Task[_T]: ...
Copy link
Member Author

Choose a reason for hiding this comment

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

Notice that _TaskYieldType was not used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem right. Event loops typically use yields to communicate between tasks and the loop, so the generator must yield objects of a specific type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Some experiments:

import asyncio
def gen():
   i = yield
   print(1, i)  # prints: `1, None`
   return 'a'

async def main():
  print('res', await asyncio.create_task(gen()))  # prints: `res, a`

asyncio.run(main())

And:

import asyncio
def gen():
   i = yield
   print(1, i)  # prints: `1, None`
   yield 'a'

async def main():
  print('res', await asyncio.create_task(gen()))

asyncio.run(main())
1 None
Traceback (most recent call last):
  File "/Users/sobolev/Desktop/cpython/build/ex.py", line 10, in <module>
    asyncio.run(main())
  File "/Users/sobolev/.pyenv/versions/3.9.9/lib/python3.9/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "/Users/sobolev/.pyenv/versions/3.9.9/lib/python3.9/asyncio/base_events.py", line 642, in run_until_complete
    return future.result()
  File "/Users/sobolev/Desktop/cpython/build/ex.py", line 8, in main
    print('res', await asyncio.create_task(gen()))  # prints: `res, a`
  File "/Users/sobolev/Desktop/cpython/build/ex.py", line 5, in gen
    yield 'a'
RuntimeError: Task got bad yield: 'a'

@github-actions

This comment has been minimized.

@sobolevn
Copy link
Member Author

sobolevn commented Jan 2, 2022

Ok, we have some problems:

self._navigationPromise = self._loop.create_task(asyncio.wait([
    self._lifecycleCompletePromise,
    self._createTimeoutPromise(),
], return_when=concurrent.futures.FIRST_COMPLETED))

Here asyncio.wait returns Future, but now self._loop.create_task() wants Coroutine. Future is not compatible with Coroutine. 🤔

@Akuli
Copy link
Collaborator

Akuli commented Jan 2, 2022

Doesn't look like a Future to me:

Python 3.9.2 (default, Feb 28 2021, 17:03:44) 
[GCC 10.2.1 20210110] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import asyncio
>>> asyncio.wait([])
<coroutine object wait at 0x7f90e8cef140>

@sobolevn
Copy link
Member Author

sobolevn commented Jan 2, 2022

@Akuli sorry, I had to clarify that I'm talking about typeshed's definition:

def wait(fs: Iterable[_FT], *, timeout: float | None = ..., return_when: str = ...) -> Future[tuple[set[_FT], set[_FT]]]: ... # type: ignore[misc]
@overload
def wait(
fs: Iterable[Awaitable[_T]], *, timeout: float | None = ..., return_when: str = ...
) -> Future[tuple[set[Task[_T]], set[Task[_T]]]]: ...

@Akuli
Copy link
Collaborator

Akuli commented Jan 2, 2022

A few observations:

  • The coro argument gets passed to Task.__init__, so that should be changed too.
  • Task.__init__ uses asyncio.coroutines.iscoroutine, which is an optimized isinstance(obj, asyncio.coroutines._COROUTINE_TYPES), not to be confused with inspect.iscoroutine which is isinstance(obj, types.CoroutineType).
  • asyncio is a mess :(

@sobolevn
Copy link
Member Author

sobolevn commented Jan 2, 2022

Ok, I have no idea what is going on:

  • Why some definitions are async def, while others are sync def that return Future?
  • Why do they return Future, why not Coroutine?
  • Why stubtest allows this?

@sobolevn
Copy link
Member Author

sobolevn commented Jan 2, 2022

Ok, my plan is to sync all async def in source code and async def in typeshed (unless there are some problems that I don't know about).

@hauntsaninja
Copy link
Collaborator

Probably shouldn't merge this as is, given the false positives. The one in my code could be potentially worked around by changing AsyncIterator.__anext__ to return a Coroutine.

Re stubtest:
It's one of many tricks stubtest doesn't know about / mypy itself largely treats def f() -> Coroutine[Any, Any, int] the same as async def f() -> int so it's not something you're forced to handle. Can't remember if I tried adding support for this to stubtest; there were several things I chose not to add based on typeshed error counts. Might have been affected by older Pythons, e.g. on Python 3.5 several functions are @coroutine not async def.

@sobolevn
Copy link
Member Author

sobolevn commented Jan 2, 2022

Totally do not merge this as-is, I will spend several days fixing all the async def / def stuff. And hopefully after that we can return to this issue.

@srittau srittau marked this pull request as draft January 2, 2022 20:50
@srittau
Copy link
Collaborator

srittau commented Jan 2, 2022

I've converted this to a draft for now. Please mark as "ready for review" when it is. :)

@sobolevn sobolevn closed this Jan 25, 2022
@sobolevn sobolevn reopened this Jan 25, 2022
@sobolevn sobolevn marked this pull request as ready for review January 25, 2022 08:35
@github-actions

This comment has been minimized.

@sobolevn
Copy link
Member Author

sobolevn commented Jan 25, 2022

I've changed all things I could find to be async def:

async def wait(fs: Iterable[_FT], *, timeout: float | None = ..., return_when: str = ...) -> tuple[set[_FT], set[_FT]]: ... # type: ignore[misc]
@overload
async def wait(
fs: Iterable[Awaitable[_T]], *, timeout: float | None = ..., return_when: str = ...
) -> tuple[set[Task[_T]], set[Task[_T]]]: ...

But, mypy-primer found some new ones 🤔

@sobolevn
Copy link
Member Author

sobolevn commented Jan 25, 2022

Analysis:

This error does not look good to me: https://github.com/Yelp/paasta/blob/5e504597ea5a2af952a12785808e523b53b7c01d/paasta_tools/instance/kubernetes.py#L545 🤔

Aha! Found the reason: the decorator annotation is not quite correct: https://github.com/Yelp/paasta/blob/5e504597ea5a2af952a12785808e523b53b7c01d/paasta_tools/async_utils.py#L93-L97

@srittau
Copy link
Collaborator

srittau commented Feb 2, 2022

@sobolevn If I understand correctly, in your opinion the primer diffs all show genuine problems?

@sobolevn
Copy link
Member Author

sobolevn commented Feb 2, 2022

@srittau yes, I think so! I've opened #7105 to fix 2.

@srittau
Copy link
Collaborator

srittau commented Feb 2, 2022

Ok, I'll have a look once the merge conflicts are resolved.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2022

Diff from mypy_primer, showing the effect of this PR on open source code:

core (https://github.com/home-assistant/core)
+ homeassistant/core.py:439: error: Argument 1 to "create_task" of "AbstractEventLoop" has incompatible type "Awaitable[_R]"; expected "Union[Coroutine[Any, Any, _R], Generator[Any, None, _R]]"  [arg-type]
+ homeassistant/core.py:470: error: Need type annotation for "task"  [var-annotated]
+ homeassistant/core.py:470: error: Argument 1 to "create_task" of "AbstractEventLoop" has incompatible type "Awaitable[_R]"; expected "Union[Coroutine[Any, Any, <nothing>], Generator[Any, None, <nothing>]]"  [arg-type]

paasta (https://github.com/yelp/paasta)
- paasta_tools/instance/kubernetes.py:553: error: Argument 1 to "append" of "list" has incompatible type "Task[Sequence[Any]]"; expected "Future[Dict[str, Any]]"
+ paasta_tools/instance/kubernetes.py:545: error: Need type annotation for "pods_task"
+ paasta_tools/instance/kubernetes.py:546: error: Argument 1 to "create_task" has incompatible type "Awaitable[Sequence[Any]]"; expected "Union[Generator[Any, None, <nothing>], Coroutine[Any, Any, <nothing>]]"
+ paasta_tools/instance/kubernetes.py:1017: error: Need type annotation for "pods_task"
+ paasta_tools/instance/kubernetes.py:1018: error: Argument 1 to "create_task" has incompatible type "Awaitable[Sequence[Any]]"; expected "Union[Generator[Any, None, <nothing>], Coroutine[Any, Any, <nothing>]]"
+ paasta_tools/instance/kubernetes.py:1202: error: Need type annotation for "pods_task"
+ paasta_tools/instance/kubernetes.py:1203: error: Argument 1 to "create_task" has incompatible type "Awaitable[Sequence[Any]]"; expected "Union[Generator[Any, None, <nothing>], Coroutine[Any, Any, <nothing>]]"

@@ -9,7 +9,7 @@ from asyncio.tasks import Task
from asyncio.transports import BaseTransport, ReadTransport, SubprocessTransport, WriteTransport
from collections.abc import Iterable
from socket import AddressFamily, SocketKind, _Address, _RetAddress, socket
from typing import IO, Any, Awaitable, Callable, Generator, Sequence, TypeVar, Union, overload
from typing import IO, Any, Awaitable, Callable, Coroutine, Generator, Sequence, TypeVar, Union, overload
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit for future reference: Coroutine should now be imported from collections.abc.

@srittau srittau merged commit 1b99812 into python:master Feb 2, 2022
@sobolevn
Copy link
Member Author

sobolevn commented Feb 2, 2022

Thanks everyone 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

asyncio.create_task hints say you can pass any Awaitable, but you cannot pass asyncio.Tasks
4 participants