Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

graingert
Copy link
Contributor

@graingertgraingert commentedDec 28, 2024
edited by bedevere-appbot
Loading

@graingertgraingert changed the titleadd eager_start to create_taskgh-128307: add eager_start to create_taskDec 28, 2024
@graingertgraingert changed the titlegh-128307: add eager_start to create_taskgh-128307: add eager_start kwarg to asyncio.create_taskDec 28, 2024
@graingertgraingertforce-pushed theadd-eager-start-to-create-task branch fromc809133 to2f101b3CompareDecember 28, 2024 10:58
@asvetlov
Copy link
Contributor

TaskGroup.create_task() aslo should accepteager_start argument I guess.

@graingert
Copy link
ContributorAuthor

TaskGroup.create_task() aslo should accepteager_start argument I guess.

yup, it's in I just forgot to add it to the news

@graingertgraingert marked this pull request as ready for reviewDecember 29, 2024 08:48
@graingertgraingert marked this pull request as draftJanuary 21, 2025 05:51
@graingertgraingert changed the titlegh-128307: add eager_start kwarg to asyncio.create_taskgh-128307: add eager_start kwarg to loop.create_taskJan 21, 2025
@graingertgraingertforce-pushed theadd-eager-start-to-create-task branch from0699ae2 to7bce401CompareJanuary 21, 2025 09:26
@graingertgraingert changed the titlegh-128307: add eager_start kwarg to loop.create_taskgh-128307: support eager_start kwarg in create_eager_task_factoryJan 21, 2025
@graingert
Copy link
ContributorAuthor

graingert commentedMay 4, 2025
edited
Loading

How would the loop even know what type task factory was installed so as to raise?

@gvanrossum
Copy link
Member

  • The segfault is being worked on, so let's assume it will get fixed in time and continue to work on this as if it's fixed. The Tuesday deadline is very close!
  • In response to Tin's nice table, that's exactly what I assume. I am okay with the exception: it represent a reality of the current implementation and I don't want to add work before beta 1. We may be able to fix that in a later feature release.
  • Thomas: "How would the loop even know what type task factory was installed so as to raise?" -- I'm not sure what this refers to. Maybe Tin's remark "there's no way to use the type system to help"? But I still don't understand it. Maybe it's not important, so I'll ignore it for now.

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 leasteager_start=None (even when explicitly given) isnot passed to the task factory, to deal with 3rd party task factories that haven't been updated to support this argument.

It might be possible to come up with a protocol whereby a task factory communicates whicheager_start values it supports (e.g. a function attribute on the factory), but that seems a future extension (we'll hopefully learn during beta whether this is needed).

@graingert
Copy link
ContributorAuthor

graingert commentedMay 5, 2025
edited
Loading

I strongly disagree with raising an exception in the currently supported loop.create_task(eager_start=True) case.

@mikeshardmind
Copy link
Contributor

I strongly disagree with raising an exception in the currently supported loop.create_task(eager_start=True) case.

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 nowmust handle this.

@graingert
Copy link
ContributorAuthor

Ah I see, yes then an exception in that case is the current behaviour

mikeshardmind reacted with thumbs up emoji

@python-cla-bot
Copy link

python-cla-botbot commentedMay 5, 2025
edited
Loading

All commit authors signed the Contributor License Agreement.

CLA signed

@gvanrossum-ms
Copy link

Ah I see, yes then an exception in that case is the current behaviour

But by looking at more of the code I just learned that the eagerness is purely implemented by passingTask(..., eager_start=True) so there is no need to have that exception in the table!

There's one bit of behavior that's changed -- not sure if we care (the docs do not mention it): if you explicitly usecreate_task(..., context=None) it will passcontext=None to the task factory, whereas the original code would not pass it. But is anyone passingcontext=None explicitly? Ditto foreager_start=None, though we could say that that's the same asFalse.

Tinche reacted with hooray emoji

@gvanrossum-ms
Copy link

I know why the tests fail, will fix in a moment.

Copy link
Member

@gvanrossumgvanrossum left a 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?

Copy link
ContributorAuthor

@graingertgraingert left a 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

@graingertgraingert changed the titlegh-128307: support eager_start kwarg in create_eager_task_factorygh-128307: support eager_start kwarg in create_eager_task_factory, and pass kwargs from asyncio.create_task and TaskGroup.create_taskMay 5, 2025
@graingert
Copy link
ContributorAuthor

LGTM! Thanks!

@gvanrossumgvanrossumenabled auto-merge (squash)May 5, 2025 04:45
@gvanrossumgvanrossum merged commit08d7687 intopython:mainMay 5, 2025
38 of 39 checks passed
@gvanrossum
Copy link
Member

W00t! Goodnight everyone. Welcome to beta 1.

hugovk, graingert, and Tinche reacted with hooray emojidimaqq and graingert reacted with heart emoji

@graingertgraingert deleted the add-eager-start-to-create-task branchMay 5, 2025 04:59
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure⚠️⚠️⚠️

Hi! The buildbotAMD64 Debian root 3.x (tier-1) has failed when building commit08d7687.

What do you need to do:

  1. Don't panic.
  2. Checkthe buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/345/builds/11228) and take a look at the build logs.
  4. Check if the failure is related to this commit (08d7687) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/345/builds/11228

