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.

[ikolomi]

@amirreza8002 @Dreamsorcerer
I have looked into the issue with the clean up. Our code does require an explicit call to close() method as presented in out examples: await client.close()

Having said that, we will look into an option to integrate the close() logic into the destructor. I am pretty sure we've already tried but will re-investigate

[Dreamsorcerer]

Having said that, we will look into an option to integrate the close() logic into the destructor. I am pretty sure we've already tried but will re-investigate

I think you got that the wrong way round. You can't close in the destructor, we just missed the close() call originally.

Only (minor) concern I see is that the close() method should probably await the _reader_loop() task to ensure any exceptions etc. get raised correctly (like https://docs.aiohttp.org/en/stable/web_advanced.html#complex-applications).

[amirreza8002]

cleaned up some stuff
and added more info to the repr message

btw did my comments on context manager come through?

[Dreamsorcerer]

btw did my comments on context manager come through?

Which ones?

[amirreza8002]

i left two comments on your review on context manager.
I'm not sure why they are not visible to you,
I'll repost them here

[amirreza8002]

first comment:

so i did try that, there is one issue tho:
how to instantiate a client without a context manager?

since glide clients are made by awaiting the create() method, we can't do that in init
so it would become something like this:

class ValkeyCache(...):
    def __init__(self, config, ...):
        self.client = await GlideClient.create(config=config)  # error
        
    async def __aenter__(self):
        return self
    
    async def __aexit__(self, *args, **kwargs):
        await self.client.close()

or is there something i'm missing?

[amirreza8002]

i have two designs in mind

class ValkeyCache(...):
    def __init__(self, config, ...):
        self.config = config
        
    async def __aenter__(self):
        await connect()
        return self
    
    async def __aexit__(self, *args, **kwargs):
        await self.client.close()
        
    async def connect(self):
        self.client = await GlideClient.create(config=self.config)

and document that the user should await on connect after instantiation

or:

class ValkeyCache(...):
    def __init__(self, config, ...):
        self.config = config
        
    async def __aenter__(self):
        self.client = await GlideClient.create(config=self.config)
        return self
    
    async def __aexit__(self, *args, **kwargs):
        await self.client.close()

    @classmethod        
    async def connect(cls, config):
        client = await GlideClient.create(config=config)
        self = cls(config=config)
        self.client = client
        return self

and instantiate using classmethod

admittedly both are somewhat ugly

[Dreamsorcerer]

I'm not sure why they are not visible to you,

Maybe you started a review and didn't submit the review?

since glide clients are made by awaiting the create() method, we can't do that in init
so it would become something like this:

Just create the client in __aenter__(). There's no need for it before that.

how to instantiate a client without a context manager?

You mean a Cache? I don't think we need another way to do so. The user can just call __aenter__() if needed. Or if we really wanted to support a separate method, we could have start()/close(), but I'm not sure that's something we need yet.

[Dreamsorcerer]

@asafpamzn @ikolomi Could one of you confirm the situation with Pypy support?

[amirreza8002]

we discussed pypy here : valkey-io/valkey-glide#3553
i think they are working on it

is the general feeling around this PR good?
should i start documenting the backend?

[amirreza8002]

hi @Dreamsorcerer

the glide team have informed me that pypy issue is fixed
but the release will come in may

[Dreamsorcerer]

Yeah, that's only a couple of weeks, so maybe we wait for that and then cleanup this PR.

[amirreza8002]

hi again 🙌🏼
seems like they released the new version
i updated the dependencies, lets see if it works as expected

[amirreza8002]

hi @ikolomi @asafpamzn
can you see if the build error we're getting here is a dependency problem?
i don't see any dependencies mentioned in glide docs, if it's a dependency problem maybe we should add it to the docs

but even when i build it in my own system, in a pypy env, it doesn't seem to work, i'll open an issue for this one

[asafpamzn]

hi @ikolomi @asafpamzn can you see if the build error we're getting here is a dependency problem? i don't see any dependencies mentioned in glide docs, if it's a dependency problem maybe we should add it to the docs

but even when i build it in my own system, in a pypy env, it doesn't seem to work, i'll open an issue for this one
sure, we are on it

[ikolomi]

hi @ikolomi @asafpamzn can you see if the build error we're getting here is a dependency problem? i don't see any dependencies mentioned in glide docs, if it's a dependency problem maybe we should add it to the docs

but even when i build it in my own system, in a pypy env, it doesn't seem to work, i'll open an issue for this one

@amirreza8002

Are you referring to this error: https://github.com/aio-libs/aiocache/actions/runs/14851993441/job/41719836603?pr=975#step:6:364

It seems you dont have the protoc installed on the build system. Github runners dont include it i believe.
We install it explicitly in our build workflow

Could you please verify

[Dreamsorcerer]

It seems you dont have the protoc installed on the build system.

So, there's no prebuilt wheels for pypy then? Would probably be very useful to have those from a user perspective.

Looks like there's a libprotoc-dev package that can be apt installed, if that resolves the problem for now.

[amirreza8002]

We install it explicitly in our build workflow

ah I didn't see that part
tho i do think this should be included in the documentation

thanks for checking this out 🙏

i reopened the issue i had on glide's repo, since it seems to persist
I'll test again once the issue is resolved