Skip to content

add: Trio support via AnyIO #183

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

Closed
wants to merge 7 commits into from

Conversation

thearchitector
Copy link

@thearchitector thearchitector commented Mar 25, 2025

Pull Request check-list

  • Do tests and lints pass with this change?
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Is there an example added to the examples folder (if applicable)?

Description of change

This PR adds Trio support to the async Valkey client through AnyIO. It's intentionally not a full refactor, just as close to a line-for-line port as possible (given small asyncio/AnyIO behavioral differences) to keep parity with the existing API.

I also updated the test suite to be anyio-based, as to test everything against whatever async runtime AnyIO supports.

@mkmkme
Copy link
Collaborator

mkmkme commented Mar 25, 2025

Hey @thearchitector,

Thanks for your contribution! Could you tell me please what is the benefit of using anyio or adding the trio support? I'll be honest, I'm not that deep into Python world and hence this is the first time I'm hearing about them :) Thanks!

@amirreza8002
Copy link
Contributor

hi
trio is a different async framework which does things a bit differently

so normally people using trio can't use libraries that are asyncio specific

anyio makes this a bit easier by allowing us to use both libraries

tho if there's gonna do more async development i suggest we also think about the problem we encountered with type hints (I'm on a phone so it's hard to link, sorry)

Copy link
Contributor

@amirreza8002 amirreza8002 left a comment

Choose a reason for hiding this comment

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

these are not an in depth and technical review, just a quick glance

Comment on lines 23 to 25
@pytest.fixture
def anyio_backend():
return "asyncio"
Copy link
Contributor

Choose a reason for hiding this comment

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

it should test against trio as well

Copy link
Author

Choose a reason for hiding this comment

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

yeah very much WIP, planning on cleaning stuff up before un-drafting

Comment on lines -654 to +661
print(repr(pool))
# print(repr(pool))
Copy link
Contributor

Choose a reason for hiding this comment

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

forgot this

Comment on lines -565 to +568
_grl: Any = asyncio.get_running_loop,
# _grl: Any = asyncio.get_running_loop,
# _sniff: Any = sniffio.current_async_library,
Copy link
Contributor

Choose a reason for hiding this comment

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

forgot these

Comment on lines -569 to +580
try:
context = {"client": self, "message": self._DEL_MESSAGE}
_grl().call_exception_handler(context)
except RuntimeError:
pass
self.connection._close()
# try:
# if _sniff() == "asyncio":
# # trio trades global exception handling for local task groups
# context = {"client": self, "message": self._DEL_MESSAGE}
# _grl().call_exception_handler(context)
# except RuntimeError:
# pass

# self.connection._close()
Copy link
Contributor

Choose a reason for hiding this comment

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

forgot this as well

Comment on lines -243 to +239
self._close()
# self._close()
Copy link
Contributor

Choose a reason for hiding this comment

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

forgot this

Comment on lines -245 to +247
def _close(self):
"""
Internal method to silently close the connection without waiting
"""
if self._writer:
self._writer.close()
self._writer = self._reader = None
# def _close(self):
# """
# Internal method to silently close the connection without waiting
# """
# if self._writer:
# self._writer.close()
# self._writer = self._reader = None
Copy link
Contributor

Choose a reason for hiding this comment

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

forgot this

@thearchitector
Copy link
Author

thearchitector commented Mar 25, 2025

yes, its a different framework based on structured concurrency principles - so basically no free-floating tasks + strong guarantees about cancellations and execution scopes. the pycon talk by the author is pretty informative, if you're curious for more

anyio is a bridge framework that implements trio principles in an agnostic way, so if im writing something using trio for structural sanity, i'd be able to use this py client (which is indeed the reason for the pr)

im happy to help with static typing as well, if the problem is illustrated somewhere (or could be)

@amirreza8002
Copy link
Contributor

im happy to help with static typing as well, if the problem is illustrated somewhere (or could be)

there has been discussions in many places
the last issue for this is #164 which should gather things together
and #162 was the one to point out the problem we have with type hinting async parts of this library

there are also general attempts to improve the typing situation of valkey-py, but i think the async parts are a different kind of problem, and need their own attention

Comment on lines +64 to +65
default=[],
action="extend",
Copy link
Author

@thearchitector thearchitector Mar 27, 2025

Choose a reason for hiding this comment

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

anyio's docs suggest (by example) parametrizing the tests by framework, but im not opposed to making this a single option per run if we want to keep it like that

on the invoke side i think it'd still make sense to specify several, but it'd just run the suite N times and dump to separate junit xmls instead of collecting all ~6.5k tests in one file

for _ in range(11)
)
)
with pytest.raises(Exception):
Copy link
Author

@thearchitector thearchitector Mar 27, 2025

Choose a reason for hiding this comment

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

note to self: exception group, same as above

@thearchitector
Copy link
Author

im closing this PR in favor of this one in valkey-glide, since it sounds like the latter library is really the future for this project

@mkmkme
Copy link
Collaborator

mkmkme commented Apr 23, 2025

This project is still being maintained. Admittedly, I don't have a lot of time lately to keep up with the tasks, but I still try to keep it afloat. Please let me know if you reconsider adding support to trio to valkey-py as well.

@thearchitector
Copy link
Author

Understood! It's a bandwidth problem on my end rn, so if space opens up in the future I'd be happy to revisit.

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.

3 participants