Failed tests:

  • test_decimal
  • test_external_inspection

Failed subtests:

  • test_async_remote_stack_trace - test.test_external_inspection.TestGetStackTrace.test_async_remote_stack_trace

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):  File"/root/buildarea/3.x.angelico-debian-amd64/build/Lib/test/test_external_inspection.py", line269, intest_async_remote_stack_traceself.assertEqual(stack_trace, expected_stack_trace)~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^AssertionError:Lists differ: [[('c[62 chars]y', 10), ('c4', '/tmp/test_python_sp6aoafc/tmp[1293 chars]]]]]] != [[('c[62 chars]y', 11), ('c4', '/tmp/test_python_sp6aoafc/tmp[1308 chars]]]]]]Traceback (most recent call last):  File"/root/buildarea/3.x.angelico-debian-amd64/build/Lib/test/test_external_inspection.py", line269, intest_async_remote_stack_traceself.assertEqual(stack_trace, expected_stack_trace)~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^AssertionError:Lists differ: [[('c[62 chars]y', 10), ('c4', '/tmp/test_python_9w2frcjy/tmp[1293 chars]]]]]] != [[('c[62 chars]y', 11), ('c4', '/tmp/test_python_9w2frcjy/tmp[1308 chars]]]]]]

@dimaqq
Copy link
Contributor

Thetest_external_inspection failure seems relevant, as some of the sample code covered by test is async code.
Perhaps the test itself needs to be updated?

@graingert
Copy link
ContributorAuthor

It's already fixed, here's the issue:#133419

@@ -386,19 +386,13 @@ def __wakeup(self, future):
Task = _CTask = _asyncio.Task


def create_task(coro, *, name=None, context=None):
def create_task(coro, **kwargs):
Copy link
Contributor

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 onhttps://github.com/python/typeshed/blob/main/stdlib/asyncio/tasks.pyi from now on?

Copy link
Member

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 addseager_startand**kwargs. In fact this should be done to all the create_task definitions found in typeshed's asyncio definitions.

Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull requestJul 12, 2025
…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>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@dimaqqdimaqqdimaqq left review comments

@gvanrossumgvanrossumgvanrossum approved these changes

@gvanrossum-msgvanrossum-msgvanrossum-ms left review comments

@gpsheadgpsheadgpshead approved these changes

@1st11st1Awaiting requested review from 1st11st1 is a code owner

@kumaraditya303kumaraditya303Awaiting requested review from kumaraditya303kumaraditya303 is a code owner

@willingcwillingcAwaiting requested review from willingcwillingc is a code owner

@asvetlovasvetlovAwaiting requested review from asvetlovasvetlov is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

10 participants
@graingert@asvetlov@hugovk@gpshead@Tinche@gvanrossum@mikeshardmind@gvanrossum-ms@bedevere-bot@dimaqq

[8]ページ先頭

©2009-2025 Movatter.jp