implement valkey support

[amirreza8002]

What do these changes do?

add support for valkey, using valkey-gilde

Related issue number

Fixes #882

Checklist

• a usable valkey client
• proper testing
• Documentation reflects the changes

[amirreza8002]

notes about this PR:

1. valkey glide does not have a connection pool and does not support some of the configs used with redis, so the current implementation walks around it and doesn't implement those as well

2. i've left some small things in there for future (e.g: set returns "OK" instead of True)

3. some tests fail, at least in my local machine, i am yet to discover what the problem actually is, they seem to be flaky and hard to debug

4. i'm thinking of removing the lua scripts, they can be written in python (i did kinda remove one cause it was problematic)

5. i think we should add some sort of shortcut to create a valkey client, since glide's way is ugly

i'll continue to debug the problems with the tests, they could be something obvious that my tired eyes can't see or something hard, idk, i really appreciate any help and ideas and tips

some extra notes:
can we enforce a formatter for the whole package? it's hard to remember to not run black . and only format specific files

also the packaging and development setup is painful

[Dreamsorcerer]

Looking good. A bit of cleanup still to do and the tests need to pass, but looks like it's mostly good.

[amirreza8002]

hi! I'm back

so currently i fixed set to return True or False
i replaced lua scripts with python code (also discovered that one of the lua scripts was probably broken)
fixed the CI image name
and some small cleanups

more importatnly tho, i looked into why tests are failing
what i discovered is that when we run the tests one by one, they work and pass
but when i run them in one place, they fail
it's probably a problem with how pytest-asyncio is configured (the warnings also indicate this)
i'd have to look more into it, meanwhile i'd appreciate any input on the matter

on the discussion about context managers, i have something like this in mind

async with ValkeyCache(config=my_config) as cache:
    await cache.set(...)

where my_cache is an instance of GlideClientConfiguration which user provides

another idea i have is to use a classmethod

async with ValkeyCache.with_context_manager(config=congi):
    ...

this way we don't need to change the signature of __init__
but it's not a big deal anyway

but i do think config should be provided by the user, so they can configure it as they please

p.s: i know the commits have gotten a bit messy, i'm keeping it like this so i can see diff, i'll clean it up before un-drafting

[Dreamsorcerer]

this way we don't need to change the signature of __init__

There is no released version with the current signature. The first version is fine.

[Dreamsorcerer]

i'd have to look more into it, meanwhile i'd appreciate any input on the matter

Once the tests actually run in the CI, I might be able to help.

p.s: i know the commits have gotten a bit messy, i'm keeping it like this so i can see diff, i'll clean it up before un-drafting

Don't bother, it makes reviewing more difficult. We'll squash commit the PR.

[Dreamsorcerer]

Ah, that's awkward, there's also no pypy version for valkey-glide?

[amirreza8002]

Ah, that's awkward, there's also no pypy version for valkey-glide?

https://github.com/valkey-io/valkey-glide/blob/main/python/pyproject.toml#L24
this says that it does
but i don't know, haven't tested it

[Dreamsorcerer]

ERROR    asyncio:base_events.py:1871 Task was destroyed but it is pending!
task: <Task cancelling name='Task-29' coro=<BaseClient._reader_loop() done, defined at /opt/hostedtoolcache/Python/3.13.2/x64/lib/python3.13/site-packages/glide/glide_client.py:509> wait_for=<Future cancelled>>

Is that a bug in glide? From what I see at a glance, the _reader_loop() task is never awaited on. It looks like it's been cancelled here, which appears to only happen when the client is garbage collected. Either we're not closing the client correctly, or glide is depending on __del__() for cleanup (it should cleanup in close() instead and await the cancelled task).

[Dreamsorcerer]

Ah, that's awkward, there's also no pypy version for valkey-glide?

https://github.com/valkey-io/valkey-glide/blob/main/python/pyproject.toml#L24 this says that it does but i don't know, haven't tested it

Oh, well, pip can't find it to install on the pypy job for some reason.

[Dreamsorcerer]

ERROR    asyncio:base_events.py:1871 Task was destroyed but it is pending!
task: <Task cancelling name='Task-29' coro=<BaseClient._reader_loop() done, defined at /opt/hostedtoolcache/Python/3.13.2/x64/lib/python3.13/site-packages/glide/glide_client.py:509> wait_for=<Future cancelled>>

