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

GH-97696: Initial work on eager tasks #98137

Closed
wants to merge 2 commits into from

Conversation

gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Oct 10, 2022

This defines an event loop method eager_task_factory() that can be made the task factory using loop.set_task_factory(loop.eager_task_factory). It will then return a Future if the coroutine completed (or failed) without ever suspending itself.

As a TEMPORARY demonstration (to be revisited), for now we call this factory from TaskGroup.create_task(), if the loop implementation defines it.

This is PYthon only, and has no tests or docs.
@@ -447,6 +447,38 @@ def create_task(self, coro, *, name=None, context=None):

return task

def eager_task_factory(self, coro, *, name=None, context=None):
Copy link
Member

@1st1 1st1 Oct 13, 2022

Choose a reason for hiding this comment

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

Hm, this doesn't have to be a loop-level method? I imagined this to be a top-level function in the asyncio package.

One would install it like this:

async def main():
    loop = asyncio.get_running_loop()
    loop.set_task_factory(asyncio.eager_task_factory)
    
    # ... the rest of the main() function ...

asyncio.run(main())

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't try very hard to make that work yet... The problem is that the new factory has to call the old factory, but (at least in the Python version) the old factory defaults to None and then create_task() just constructs an instance of the Task class. But when there's a different loop I don't know how to do the equivalent thing -- can I just assume that get_task_factory() shouldn't return None if they have a different task class? (Is uvloop's factory ever None?)

Copy link
Member

Choose a reason for hiding this comment

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

The problem is that the new factory has to call the old factory

Why? I don't think this is a requirement. I remember we discussed it with the instagram people and the idea was to have two functions in asyncio:

# The custom eager factory API that instagram wants to use because
# they have a bunch of instrumentation on top of asyncio.Task:
def asyncio.create_eager_task_factory(custom_task_constructor):

    def factory(coro):
        try:
            result = coro.send(None)
        except StopIteration as si:
             f = Future()
             f.set_result(si.value)
             return f
        except Exception as ex:
             f = Future()
             f.set_exception(ex)
             return f
        else:
             return custom_task_constructor(coro, coro_result=result)

    return factory

# The default eager factory API that most users will use:
asyncio.eager_task_factory = create_eager_task_factory(asyncio.Task)

The implementation of loop.set_task_factory and loop.create_task methods would stay the same. We'd have to add the coro_result= keyword-only argument to asyncio.Task (both Python and C versions), but that I think is acceptable.

I just assume that get_task_factory() shouldn't return None if they have a different task class? (Is uvloop's factory ever None?)

I think None is just a marker for the "default factory is used" scenario. I don't think we were ever wanted to chain factories.


Overall I really want this to be implemented not on the event loop, the loop API surface is already very large.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, that's a nice solution. If I get back to this before anyone else I'll use that.

itamaro added a commit to itamaro/cpython that referenced this pull request Feb 1, 2023
itamaro added a commit to itamaro/cpython that referenced this pull request Feb 1, 2023
@gvanrossum gvanrossum closed this Feb 1, 2023
itamaro added a commit to itamaro/cpython that referenced this pull request Feb 6, 2023
@gvanrossum gvanrossum deleted the asyncio-eager-tasks branch August 17, 2023 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants