hynek / prometheus-async

Async Python helpers for the official prometheus-client.
https://prometheus-async.readthedocs.io/
Apache License 2.0
159 stars 15 forks source link

Add type hints #21

Closed Tinche closed 2 years ago

Tinche commented 2 years ago

This is the best we can do, typing-wise, until Mypy has better support for the ParamSpec PEP (as far as I know).

Unfortunately, Mypy will allow the async decorators to be applied to non-async functions. Still, it's a big improvement since the type information of wrapped functions/coroutines won't be lost any more.

I also refactored aio.time(), it was creating a function on every call? Naughty naughty.

hynek commented 2 years ago

Are we relying on implicit Any here?

Tinche commented 2 years ago

Are we relying on implicit Any here?

I don't think so, at least not for the decorator case. Can you clarify?

hynek commented 2 years ago

It's just that it's missing most type hints so I wonder about that.

Tinche commented 2 years ago

True, but:

There's also the issue of prometheus-client being untyped, but that's not our problem (fortunately or unfortunately)

Tinche commented 2 years ago

Looks like latest Mypy can handle ParamSpec, so I've changed the PR to use it. This will give an error if you're annotating an ordinary function, but wraps coroutines properly.

hynek commented 2 years ago

Hmmm I wondered if we can use it, but is there a way to overload such that it works with functions returning awaitable too? I have Futures somewhere in my somewhere. 🤔

Tinche commented 2 years ago

Have you ever known me to shirk from a challenge? ;)

Give me an example and I will do my best to make it type.

Also please approve tests ;)

hynek commented 2 years ago

Something like:

@time
def f() -> async.Future:
    return asyncio.Future()

basically? It makes a bit more sense if its part of something bigger. 🤔

Tinche commented 2 years ago

asyncio.Future is a subtype of Awaitable though, so it should typecheck.

import asyncio

from prometheus_async.aio import time

@time("test")
def f(i: int) -> asyncio.Future:
    return asyncio.Future()

reveal_type(f)
a01.py:11: note: Revealed type is "def (i: builtins.int) -> asyncio.futures.Future*[Any]"

Looking at the test failures, looks like we need typing_extensions on all versions. Is that ok?

hynek commented 2 years ago

Could you add a typing_examples.py that shows all of this off?

typing_extensions is fine.

Tinche commented 2 years ago

@hynek I added a file with examples, let me know if that's what you had in mind.

hynek commented 2 years ago

So I've tried installing your branch into my most precious asyncio app and it seems to be passing – that's good!

Then I tried running mypy against prometheus-async and got a bunch of errors:

mypy src
src/prometheus_async/aio/_decorators.py:29: error: Single overload definition, multiple required
src/prometheus_async/aio/_decorators.py:73: error: Single overload definition, multiple required
src/prometheus_async/aio/_decorators.py:75: error: Incompatible default for argument "exc" (default has type "Type[BaseException]", argument has type "BaseException")
src/prometheus_async/aio/_decorators.py:113: error: Single overload definition, multiple required
src/prometheus_async/aio/web.py:35: error: Incompatible types in assignment (expression has type "None", variable has type Module)
src/prometheus_async/tx/_decorators.py:31: error: Single overload definition, multiple required
src/prometheus_async/tx/_decorators.py:72: error: Single overload definition, multiple required
src/prometheus_async/tx/_decorators.py:111: error: Single overload definition, multiple required
Found 8 errors in 3 files (checked 7 source files)

I'm a bit concerned by the overload error – what gives? Are we doing something wrong here?

Tinche commented 2 years ago

OK, I added multiple overloads everywhere. I guessed for Twisted, could you double-check for me?

hynek commented 2 years ago

I also have code similar to this:

await time(
    SomeHistogram,,
    self._send_cmd(cmd, cmd.names),
)

with

    async def _send_cmd(
        self, cmd: IDomainCommand, domains: list[str]
    ) -> tuple[list[str], IEPPResult]:
        ...

that gives me:

src/domains_svc/proxy_client.py:159: error: No overload variant of "time" matches argument types "Any", "Coroutine[Any, Any, Tuple[List[str], IEPPResult]]"
src/domains_svc/proxy_client.py:159: note: Possible overload variants:
src/domains_svc/proxy_client.py:159: note:     def time(metric: Any) -> Callable[[Callable[P, R]], Callable[P, R]]
src/domains_svc/proxy_client.py:159: note:     def [T] time(metric: Any, future: Future[T]) -> Awaitable[T]

I guess we need to add typing.Coroutine overloads?

Tinche commented 2 years ago

I also have code similar to this:

await time(
    SomeHistogram,,
    self._send_cmd(cmd, cmd.names),
)

with

    async def _send_cmd(
        self, cmd: IDomainCommand, domains: list[str]
    ) -> tuple[list[str], IEPPResult]:
        ...

that gives me:

src/domains_svc/proxy_client.py:159: error: No overload variant of "time" matches argument types "Any", "Coroutine[Any, Any, Tuple[List[str], IEPPResult]]"
src/domains_svc/proxy_client.py:159: note: Possible overload variants:
src/domains_svc/proxy_client.py:159: note:     def time(metric: Any) -> Callable[[Callable[P, R]], Callable[P, R]]
src/domains_svc/proxy_client.py:159: note:     def [T] time(metric: Any, future: Future[T]) -> Awaitable[T]

I guess we need to add typing.Coroutine overloads?

Oh interesting. We can relax the type constraint to be Awaitable. Doing it

hynek commented 2 years ago

Hopefully last snag: our types don't allow for decorator-style usage for Twisted (see CI failure).