[Discussion] Revisit How We Handle Async and Sync

[chayim]

As of now, redis-py has support for async, and sync usage. The goal is to continue having these within the same library.

However, things seem 'off', or at least unwieldy when handling both cases. While the APIs need to stay the same, this Issue exists to discuss in full - how we can improve this. Let's start this conversation here.

Relevant conversation in another PR: #2096 (comment)

CC: @Andrew-Chen-Wang, @dvora-h, @WisdomPill
Also please feel free to add any others 👍

[Andrew-Chen-Wang]

I guess first obligatory question is what are your current pain points?

One thing that pains me was the size of commands/core.py. Potential solutions:

1. A pyi file to store annotations and docs
2. Use __call__ or __getattr__ to execute methods that only simply return execute_command
3. Put mixins in their own dedicated file

I don't think there's a way for us to combine async and sync methods of core classes like Redis and AsyncRedis since I/O in Python is split by how functions are implemented (eg utilization of async/await keywords). It's just the troubles of maintaining a sync and async API in Python -- or at least as far as I can tell given Elasticsearch-py, Django, and psycopg.

cc @seandstewart

[seandstewart]

Hey folks -

This is precisely why I opened #2039.

We need to move our protocol implementation for serialization and deserialization away from the IO boundary so that we can reduce the amount of doubled implementation but also so that we can properly type our IO implementations without violating the Liskov substitution principle.

[seandstewart]

This will also make testing protocols far simpler!

[Andrew-Chen-Wang]

I'm +1 on the SansIO approach for the aforementioned reasons.

I think there's a steep learning curve to it though (short docs, good talk; difficult to gain that particular mindset). It just reminds me of the complicated structure, at least to me, of aioredis 1.3.1. I also think once PubSub is implemented SansIO style (maybe take inspiration from wsproto? I'm also just reading up on the websockets package's move towards SansIO in addition to how their integrations worked with packages that did both HTTP and ws), it would get the go-ahead and the current implementation would become legacy.

[WisdomPill]

First time I saw this SansIO and it looks promising and clean.

@seandstewart thanks for bringing this up.

One question, how will this make typing behave? for example, that the async command returns an awaitable and the sync one not.
(I just started reading about SansIO, so this might be a stupid question)

[utkarshgupta137]

I've been implementing Async Cluster support in #2099 and while optimising it, I've made so many changes that the sync & async implementations have diverged a lot. At some point, I would like to carry over all the changes to the sync implementation as well.
I've gone through @seandstewart's implementation, and it looks much easier to maintain. I can migrate cluster support to the sansio approach if we go ahead with it.

[utkarshgupta137]

As a secondary objective, can we try integrating AnyIO?

[seandstewart]

One question, how will this make typing behave?

• @WisdomPill -
  Since the IO-based client becomes a wrapper over the sans-IO implementation, you can strongly type the sans-IO implementation, then essentially "wrap" the return value using generics. It's still a bit difficult to get precise types given the sheer number of commands - type stubs may get us further than in-line typing in this regard.

It seems there's broad support for this approach, should we look at defining an implementation plan? The work I've done is a decent step up, but it's far complete. Some potential approaches:

1. Put this behind a major version change and accept that it will be a full re-write. This will probably be the easiest to ship initially, but may prove difficult to vet.
2. Add the sansio package to redis-py as an experimental feature while we build out feature support and testing.

• This would allow developers to access the experimental API while we work on expanding support and test coverage, which would give us the benefit of user feedback.

I'm sure there are others!

[Andrew-Chen-Wang]

Let's go with 2. There's no rush to replace the async module just yet, and I don't think, down the road, fully replacing the async module in another major version will be that difficult.

[utkarshgupta137]

How about a mixture of both? At some point, the sansio approach is going to be the only one & it warrants its own major version. So we can release an alpha with sansio in a separate branch instead of a separate experimental feature. It may've to be a long-running alpha/beta/rc.

[chayim]

I just want to make sure we're on the same page about some goals - these are the baseline items that matter the most to me:

1. We improve the developer experience for those contributing to this library
2. We don't negatively impact (i.e break APIs) for the user facing part of this library. If we do - there's a major version bump, and a conversation to be had clearly. But, it needs to be simple.
3. We imrove the performance of the library along the way
4. We have amazing test coverage (personal, impossible goal is 100%, always).

For the record, I think the sansio approach makes this possible, and likely, just with some tweaks required from our side. I'll always err on the side of "a bit more pain" for maintainers vs consumers if I have to.

This library is already getting beyond complex, and needing cleanup. Something that we're embarking on shorter term.

[Andrew-Chen-Wang]

Just wanted to bump this thread. Having the Sans-IO package in redis-py, even if incomplete, will definitely bloat the library a bit but will make experimentation in the community possible without making a new package.

[utkarshgupta137]

I've been working on rust redis client & I noticed that they don't have separate clients for sync/async (this is partly because of the rust ecosystem), which I think is a better experience. Instead, you pass arguments to a client and then get a sync or async connection from it. Similarly, they don't have separate structs for sync/async pipelines.

[github-actions]

This issue is marked stale. It will be closed in 30 days if it is not updated.

[dvora-h]

Bump

[github-actions]

This issue is marked stale. It will be closed in 30 days if it is not updated.