GH-124639: add back loop param to staggered_race #124700

kumaraditya303 · 2024-09-27T19:07:01Z

Issue: Please restore the loop param to staggered.staggered_race #124639

ZeroIntensity · 2024-09-27T19:10:35Z

I think staggered_race should emit a DeprecationWarning, so we can officially make it private in 3.16

gvanrossum

Could you add a deprecation warning when loop is not None (or maybe only if it’s not equal to tg._loop)?

kumaraditya303 · 2024-09-27T19:14:21Z

Could you add a deprecation warning when loop is not None (or maybe only if it’s not equal to tg._loop)?

I'll handle change for 3.14 separately, let's first get this done so that it can be backported to stable releases. We cannot add deprecations to stable releases AFAIR.

Lib/asyncio/staggered.py

gvanrossum · 2024-09-27T19:22:18Z

Got it!

…

--Guido (mobile)

On Fri, Sep 27, 2024 at 12:19 Kumar Aditya ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Lib/asyncio/staggered.py <#124700 (comment)>: > @@ -82,7 +82,11 @@ async def run_one_coro(this_index, coro_fn, this_failed): raise _Done try: - async with taskgroups.TaskGroup() as tg: + tg = taskgroups.TaskGroup() + # Intentionally override the loop in the TaskGroup to avoid + # using the running loop, preserving backwards compatibility + tg._loop = loop Also how sure are you that TaskGroup doesn’t use its _loop before you get to this point? Taskgroup only starts using the loop after __aenter__ https://github.com/python/cpython/blob/0881e2d3b1212d988733f1d3acca4011ce5e6280/Lib/asyncio/taskgroups.py#L54-L60 See lines 58-59 — Reply to this email directly, view it on GitHub <#124700 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAWCWMSEGTWSCE2YI57CY3LZYWVTBAVCNFSM6AAAAABO7XLLR2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMZUGQ3TONJSGI> . You are receiving this because your review was requested.Message ID: ***@***.***>

ZeroIntensity

LGTM. I don't know if the bot will like the 3.13 backport, because the backport for the original PR never got merged (unless I missed that, IIRC Kirill added DO-NOT-MERGE).

gvanrossum

Looks good to me.

gvanrossum

Sorry, needs a test.

bedevere-app · 2024-09-27T20:38:59Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

And if you don't make the requested changes, you will be put in the comfy chair!

gvanrossum · 2024-09-28T21:13:14Z

@kumaraditya303 Why did you mark this as a release blocker? The original PR has not been backported to 3.13. Or is it only a blocker for 3.12? My guess is (and I would agree) that Thomas will not allow the original PR in 3.13.0, and then this can't be merged there either. It can all go into 3.13.1, as well as into the 3.12 branch, of course. Let's just make sure that the original PR doesn't go into a 3.12 release without this one as well.

(I will next actually review the test.)

gvanrossum · 2024-09-28T22:14:20Z

Lib/test/test_asyncio/test_staggered.py

@@ -121,6 +121,25 @@ async def coro(index):
        self.assertIsInstance(excs[0], ValueError)
        self.assertIsNone(excs[1])

+    def test_loop_argument(self):


Ah, so this is just the original test restored. See my comment #124639 (comment) for why I'm not yet approving this.

Hm, I'm not so sure there ever were tests for staggered in 3.12. What I thought I saw was probably in a different branch. :-( Sorry.

There was no tests for it and still has less in main than it should

bdraco

Tested on a production system, details
#124639 (comment)

gvanrossum

This version can go in, since it does the right thing when the current loop is passed in using loop=get_running_loop() (which is the only use case in aiohttp -- they just pass it in redundantly). We really need to land the 3.12 backport ASAP, since the next 3.12 release happens Tuesday (Oct 1).

However, if you pass a different eventloop (e.g. loop=new_event_loop()) things break terribly (I tried this in the test and it just hung).

