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

new type hints i wrote seem to be wrong #162

Closed
amirreza8002 opened this issue Dec 19, 2024 · 22 comments · Fixed by #163
Closed

new type hints i wrote seem to be wrong #162

amirreza8002 opened this issue Dec 19, 2024 · 22 comments · Fixed by #163

Comments

@amirreza8002
Copy link
Contributor

hi
so in the past few commits i have deleted Awaitable from the type hints
the argumnet was that async methods don't return awaitable
which is right

but valkey-py methods aren't async, they are sync methods that could return an awaitable

async def f()  -> int:
    return 2
    
def a() -> Awaitable:
    return f

so having Awaitable in the return types, while annoying, are correct
this problem was pointed out by fix error in python's discord

there is also the suggestion to have different methods for sync and async, or go fully sync
but that's not for me to decide

the question i have is, how would go about reverting the changes?

@mkmkme
Copy link
Collaborator

mkmkme commented Dec 19, 2024

Hey,

Could you point to the place where we could return an awaitable? IIRC all the methods you changed the type hints didn't in fact return a functor, but instead they return an actual value. But I might be wrong.

@amirreza8002
Copy link
Contributor Author

amirreza8002 commented Dec 19, 2024

hi
all of the changes i made in regards to Awaitable seem to be wrong

let's take set() for example:
there is only one set method, both for sync, and async clients

the difference is in the client, as you know, methods in valkey-py all call execute_command
which is either sync or async

if execute_command is sync, set() and the like will return a value, if it is async, they will return an awaitable

which is not very type checker and IDE friendly, but it is true

@mkmkme
Copy link
Collaborator

mkmkme commented Dec 19, 2024

As I mentioned here, async functions don't return Awaitable[str] instead of str, they return str.

In the example you provided in this issue, the function returns Awaitable because it returns a functor.

That's the reason I'm asking you to provide a concrete example.


Correct me if I'm wrong, but the difference between sync and async clients in valkey-py is such that you're writing

r.set("foo", "bar")

in sync and

await r.set("foo", "bar")

in async. In that case, your type hints are correct.
We would have to use Awaitable as the return value if async client worked like this

ret = r.set("foo", "bar")
await ret()

@amirreza8002
Copy link
Contributor Author

amirreza8002 commented Dec 19, 2024

python -m asyncio

async def f():
    return 1


a = f()
print(a)
# <coroutine object f at ...>
await a
# 1

ultimately this is what's happening

valkey-py doesn't have async methods as public API, it has sync methods returning a coroutine object that you can await

@mkmkme
Copy link
Collaborator

mkmkme commented Dec 19, 2024

Yeah I understand that async function returns a coroutine. However, I'd like to reiterate that PEP 484 explicitly covers this. Have a look at this link. I'll quote the relevant part below:


Coroutines introduced in PEP 492 are annotated with the same syntax as ordinary functions. However, the return type annotation corresponds to the type of await expression, not to the coroutine type:

async def spam(ignored: int) -> str:
    return 'spam'

async def foo() -> None:
    bar = await spam(42)  # type: str

@graingert
Copy link

No this is not how PEP 484 or PEP 492 works for subclass overriding of synchronous methods.

for async def type hints the return type is automatically upgraded to Coroutine[Any, Any, T], this does not mean an asynchronous method is allowed to override a synchronous one

Use typing.reveal_type(foo) to see

@mkmkme
Copy link
Collaborator

mkmkme commented Dec 19, 2024

@graingert are you saying that our functions should have a type hint of Union[str, Coroutine[Any, Any, str]] then? Or am I getting it wrong?

@mkmkme
Copy link
Collaborator

mkmkme commented Dec 19, 2024

@graingert

Use typing.reveal_type(foo) to see

I'm not sure what you're trying to say here:

In [5]: r = valkey.Valkey(host="localhost", port=6379)

In [6]: typing.reveal_type(r.hstrlen)
Runtime type is 'method'
Out[6]: <bound method HashCommands.hstrlen of <valkey.client.Valkey(<valkey.connection.ConnectionPool(<valkey.connection.Connection(host=localhost,port=6379,db=0)>)>)>>

In [7]: typing.reveal_type(r.set)
Runtime type is 'method'
Out[7]: <bound method BasicKeyCommands.set of <valkey.client.Valkey(<valkey.connection.ConnectionPool(<valkey.connection.Connection(host=localhost,port=6379,db=0)>)>)>>

@graingert
Copy link

@graingert are you saying that our functions should have a type hint of Union[str, Coroutine[Any, Any, str]] then? Or am I getting it wrong?

