FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[3.13] gh-133745: Fix asyncio task factory name/context kwarg breaks by graingert · Pull Request #133948 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

[3.13] gh-133745: Fix asyncio task factory name/context kwarg breaks - #133948

Merged
gvanrossum merged 17 commits into
python:3.13from
graingert:fix-name-passed-to-task-factory
May 18, 2025
ConversationCommits (17)ChecksFiles changed
Merged

[3.13] gh-133745: Fix asyncio task factory name/context kwarg breaks#133948
gvanrossum merged 17 commits into
python:3.13from
graingert:fix-name-passed-to-task-factory

Conversation

graingert commented May 12, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

bedevere-app Bot mentioned this pull request May 12, 2025
Closed
graingert changed the title gh-128308: Fix asyncio task factory name/context kwarg breaks gh-133745: Fix asyncio task factory name/context kwarg breaks May 12, 2025
bedevere-app Bot mentioned this pull request May 12, 2025
Closed
graingert force-pushed the fix-name-passed-to-task-factory branch from f298146 to 5c3c953 Compare May 12, 2025 19:13
graingert changed the title gh-133745: Fix asyncio task factory name/context kwarg breaks [3.13] gh-133745: Fix asyncio task factory name/context kwarg breaks May 12, 2025
graingert mentioned this pull request May 12, 2025
Closed
gvanrossum reviewed May 12, 2025
View reviewed changes

gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think this is the right thing, just one refactoring suggestion.

We should ask Alice to try this (maybe after it's in the nightly builds).

Comment thread Lib/asyncio/base_events.py Outdated
gvanrossum reviewed May 12, 2025
View reviewed changes

gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

To be clear, this is supposed to happen instead of the rollback, right?

graingert commented May 13, 2025

Copy link
Copy Markdown
Contributor Author

To be clear, this is supposed to happen instead of the rollback, right?

that's my understanding, yet

graingert commented May 13, 2025
View reviewed changes
Comment thread Lib/asyncio/base_events.py Outdated
gvanrossum reviewed May 13, 2025
View reviewed changes

gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Despite what the comment says, you're just removing the try/except here. Was that intentional?

graingert commented May 13, 2025

Copy link
Copy Markdown
Contributor Author

Despite what the comment says, you're just removing the try/except here. Was that intentional?

Ah it looks like that in the diff, but I also added an else and removed the early return. Execution follows after the if/else and returns the task deleting it 'after' the return

gvanrossum commented May 14, 2025

Copy link
Copy Markdown
Member

Got it. What's difficult or controversial about the news fragment? Just that we're waffling about the feature? I think it's reasonable to say something like this:

In 3.13.3 we accidentally changed the signature of create_task() and how it calls a custom task factory in a backwards incompatible way. Since some 3rd party libraries have already made changes to work around the issue that might break if we simply reverted the changes, we're instead changing things to be backwards compatible with 3.13.2 while still supporting those workarounds. In particular, the special-casing of name and context is back (until 3.14) and consequently eager tasks may still find that their name hasn't been set before they execute their first line of code.

WDYT? If you agree, let's merge this.

graingert marked this pull request as ready for review May 14, 2025 08:16
gvanrossum reviewed May 14, 2025
View reviewed changes

gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I am happy with the code and the news blurb.

I'd like @kumaraditya303 to review this too, to be extra sure that we don't have to backtrack more in 3.13.5.

And don't we need something in the docs about this change? I just noticed that it looks like the previous change didn't even have a versionchanged (3.13.3) directive. We should add that retroactively, and another versionchanged (3.13.4) mentioning what we changed there.

kumaraditya303 reviewed May 16, 2025
View reviewed changes

kumaraditya303 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, @graingert do you want to add the version changed entry?

graingert commented May 17, 2025
View reviewed changes
Comment thread Doc/library/asyncio-task.rst Outdated
-----------------------------------------------

.. function:: create_task(coro, *, name=None, context=None)
.. function:: create_task(coro, *, name=None, context=None, eager_start=None, **kwargs)

graingert May 17, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

oh hang on these didn't change until 3.14

graingert commented May 17, 2025
View reviewed changes
Comment thread Doc/library/asyncio-eventloop.rst Outdated
kumaraditya303 reviewed May 17, 2025
View reviewed changes
Comment thread Doc/library/asyncio-eventloop.rst Outdated
Update Doc/library/asyncio-eventloop.rst
8088dac 
Co-authored-by: Kumar Aditya <kumaraditya@python.org>
gvanrossum reviewed May 18, 2025
View reviewed changes
Comment thread Doc/library/asyncio-eventloop.rst Outdated
Comment on lines +379 to +385
An optional keyword-only *eager_start* argument allows eagerly starting
the execution of the :class:`asyncio.Task` at task creation time.
If set to ``True`` and the event loop is running, the task will start
executing the coroutine immediately, until the first time the coroutine
blocks. If the coroutine returns or raises without blocking, the task
will be finished eagerly and will skip scheduling to the event loop.

gvanrossum May 18, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't believe we should document eager_start here at all. I doesn't require special treatment, it can just be put in **kwargs. It looks like we added this in 3.13.4, and such feature changes in a bugfix release are frowned upon. We should only advertise **kwargs as an unfortunate added in3.13.3 and fixed in 3.13.4.

Comment thread Doc/library/asyncio-eventloop.rst Outdated
Comment thread Doc/library/asyncio-eventloop.rst Outdated
Comment thread Doc/library/asyncio-eventloop.rst Outdated
Comment thread Doc/library/asyncio-eventloop.rst Outdated
Apply suggestions from code review
5c26ed7 
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
graingert commented May 18, 2025
View reviewed changes
Comment thread Doc/library/asyncio-eventloop.rst Outdated
graingert commented May 18, 2025
View reviewed changes
Comment thread Doc/library/asyncio-eventloop.rst Outdated
gvanrossum approved these changes May 18, 2025
View reviewed changes

gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Everything looks great! I will merge this now.

gvanrossum merged commit fd6a602 into python:3.13 May 18, 2025

gvanrossum commented May 18, 2025

Copy link
Copy Markdown
Member

Thanks Thomas for moving this forward! Hopefully this is the end of the story for 3.13. For 3.14 I think we ought to add a bit to whatsnew/3.14.rst.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

gvanrossum gvanrossum approved these changes
kumaraditya303 kumaraditya303 approved these changes
1st1 Awaiting requested review from 1st1 1st1 is a code owner
asvetlov Awaiting requested review from asvetlov asvetlov is a code owner
willingc Awaiting requested review from willingc willingc 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.

3 participants

Back | FazBrowse Home | New Git URL