Consider rolling back the TaskGroup hack (and the comment, alas) and instead just checking that loop is None or loop is get_running_task(), raising ValueError if it isn't.

But that can be a separate PR, so you don't have to stress about the release, or how to test it.

kumaraditya303 · 2024-09-29T02:54:37Z

It's a blocker for 3.12 so I marked it

miss-islington-app · 2024-09-29T03:12:50Z

Thanks @kumaraditya303 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington-app · 2024-09-29T03:12:53Z

Sorry, @kumaraditya303, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker e0a41a5dd12cb6e9277b05abebac5c70be684dd7 3.13

(cherry picked from commit e0a41a5) Co-authored-by: Kumar Aditya <kumaraditya@python.org>

bedevere-app · 2024-09-29T03:13:14Z

GH-124744 is a backport of this pull request to the 3.12 branch.

…124744) GH-124639: add back loop param to staggered_race (GH-124700) (cherry picked from commit e0a41a5) Co-authored-by: Kumar Aditya <kumaraditya@python.org>

…n#124700)" This reverts commit e0a41a5.

…am (#124810) * Revert "GH-124639: add back loop param to staggered_race (#124700)" This reverts commit e0a41a5. * Revert "gh-124309: Modernize the `staggered_race` implementation to support eager task factories (#124390)" This reverts commit de929f3.

…wnstream (pythonGH-124810) * Revert "pythonGH-124639: add back loop param to staggered_race (pythonGH-124700)" This reverts commit e0a41a5. * Revert "pythongh-124309: Modernize the `staggered_race` implementation to support eager task factories (pythonGH-124390)" This reverts commit de929f3. (cherry picked from commit 133e929) Co-authored-by: Peter Bierma <zintensitydev@gmail.com>

…ownstream (GH-124810) (#124817) gh-124309: Revert eager task factory fix to prevent breaking downstream (GH-124810) * Revert "GH-124639: add back loop param to staggered_race (GH-124700)" This reverts commit e0a41a5. * Revert "gh-124309: Modernize the `staggered_race` implementation to support eager task factories (GH-124390)" This reverts commit de929f3. (cherry picked from commit 133e929) Co-authored-by: Peter Bierma <zintensitydev@gmail.com>

add back loop param

284f353

kumaraditya303 added the skip news label Sep 27, 2024

kumaraditya303 requested review from 1st1, asvetlov, gvanrossum and willingc as code owners September 27, 2024 19:07

bedevere-app bot added the awaiting core review label Sep 27, 2024

bedevere-app bot mentioned this pull request Sep 27, 2024

Please restore the loop param to staggered.staggered_race #124639

Closed

kumaraditya303 added needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes topic-asyncio labels Sep 27, 2024

kumaraditya303 requested a review from Yhg1s September 27, 2024 19:10

gvanrossum reviewed Sep 27, 2024

View reviewed changes

Lib/asyncio/staggered.py Show resolved Hide resolved

kumaraditya303 added 3 commits September 28, 2024 00:56

add comment

207b36e

fix typo

fa6a3d7

fix another typo :()

0027565

ZeroIntensity approved these changes Sep 27, 2024

View reviewed changes

gvanrossum reviewed Sep 27, 2024

View reviewed changes

gvanrossum approved these changes Sep 27, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Sep 27, 2024

gvanrossum requested changes Sep 27, 2024

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting merge labels Sep 27, 2024

add test

690746c

kumaraditya303 requested a review from gvanrossum September 28, 2024 03:26

kumaraditya303 added the release-blocker label Sep 28, 2024

ZeroIntensity mentioned this pull request Sep 28, 2024

Using eager task factory with aiohttp 3.10.0 and cpython 3.12+ raise an AssertionError in stdlib asyncio.staggered (fixed in aiohappyeyeballs 2.4.3+) aio-libs/aiohttp#8599

Closed

1 task

Eclips4 approved these changes Sep 28, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting changes labels Sep 28, 2024

gvanrossum reviewed Sep 28, 2024

View reviewed changes

bdraco approved these changes Sep 29, 2024

View reviewed changes

gvanrossum approved these changes Sep 29, 2024

View reviewed changes

kumaraditya303 merged commit e0a41a5 into python:main Sep 29, 2024
33 of 35 checks passed

bedevere-app bot removed the awaiting merge label Sep 29, 2024

miss-islington-app bot assigned kumaraditya303 Sep 29, 2024

kumaraditya303 deleted the async-fix branch September 29, 2024 03:13

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 29, 2024

pythonGH-124639: add back loop param to staggered_race (pythonGH-124700)

dd2fc3f

(cherry picked from commit e0a41a5) Co-authored-by: Kumar Aditya <kumaraditya@python.org>

bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Sep 29, 2024

kumaraditya303 added a commit to miss-islington/cpython that referenced this pull request Sep 29, 2024

pythonGH-124639: add back loop param to staggered_race (python#124700)

1d5c99d

ZeroIntensity added a commit to ZeroIntensity/cpython that referenced this pull request Sep 30, 2024

Revert "pythonGH-124639: add back loop param to staggered_race (pytho…

6d50ee9

…n#124700)" This reverts commit e0a41a5.

miss-islington mentioned this pull request Oct 1, 2024

[3.12] gh-124309: Revert eager task factory fix to prevent breaking downstream (GH-124810) #124817

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-124639: add back loop param to staggered_race #124700

GH-124639: add back loop param to staggered_race #124700

kumaraditya303 commented Sep 27, 2024 •

edited by bedevere-app bot

Loading

ZeroIntensity commented Sep 27, 2024

gvanrossum left a comment

kumaraditya303 commented Sep 27, 2024 •

edited

Loading

gvanrossum commented Sep 27, 2024 via email

ZeroIntensity left a comment

gvanrossum left a comment

gvanrossum left a comment

bedevere-app bot commented Sep 27, 2024

gvanrossum commented Sep 28, 2024

gvanrossum Sep 28, 2024

gvanrossum Sep 29, 2024

kumaraditya303 Sep 29, 2024

bdraco left a comment

gvanrossum left a comment

kumaraditya303 commented Sep 29, 2024

miss-islington-app bot commented Sep 29, 2024

miss-islington-app bot commented Sep 29, 2024

bedevere-app bot commented Sep 29, 2024

GH-124639: add back loop param to staggered_race #124700

GH-124639: add back loop param to staggered_race #124700

Conversation

kumaraditya303 commented Sep 27, 2024 • edited by bedevere-app bot Loading

ZeroIntensity commented Sep 27, 2024

gvanrossum left a comment

Choose a reason for hiding this comment

kumaraditya303 commented Sep 27, 2024 • edited Loading

gvanrossum commented Sep 27, 2024 via email

ZeroIntensity left a comment

Choose a reason for hiding this comment

gvanrossum left a comment

Choose a reason for hiding this comment

gvanrossum left a comment

Choose a reason for hiding this comment

bedevere-app bot commented Sep 27, 2024

gvanrossum commented Sep 28, 2024

gvanrossum Sep 28, 2024

Choose a reason for hiding this comment

gvanrossum Sep 29, 2024

Choose a reason for hiding this comment

kumaraditya303 Sep 29, 2024

Choose a reason for hiding this comment

bdraco left a comment

Choose a reason for hiding this comment

gvanrossum left a comment

Choose a reason for hiding this comment

kumaraditya303 commented Sep 29, 2024

miss-islington-app bot commented Sep 29, 2024

miss-islington-app bot commented Sep 29, 2024

bedevere-app bot commented Sep 29, 2024

kumaraditya303 commented Sep 27, 2024 •

edited by bedevere-app bot

Loading

kumaraditya303 commented Sep 27, 2024 •

edited

Loading