[Feature request] Typing support and migrating from redis-py

[Rafiot]

Version: 6.0

Platform: Any

Description: The typing provided by the redis package is terrible and the recommended way is to use types-redis but this one obviously doesn't work with the valkey package, and there is no types-valkey (yet?).

I'm playing around to see if I can find a janky solution, but is there any chance you're planning to merge (and support) the types hints directly in the package? Or provide a types-valkey package at some point?

[aiven-sal]

Hi!
I'd say that ideally we would like to have proper typing support. Of course adding types in a project of this size isn't a simple task. Re-using the work done for "types-redis" could certainly help.
Have you used that? Does it work well with recent versions of redis-py? It seems to be stuck at 4.6

[Rafiot]

Hi! Yes, I'm using it with redis-py 5.0.8 and it works perfectly fine, but I might very well not be using new features that are unsupported (python/typeshed#10592).

If I have time today, I'll have a look on how types-redis does to independently support async vs. non async calls. It might very well be a quick fix I to push to valkey-py.

[Rafiot]

Alright, so this is going to be a fun project. I may tell you things you already know, but that was new to me and there are a few questions to answer before starting to work on that.

First of all, as far as I can tell, the typing hints are completely broken and never work for anyone:

Async

import asyncio

from valkey.asyncio import Valkey


async def test() -> dict[bytes, bytes]:
    v: Valkey = Valkey()
    response_hset: int = await v.hset('foo', mapping={'bar': 'baz'})
    print(response_hset)
    h: dict[bytes, bytes] = await v.hgetall('foo')
    print(h)
    return h

asyncio.run(test())

$ (git::main) mypy test_async.py 
test_async.py:10: error: Incompatible types in "await" (actual type "Awaitable[int] | int", expected type "Awaitable[Any]")  [misc]
test_async.py:12: error: Incompatible types in "await" (actual type "Awaitable[dict[Any, Any]] | dict[Any, Any]", expected type "Awaitable[Any]")  [misc]
Found 2 errors in 1 file (checked 1 source file)

Sync

from valkey import Valkey

v: Valkey = Valkey()
v.hset('foo', mapping={'bar': 'baz'})
h: dict[bytes, bytes] = v.hgetall('foo')
print(h)

$ (git::main) mypy test.py 
test.py:7: error: Incompatible types in assignment (expression has type "Awaitable[dict[Any, Any]] | dict[Any, Any]", variable has type "dict[bytes, bytes]")  [assignment]
Found 1 error in 1 file (checked 1 source file)

That's because the way sync/async is implemented. For example, if you take hgetall, it's in valkey/commands/core.py -> HashCommands.hgetall, calls execute_command, and returns a dict.

And the Valkey inherits all the command classes so you we can do client.hgetall(...)

The way the async library works was very confusing to me at first because it's just defining the async class this way AsyncHashCommands = HashCommands. But then, execute_command is overwritten and this call returns an Awaitable.

TL;DR: It is a sane way to avoid too much code duplication, but makes typing a bit of a nightmare.

I'm not totally certain on the path forward from here but it would make sense to fix it as it seems the way to get it to work as-is looks like that, and I hate it.

I think the clean-ish way to do it without duplicating code is to integrate the .pyi files from types-redis into the valkey package, it seems to be possible.

Sorry that was pretty long, but it took me a while to understand what was going on.

What do you think?

[aiven-sal]

Thanks for the through analysis!
My understanding is the the types in types-redis are not perfect (according to what I read in some comments).
But the current situation is that we can't enable type checking in the CI or in our IDEs because types are completely broken.
So if you say that types-redis works for you, using those types is probably going to be an improvement for everyone. I guess we won't be able to enable checks in CI anyway, but it should be a step in the right direction.

And yes, I agree that integrating those type annotations in valkey-py is the best way forward, rather than relying on external stubs.

[mkmkme]

Hey @Rafiot ,

Thanks for the report! Do you want to handle it yourself? Otherwise I can have a look at it.

[Rafiot]

Hey @mkmkme, I'll try to get starting on it soon, but I have a few more urgent tasks first, so I'm not totally sure it will happen this week.

Regardless, it's going to take quite a bit of time, and I have a very limited understanding of the codebase (besides the report above), but I think the first thing to do is getting mypy to run on the repo, and increment from there.

If you (or anyone else) have time to work on it, it might make sense to have a dedicated branch for that?

[golgor]

@Rafiot I'm a fairly new user of Redis/Valkey but I got annoyed by the lack of typing support from day 1 basically. I might be able to help out, I will at least track what you are doing and if I see something where I can help out I will try to.

Not sure if it is possible or acceptable to ask in the typeshed-repo if any of the maintainers behind types-redis is interested to support?

Thanks a lot for raising this!

[Rafiot]

Good point, I opened an issue with the maintainers of types-redis, we will see if they have ideas.

[amirreza8002]

hi @Rafiot

i was wondering if you don't mind if i send some patches on type hints that i know are wrong

one example being sismember type hinting value as str, but accepts bytes (and maybe others) :)

let me know what you think

[Rafiot]

Please all the PRs you feel are necessary. I barely started working on it and the starting point will be types-redis, so the most accurate it is, the better.

EDIT: Just to make sure, you're not talking of patching the type hints in this repo, right? As far as can tell, they are pretty much useless and they will most probably go away.

[Rafiot]

ok, so I did something quick and dirty (emphasis on dirty), and copied the content of types-redis in to valkey-py and renamed Redis -> Valkey / Hiredis -> Libvalkey / ...

With that, the very simple scripts above pass fine with mypy. It's not even close to be mergeable, but that's an okay starting point.

I think my next steps will be to make it possible to run mypy on valkey-py. It's gonna take a while and if some on you want to collaborate on that, it might make sense to have a branch to push changes gradually.

[aiven-sal]

Awesome!

[Rafiot]

So just FYI, I'll slowly work on getting mypy to run on the branch in my repo. It's gonna take a while but if someone wants to contribute, please go for it.

And I'll also merge the changes from types-redis as they come in.

[mkmkme]

Sounds great, thanks!

[Rafiot]

What's your feeling regarding PEP563? I find it much more readable and pleasant to use, but it requires python 3.7+.

[amirreza8002]

What's your feeling regarding PEP563? I find it much more readable and pleasant to use, but it requires python 3.7+.

since this packages supports 3.8+ i think using __future__ would be good

my IDE would be a lot happier with new type hints, it doesn't work well with Union and stuff🙃

[Rafiot]

Good point, the python version won't be an issue. I might wait for a first working package without PEP 563 just so merging from the typeshed is easy-ish.

[Rafiot]

Alright, todays update: mypy passes on most of the public interface, and the tests for the commands in sync and async: https://github.com/Rafiot/valkey-py/blob/types/.mypy.ini

What do you think would be a good next step?

[mkmkme]

I think one of the next steps should be to merge the signatures into the code. Having them in separate .pyi files is not that handy IMHO

[Rafiot]

On the long run, agreed, but due to the way the async is implemented, we will need to keep a .pyi file at least for that (or rewrite it completely). The main reason I'm not totally sure it is a good thing to do immediately is that it will make the merge a lot more complicated to review: I'm trying to change the minimal amount in the .py files and make sure it works.