Open chayim opened 2 years ago
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:
__call__
or __getattr__
to execute methods that only simply return execute_command
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
Hey folks -
This is precisely why I opened https://github.com/redis/redis-py/issues/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.
This will also make testing protocols far simpler!
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.
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)
I've been implementing Async Cluster support in https://github.com/redis/redis-py/pull/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.
As a secondary objective, can we try integrating AnyIO?
One question, how will this make typing behave?
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:
I'm sure there are others!
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.
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.
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:
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.
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.
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.
This issue is marked stale. It will be closed in 30 days if it is not updated.
Bump
This issue is marked stale. It will be closed in 30 days if it is not updated.
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: https://github.com/redis/redis-py/pull/2096#discussion_r847382190
CC: @Andrew-Chen-Wang, @dvora-h, @WisdomPill Also please feel free to add any others :+1: