Type hints

[Tinche]

Hello.

We are currently using aioredis with a ton of internal type stubs for it, but aioredis is kind of in a transitional/crisis period right now. So we would like to switch to aredis/coredis. If I submitted PRs with type annotations for a bunch of methods, would you consider merging them in?

[alisaifee]

Hi @Tinche - I'd strongly recommend holding off on spending any time on adding type hints (or any non bug fix changes) on the state of the code that is in master branch right now (which is approximately == 2.x branch). The 2.x series that I've sort of frozen is backward compatible with aredis and contains some of the pending pull requests/fixes + python 3.10 support.

I'm currently (~85% done) in the midst of a significant refactor which involves some breaking changes, but also has proper type hinting support as a required goal. I hope to get something in master and have atleast a beta release out in the next couple of days, and hopefully wrap up documentation cleanup next weekend.

The TL;DR of the breaking changes are:

• Better consistency with redis docs in terms of method argument names & types (and default values)
• Use of enums for pure tokens
• Stricter return types and eventually, overloads for methods that can return different types
• Removal of non essential extras such as cache
• [Most probably] drop support for python 3.7
• [Most probably] drop support using both bytes and strings and just defaulting to strings in and out with always decoded responses

In most simple cases the apis will not be significantly different from either redis-py or aredis so you should be able to drop it in to your code base with trivial effort.

Given what I've described above regarding breaking changes - if that would be manageable for your code base, I'd love to get some eyes (other than mine 😁) on the 3.x candidate.

An example showing a combination of some of these changes:
Before:

   async def georadius(
        self,
        key,
        longitude,
        latitude,
        radius,
        unit=None,
        withdist=False,
        withcoord=False,
        withhash=False,
        count=None,
        sort=None,
        store=None,
        store_dist=None,
        any=False,
    ):

After:

    async def georadius(
        self,
        key: str,
        longitude: Union[int, float],
        latitude: Union[int, float],
        radius: Union[int, float],
        unit: Literal[PureToken.M, PureToken.KM, PureToken.FT, PureToken.MI],
        withcoord: Optional[bool] = None,
        withdist: Optional[bool] = None,
        withhash: Optional[bool] = None,
        count: Optional[int] = None,
        any_: Optional[bool] = None,
        order: Optional[Literal[PureToken.ASC, PureToken.DESC]] = None,
        store: Optional[str] = None,
        storedist: Optional[str] = None,
    ) -> Union[int, Tuple[Union[str, GeoSearchResult], ...]]:

Where GeoSearchResult and GeoCoordinates are still tuples but named.:

class GeoCoordinates(NamedTuple):
    latitude: float
    longitude: float


class GeoSearchResult(NamedTuple):
    name: str
    distance: float
    geohash: int
    coordinates: GeoCoordinates

[Tinche]

All of that sounds great, except I wouldn't drop support for bytes. With some clever typing, I think bytes and strings can coexist fine out of the box. For example, you might parametrize the StrictRedis class by the default return type, i.e. have Redis[bytes] and Redis[str]`, based on whether the encoding was provided on on construction.

Also you don't have to use Union[int, float], float accepts ints by design and I find it much more readable.

I'd be glad to do a review so ping me. For us performance is very important, so I'd want to ensure the result is still as fast as aredis ;)

[alisaifee]

Thanks - that is exactly the kind of feedback I'd love to get. I was toying with making Redis use generics for the return type - and you're absolutely right about not needing the union 👍

[alisaifee]

Sorry for the delay but I've unfortunately not had much time in the last few weeks to work on this. Having said that I just noticed a major addition in redis-py itself to start adding async support (redis/redis-py#1899) which is making me seriously rethink the usefulness of this project.

[Tinche]

Good to know, however if this is just bringing aioredis 2.0 into redis-py (which is what it looked like the last time I looked), this project can still be useful. aioredis 2.0 was/(probably still) is much slower than aioredis 1.3 or aredis.

[Andrew-Chen-Wang]

As maintainer of aioredis and also the person who did the redis py migration, having another option in the meantime is great, especially a maintained one. I would simply be careful around adding features; it gets very hectic very fast. Additionally, one of the other aioredis maintainers is refactoring the asyncio portion to improve performance back to original status, but until then this library is quite useful :)

[alisaifee]

This is an interesting and difficult point in the life of redis python, especially for asyncio.
I've not been able to manage even really getting half way done with the refactoring of aredis to a point that I can reason around the code enough to quickly iterate on adding features (since it was written by someone else).

I will probably have some time the next few weeks to see the type hinting support through. The half baked version so far is in 6ba8675 but I'm indeed starting to feel that this will quickly get out of hand for a single maintainer

[alisaifee]

My design goals for the refactoring are:

• Stay as close to the redis command definition (both for command parameters and return types)
• Minimal runtime overhead
• Runtime type safety as an optional opt-in using beartype
• Departure from the implicit response destructuring using global mapping and try to "inline" how a command is parsed and transformed in to a python equivalent
• Full support for RESP3 to minimize the guess work around parsing response types.

[alisaifee]

@Tinche perhaps you can look at #20 which includes type hints. From my local testing the performance doesn't diverge from the 2.x branch but a second opinion wouldn't hurt.

[alisaifee]

I've merged the current state of type hints / updates to master and made an initial 3.0.0rc1 release.

Specifically related to the type hints you can see the WIP docs and the API documentation

[Tinche]

@alisaifee Sorry, I was super busy but I might be able to review soon.

[alisaifee]

@Tinche thanks! If you could hold off for a little longer (or keep this in mind while reviewing): I made a significant mistake before merging to master / cutting the rc1 release - which is I slipped non optional multiple arguments as variadic in a lot of methods (which is literally meant to be one of the breaking changes). I'll aim at another rc by end of day.

[alisaifee]

FYI I tagged / released3.0.0rc2 which contains the breaking api changes that I was referring to.

[alisaifee]

Closing this as type hints are mostly in place now.