Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-128307: support eager_start kwarg in create_eager_task_factory, and pass kwargs from asyncio.create_task and TaskGroup.create_task#128306
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-128307: support eager_start kwarg in create_eager_task_factory, and pass kwargs from asyncio.create_task and TaskGroup.create_task #128306
Uh oh!
There was an error while loading. Please reload this page.
Conversation
c809133 to 2f101b3CompareUh oh!
There was an error while loading. Please reload this page.
Misc/NEWS.d/next/Library/2024-12-28-11-01-36.gh-issue-128307.BRCYTA.rst Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
asvetlov commented Dec 28, 2024
|
graingert commented Dec 28, 2024
yup, it's in I just forgot to add it to the news |
Misc/NEWS.d/next/Library/2024-12-28-11-01-36.gh-issue-128307.BRCYTA.rst Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Misc/NEWS.d/next/Library/2024-12-28-11-01-36.gh-issue-128307.BRCYTA.rst Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
Misc/NEWS.d/next/Library/2024-12-28-11-01-36.gh-issue-128307.BRCYTA.rst Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
0699ae2 to 7bce401CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
graingert commented May 4, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
How would the loop even know what type task factory was installed so as to raise? |
gvanrossum commented May 4, 2025
Assuming the segfault will be fixed, let's just work through adding eager_start to all create_task methods. (We can test with the crux of that fix patched in.) @graingert Do you need help writing any code? One thing I'd like to ensure is that at least It might be possible to come up with a protocol whereby a task factory communicates which |
graingert commented May 5, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
I strongly disagree with raising an exception in the currently supported loop.create_task(eager_start=True) case. |
mikeshardmind commented May 5, 2025
So what would you have happen if the task factory doesn't support eager starts? This API is a breaking change if lazy task factories now must handle this. |
graingert commented May 5, 2025
Ah I see, yes then an exception in that case is the current behaviour |
Uh oh!
There was an error while loading. Please reload this page.
python-cla-botbot commented May 5, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
gvanrossum-ms commented May 5, 2025
But by looking at more of the code I just learned that the eagerness is purely implemented by passing There's one bit of behavior that's changed -- not sure if we care (the docs do not mention it): if you explicitly use |
gvanrossum-ms commented May 5, 2025
I know why the tests fail, will fix in a moment. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
gvanrossum left a comment
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.
@graingert Are you okay if I merge this once the tests pass?
graingert left a comment
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.
Just giving everything a final look over
graingert commented May 5, 2025
LGTM! Thanks! |
08d7687 into python:mainUh oh!
There was an error while loading. Please reload this page.
gvanrossum commented May 5, 2025
W00t! Goodnight everyone. Welcome to beta 1. |
bedevere-bot commented May 5, 2025
|
dimaqq commented May 6, 2025
The |
graingert commented May 6, 2025
It's already fixed, here's the issue: #133419 |
| defcreate_task(coro, *, name=None, context=None): | ||
| defcreate_task(coro, **kwargs): |
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.
From the Python user perspective, I don't like this change.
It obscures the allowed kwargs and makes it harder for IDEs to provide suggestions.
I suppose we'd be relying on https://github.com/python/typeshed/blob/main/stdlib/asyncio/tasks.pyi from now on?
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 agree it would be nice if we could avoid the **kwargs, but it make the implementation more complex at every level of indirection (asyncio.create_task -> loop.create_task -> Task, and also TaskGroup.create_task -> loop.create_task -> Task).
IDEs should be using typeshed anyway. (And we should update tasks.pyi to have a case for 3.14 that adds eager_startand**kwargs. In fact this should be done to all the create_task definitions found in typeshed's asyncio definitions.
…ory and various create_task functions (python#128306) Some create_task() functions were changed from `name=None, context=None` to `**kwargs`. Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
Uh oh!
There was an error while loading. Please reload this page.