GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-46613: Add PyType_GetModuleByDef to the public & limited API by encukou · Pull Request #31081 · python/cpython

Conversation

@encukou

@encukou encukou commented Feb 2, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Remove the underscore from the name (everywhere)
  • Add to limited API

https://bugs.python.org/issue46613

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@encukou
Make PyType_GetModuleByDef public (remove underscore) & add it to lim…
f27b002
…ited API

- Remove the underscore from the name (everywhere)
- Add to limited API
@encukou
encukou requested review from a team, berkerpeksag, markshannon and rhettinger as code owners February 2, 2022 17:00
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Feb 2, 2022
@bedevere-bot bedevere-bot added the awaiting core review label Feb 2, 2022
@encukou
Fix ReST syntax
5fd18c8
erlend-aasland
erlend-aasland approved these changes Feb 4, 2022
View reviewed changes
shihai1991
shihai1991 approved these changes Feb 9, 2022
View reviewed changes

@shihai1991 shihai1991 left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks, LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@shihai1991

shihai1991 commented Feb 9, 2022

Copy link
Copy Markdown
Member

whatsnew/3.11.rst shoud be updated?

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Feb 9, 2022

Copy link
Copy Markdown
Contributor

whatsnew/3.11.rst shoud be updated?

Yes, good catch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou
Add a What's New entry
764051c
@encukou

encukou commented Feb 11, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Here it is!
I can't quite come up with a way to summarize it for people who haven't read up on the topic. Hopefully it's enough.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Feb 11, 2022

Copy link
Copy Markdown
Member

Would it make sense to not add it to the limited API in Python 3.11, try to convert a few third party C extensions using it, and if everything goes well, add it to the limited API?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou

encukou commented Feb 11, 2022

Copy link
Copy Markdown
Member Author

Would it make sense to not add it to the limited API in Python 3.11, try to convert a few third party C extensions using it, and if everything goes well, add it to the limited API?

It would! OTOH, addition to the limited API is also easy to revert, and the function itself already proved useful in stdlib.
Should I remove the stable ABI addition for now?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Feb 11, 2022

Copy link
Copy Markdown
Contributor

I think that makes sense. AFAICS, only a handful of devs (Christian Heimes, Hai Shi, Petr, Victor, me) have been using this function in the CPython code base. In this case, maybe it would make sense to try it out in the wild first. Who could we ask?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Feb 11, 2022

Copy link
Copy Markdown
Member

Should I remove the stable ABI addition for now?

For Py_NewRef/Py_XNewRef, I chose to add it to the stable ABI immediately. For PyType_GetModuleByDef(), I'm more scared because I introduced a bug and noticed it before your recent review. Sadly, only a minority of 3rd party C extensions are using heap types. Moreover, we discovered issues with heap types defined in the stdlib in Python 3.10:

  • Some heap types didn't implement fully the GC protocol: no Py_TPFLAGS_HAVE_GC flag, or no traverse function, or objects not tracked by the GC
  • Types became mutable: add Py_TPFLAGS_HAVE_GC flag
  • tp_new=NULL didn't work anymore: add Py_TPFLAGS_DISALLOW_INSTANTIATION flag

These bugs made me even more careful than I was previously.

Maybe PyType_GetModuleByDef() is fine, but it's just that heap types in general became "a minefield" if you don't pay attention.

I love heap types, I want to use them everywhere. Maybe we need to enhance the documentation, add more warnings in PEP 630, and even implement new runtime checks (in development/debug mode?).

For example, creating a type with Py_TPFLAGS_HAVE_GC but with no traverse function now raises an exception.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou
Revert addition to limited API
130d9ef
@encukou

encukou commented Feb 11, 2022

Copy link
Copy Markdown
Member Author

Makes sense, but I'm skeptical of actually finding the issues (ones we can't fix in CPython patch release) unless this is in the limited API. Heap types -- and this function -- aren't very useful for existing projects if they aren't switching to the limited API.
Essentially all of the testing will be on that handful of devs anyway.

more warnings in PEP 630

Those are coming up!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou
... and move it to the right header
aae4347
vstinner
vstinner reviewed Feb 11, 2022
View reviewed changes
Comment thread Doc/c-api/type.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/c-api/type.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/c-api/type.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/c-api/type.rst
:c:func:`PyModule_GetState()` to get module state from slot methods (such as
:c:member:`~PyTypeObject.tp_init` or :c:member:`~PyNumberMethods.nb_add`)
and other places where a method's defining class cannot be passed using the
:c:type:`PyCMethod` calling convention.

@vstinner vstinner Feb 11, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You may mention somewhere that this function only works on a type created by PyType_FromModuleAndSpec.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@encukou encukou Feb 11, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It works with any method that attaches the module to the class. Currently there's just one, but I've been adding new functions with extra arguments relatively fast. I'd rather not document one specific function.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
encukou and others added 2 commits February 11, 2022 16:12
@encukou @vstinner
Apply suggestions from code review
c20313f
Co-authored-by: Victor Stinner <[email protected]>
@encukou
Fix ReST syntax
81b460c
vstinner
vstinner approved these changes Feb 11, 2022
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM.

It's better with the correct return type in the doc :-)

I hope that we will be able to add it to the limited C API soon!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Feb 11, 2022
@encukou
encukou merged commit 2049469 into python:main Feb 11, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Feb 11, 2022
@encukou
encukou deleted the pytype_getmodulebydef branch February 11, 2022 16:22
@encukou

encukou commented Feb 11, 2022

Copy link
Copy Markdown
Member Author

Thank you!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland pushed a commit to erlend-aasland/devguide that referenced this pull request Sep 8, 2023
@encukou @vstinner @erlend-aasland
bpo-46613: Add PyType_GetModuleByDef to the public API (python/cpytho…
f80cb71
…n#31081)

* Make PyType_GetModuleByDef public (remove underscore)

Co-authored-by: Victor Stinner <[email protected]>
AA-Turner pushed a commit to AA-Turner/devguide that referenced this pull request Sep 13, 2023
@encukou
bpo-46613: Add PyType_GetModuleByDef to the public API (python/cpytho…
9d1a7ad
…n#31081)

* Make PyType_GetModuleByDef public (remove underscore)

Co-authored-by: Victor Stinner <[email protected]>

GitHub-Issue-Link: python/cpython#90771
erlend-aasland pushed a commit to python/devguide that referenced this pull request Sep 26, 2023
@encukou @erlend-aasland
bpo-46613: Add PyType_GetModuleByDef to the public API (python/cpytho…
67be7bd
…n#31081)

* Make PyType_GetModuleByDef public (remove underscore)

Co-authored-by: Victor Stinner <[email protected]>

GitHub-Issue-Link: python/cpython#90771
@encukou encukou mentioned this pull request Apr 10, 2022
Add PyType_GetModuleByDef to the public & limited API #90771
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes
@shihai1991 shihai1991 shihai1991 approved these changes
@erlend-aasland erlend-aasland erlend-aasland approved these changes
@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner
@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@encukou @shihai1991 @erlend-aasland @vstinner @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.