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

Variadic Generics in asyncio.gather() #8085

Closed
wants to merge 4 commits into from

Conversation

Harry-Lees
Copy link
Contributor

asyncio.gather() supports an arbitrary number of coroutines as arguments. In versions below 3.11, it wasn't possible to fully type this.

It should now be possible to represent this annotation using Variadic Generics (PEP 646).

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

AlexWaygood commented Jun 15, 2022

I'm afraid we're a long way off from being able to use PEP 646 in typeshed 😞

Pyright supports PEP 646. I think pyre might also, but I'm not sure. Neither mypy nor pytype yet support PEP 646, however, and we would need both to support PEP 646 before we could use variadic generics in typeshed.

@@ -12,6 +12,7 @@ if sys.version_info >= (3, 9):
from types import GenericAlias
if sys.version_info >= (3, 11):
from contextvars import Context
from typing_extensions import TypeVarTuple, Unpack
Copy link
Member

@AlexWaygood AlexWaygood Jun 15, 2022

Choose a reason for hiding this comment

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

While TypeVarTuple and Unpack are only available from typing on 3.11+, they are available from typing_extensions on all versions of Python. That means that (if we could merge this, which we unfortunately can't), you would want to import them from typing_extensions unconditionally, and change the signature of asyncio.gather() in all Python versions. (Although the stdlib only supports variadic generics on 3.11+, type checkers that support PEP 646 understand how it works on earlier versions of Python as well.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm afraid we're a long way off from being able to use PEP 646 in typeshed 😞

Pyright supports PEP 646. I think pyre might also, but I'm not sure. Neither mypy nor pytype yet support PEP 646, however, and we would need both to support PEP 646 before we could use variadic generics in typeshed.

Yeah, I was actually using Pyright when making this, I noticed on the pipeline that MyPy doesn't support it :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TypeVarTuple and Unpack are available from typing_extensions on all versions of Python, so (if we could merge this, which we unfortunately can't), you would want to import them from typing_extensions unconditionally, and change the signature of asyncio.gather() in all Python versions.

Interesting, I didn't actually see that when I was reading the PEP, I actually think that TypeVarTuple and Unpack should be available in typing from 3.11 onwards? But even Pyright didn't recognise the imports when I tested on 3.11b0

Copy link
Member

@AlexWaygood AlexWaygood Jun 15, 2022

Choose a reason for hiding this comment

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

I actually think that TypeVarTuple and Unpack should be available in typing from 3.11 onwards

Yes, they're available from typing in the standard library on 3.11+, and they're available from the third-party backports library typing_extensions (which at typeshed, we pretend is part of the stdlib) on 3.6+

@Harry-Lees
Copy link
Contributor Author

Seeing as this won't be merged, would you like me to close it here, or I can fix the issues you mentioned and just leave it until it's possible to merge?

@srittau srittau added the status: deferred Issue or PR deferred until some precondition is fixed label Jun 15, 2022
@srittau
Copy link
Collaborator

srittau commented Jun 15, 2022

I think it's fine to prepare this PR. I marked it as "deferred".

@AlexWaygood
Copy link
Member

I think it's fine to prepare this PR. I marked it as "deferred".

I'm also fine with this. Be aware, though, that it might be a long time before it gets merged :)

Mypy has python/mypy#12280 to track adding support, but pytype doesn't yet have a tracking issue.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

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

anyio (https://github.com/agronholm/anyio)
+ src/anyio/_backends/_asyncio.py:2096: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]