Is that a bug in glide? From what I see at a glance, the _reader_loop() task is never awaited on. It looks like it's been cancelled here, which appears to only happen when the client is garbage collected. Either we're not closing the client correctly, or glide is depending on __del__() for cleanup (it should cleanup in close() instead and await the cancelled task).

Because we fail on warnings, that may be all the errors occurring in the tests. If fixed, that may resolve most of them.

[Dreamsorcerer]

That's also the reason the tests work when run individually. For a single test run, the garbage collection won't happen until after the test run has completed, so the test ends up passing.

[amirreza8002]

Ah, that's awkward, there's also no pypy version for valkey-glide?

https://github.com/valkey-io/valkey-glide/blob/main/python/pyproject.toml#L24 this says that it does but i don't know, haven't tested it

Oh, well, pip can't find it to install on the pypy job for some reason.

i did try to install it locally
it did install, tho it's broken

[amirreza8002]

ERROR    asyncio:base_events.py:1871 Task was destroyed but it is pending!
task: <Task cancelling name='Task-29' coro=<BaseClient._reader_loop() done, defined at /opt/hostedtoolcache/Python/3.13.2/x64/lib/python3.13/site-packages/glide/glide_client.py:509> wait_for=<Future cancelled>>

Is that a bug in glide? From what I see at a glance, the _reader_loop() task is never awaited on. It looks like it's been cancelled here, which appears to only happen when the client is garbage collected. Either we're not closing the client correctly, or glide is depending on __del__() for cleanup (it should cleanup in close() instead and await the cancelled task).

hmm, might be
i'll test it with different scopes, also i'll test it with anyio since they seem to handle tests a bit differently
if i find out it's breaking cause of our setup, i'll report back
otherwise i'll open an issue on glide's repo

[amirreza8002]

so that fixed the problem...
imma ask you to forget what you see in that commit..

there are some ut problems, i'll fix that soon then work on the context manager and docs

[asafpamzn]

the _reader_loop() task is never awaited on. It looks like it's been cancelled here, which appears to only happen when the client is garbage collected. Either we're not closing the client correctly, or glide is depending on __del__() for cleanup (it should cleanup in close() instead and await the cancelled task).

Might be,
@amirreza8002 , please let us know if you need my help

[Dreamsorcerer]

the _reader_loop() task is never awaited on. It looks like it's been cancelled here, which appears to only happen when the client is garbage collected. Either we're not closing the client correctly, or glide is depending on __del__() for cleanup (it should cleanup in close() instead and await the cancelled task).

Might be, @amirreza8002 , please let us know if you need my help

Looks like it just missed the close. The close() method should probably await the task though to ensure any exceptions etc. get raised correctly (like https://docs.aiohttp.org/en/stable/web_advanced.html#complex-applications).

[amirreza8002]

hi @asafpamzn
i've wondered about two things

one is the lack of a connection pool that we see in other clients
is this intentional? or something that's planned for future?

two os the lack of context manager support
also if it's intentional or something for future
thanks for allocating your time for this

[amirreza8002]

hi again 👋🏼
so as far as i can tell this should do it

let me know what you think
if it's good, i should get to docs😮‍💨

[amirreza8002]

hi 👋
so as you might have heard my country has been unlawfully attacked by the Zionist regime

and sadly i don't have the ability to dedicate myself to this project currently,
and since there is no clear image of when things will calm down, i can't give a date on when i will return to this

hopefully when things do calm down I'll come back and finish up,
but if there is any urgency on this, or if i don't come back, I'm more than happy if any interested person would step in to finish this

hope this pr benefits people, and y'all have a great time ❤️

[ikolomi]

@amirreza8002 This what happens when your country's regime spreads terror across the entire region and tries to acquire nukes. Hope Iranian people will use this opportunity to set themselves free from the religious dictatorship. There can be peace between us as it once was.

[amirreza8002]

👀
I'm not sure what you mean by "this what happens..."
we're safe and sound
just distracted by the news of our responses

[sakosha]

Hey @amirreza8002! Hope you are safe!
Do you mind, if I finish this PR?

[amirreza8002]

hello 👋
thank you
of course, if you want, go for it

[Dreamsorcerer]

Thanks for sticking with this PR, it turned into a much larger PR than I originally expected. That's been really helpful.
If anyone can create a followup PR for updating the docs, that would be much appreciated. No rush though.