Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bugfix] Add redis-py >= 4.2.0 support #546

Merged

Conversation

laggardkernel
Copy link
Contributor

@laggardkernel laggardkernel commented Aug 21, 2021

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


Update:

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-abandoned/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

  • I think the code is well written
  • 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 laggardkernel changed the title [Bugfix] Add aioredis 2.x support [WIP] Add aioredis 2.x support Aug 21, 2021
@laggardkernel laggardkernel changed the title [WIP] Add aioredis 2.x support [Bugfix] Add aioredis 2.x support Aug 22, 2021
@gerardcl
Copy link

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

@markzporter
Copy link

Would love to see this merged

@Boring-Mind
Copy link

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

@JimNero009
Copy link

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.

@Boring-Mind
Copy link

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.

@JimNero009
Copy link

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?

@argaen
Copy link
Member

argaen commented Dec 20, 2021

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.

@pboers1988
Copy link

@argaen has anybody volunteerd as of yet?

@Dreamsorcerer
Copy link
Member

If anyone wants to make a start, feel free to start PRs for migrating the CI to Github. See: #536 (comment)

@Dreamsorcerer
Copy link
Member

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 Dreamsorcerer mentioned this pull request Mar 15, 2022
@laggardkernel
Copy link
Contributor Author

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

@Mark90
Copy link
Contributor

Mark90 commented May 9, 2022

@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
Copy link
Contributor

Mark90 commented May 9, 2022

@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?

@Dreamsorcerer
Copy link
Member

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.

@laggardkernel laggardkernel changed the title [Bugfix] Add aioredis 2.x support [WIP][Bugfix] Add aioredis 2.x support May 9, 2022
@laggardkernel
Copy link
Contributor Author

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 laggardkernel force-pushed the bugfix/aioredis-2.x-support branch 2 times, most recently from b79c9a0 to 97e4a1e Compare May 11, 2022 14:21
@laggardkernel
Copy link
Contributor Author

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 laggardkernel changed the title [WIP][Bugfix] Add aioredis 2.x support [Bugfix] Add aioredis 2.x support May 12, 2022
@laggardkernel
Copy link
Contributor Author

laggardkernel commented May 12, 2022

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 laggardkernel changed the title [Bugfix] Add aioredis 2.x support [Bugfix] Add redis-py >= 4.2.0 support May 12, 2022
The redis backend in aiocache only supports aioredis 2.x now.

aioredis 2.x is refactored based on redis-py, which introduces
breaking changes. The conn, pool management in our redis backend
is redundant and dropped now.
`redis>=4.2.0` merge async support from `aioredis`.
Considering `redis` is bettern maintained and updated, let's switch
from `aioredis` to `redis`.
@laggardkernel
Copy link
Contributor Author

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.

@joeblackwaslike
Copy link

@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

@laggardkernel
Copy link
Contributor Author

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.

@argaen
Copy link
Member

argaen commented Dec 2, 2022

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

@Krukov
Copy link

Krukov commented Dec 2, 2022

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

@Dreamsorcerer
Copy link
Member

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.

Copy link
Member

@Dreamsorcerer Dreamsorcerer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll finish looking through the rest later in the week.

aiocache/backends/redis.py Outdated Show resolved Hide resolved
aiocache/backends/redis.py Outdated Show resolved Hide resolved
aiocache/backends/redis.py Outdated Show resolved Hide resolved
aiocache/backends/redis.py Outdated Show resolved Hide resolved
aiocache/backends/redis.py Outdated Show resolved Hide resolved
aiocache/backends/redis.py Outdated Show resolved Hide resolved
aiocache/backends/redis.py Outdated Show resolved Hide resolved
aiocache/backends/redis.py Outdated Show resolved Hide resolved
aiocache/backends/redis.py Outdated Show resolved Hide resolved
aiocache/backends/redis.py Outdated Show resolved Hide resolved
aiocache/backends/redis.py Outdated Show resolved Hide resolved
aiocache/backends/redis.py Outdated Show resolved Hide resolved
tests/performance/conftest.py Outdated Show resolved Hide resolved
tests/ut/backends/test_redis.py Show resolved Hide resolved
tests/ut/backends/test_redis.py Show resolved Hide resolved
@Dreamsorcerer
Copy link
Member

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

@codecov
Copy link

codecov bot commented Dec 26, 2022

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%) ⬇️

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%) ⬆️
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.

@Dreamsorcerer Dreamsorcerer merged commit fa80aa4 into aio-libs:master Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

redis doesn't work after aioredis package was updated