-
Notifications
You must be signed in to change notification settings - Fork 60
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
Replace default asyncio by uvloop when available #821
base: branch-0.24
Are you sure you want to change the base?
Conversation
Very cool! Thanks for sharing this Peter 😄 cc @jcrist (who may find this of interest) |
@pentschev Awesome! |
Part of the reason this didn't have to touch much code is we are using event loops throughout and calling methods on them, which should just work with There was one other line that we might want to update somehow. Filed issue ( #822 ) on it in case we want to handle that separately (though it hopefully is a small change once we decide what we want to do there) |
IMO a library like ucx-py shouldn't configure an eventloop at all, that's something for an end-user application to do (similar to configuring logging handlers, libraries should only produce logs, applications should configure handlers). So something like dask (an application) can configure a specific event loop to use (currently configurable with UVLoop isn't always faster, and there's sometimes good reasons for specifying which event loop to use. If y'all merge this, we'll have to ensure that |
try: | ||
import uvloop | ||
|
||
uvloop.install() | ||
except ImportError: | ||
pass |
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.
Per Jim's comment, perhaps this should be moved to the benchmarking script instead?
I agree, this is something I was exploring only but I feared exactly that would be problematic.
Could you point to some examples of uvloop being slower (or at least not faster) and reasons why someone would not want to use them? |
Sure. One example is tornado, where uvloop is ~ the same performance as asyncio for the backing loop (and sometimes worse). The main reason is transparency though. I don't like it when libraries implicitly configure something as fundamental as the event loop implementation. Bokeh does this already to work around a no-longer-valid issue with tornado and it took me a couple hours to track down where the implicit change was coming from. |
Interesting. Do you have reproducers or results for that somewhere? I would be interested in taking a closer look at that if that's available.
Ah yes, I misunderstood your initial statement, and completely agree implicitly configuring things like that is a terrible practice. Is it possible to globally configure what event loop to use with Dask today? Certainly some configuration like that would be a more useful approach for Dask+UCX-Py. For UCX-Py standalone usage, we could always introduce some configuration to choose between default asyncio event loop or uvloop as well, or just instruct users to use uvloop when they want. |
It's definitely all application specific, but just try enabling uvloop for some tornado workflow and take a look. One example would be the benchmarks I recently did for the asyncio-comms PR in distributed: dask/distributed#5450 (comment). Uvloop was usually negligibly faster, and sometimes slower.
Yes, set
That'd be my recommendation. |
Thanks for the details @jcrist , really appreciate it! Will do some more testing with uvloop and try to come up with a nice, non-intrusive solution. |
Replace default asyncio by uvloop when available. Provides up to 2x speedup in preliminary benchmarks.
asyncio
uvloop