mypy_primer (https://github.com/hauntsaninja/mypy_primer)
+ mypy_primer.py:266: error: Need more than 1 value to unpack (2 expected)
+ mypy_primer.py:272: error: Need more than 1 value to unpack (2 expected)
+ mypy_primer.py:273: error: Cannot determine type of "new_mypy"
+ mypy_primer.py:274: error: Cannot determine type of "old_mypy"
+ mypy_primer.py:276: error: Cannot determine type of "new_version"
+ mypy_primer.py:277: error: Cannot determine type of "old_version"
+ mypy_primer.py:279: error: Cannot determine type of "new_mypy"
+ mypy_primer.py:279: error: Cannot determine type of "old_mypy"
+ mypy_primer.py:414: error: Need more than 1 value to unpack (2 expected)
+ mypy_primer.py:418: error: Cannot determine type of "new_result"
+ mypy_primer.py:418: error: Cannot determine type of "old_result"

aiohttp (https://github.com/aio-libs/aiohttp)
+ aiohttp/connector.py:377: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ aiohttp/web.py:446: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]

freqtrade (https://github.com/freqtrade/freqtrade)
+ freqtrade/exchange/exchange.py:1744: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"

core (https://github.com/home-assistant/core)
+ homeassistant/util/async_.py:200: error: Argument "return_exceptions" to "gather" has incompatible type "bool"; expected "Literal[False]"  [arg-type]
+ homeassistant/requirements.py:209: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/helpers/trigger.py:125: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/auth/__init__.py:58: error: Incompatible types in assignment (expression has type "List[<nothing>]", variable has type "Tuple[Any]")  [assignment]
+ homeassistant/auth/__init__.py:70: error: Incompatible types in assignment (expression has type "List[<nothing>]", variable has type "Tuple[Any]")  [assignment]
+ homeassistant/helpers/restore_state.py:288: error: Need more than 1 value to unpack (2 expected)  [misc]
+ homeassistant/helpers/restore_state.py:292: error: Cannot determine type of "data"  [has-type]
+ homeassistant/helpers/restore_state.py:296: error: Need more than 1 value to unpack (2 expected)  [misc]
+ homeassistant/helpers/restore_state.py:300: error: Cannot determine type of "data"  [has-type]
+ homeassistant/components/device_automation/__init__.py:209: error: Unused "type: ignore" comment
+ homeassistant/components/device_automation/__init__.py:209: error: Incompatible return value type (got "Tuple[Any]", expected "List[Union[List[Dict[str, Any]], Exception]]")  [return-value]
+ homeassistant/components/device_automation/__init__.py:209: note: Error code "return-value" not covered by "type: ignore" comment
+ homeassistant/components/device_automation/__init__.py:214: error: Argument "return_exceptions" to "gather" has incompatible type "bool"; expected "Literal[False]"  [arg-type]
+ homeassistant/helpers/condition.py:973: error: Incompatible return value type (got "Tuple[Any]", expected "List[Union[Dict[str, Any], Template]]")  [return-value]
+ homeassistant/helpers/script.py:255: error: Incompatible return value type (got "Tuple[Any]", expected "List[Dict[str, Any]]")  [return-value]
+ homeassistant/helpers/script.py:1017: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/components/zeroconf/__init__.py:190: error: Need more than 1 value to unpack (2 expected)  [misc]
+ homeassistant/components/zeroconf/__init__.py:193: error: Cannot determine type of "zeroconf_types"  [has-type]
+ homeassistant/components/zeroconf/__init__.py:193: error: Cannot determine type of "homekit_models"  [has-type]
+ homeassistant/components/ssdp/__init__.py:322: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/components/ssdp/__init__.py:394: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/components/yeelight/scanner.py:86: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/components/wiz/discovery.py:29: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/components/upnp/device.py:155: error: Tuple index out of range  [misc]
+ homeassistant/components/upnp/device.py:156: error: Tuple index out of range  [misc]
+ homeassistant/components/upnp/device.py:157: error: Tuple index out of range  [misc]
+ homeassistant/components/upnp/device.py:167: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/components/tractive/__init__.py:100: error: Incompatible types in assignment (expression has type "List[Any]", variable has type "Tuple[Any]")  [assignment]
+ homeassistant/components/tractive/__init__.py:130: error: Need more than 1 value to unpack (3 expected)  [misc]
+ homeassistant/components/tractive/__init__.py:134: error: Cannot determine type of "tracker_details"  [has-type]
+ homeassistant/components/tractive/__init__.py:134: error: Cannot determine type of "hw_info"  [has-type]
+ homeassistant/components/tractive/__init__.py:134: error: Cannot determine type of "pos_report"  [has-type]
+ homeassistant/components/tautulli/coordinator.py:54: error: Need more than 1 value to unpack (3 expected)  [misc]
+ homeassistant/components/steamist/discovery.py:75: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/components/simplisafe/__init__.py:644: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/components/ridwell/__init__.py:61: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/components/notion/__init__.py:67: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/components/iqvia/__init__.py:91: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/components/firmata/__init__.py:218: error: Need type annotation for "results" (hint: "results: List[<type>] = ...")  [var-annotated]
+ homeassistant/components/firmata/__init__.py:220: error: Incompatible types in assignment (expression has type "Tuple[Any]", variable has type "List[Any]")  [assignment]
+ homeassistant/components/elkm1/discovery.py:60: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/components/dnsip/config_flow.py:67: error: Tuple index out of range  [misc]
+ homeassistant/components/backup/manager.py:165: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/components/backup/manager.py:213: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/components/aws/__init__.py:135: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/components/upnp/__init__.py:243: error: Tuple index out of range  [misc]
+ homeassistant/components/overkiz/__init__.py:61: error: Need more than 1 value to unpack (2 expected)  [misc]
+ homeassistant/components/overkiz/__init__.py:81: error: Cannot determine type of "setup"  [has-type]
+ homeassistant/components/overkiz/__init__.py:82: error: Cannot determine type of "setup"  [has-type]
+ homeassistant/components/overkiz/__init__.py:99: error: Cannot determine type of "scenarios"  [has-type]
+ homeassistant/components/overkiz/__init__.py:118: error: Cannot determine type of "setup"  [has-type]
+ homeassistant/components/zha/__init__.py:163: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/components/flux_led/discovery.py:190: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/components/analytics/analytics.py:190: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/components/amcrest/camera.py:415: error: Need more than 1 value to unpack (6 expected)  [misc]
+ homeassistant/components/slack/notify.py:263: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/components/homekit/config_flow.py:168: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/components/zwave_js/services.py:426: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/components/zwave_js/services.py:459: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/components/zwave_js/services.py:527: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ homeassistant/components/zwave_js/services.py:639: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]

steam.py (https://github.com/Gobot1234/steam.py)
+ steam/abc.py:634: error: Missing positional arguments "info", "customisation_info" in call to "Profile"  [call-arg]
+ steam/user.py:333: error: Missing positional arguments "info", "customisation_info", "items" in call to "ClientUserProfile"  [call-arg]

boostedblob (https://github.com/hauntsaninja/boostedblob)
+ boostedblob/syncing.py:63: error: Need more than 1 value to unpack (2 expected)
+ boostedblob/syncing.py:64: error: Cannot determine type of "src_files"
+ boostedblob/syncing.py:64: error: Cannot determine type of "dst_files"

aioredis (https://github.com/aio-libs/aioredis)
+ aioredis/connection.py:1495: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]
+ aioredis/connection.py:1664: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"  [arg-type]

paasta (https://github.com/yelp/paasta)
+ paasta_tools/instance/kubernetes.py:647: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"
+ paasta_tools/instance/kubernetes.py:736: error: Incompatible return value type (got "Tuple[Any]", expected "List[KubernetesVersionDict]")
+ paasta_tools/instance/kubernetes.py:772: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"
+ paasta_tools/instance/kubernetes.py:921: error: Need more than 1 value to unpack (2 expected)
+ paasta_tools/instance/kubernetes.py:938: error: Cannot determine type of "previous_tail_lines"
+ paasta_tools/instance/kubernetes.py:942: error: Cannot determine type of "tail_lines"
+ paasta_tools/instance/kubernetes.py:1008: error: Incompatible return value type (got "Tuple[Any]", expected "List[KubernetesVersionDict]")

dragonchain (https://github.com/dragonchain/dragonchain)
+ dragonchain/broadcast_processor/broadcast_processor.py:196:62: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"
+ dragonchain/broadcast_processor/broadcast_processor.py:336:62: error: Argument "return_exceptions" to "gather" has incompatible type "Literal[True]"; expected "Literal[False]"

@AlexWaygood
Copy link
Member

As you can see by the pyright failures, this unfortunately isn't a valid use of PEP 646 as it currently exists. TypeVarTuples, just like normal TypeVars, aren't subscriptable. I think this means that we won't actually be able to use TypeVarTuples for asyncio.gather() at all. The type-dependency here doesn't seem to me to be expressable with variadic generics as they currently exist in the type system, due to the fact that we have tuple[_FutureT[_T1], _FutureT[_T2]] going in, and Future[tuple[_FutureT[_T1], _FutureT[_T2]]] coming out. We have syntax to express a tuple consisting of a variadic number of TypeVars (Unpack[Ts]), but we don't (AFAIK) have syntax to express a tuple consisting of a variadic number of objects parameterised by a variadic number of TypeVars... which I think is what we need here.

(I'm really not an expert on PEP 646 though -- @JelleZijlstra, could you confirm?)

We can, I think (once it's supported by mypy/pyright), use PEP 646 to make the signature a little more readable, however. I had a play around, and did a proof-of-concept on my fork over here: AlexWaygood@6efe7ec

@AlexWaygood
Copy link
Member

AlexWaygood commented Jun 15, 2022

There's also two other subtleties that are captured by the current signature, but aren't captured by this PR currently:

  1. Prior to 3.10, asyncio.gather() had an optional loop parameter.
  2. If return_exceptions is set to True, the return type is different.

@Harry-Lees
Copy link
Contributor Author

There's also two other subtleties that are captured by the current signature, but aren't captured by this PR currently:

  1. Prior to 3.10, asyncio.gather() had an optional loop parameter.
  2. If return_exceptions is set to True, the return type is different.

I actually caught these when I removed some of the other @overload variants, I didn't immediately fix them because I noticed some other issues, I've just been tinkering in a test file to try and better understand what's going on :)

@Harry-Lees
Copy link
Contributor Author

Harry-Lees commented Jun 15, 2022

We have syntax to express a tuple consisting of a variadic number of TypeVars (Unpack[Ts]), but we don't (AFAIK) have syntax to express a tuple consisting of a variadic number of objects parameterised by a variadic number of TypeVars... which I think is what we need here.

Sorry, I don't quite follow this. Forgive me for explaining back to you, I'm just trying to think it through out loud :P.

You're saying that because we're using the same generic return type _T for all the functions in the sequence, we can't represent that each function has a unique return type. For example the following annotation

def example(*args: *_Ts[_T]) -> tuple[*_Ts[_T]]:

would be correct for this:

example(1, 2, 3)

where the return type would be tuple[int, int, int]

but wouldn't be correct for this

example("1", 2, 3, 4, 5)

because in the second example the arguments aren't homogeneous?

Okay, I think I've figured it out now :D

@Harry-Lees
Copy link
Contributor Author

due to the fact that we have tuple[_FutureT[_T1], _FutureT[_T2]] going in, and Future[tuple[_FutureT[_T1], _FutureT[_T2]]]

For gather() I think we have tuple[_FutureT[_T1], FutureT[_T2]] going in and then Future[tuple[_T1, _T2]] coming out because gather() will just return a list of the results of whatever was passed in.

Also, while we're here

Python 3.10.1 (main, Feb  3 2022, 11:22:46) [Clang 12.0.5 (clang-1205.0.22.11)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import asyncio
>>> async def main():
...     print(await asyncio.gather(asyncio.sleep(1)))
... 
>>> asyncio.run(main())
[None]

I think asyncio.gather() returns a list not a tuple, is there a reason that it's annotated as such or should I open a new issue?

@AlexWaygood
Copy link
Member

I think asyncio.gather() returns a list not a tuple, is there a reason that it's annotated as such or should I open a new issue?

Yes, this is a deliberate decision due to the fact that the precise type of the parameters is more important here than the precise type of the sequence. See the discussion in #1550.

@AlexWaygood
Copy link
Member

For gather() I think we have tuple[_FutureT[_T1], FutureT[_T2]] going in and then Future[tuple[_T1, _T2]] coming out because gather() will just return a list of the results of whatever was passed in.

Yes, but unfortunately I don't think this is fully expressible in the type system even with PEP 646 ://

@hmc-cs-mdrissi
Copy link
Contributor

hmc-cs-mdrissi commented Jun 16, 2022

This needs the missing Map operator. So you could one day do Map[F, Ts] where F is a generic type. There’s a pep draft for type level Map that would handle this but that draft I think is currently on hold and probably wouldn’t mean too much when mypy/pytype first needs to get normal 646.

In a couple years hopefully when we have type level Map then this signature becomes possible. Some builtins like zip and map would also benefit if we had it.

@AlexWaygood
Copy link
Member

I'd be happy to leave this open if it was just a case of "let's wait for PEP 646 support". But given that it seems like it actually depends on a feature that hasn't even been fully specified yet, let alone had a PEP approved by the Steering Council and been implemented at runtime... I think I'm going to close this for now. But thank you for the PR -- you're absolutely right that the stub is far from perfect at the moment 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: deferred Issue or PR deferred until some precondition is fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants