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

Add Async Support #1899

Merged
merged 28 commits into from
Feb 22, 2022
Merged

Add Async Support #1899

merged 28 commits into from
Feb 22, 2022

Conversation

Andrew-Chen-Wang
Copy link
Contributor

@Andrew-Chen-Wang Andrew-Chen-Wang commented Jan 25, 2022

Adds initial async support from aioredis.

Things to do:

  • Debate on whether to add the parallel test_commands.py from aioredis (I don't think it's necessary; lots of duplicate code) when logic is the same. The only async test cases needed for commands is when something is not implemented
  • Add Redis modules testing
  • also make sure this stuff actually works. Ensure uvloop env is running
  • Add cluster support

TODO in other PRs:

  • De-duplicate code
  • Implement 100% coverage of commands on async side

* We need Python 3.7 from async-timeout plus 3.6 EOLed
* test_commands not implemented yet
* Added uvloop support (hopefully)
@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2022

Codecov Report

Merging #1899 (d688f55) into master (e3c989d) will decrease coverage by 0.24%.
The diff coverage is 92.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1899      +/-   ##
==========================================
- Coverage   92.94%   92.69%   -0.25%     
==========================================
  Files          78       99      +21     
  Lines       16785    20717    +3932     
==========================================
+ Hits        15600    19204    +3604     
- Misses       1185     1513     +328     
Impacted Files Coverage Δ
redis/commands/core.py 88.23% <ø> (-2.18%) ⬇️
setup.py 0.00% <ø> (ø)
whitelist.py 0.00% <0.00%> (ø)
redis/asyncio/utils.py 50.00% <50.00%> (ø)
redis/commands/sentinel.py 64.51% <66.66%> (+0.23%) ⬆️
tests/conftest.py 87.60% <74.28%> (-2.50%) ⬇️
tests/test_asyncio/conftest.py 74.50% <74.50%> (ø)
redis/asyncio/connection.py 82.77% <82.77%> (ø)
redis/asyncio/sentinel.py 87.42% <87.42%> (ø)
redis/typing.py 90.32% <90.32%> (ø)
... and 21 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 e3c989d...d688f55. Read the comment docs.

@Andrew-Chen-Wang
Copy link
Contributor Author

Andrew-Chen-Wang commented Jan 25, 2022

@chayim I think for the most part, everything from aioredis has been moved. I type annotated commands. Just need to resolve Redis Modules, cluster support, and CI errors (you can take a look at my aioredis PR unless I forgot to push my changes; in which case, the fashion is similar with Protocols, similar to swift protocols).

Besides that my midterms are coming up so won't have time to resolve this mess for a bit. Please ping me in this PR or other comm channels if you've got Qs!

There's also no need to de-duplicate all the code in this PR. The primary goal of this pr in my opinion is to just add support first. Then we can nitpick

Copy link

@pssolanki111 pssolanki111 left a comment

Choose a reason for hiding this comment

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

Hey @chayim

We'd appreciate if this can be merged in redis-py so that async APIs can be supported too. It will be a great addition.

@chayim
Copy link
Contributor

chayim commented Feb 9, 2022

@pssolanki111 Yup, this is the plan. Between COVID and thing, it's slowed a bit - but the goal is to merge this in. Just want to ensure all tests currently run in the harness (they don't). @dvora-h is working on that.

Thanks for the confirmation!

@chayim
Copy link
Contributor

chayim commented Feb 9, 2022

@pssolanki111 @Andrew-Chen-Wang can you folks help us out? In GH actions (and locally on my Linux desktops) we run into the following event loop issue, across python versions. You're marked the tests with pytest.mark.xfail which I would assume means they can fail, and life can go on safely. At the same time, they're not. Do you have any thoughts - I'm in no way an async expert and will only start to dig in.

tests/test_asyncio/test_multiprocessing.py Process Process-10:
Traceback (most recent call last):
  File "/usr/lib/python3.10/multiprocessing/process.py", line 315, in _bootstrap
    self.run()
  File "/usr/lib/python3.10/multiprocessing/process.py", line 108, in run
    self._target(*self._args, **self._kwargs)
  File "/data/repos/redis/redis-py/asyncio/tests/test_asyncio/test_multiprocessing.py", line 51, in target
    asyncio.get_event_loop().run_until_complete(atarget(conn))
  File "/usr/lib/python3.10/asyncio/base_events.py", line 617, in run_until_complete
    self._check_running()
  File "/usr/lib/python3.10/asyncio/base_events.py", line 577, in _check_running
    raise RuntimeError('This event loop is already running')
RuntimeError: This event loop is already running
xProcess Process-11:

@Andrew-Chen-Wang
Copy link
Contributor Author

@chayim @dvora-h we do not have multiprocessing figured out on aioredis either. if it's causing the entire test run to fail, we should skip the tests.

@Andrew-Chen-Wang
Copy link
Contributor Author

Andrew-Chen-Wang commented Feb 9, 2022 via email

@pssolanki111
Copy link

@chayim As @Andrew-Chen-Wang pointed out, it's an issue with multiprocessing.

More specifically it appears to be a race condition with the event loop. I have not looked at the source, only looking at the source; looks like an attempt to run multiple event loops in multiple processes? If so, that will not work. Processes in python have some veeeeery wierd behaviors (I've been bruised by it a lot, so I know).

so the suggestions are,

  • if it's imperative to run the tests, attempt changing the structure to spawn a new event loop per process (again not ideal, not recommended).
  • Restructure the tests themselves to not involve mp. People needing concurrecy with aioredis (or redis-py) should be fine with asyncio+uvloop and threading concurrency implementation. Almost all the work in both packages are IO bound, hence it makes even more sense.

I'm interested to know though why we even have mp in the tests in the first place.

@Andrew-Chen-Wang
Copy link
Contributor Author

@pssolanki111 aioredis is just a port of redis py, so the tests mustve come with it.

cc @seandstewart if you'd like to join in on the fun.

@pssolanki111
Copy link

@Andrew-Chen-Wang ahhh that makes sense. That just means there is no need for those tests. Async behavior with mp is NOT recommended, nor it is a good design pattern.

Have i used mp or threading with redis-py? yes.

will I use mp with aioredis? hell no.

@chayim
Copy link
Contributor

chayim commented Feb 10, 2022

@pssolanki111 You raise a great point here. multiproc with async really isn't a great design pattern. My limited async experience agrees.

Keeping that in mind, pushing up a removal for the multiprocessing async tests, and running through.

@chayim
Copy link
Contributor

chayim commented Feb 10, 2022

Any chance someone (plural!) would love to help lift the examples into the examples folder? Today, we accomplish this by editing docs/examples.rst, and adding new jupyter notebooks in the docs/examples folder.

I think if we do this - we're going to have an amazing PR! In my mind, when this lands - I could easily see a 4.2.0beta1 coming out of this.

What does everyone think?

@pssolanki111
Copy link

@chayim

Any chance someone (plural!) would love to help lift the examples into the examples folder? Today, we accomplish this by editing docs/examples.rst, and adding new jupyter notebooks in the docs/examples folder.

this might just be me but i'm not certain what exactly you meant there : D

@seandstewart
Copy link

seandstewart commented Feb 10, 2022

I'm very excited to see such progress on this PR. I do want to stress that we have quite a few issues which have emerged out of airodes-py's port to align with the implementation here.

aio-libs-abandoned/aioredis-py#1225

So after this initial move, there's plenty of room for improvement.

One thing I've been digging into is our degraded performance compared to aioredis v1.3 (the current impl is between 4-10x slower on simple get/sets) - there are some major reasons why we moved away from that code from a design and maintenance standpoint, but I think there's a lot we can learn from it about our parsing & IO-bound workloads.

@Andrew-Chen-Wang
Copy link
Contributor Author

Andrew-Chen-Wang commented Feb 12, 2022

I think it is a good idea to get this PR merged first. That way, we can quickly add other things from aio-libs-abandoned/aioredis-py#1225 such as aio-libs-abandoned/aioredis-py#1207, aligning the constructors for important classes like Redis and ConnectionPool, type annotations, and cluster support.

I also agree with seandstewart. The performance degradation is a major factor for many organizations not to upgrade aioredis. Having any assistance for Sean to resolve the out-of-order-response issue would be great (I will also look into it once I find the time)!

Thank you so much for helping keep asyncio for Redis alive!

@seandstewart
Copy link

We should think about feature-flagging async support to 3.7+ (I'd even argue potentially 3.8+). There were numerous changes and bugfixes to asyncio and concurrent.futures which directly affect our ability to serve a quality asyncio-based client.

@chayim
Copy link
Contributor

chayim commented Feb 17, 2022

@seandstewart What are you suggesting? Having classes within a single library that is dependent on a specific version of python strikes me as both difficult, and poor form - but perhaps I misunderstood you. Can you educate me?
As is, we currently don't pass on python3.6 (yet) I started an experiment locally to clean a bunch of that (and move files around) but it's.. woefully inadequate.

chayim and others added 2 commits February 17, 2022 18:22
fixing async tests

core.command typing that broke command tests
@Andrew-Chen-Wang
Copy link
Contributor Author

On my phone, so quick review: supported_errors in retry.py should be Tuple[Type[Error]]. When grabbing the loop, we can make a fn instead called "get_event_loop"

@chayim
Copy link
Contributor

chayim commented Feb 22, 2022

On my phone, so quick review: supported_errors in retry.py should be Tuple[Type[Error]]. When grabbing the loop, we can make a fn instead called "get_event_loop"

Two different sets of issues are left:

  1. A pytest_asyncio issue in 3.6 (I'm fixing)
  2. Currently, the cluster is broken (hopefully next) in this PR.

updating docs to point to async connections

adding python deprecation notice
@chayim
Copy link
Contributor

chayim commented Feb 22, 2022

Dear everyone who has been involved.... Everything now passes. Things are more sane!

Seriously though, we'll get this in (today)! We'll clean up and merge another couple of PRs, and 4.2.0rc1 is on it's way.

A hearty THANK YOU to everyone involved. I'm so grateful for the amazing community we have, and looking forward to our future collaboration!

@chayim chayim merged commit d56baeb into redis:master Feb 22, 2022
@Andrew-Chen-Wang Andrew-Chen-Wang deleted the async branch February 22, 2022 13:19
Themimitoof added a commit to Themimitoof/dj-blacksmith that referenced this pull request May 9, 2023
In order to improve the compatibility for python 3.11, aioredis is
replaced by redis. The package has been deprecated in Feb 21, 2023
since the introduction of aioredis into redis package with the
4.2.0rc1 release.

For more info see:

 - redis/redis-py#1899
 - https://github.com/aio-libs/aioredis-py#-aioredis-is-now-in-redis-py-420rc1-
Themimitoof added a commit to Themimitoof/dj-blacksmith that referenced this pull request May 9, 2023
In order to improve the compatibility for python 3.11, aioredis is
replaced by redis. The package has been deprecated in Feb 21, 2023
since the introduction of aioredis into redis package with the
4.2.0rc1 release.

For more info see:

 - redis/redis-py#1899
 - https://github.com/aio-libs/aioredis-py#-aioredis-is-now-in-redis-py-420rc1-
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants