AioRedis v2 Changes / Migration

[jtbaker]

A project I'm working on needs to use both arq and aioredis to connect to different redis databases with separate use cases. Following the aioredis docs for using their 2.x release as instructed here aio-libs-abandoned/aioredis-py#987, I installed using the 2.0.0a1 tag. After installing this dependency, running import arq throws this stack trace:

config.py:2: in <module>
    import arq
../../../venv/lib/python3.9/site-packages/arq/__init__.py:1: in <module>
    from .connections import ArqRedis, create_pool  # noqa F401
../../../venv/lib/python3.9/site-packages/arq/connections.py:13: in <module>
    from aioredis import MultiExecError, Redis
E   ImportError: cannot import name 'MultiExecError' from 'aioredis'

I guess with the 2.0 aioredis release, the naming/location of the module's exceptions, and perhaps other module internals have changed. Is migrating to to use this recent aioredis release a goal of the project? I'd be happy to dig in to it further and possibly put together a PR if it was deemed appropriate.

Thanks for your work on this project, it's been quite useful.

[SamiAlsubhi]

aioredis 2.0 is not finalized and not stable yet.

[dannya]

Seems that aioredis 2.0 has just been finalized with a stable release:

https://github.com/aio-libs/aioredis-py/releases

[havardthom]

+1

I'm unable to upgrade aioredis to 2.0.0 in my project due to arq still being on 1.x version

[JrooTJunior]

@samuelcolvin, do you have any plans to fix compatibility with aioredis 2.0?

[samuelcolvin]

I'm keen, but I'm super busy atm. I'll look soon.

[Olegt0rr]

I'm keen, but I'm super busy atm. I'll look soon.

Hope in this 20 days you've managed your super-business :)

+ related PR: #259

[samuelcolvin]

No @Olegt0rr, new born babies take more than 20 days to bring up.

I'll review this when I have time.

In the meantime, if you don't want to use free software I build and maintain, don't.

[waza-ari]

Honestly @Olegt0rr you have a strange attitude towards open source projects maintained on a private and voluntary basis.

@samuelcolvin I feel this need to be said: thanks for all your work, and congratulations to your new family member. Enjoy the time as much as you can!

The only thing I'm concerned with is that the library as it is right now is broken for fresh installs, as arq/setup.py contains:

