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

Support custom IOLOOPs #2435

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

gnir-work
Copy link

@gnir-work gnir-work commented Aug 18, 2024

Summary

This is merge of two PR's

  1. Allow passing a custom loop setup function instead of None #2391
    Support passing a custom IOLoop setup function in order to support running uvicorn in custom ioloop implementation other that the default and uvloop.
    For example this feature will allow using this monitored IOLoop inside uvicorn applications :)
    Link to discussions thread - Add support for custom ioloop implementations #2256

  2. use asyncio.run(..., loop_factory) to avoid asyncio.set_event_loop_policy #2130
    set_event_loop_policy will be deprecated in python 3.13 and uvloop.EventLoopPolicy is deprecated already, this backports and uses the preferred loop_factory keyword to configure asyncio.run

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@gnir-work gnir-work force-pushed the feature/support-custom-ioloop branch 3 times, most recently from 32e28d0 to 46078a9 Compare August 18, 2024 21:43
@gnir-work gnir-work force-pushed the feature/support-custom-ioloop branch 3 times, most recently from 38ee9b2 to 6c5b1a1 Compare August 18, 2024 21:59
@@ -70,10 +71,6 @@ def __init__(self, *args: Any, **kwargs: Any) -> None:

self.config = Config(**config_kwargs)

def init_process(self) -> None:
Copy link
Author

@gnir-work gnir-work Aug 18, 2024

Choose a reason for hiding this comment

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

Not sure why @graingert removed this from here, please make sure its ok :)

Copy link
Member

Choose a reason for hiding this comment

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

the setup_event_loop method is removed, so we don't need to call it in init_process. We also don't need to have a method just to call super - that's the default

@gnir-work gnir-work force-pushed the feature/support-custom-ioloop branch from 6c5b1a1 to fcb1cbe Compare August 18, 2024 22:08
@gnir-work gnir-work marked this pull request as ready for review August 18, 2024 22:13
@gnir-work
Copy link
Author

Any updates regarding this? @Kludex

@gnir-work
Copy link
Author

Hey guys,
Would really love to push this one forward.
We are currently having a real hard time integrating IOLoop monitoring because of the hacks needed when running a custom IOLoop implementation

Thanks :)

@gnir-work
Copy link
Author

@Kludex Are you a maintainer?
I would really love to merge this functionality (given the green light on merging this I will fix the unit tests that seem to have broken over time)

Comment on lines +9 to +10
if sys.platform == "win32" and not use_subprocess:
return asyncio.ProactorEventLoop
Copy link
Member

Choose a reason for hiding this comment

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

Reference for me later: python/cpython#122240

uvicorn/_compat.py Outdated Show resolved Hide resolved
tests/test_compat.py Outdated Show resolved Hide resolved
tests/test_compat.py Outdated Show resolved Hide resolved
tests/test_compat.py Outdated Show resolved Hide resolved
uvicorn/config.py Outdated Show resolved Hide resolved
tests/test_compat.py Outdated Show resolved Hide resolved
tests/test_config.py Outdated Show resolved Hide resolved
@graingert
Copy link
Member

this is a bit more important to get done as 3.14 deprecated the policy system, and we need a way to enable eager tasks

@graingert graingert mentioned this pull request Dec 30, 2024
3 tasks
@Kludex
Copy link
Member

Kludex commented Dec 30, 2024

Question to my future self, or others:

Just to confirm, this is not a breaking change, given that --loop is currently not accepting a different loop other than asyncio and uvloop, right? Or is it?

@graingert
Copy link
Member

Question to my future self, or others:

Just to confirm, this is not a breaking change, given that --loop is currently not accepting a different loop other than asyncio and uvloop, right? Or is it?

it should be not a breaking change.... mostly

there's some differences - we're not using the broken and deprecated policy system anymore so that means anyone using asyncio on windows from a thread will get the default asyncio.EventLoop in their asyncio.run calls, rather than the SelectorEventLoop.

There's another change in that setting the event loop factory to "not None" results in asyncio.set_event_loop() not being called, but this is a no-op on windows anyway. (We do still call set_event_loop on posix, where on old python versions on linux this has observable differences with how child watchers work)

@graingert
Copy link
Member

@Kludex ok I've stopped fiddling with this PR now - it's good to be reviewed

@graingert graingert requested a review from Kludex December 30, 2024 10:07
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