Skip to content
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

PEP 733: An Evaluation of Python's Public C API #3491

Merged
merged 49 commits into from
Nov 1, 2023

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Oct 16, 2023

Any author who has no more comments can go ahead and approve.
I'll make some efforts to chase those who don't approve, but if not successful I may have to remove them as authors.

Approving authors

Basic requirements (all PEP Types)

  • Read and followed PEP 1 & PEP 12
  • File created from the latest PEP template
  • PEP has next available number, & set in filename (pep-NNNN.rst), PR title (PEP 123: <Title of PEP>) and PEP header
  • Title clearly, accurately and concisely describes the content in 79 characters or less
  • Core dev/PEP editor listed as Author or Sponsor, and formally confirmed their approval
  • Author, Status (Draft), Type and Created headers filled out correctly
  • PEP-Delegate, Topic, Requires and Replaces headers completed if appropriate
  • Required sections included
    • Abstract (first section)
    • Copyright (last section; exact wording from template required)
  • Code is well-formatted (PEP 7/PEP 8) and is in code blocks, with the right lexer names if non-Python
  • PEP builds with no warnings, pre-commit checks pass and content displays as intended in the rendered HTML
  • Authors/sponsor added to .github/CODEOWNERS for the PEP

📚 Documentation preview 📚: https://pep-previews--3491.org.readthedocs.build/pep-0733/

@iritkatriel iritkatriel requested a review from a team as a code owner October 16, 2023 15:21
@cfbolz
Copy link
Contributor

cfbolz commented Oct 16, 2023

great draft!

In my opinion it's limiting to not list the CPython core developers as an explicit stakeholder group. CPython is a producer of the API, but it also consumes the API itself, and it has quite specific needs for what the API should look like that are different from everyone else. We should think about and list those specific needs and not leave them out as implicitly understood (I certainly don't completely understand them).

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

You can use PEP 732 733 (#3489 can take 732 as it was submitted first).

There is a very large number of authors (Victor also mentioned this on the PEP). I feel this means we should wait for all authors to confirm that they are OK with being listed as authors, since even if they contributed something to the problems repo, they may not necessarily want their name on this PEP.

An alternative could be to have only you (Irit) and maybe a few others as an author, and list everyone else in the Acknowledgments section.

@timfel
Copy link
Contributor

timfel commented Oct 16, 2023

great draft!

In my opinion it's limiting to not list the CPython core developers as an explicit stakeholder group. CPython is a producer of the API, but it also consumes the API itself, and it has quite specific needs for what the API should look like that are different from everyone else. We should think about and list those specific needs and not leave them out as implicitly understood (I certainly don't completely understand them).

@cfbolz there was some discussion around this here: capi-workgroup/problems#73

@timfel
Copy link
Contributor

timfel commented Oct 16, 2023

Thanks for the great write up @iritkatriel! For Stepan and me the Email addresses are Stepan Sindelar [email protected] and myself [email protected].

An alternative could be to have only you (Irit) and maybe a few others as an author, and list everyone else in the Acknowledgments section.

I would also think that's a good alternative if many others is too uncommon or not considered useful for other reasons.

@hugovk
Copy link
Member

hugovk commented Oct 16, 2023

I've updated the top post with the checklist, please go through and check off things as they're done.

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Thank you for putting this together!

peps/pep-9999.rst Outdated Show resolved Hide resolved
peps/pep-9999.rst Outdated Show resolved Hide resolved
peps/pep-9999.rst Outdated Show resolved Hide resolved
peps/pep-9999.rst Outdated Show resolved Hide resolved
peps/pep-9999.rst Outdated Show resolved Hide resolved
peps/pep-9999.rst Outdated Show resolved Hide resolved
peps/pep-9999.rst Outdated Show resolved Hide resolved
peps/pep-9999.rst Outdated Show resolved Hide resolved
peps/pep-9999.rst Outdated Show resolved Hide resolved
peps/pep-9999.rst Outdated Show resolved Hide resolved
@iritkatriel iritkatriel changed the title first draft of C API PEP as copied from the capi-workgroup repo PEP 733: An Evaluation of Python's Public C API Oct 16, 2023
@iritkatriel
Copy link
Member Author

Any author who has no more comments can go ahead and approve.
I'll make some efforts to chase those who don't approve, but if they are not successful I may have to remove them as authors.

peps/pep-0733.rst Outdated Show resolved Hide resolved
peps/pep-0733.rst Outdated Show resolved Hide resolved
Co-authored-by: Antoine Pitrou <[email protected]>
peps/pep-0733.rst Outdated Show resolved Hide resolved
peps/pep-0733.rst Outdated Show resolved Hide resolved
Copy link
Member

@JelleZijlstra JelleZijlstra 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 to me, with a number of minor suggestions.

