Skip to content

gh-121474: Add threading.Barrier parties arg sanity check. #121480

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.

Sign up for GitHub

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

Jump to bottom
Merged
serhiy-storchaka merged 1 commit into from
Jul 30, 2024
Merged

gh-121474: Add threading.Barrier parties arg sanity check. #121480

serhiy-storchaka merged 1 commit into from
Jul 30, 2024

Conversation

@pygeek
Copy link
Contributor

@pygeek pygeek commented Jul 8, 2024 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Jul 8, 2024
Closed
@pygeek pygeek changed the title gh-121474: Added Barrier parties sanity chack. gh-121474: Add Barrier parties sanity chack. Jul 8, 2024
@pygeek pygeek changed the title gh-121474: Add Barrier parties sanity chack. gh-121474: Add threading.Barrier parties arg sanity check. Jul 8, 2024
@pygeek pygeek force-pushed the fix-barrier branch 2 times, most recently from 6878079 to 58880e2 Compare July 8, 2024 08:47
@graingert
Copy link
Contributor

graingert commented Jul 8, 2024

This matches the asyncio.Barrier now, which is great. But I notice neither handle 1.5 and other non-integral floats well

@pygeek
Copy link
Contributor Author

pygeek commented Jul 8, 2024 •
edited
Loading

This matches the asyncio.Barrier now, which is great. But I notice neither handle 1.5 and other non-integral floats well

I noticed this too, but neither do any of the primitives in threading. My primary intention was to get Barrier in line with the other primitives.

I plan on creating a separate Discuss thread on the matter, once/if this current PR is merged.

My guess is that since the float behavior among primitives hasn't been a noted problem until now, that it wouldn't make sense to make the sweeping changes necessary to fix them at this point.

ZeroIntensity
ZeroIntensity requested changes Jul 10, 2024
View reviewed changes
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

Looks good, other than a nitpick and PEP 8 compliance 🙂

@pygeek
Copy link
Contributor Author

pygeek commented Jul 10, 2024 •
edited
Loading

Looks good, other than a nitpick and PEP 8 compliance 🙂

Nice catch. I don't think the linter runs on test files. I literally copied the check from https://github.com/python/cpython/pull/121480/files/58880e2d5387680385e3802d8e18d2d2de8ef470#diff-1d04f1b4512c48ecc14e3eae5713be5dea3bdbeba07f9d77bb08089103c880dbL754-L755 .

I'm not sure if the omission of the linter is intentional.
Does it make sense to have a second lint check in CI that just checks Lib/tests?

Edit:

I double checked. There is a linter on Lib/tests, but ruff format --check isn't being used.

@ZeroIntensity
Copy link
Member

ZeroIntensity commented Jul 10, 2024

Does it make sense to have a second lint check in CI that just checks Lib/tests?

Probably not, and out of the scope of this issue anyways.

@pygeek
Copy link
Contributor Author

pygeek commented Jul 10, 2024

Does it make sense to have a second lint check in CI that just checks Lib/tests?

Probably not, and out of the scope of this issue anyways.

I was speaking more generally, outside the context of this PR. Anyway, I'll start a separate thread in Discuss on the topic.

ZeroIntensity
ZeroIntensity approved these changes Jul 10, 2024
View reviewed changes
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

LGTM!

serhiy-storchaka
serhiy-storchaka approved these changes Jul 30, 2024
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

@serhiy-storchaka serhiy-storchaka merged commit d27a53f into python:main Jul 30, 2024
@serhiy-storchaka serhiy-storchaka added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Jul 30, 2024
@miss-islington-app
Copy link

miss-islington-app bot commented Jul 30, 2024

Thanks @pygeek for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@miss-islington-app
Copy link

miss-islington-app bot commented Jul 30, 2024

Thanks @pygeek for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 30, 2024
@pygeek @miss-islington
pythongh-121474: Add threading.Barrier parties arg sanity check. (pyt…
4212392 
…honGH-121480)

(cherry picked from commit d27a53f)

Co-authored-by: Clinton <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 30, 2024
@pygeek @miss-islington
pythongh-121474: Add threading.Barrier parties arg sanity check. (pyt…
f7c7070 
…honGH-121480)

(cherry picked from commit d27a53f)

Co-authored-by: Clinton <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Jul 30, 2024

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

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Jul 30, 2024
@bedevere-app
Copy link

bedevere-app bot commented Jul 30, 2024

GH-122444 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jul 30, 2024
@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Jul 30, 2024

Thank you for your contribution @pygeek.

serhiy-storchaka pushed a commit that referenced this pull request Jul 30, 2024
@miss-islington @pygeek
[3.12] gh-121474: Add threading.Barrier parties arg sanity check. (GH…
ca22750 
…-121480) (GH-122443)

(cherry picked from commit d27a53f)

Co-authored-by: Clinton <[email protected]>
serhiy-storchaka pushed a commit that referenced this pull request Jul 30, 2024
@miss-islington @pygeek
[3.13] gh-121474: Add threading.Barrier parties arg sanity check. (GH…
202cfbf 
…-121480) (GH-122444)

(cherry picked from commit d27a53f)

Co-authored-by: Clinton <[email protected]>
@pygeek pygeek deleted the fix-barrier branch July 30, 2024 14:50
blhsing pushed a commit to blhsing/cpython that referenced this pull request Aug 22, 2024
@pygeek @blhsing
pythongh-121474: Add threading.Barrier parties arg sanity check. (pyt…
9cf446b 
…honGH-121480)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@graingert graingert graingert left review comments

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@ZeroIntensity ZeroIntensity ZeroIntensity approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pygeek @graingert @ZeroIntensity @serhiy-storchaka