-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
feat(python): better async_collect #10912
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really know enough about async to judge this PR on its merits, but form-wise it looks great with updated docs and everything.
Looks like the method doesn't have a Parameters section in the docstring though. That could be useful to document this new parameter properly.
Thank you @Object905 can you do a rebase? |
Sure, will do, a bit later. Also want to write some test, because there were some corner cases that i've tested manually, especially in gevent, where it has different loop implementations and defaults to different one on windows. Seems like it wont be a problem given how public api documented, but who knows. |
a1212ae
to
ef111de
Compare
Ready for review |
9b256f3
to
9a87be4
Compare
@Object905 can you do another pass for @jakob-keller's review? Then we can work to merging this. |
…able because of python3.8
… add parameters to documentation
ff1368c
to
7c7911c
Compare
Sorry for the delay. Fixed everything regarding punctuation and alphabetic sorting. Regarding pytest-asyncio. I don't think its justified for such small test suite. Also because tests are parametrized for exception this wrapper does more than just wraps asyncio.run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, great job @Object905!
I also did a tiny bit of testing of last week's version and it passed my internal test suite. I am planning to use this in real life, once released.
Thanks for the PR @Object905 and thanks for the review @jakob-keller. Much appreciated as I am not really familiar with this. |
Time for a release? 🙂 |
No tests to check if it's non-blocking? On sufficiently complex queries, |
In #10616 @jakob-keller proposed to simplify this to
await ldf.collect_async()
for asyncio. Not it's possible.Api now is way nicer and defaults to asyncio, since its most common usage. Don't need to pass Queue instances.
Also since they had to be thread safe - default queues from asyncio and gevent were not compatible and external dependency janus was required to safely use them.
asyncio implementation turned out fairly simple
But with gevent I was not able to to do this with ILoop.run_callback_threadsafe for some reason due to
LoopExit: This operation would block forever
, while they say it's a asyncio.loop.call_soon_threadsafe alternative.Maybe there are some checks that are not compatible with how objects are called from out of thread rust side...
Managed to use lower level api, but with nuance that results into AsyncResult are only being set when context switch occurs. (get(block=True), get(block=False, timeout=...) works, but get(block=False, timeout=None) does not),
worked around making result a property to check for this case.