install_requires=[
 'aioredis>=1.1.0',

So by default, it will now install aioredis 2.0.0, which is incompatible with arq. Short-term option is to manually install aioredis and force it to use a 1.x version:

pipenv install aioredis=='1.3.1'

[samuelcolvin]

Thanks for understanding.

I'll do a patch release next week to restrict aioredis, then a new release to upgrade properly.

[Olegt0rr]

@samuelcolvin, @waza-ari,
It seems that you misunderstanding me. I've tried to send a reminder this way, but didn't want to rush you (sorry for my eng).

@samuelcolvin, thanks for your projects! They're really valuable for me! Congratulation for your family! Being a father is a wonderful thing! All of the rest can wait :)

[samuelcolvin]

thanks everyone for your patience on this, and thanks @Olegt0rr for explaining. I know how frustrating delays in open source libraries like this can be.

I've released 0.22 which restricts aioredis to <2, I'll review #259 now and work on getting a new release out.

I think this new release will have to drop support for aioredis<2 since the interface has changed a fair bit.

[Olegt0rr]

@samuelcolvin,

Dropping v1 support may make your users sad (if the user has some kind of libraries: with v1-only and v2-only support - he is trapped and can't update any of them)
In aiogram we have a long discussion with many pros and cons and then decided to add temporary (till the next major release) support for both versions via the adapter class. I would like to hope that you will choose this way too. Anyway I'm glad to see you back :)

P.S.: note that new aioredis has troubles with correct closing, it seems that they will go back from __del__ -close to explicit calling .close() method.

[samuelcolvin]

i agree aioredis 2 has some problems, as well as the issues with __del__ there's a lot of problems with minimal docs and with encoding strings.

Therefore, support for aioredis 2 might but be available soon.

I don't agree about supporting both versions of aioredis: arq makes pretty deep usage of redis, supporting both versions would require duplicating massive chunks of the library, in places the behaviour of the two versions is fundamentally different, supporting both may mean subtly different behaviour. In the end if you want to use the older version if aioredis, use the older version of arq.

I don't know why or how deeply aiogram uses redis, but I don't think it's really a good analogue to arq.

[Olegt0rr]

@samuelcolvin,
Redis(..., decode_responses=True) doesn't solve encoding issues?

[samuelcolvin]

well, we need to sometimes get() pickle data which is binary, and sometimes get strings. So as pointed out on #259, we have to omit decode_responses then do a lot of .decode().

[AngellusMortis]

Probably worth just replacing this with redis-py support since it looks like aioredid and redis-py combined

https://github.com/redis/redis-py/releases/tag/v4.2.0rc1

[samuelcolvin]

Update on migrating to new redis connection

Good news! After two days of banging my head against the wall, I've at last got #259 to pass and merged it. Thank you so much @Yolley for your initial work on this.

In the end part of the problem was that aioredis 2.0.1 is broken in numerous subtle ways. In the end I migrated to redis-py's new async support (thanks for the suggestion @AngellusMortis) since that takes from master of aioredis which fixes some of the problems.

With this we loose the type hints which were introduced in aioredis 2.

There are still a few things to do before releasing a new version:

1. The distinction between pools and connections has changed a lot between aioredis 1.3, aioredis 2 and redis-py 4.2.0rc2 - we may need to rename things like create_pool to avoid confusion.
2. The sentinel test was breaking other tests in very weird ways - currently I've skipped that test but we should add proper sentinel tests
3. the docs might need updating
4. Perhaps we should wait for redis-py to release 4.2.0 before a full release

Most important of all: Please test this on production code and let me know if it's working.

[JonasKs]

Awesome!

Do you want to release a pre-release for easier installs for us? I'll port over all our projects tomorrow and see how it behaves.

[AngellusMortis]

Perhaps we should wait for redis-py to release 4.2.0 before a full release

I think it may be good to cut a new release and maybe remove the <2 restriction, but with the warning that aioredis v2 has issues. Then for the next version switch to redis-py. I would really like to see a new version sooner rather than later because there are a lot of other unreleased features (such as #274)

[waza-ari]

First of all, awesome work! Thank you all! I'm happy to provide testing as well, maybe in our dev environment though :)

Perhaps we should wait for redis-py to release 4.2.0 before a full release

I think it may be good to cut a new release and maybe remove the <2 restriction, but with the warning that aioredis v2 has issues. Then for the next version switch to redis-py. I would really like to see a new version sooner rather than later because there are a lot of other unreleased features (such as #274)

Lifting the restriction would just break new installs, wouldn't it? It would still install aioredis in v2, which breaks the entire setup. Having some kind of intermediate release with the redis-py dependency makes sense to me though, it could simplify testing.

[samuelcolvin]

Do you want to release a pre-release for easier installs for us? I'll port over all our projects tomorrow and see how it behaves.

Will do.

I think it may be good to cut a new release and maybe remove the <2 restriction, but with the warning that aioredis v2 has issues.

Sorry, it I wasn't clear before. arq now has no dependency on aioredis instead it uses redis.asyncio which is available in redis>=4.2.0rc2.

This was required because aioredis 2.0.1 is broken. In particular Redis.close(), is a misnomer - it does not close the connection(s), instead it releases the current connection back to the pool and leaves the pool connected! This was causing many tests to fail. This is partially fixed in aioredis master (and redis.asyncio which is really just a copy&paste of recent aioredis master) by providing the close_connection_pool argument to Redis.close(). There might be more errors fixed in aioredis master/redis.asyncio, I don't know, I got the tests to pass at last and left it there.

Honestly, I'm not very impressed by either aioredis or redis.asyncio, but the previous plan I had to build my own redis library for python is not feasible right now. Hence testing these changes is important.

[samuelcolvin]

I've decided against renaming create_pool since the new ArqRedis / Redis class behaves a bit like a pool and renaming the function would break a lot of code.

I've released v0.23a1, please test!

[JonasKs]

I changed from the fork to v0.23a1, and all tests still pass, and everything seems to be running as it should for now.

EDIT: I had some issues with msgpack previously, I don't use it anymore (since I'm currently triggering tasks with pydantic models as inputs), but I will try to reproduce this weekend for good measure.

[AngellusMortis]

Everything looks good for far after updating in my project.

[havardthom]

Just testet Arq v0.23v1 on our project and all jobs a running fine.

Although I'm missing support for passing username to RedisSettings now that the redis client supports ACL-protected instance: https://aioredis.readthedocs.io/en/latest/getting-started/#connecting-to-an-acl-protected-redis-instance

[samuelcolvin]

Is the username issue something we can fix in arq?

[havardthom]

Yes I believe it's just adding the attribute to RedisSettings class and passing it correctly when creating the redis client.

[samuelcolvin]

PR welcome.

[havardthom]

Nvm, this was actually merged to master 4 days ago: #299

[samuelcolvin]

Great, I'll include that in v0.23

[JonasKs]

This can be closed @samuelcolvin