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

BUG: apply critical sections around populating the dispatch cache#27400

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
ngoldbaum merged 2 commits intonumpy:maintenance/2.1.xfromcharris:backport-27392
Sep 17, 2024

Conversation

charris
Copy link
Member

Backport of#27392.

Fixes#27386.

This moves the locking to a higher conceptual level in the code. Now we lock the entire ufunc object whenever we go intopromote_and_get_info_and_ufuncimpl.

In my testing I'm not able to reproduce the duplicate identity cache entry error@jakevdp saw in theml_dtypes CI locally after making this change. Unfortunately it's a multithreaded issue and inherently flaky, so I can't be 100% positive this fixes it.

Also side benefit of being a lot simpler!

Updatingpythoncapi-compat lets us use the critical section macros without putting them insidePy_GIL_DISABLED macros. They're just open and close brace on the GIL-enabled build.

One question for@seberg or maybe@mhvk: is dtype promotion ever re-entrant? I don't think so but I'd like to double-check because I think it might be problematic if we ever recursively created critical sections here.

@charrischarris added 00 - Bug 08 - BackportUsed to tag backport PRs 39 - free-threadingPRs and issues related to support for free-threading CPython (a.k.a. no-GIL, PEP 703) labelsSep 16, 2024
@charrischarris added this to the2.1.1 release milestoneSep 16, 2024
@charris
Copy link
MemberAuthor

@ngoldbaum Might checkdispatching.c as it differs from the original.

@ngoldbaum

This comment has been minimized.

@charris
Copy link
MemberAuthor

@ngoldbaum Looks likePy_BEGIN_CRITICAL_SECTION has unexpected semantics :) I wondered why you moved the declarations out of the section.

@ngoldbaum
Copy link
Member

Oh sorry about my comment earlier, I thought you were commenting on the original PR.

This is definitely wrong, the critical section API is defined with macros that include braces so you can't put them in error handling sections. I'm not sure why the backport didn't apply cleanly - if you don't mine waiting I can probably push the correct backport to this PR sometime tomorrow. I'm out sick today.

@charris
Copy link
MemberAuthor

I'm not sure why the backport didn't apply cleanly

Sebastian cleaned out the legacy promotion mode, so the protected section is much smaller in main.

@ngoldbaum
Copy link
Member

I can see why this solution wasn't obvious to me back in June! Thanks for working through this.

This looks correct to me.

I tried reproducing the error in theml_dtypes tests using this branch and did not see them, so I think thisis correct.

@ngoldbaumngoldbaum merged commitd40c363 intonumpy:maintenance/2.1.xSep 17, 2024
64 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
00 - Bug08 - BackportUsed to tag backport PRs39 - free-threadingPRs and issues related to support for free-threading CPython (a.k.a. no-GIL, PEP 703)
Projects
None yet
Milestone
2.1.1 release
Development

Successfully merging this pull request may close these issues.

2 participants
@charris@ngoldbaum
[8]ページ先頭
©2009-2025 Movatter.jp