peps/pep-0733.rst Outdated Show resolved Hide resolved
peps/pep-0733.rst Outdated Show resolved Hide resolved
peps/pep-0733.rst Show resolved Hide resolved
peps/pep-0733.rst Outdated Show resolved Hide resolved
peps/pep-0733.rst Outdated Show resolved Hide resolved
peps/pep-0733.rst Show resolved Hide resolved
@cfbolz
Copy link
Contributor

cfbolz commented Oct 17, 2023

@cfbolz there was some discussion around this here: capi-workgroup/problems#73

Thanks @timfel, I had indeed not seen that. I don't want to rehash that discussion (even if I ultimately don't completely agree), so you can ignore my comment.

Copy link
Member

@gpshead gpshead left a comment

Choose a reason for hiding this comment

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

Overall this is in great shape, thanks for writing it up! I left a few comments, though many of them are informational rather than things in need of change here.

peps/pep-0733.rst Outdated Show resolved Hide resolved
peps/pep-0733.rst Outdated Show resolved Hide resolved
peps/pep-0733.rst Show resolved Hide resolved
peps/pep-0733.rst Outdated Show resolved Hide resolved
peps/pep-0733.rst Outdated Show resolved Hide resolved
Copy link
Member

@markshannon markshannon 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.
A few comments, mostly about reference terminology.

peps/pep-0733.rst Outdated Show resolved Hide resolved
peps/pep-0733.rst Outdated Show resolved Hide resolved
peps/pep-0733.rst Show resolved Hide resolved
peps/pep-0733.rst Outdated Show resolved Hide resolved
peps/pep-0733.rst Outdated Show resolved Hide resolved
@malemburg
Copy link
Member

@malemburg @Thrameos @nascheme @markshannon @gpshead @serhiy-storchaka @woodruffw

This is getting close to being merged. If you won't review and approve in the next few days I will remove you from the author list. (You can still review later and asked to be put back on).

I'll review this weekend. I'm missing a few things from the "Common Actions" section, in particular accessing existing objects and calling methods/functions on them (rather than only creating them and then calling them). I'm also missing a statement to address the concern I raised about the general focus of the C API in terms of consistency, completeness and usefulness (see the discussion in capi-workgroup/problems#71), but more on that in the next two days.

Copy link

@Thrameos Thrameos left a comment

Choose a reason for hiding this comment

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

An items that I don't see captured that were relevant to this PEP were the lack of ability to construct derived classes for PyLong (which internally does hacks in which it constructs a PyLong and then copies it contents). This same hack has to be done to extend when with other language binds (such as JInt).

In general all types that one can extend from within Python should also be extendable within the CAPI with the same ease.

peps/pep-0733.rst Outdated Show resolved Hide resolved
@Thrameos
Copy link

@malemburg sorry for the slow review. My laptop died 3 months ago and was unable to be replaced at work due to supply chain issues until this weekend. And as I am not permitted to bring my cell phone into perform the two factor authentication, I was locked out for participating.

Copy link
Member

@malemburg malemburg left a comment

Choose a reason for hiding this comment

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

Thank you for working through all those "problems" issues in the capi-workgroup repo and creating this writeup, @iritkatriel. I guess this will not be only PEP that'll get written on the topic, but it's definitely a good start in the right direction.

I left a few comments on the PEP, which may help improve it some more.

peps/pep-0733.rst Outdated Show resolved Hide resolved
peps/pep-0733.rst Outdated Show resolved Hide resolved
peps/pep-0733.rst Show resolved Hide resolved
peps/pep-0733.rst Outdated Show resolved Hide resolved
peps/pep-0733.rst Outdated Show resolved Hide resolved
peps/pep-0733.rst Show resolved Hide resolved
peps/pep-0733.rst Outdated Show resolved Hide resolved
peps/pep-0733.rst Show resolved Hide resolved
peps/pep-0733.rst Show resolved Hide resolved
peps/pep-0733.rst Show resolved Hide resolved
peps/pep-0733.rst Outdated Show resolved Hide resolved
@Thrameos
Copy link

Thrameos commented Oct 30, 2023 via email

@iritkatriel
Copy link
Member Author

I'll give this another day or two and if no more comments I'll remove the (few) authors who have not approved and merge.

peps/pep-0733.rst Outdated Show resolved Hide resolved
Co-authored-by: Stepan Sindelar <[email protected]>
peps/pep-0733.rst Outdated Show resolved Hide resolved
peps/pep-0733.rst Show resolved Hide resolved
peps/pep-0733.rst Show resolved Hide resolved
peps/pep-0733.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

LGTM! I left one comment about stable_abi.toml being a noteworthy source of machine-readable information about the C API.

@iritkatriel
Copy link
Member Author

Thank you everyone!

@iritkatriel iritkatriel merged commit 02da28b into python:main Nov 1, 2023
6 checks passed
@vstinner
Copy link
Member

vstinner commented Nov 1, 2023

Congrats @iritkatriel for managing to drive the writing of this very important document!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-pep A new draft PEP submitted for initial review
Projects
None yet
Development

Successfully merging this pull request may close these issues.