I'm saying you should have two totally separate classes one with async methods and one with sync methods, and you shouldn't subclass the class with sync methods in your async class

@mkmkme
Copy link
Collaborator

mkmkme commented Dec 19, 2024

Could you point me to some PEP or anything that would state that?

@graingert
Copy link

@graingert

Use typing.reveal_type(foo) to see

I'm not sure what you're trying to say here:

In [5]: r = valkey.Valkey(host="localhost", port=6379)

In [6]: typing.reveal_type(r.hstrlen)
Runtime type is 'method'
Out[6]: <bound method HashCommands.hstrlen of <valkey.client.Valkey(<valkey.connection.ConnectionPool(<valkey.connection.Connection(host=localhost,port=6379,db=0)>)>)>>

In [7]: typing.reveal_type(r.set)
Runtime type is 'method'
Out[7]: <bound method BasicKeyCommands.set of <valkey.client.Valkey(<valkey.connection.ConnectionPool(<valkey.connection.Connection(host=localhost,port=6379,db=0)>)>)>>

I meant for you to use reveal_type with a type checker

@graingert
Copy link

Could you point me to some PEP or anything that would state that?

https://mypy.readthedocs.io/en/latest/more_types.html#typing-async-await

The result of calling an async def function without awaiting will automatically be inferred to be a value of type Coroutine[Any, Any, T], which is a subtype of Awaitable[T]

@mkmkme
Copy link
Collaborator

mkmkme commented Dec 19, 2024

The result of calling an async def function without awaiting will automatically be inferred to be a value of type Coroutine[Any, Any, T], which is a subtype of Awaitable[T]

This is the main point here. Our async client methods are meant to be await-ed, as any other async method.

And this doesn't show me why these statements are true:

this does not mean an asynchronous method is allowed to override a synchronous one

you should have two totally separate classes one with async methods and one with sync methods, and you shouldn't subclass the class with sync methods in your async class

If you could provide any resource that would state that I would really appreciate that. Brief googling didn't confirm this to me.

@mkmkme
Copy link
Collaborator

mkmkme commented Dec 19, 2024

And if you could show me what issue you're actually trying to solve (error or a type checker warning) I would also appreciate that

@graingert
Copy link

@mkmkme
Copy link
Collaborator

mkmkme commented Dec 19, 2024

Okay, so you're referring to the mypy error. I see. Thanks, I'll try to address it.

@graingert
Copy link

@mkmkme
Copy link
Collaborator

mkmkme commented Dec 19, 2024

Here's another example:
https://mypy-play.net/?mypy=latest&python=3.12&gist=0e7063f1f9d9ee4e8ac6fe67f6a5d40d

Yeah this one is understandable. However, I'd argue that in practice this will be never hit in valkey-py, since in order to get the "child" class you'll have to type

from valkey.async.client import Valkey

instead of

from valkey.client import Valkey

which is a strong hint everything going on there will be async.

But I'll see what can be done with the typing. Thanks!

@graingert
Copy link

graingert commented Dec 19, 2024

This can be hit without getting the imports wrong because type checkers will assume the lowest common superclass in a bunch of cases, which will be your superclass with sync methods

The Async client and the Sync client need to not share a common superclass (except object of course)

@amirreza8002
Copy link
Contributor Author

hi @graingert

so you're saying

class A:
    def a(): 
        ...


class B(A):
    async def a():
        ...

is wrong?
cause i do have a lot of those :)

but in this case execute_command is not subclassed, sync and async clients don't inherite from each other, they do share the commands tho

and for writing async clients from scratch, wouldn't that make the source code double as big?

@graingert
Copy link

Yep it's wrong, and fails when you type check it

@mkmkme
Copy link
Collaborator

mkmkme commented Dec 19, 2024

Thanks everyone for your input!

We discussed this internally and decided to do the following:

  1. @amirreza8002 I will revert your typing PRs now and close this ticket
  2. I will publish a new beta since it contains the fix for Python 3.13 and we need that
  3. I will create a new ticket to revisit the typing system within the repository. There's a chance it will take quite a lot of time and effort so I can't promise it to have very high priority

This was referenced Dec 19, 2024
mkmkme added a commit that referenced this issue Dec 19, 2024
Revert some typing commits

Issue #162 revealed that the issues with typing in valkey-py are quite a bit deeper and require some rework of internal classes. The changes introduced in those commits turned out to not fix anything.

At this point we need to publish the new beta because of accumulated fixes since 6.1.0b1. The typing system fixes will be introduced later.

Closes #162
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 a pull request may close this issue.

3 participants