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.

[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😮‍💨

[Dreamsorcerer]

I find this a bit awkward and confusing. I think I'd rather make the config required and simplify it to:

Suggested change

	client: Optional[GlideClient] = None,
	serializer: Optional["BaseSerializer"] = None,
	namespace: str = "",
	key_builder: Callable[[str, str], str] = lambda k, ns: f"{ns}:{k}" if ns else k,
	backend: type[GlideClient] = GlideClient,
	config: GlideClientConfiguration = None,
	**kwargs: Any,
	config: GlideClientConfiguration,
	serializer: Optional["BaseSerializer"] = None,
	*,
	namespace: str = "",
	key_builder: Callable[[str, str], str] = lambda k, ns: f"{ns}:{k}" if ns else k,
	**kwargs: Any,

[asafpamzn]

@amirreza8002

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

We will add it the future. But, connection pool is mainly used for sync API as each thread has to get a connection from the pool and return it once it got the response back from Valkey. For async mode it is less important as multiple tasks can share the same connection.
If it is blocking you we can add it. It will take sometime though.

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

I will work with the other maintainers to add it in the next release in end of May.
I think that it should be straight forward and there should not be an objection. I will get back to you.

@amirreza8002 Are there any blockers? If yes, we can add them and release a patch version. It would be amazing to get valkey-glide integrated to aiocache.

[amirreza8002]

hi @asafpamzn , thanks for your response 🙌

about blockers, dreamsourcerer would have a better take

i did make a pr about something I'd seen here, but i wouldn't call that a blocker

we're currently discussing how to implement a context manager, i agree that the current implementation is ugly
but i need some good sleep before working on it anymore
in the mean time, if anyone has any ideas on it, I'd appreciate it

[ikolomi]

@amirreza8002 @Dreamsorcerer
Hi Guys,
I am also Glide's maintainer, nice to meet.

I will be looking into the issue with not tearing down the client object correctly, we might have a bug. Will update

[Dreamsorcerer]

about blockers, dreamsourcerer would have a better take

Main thing I'd like looked at ASAP is Pypy. If it's supposed to be supported, why can't the CI install it?

[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.