Skip to content

gh-117953: Let update_global_state_for_extension() Caller Decide If Singlephase or Not #118193

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

ericsnowcurrently
Copy link
Member

@ericsnowcurrently ericsnowcurrently commented Apr 23, 2024

@ericsnowcurrently ericsnowcurrently force-pushed the caller-decides-singlephase-for-update_global_state_for_extension branch from 2a91353 to 81daac5 Compare April 23, 2024 18:45
@ericsnowcurrently ericsnowcurrently changed the title gh-117953: Let update_global_state_for_extension() Caller Decides Singlephase or Not gh-117953: Let update_global_state_for_extension() Caller Decide If Singlephase or Not Apr 23, 2024
@ericsnowcurrently ericsnowcurrently force-pushed the caller-decides-singlephase-for-update_global_state_for_extension branch from 81daac5 to fe993a0 Compare April 24, 2024 15:58
@ericsnowcurrently ericsnowcurrently marked this pull request as ready for review April 24, 2024 15:58
@ericsnowcurrently ericsnowcurrently removed the request for review from kumaraditya303 April 24, 2024 15:58
@ericsnowcurrently ericsnowcurrently enabled auto-merge (squash) April 24, 2024 16:01
@ericsnowcurrently ericsnowcurrently merged commit 1acd249 into python:main Apr 24, 2024
35 checks passed
@ericsnowcurrently ericsnowcurrently deleted the caller-decides-singlephase-for-update_global_state_for_extension branch April 24, 2024 16:36
@encukou
Copy link
Member

encukou commented Apr 26, 2024

AFAIK, the idea back then was that single/multiple phase should be determined by the return type from the init function, not by the existence of slots; NULL slots would be equivalent to zero slots (a multi-phase module that doesn't need custom initialization).

I see that multiphase with m_slots=NULL isn't tested explicitly, though.

@ericsnowcurrently
Copy link
Member Author

Thanks for bringing this up. What you've said makes sense and appreciate the clarity of it. There are indeed some gaps (fairly small, I expect), both functionally and in tests. It isn't clear what the impact is in practice, but I definitely think they should be addressed regardless. I'll be sure to open some issues in the next week or two.

FWIW, the general thought of kinds of extension modules was already on my mind (and a bit clearer in my original mega-PR, gh-118116). For now I have a later PR (gh-118205) that is a bit more deliberate about keeping track of what the init function returns. While that PR doesn't do so currently, I might look at explicitly tracking the kind on the module def. That would help address the point you've brought up.

@encukou
Copy link
Member

encukou commented Apr 29, 2024

Thanks! Following up here: #117953 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants