Closed seandstewart closed 3 weeks ago
cc: @chayim, @Andrew-Chen-Wang
I'd love to discuss this more, let me know your thoughts 👍
This looks great! Thanks so much for this! @seandstewart
Code-wise:
On an initial look through, it looks like the problem of ordering has been resolved really well. I love the variable docs. It was interesting reading through io module with the Events. A concern I have would be during a connection timeout / kill that was fixed in redis-py.
I'm not entirely sure what this line is waiting for; explanation would be great!
The PythonPaser has these readprimitive
methods. Are these the only ones we need?
Nit picks:
Unless I'm not understanding the code properly, returning exceptions is a little... interesting? I'd expect explicit RedisError-inherited exceptions. Such as here: https://github.com/seandstewart/redis-py-sansio/blob/7400a38ae9fa92db7e1164a5731036675f7e3551/redis/sansio/_parser.py#L149 it would be nice to have
class RedisError(Exception):
def __init__(self, exc=None):
self.exc = exc
...
except Exception as e: raise RedisError(e)
Coming from a Django background, I'd prefer if things were explicit/verbose. For instance, many people already know what kw in kw_only means, but I'd still prefer the long kwargs, plural.
Organizationally, mostly for @chayim:
I think organizationally, the asyncio module can use the new code base as many people are getting used to the new code anyways + the interface hasn't changed much whereas the sync client would still default to the current implementation in redis-py with an additional option being the Sans IO implementation.
I will take a better look at this sometime in the last week of this month. Thanks again @seandstewart !!!
I really like this approach. Equally I echo explicit versus implicit choices. While personally I prefer implicit, for library and communal use I think explicit, and even verbose is more valuable.
I'd generally like to see the clients themselves become simpler, and rely on fixed features that are then included, and perhaps overridden re-implemented as necessary (clearly timeout is an example). I've been thinking of introducing a ConnectionFactory, as part of this - which in turn would make it easier for developers to discover connection types, though I don't yet understand how that would impact the async world.
@seandstewart First off, loving this. Second - WDYT?
@Andrew-Chen-Wang @chayim -
Thanks for taking a look at this! I'm so glad to find us all in this place and I appreciate the positive response to these efforts. I'll try and respond to each question in turn 👍
wait...()
methods force the generator to yield
if there is no data in the parse buffer. This allows the main process to context-switch, which in turn opens other workloads within the process to fill the buffer.read<primitive>()
methods. Are these the only ones we need?
hiredis.Reader
. This allows our protocol to not care which one is used, which simplifies the implementation.ResponseError
: always returned, it’s the parsed value of an ERR
response.InvalidResponse
: always raised, the server returned an un-parseable response (likely we’re not talking to a valid Redis server). kw_only
parameter for attr.define
. I don’t have any control over that. FWIW, Python 3.10 adopted this same parameter for its dataclass implementation. I’m not adamant that we use attrs for these data objects, but it makes the definition of slot-based classes much simpler. Once 3.10 is the LCD, I’d advocate we drop the dependency.Super stoked to be collaborating on this and can’t wait to see where this discussion leads! Hope these responses are helpful.
Looks like we've done the same thing. I have just write a resp protocol parser in python. The difference is sioresp only contains a parser, without any io-related implementation.
This issue is marked stale. It will be closed in 30 days if it is not updated.
This should be re-opened
Agreed that this should be reopened, especially if there's still a gap in performance between https://github.com/aio-libs/aioredis-py/tree/v1.3.1 and redis-py
Sorry folks, auto-closed, reopened.
I haven't forgotten about this proposal! Things have evolved over the last year, but I would still love to find a path forward with this 🚀
This issue is marked stale. It will be closed in 30 days if it is not updated.
Hey folks - I've been working hard on the performance issue the currently asyncio implementation faces when compared to https://github.com/aio-libs/aioredis-py/tree/v1.3.1
We're tracking this as part of the meta-issue https://github.com/aio-libs/aioredis-py/issues/1225 and is considered a top priority to encourage folks to adopt the async client. (Here's a direct link to the issue: https://github.com/aio-libs/aioredis-py/issues/1208)
As part of this work, I've also been cognizant of the overall maintenance burden of maintaining two client implementations in a single code-base. Using the principles laid out in https://sans-io.readthedocs.io/, I have taken a stab at implementing the core client logic in a Sans-IO, event-driven model. You can see the work here:
https://github.com/seandstewart/redis-py-sansio
I think this could be a very worthwhile endeavor to take on - it allows the codebase to diverge only in minimal ways for the specific IO implementations, as illustrated in the high-level client & connection modules.
Looking forward to more collaboration!