aiocache icon indicating copy to clipboard operation
aiocache copied to clipboard

[Bugfix] Add redis-py >= 4.2.0 support

Open laggardkernel opened this issue 2 years ago • 18 comments

2022.05.12: switched backend from aioredistoredis-py >= 4.2.0`.


Update:

  • To test with aioredis 2.0.0 automatically. Travis and tox configurations need to be updated.
  • Fixes aio-libs/aiocache#543

What do these changes do?

aioredis 2.0.0 released with breaking API change. Update current backend implementation of airedis/backends/redis.py.

Breaking aioredis changes encounter during making this PR. aio-libs/aioredis-py#1109

Are there changes in behavior for the user?

Nope. Lower level redis backend change. No public API affected.

Related issue number

Checklist

  • [x] I think the code is well written
  • [x] Unit tests for the changes exist
  • [ ] Documentation reflects the changes
  • [ ] Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> (e.g. 588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the PR
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.

laggardkernel avatar Aug 21 '21 09:08 laggardkernel

hi @laggardkernel the PR LGTM! I like it! 👍 let me know if you want me to help on anything missing from the list! thanks a ton

gerardcl avatar Aug 28 '21 09:08 gerardcl

Would love to see this merged

markzporter avatar Sep 09 '21 12:09 markzporter

@laggardkernel Thank you for your efforts! It's a really wanted feature.

Boring-Mind avatar Oct 28 '21 13:10 Boring-Mind

Is this project still maintained? How can we as a group go about getting this merged? For us, this is blocking the migration of some keen souls to use Python 3.10 in the company.

JimNero009 avatar Dec 17 '21 13:12 JimNero009

Is this project still maintained? How can we as a group go about getting this merged? For us, this is blocking the migration of some keen souls to use Python 3.10 in the company.

You can create a fork from this project and make any adjustments you want. It's pretty easy. If you need to download the package from pip, please upload it to pypi.

Boring-Mind avatar Dec 17 '21 15:12 Boring-Mind

We may indeed have to go down that route if there is no chance of this change getting into the codebase any time soon -- is this the case? The changes here seem functionally complete?

In other words, is it 'official' that this codebase is no longer maintained?

JimNero009 avatar Dec 17 '21 15:12 JimNero009

Hi, sadly I'm not maintaining this project anymore but I'm happy to invite people to the project in case anyone wants to volunteer.

argaen avatar Dec 20 '21 01:12 argaen

@argaen has anybody volunteerd as of yet?

pboers1988 avatar Feb 22 '22 13:02 pboers1988

If anyone wants to make a start, feel free to start PRs for migrating the CI to Github. See: https://github.com/aio-libs/aiocache/issues/536#issuecomment-1024448383

Dreamsorcerer avatar Feb 26 '22 18:02 Dreamsorcerer

I think keeping up the multi-version support is unsustainable and has little benefit. I'd like to just set the minimum version to aioredis>=2 for the next release and keep things simple. If someone has time to update this PR to only use v2, that would be great. Otherwise, I'll look at updating later.

Dreamsorcerer avatar Mar 14 '22 22:03 Dreamsorcerer

@Dreamsorcerer Okay, I'd like to have a try to adapt the PR with only aioredis>=2 support when I have spare time.

laggardkernel avatar Mar 16 '22 05:03 laggardkernel

@laggardkernel Did you get blocked on something, or just didn't get around to this? Otherwise, me (or someone else) could try to pick this up.

Mark90 avatar May 09 '22 12:05 Mark90

@Dreamsorcerer Just noticed that aioredis-py suggests using the "original" redis-py instead

Aioredis is now in redis-py 4.2.0rc1+

Would you agree it makes more sense to refactor aiocache to use redis-py than to make it ready for aioredis-py 2.x when the latter is likely to be deprecated?

Mark90 avatar May 09 '22 12:05 Mark90

Yes, might as well move to that library then. Though it's the same work, it looks like they've merged aioredis upstream, so the only difference currently is the import statement.

Dreamsorcerer avatar May 09 '22 16:05 Dreamsorcerer

WIP: Updated the PR with only aioredis 2.x support. The test is fixed and passed, but the pytest fixtures may need some updates. I'll check that later.

Would you agree it makes more sense to refactor aiocache to use redis-py than to make it ready for aioredis-py 2.x when the latter is likely to be deprecated?

I have read both aioredis-py and redis-py before. The last version of redis-py I read is still 4.1. I'll check the latest release of redis-py soon.


@Mark90 The time when I opened the PR I was unemployed. I had a good time browsing and reading all kinds of Python libraries, learning how they are designed. Now i'm back to work and kind of busy. Sorry. 🙇‍♂️

laggardkernel avatar May 09 '22 17:05 laggardkernel

aioredis 2.x only support is done.

This PR is firstly made with backward compatiblity taken into consideration. It supported for both aioredis 1.x and 2.x. After the author agreed to drop support for aioredis 1.x, I went into a wrong direction that kept the old design of redis cache backend, which managed conn and pool manually. This is required when integrating with aioredis 1.x but not with 2.x

Luckily, I realised it after a while, and refactored the code to be similar with the memecached cache backend. It's much simpler and much more elegant now.


Most of my time is not spent on refactoring the redis cache backend code. It's taken to make a correct mock of the aioredis.Redis instance. FYI, methods like .get() .set() on Redis are not coroutine. In fact, when we do await redis.get() we're await its returned value, which is a coroutine.

Autospeccing aioredis.Redis doesn't exactly reflect the above behavior. I mocked the .get(), .set() methods with AsyncMock now to pass the tests. Is there a better way to mock the methods more precisely? I guess it doesn't matter, cause all we wanna do is to ensure Redis.method gets called. Whether Redis.method communicates with the redis server correctly is tested in aioredis.

Sorry, I haven't had time to check async support in redis-py yet.

laggardkernel avatar May 11 '22 15:05 laggardkernel

Done.

The redis >= 4.2.0 async support is based on aioredis and the APIs are compatible. I switched to from aioredis to redis easily. All redis related tests are fixed and passed on my local machine.

There're some breaking changes compared with aioredis 1.x, I'll detail them as code comment conversions.


Update 1: forgot to update aioredis related part in docs and tox.ini. I'll do it later.

laggardkernel avatar May 12 '22 14:05 laggardkernel

Sorry, I thought the pre-commit hooks was enough to detect the linting err. Maybe some of my commits were made without git hooks installed yet.

Anyway, I fixed the linting err with make lint. I re-disabled the performance tests. Haven't dig it deeper to fix them yet. Not only redis, there're also some problems with the memcached performance tests as well, during the test running on my local machine.

BTW, I rebased the PR after the "test key refactoring" merged today.

laggardkernel avatar May 13 '22 05:05 laggardkernel

@laggardkernel @argaen Do you guys need someone to take this PR over the finish line? If you want to add me as a maintainer I can finish any remaining tasks necessary and cut a new release. Thx

joeblackwaslike avatar Dec 01 '22 18:12 joeblackwaslike

The PR is completed. Just need another run of the test by the CI, which is not triggered automatically. Part of the performance test is disabled, cause it doesn't fit the connection pool in redis-py.

@joeblackwaslike Glad about the help you offered. I'm afraid there's nothing we could do. Just the project lacks maintenance. If you need to use aioredis in your own project, you'd better fork the repo, merge this PR and build a package by yourself.

laggardkernel avatar Dec 02 '22 03:12 laggardkernel

Hi, as mentioned above, I'm not maintaining this project anymore but I'm happy to invite any maintainers who would like to contribute. @laggardkernel @joeblackwaslike let me know and I'll invite you

argaen avatar Dec 02 '22 03:12 argaen

Hi there,

If someone here looking for alternative with more up to date support:

https://github.com/Krukov/cashews - ready to be used and contribute

  • using redis-py
  • python 3.7-3.11

Krukov avatar Dec 02 '22 07:12 Krukov

I'll refresh myself on where this is at in a couple of days. Tests probably just need to have master merged in order to run, maybe a force-push messed it up or something.

Dreamsorcerer avatar Dec 02 '22 17:12 Dreamsorcerer

OK, looking good. Just some minor niggles I'd like resolved. Sorry it's taken so long.

Dreamsorcerer avatar Dec 11 '22 20:12 Dreamsorcerer

Codecov Report

Merging #546 (6c74ed3) into master (760ccab) will decrease coverage by 4.65%. The diff coverage is 86.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #546      +/-   ##
==========================================
- Coverage   98.17%   93.52%   -4.66%     
==========================================
  Files          13       36      +23     
  Lines        1040     3967    +2927     
  Branches      140      620     +480     
==========================================
+ Hits         1021     3710    +2689     
- Misses         15      248     +233     
- Partials        4        9       +5     
Flag Coverage Δ
unit 93.49% <86.55%> (-4.68%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiocache/factory.py 100.00% <ø> (ø)
tests/acceptance/test_factory.py 100.00% <ø> (ø)
tests/performance/test_concurrency.py 0.00% <ø> (ø)
tests/performance/test_footprint.py 0.00% <0.00%> (ø)
aiocache/__init__.py 83.33% <66.66%> (ø)
aiocache/backends/redis.py 95.93% <91.52%> (+2.47%) :arrow_up:
tests/acceptance/test_base.py 100.00% <100.00%> (ø)
tests/performance/conftest.py 63.63% <100.00%> (ø)
tests/ut/backends/test_redis.py 100.00% <100.00%> (ø)
tests/ut/test_exceptions.py 100.00% <0.00%> (ø)
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 760ccab...6c74ed3. Read the comment docs.

codecov[bot] avatar Dec 26 '22 14:12 codecov[bot]