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

decision: Should we add non-borrowed-ref public C APIs, if so, is there a naming convention? #201

Closed
3 tasks
gpshead opened this issue Jul 2, 2023 · 8 comments

Comments

@gpshead
Copy link
Member

gpshead commented Jul 2, 2023

Decide or delegate a single decider for each of the following things:

  1. Q1: Should we add non-borrowed-reference public C APIs where only borrowed-reference ones exist.
    1. in general? or only specific APIs (can we list categories we know we want to do this for if so?).
  2. Q2: if yes to Q1, is there a preferred naming convention to use for new public C APIs that return a strong reference when the earlier APIs these would be parallel versions of only returned a borrowed reference.

Example: PyDict_GetItem()...

Discussion contexts: python/cpython#106004 works on the above, has a PR, and links to our modern devguide advice and the broader C-API discussion. But that PR wound up going down a bike shedding rabbit hole over the name and some disagreement on whether or not we should do this despite some support. Understandably frustrating to @vstinner. We can resolve this.

c-api discussion around future API naming: capi-workgroup/problems#52

@erlend-aasland
Copy link

FTR, related APIs with the Ref suffix:

@encukou
Copy link
Member

encukou commented Jul 3, 2023

If yes to Q1, there's another follow-up question: what to do with the orphaned old API. PEP-387 says how to deprecate/remove it, but not if or when to do so.

@vstinner
Copy link
Member

I understood that the Steering Council meets once per week, every monday. @gpshead created this issue at July 2, so I understand that you had 2 meetings to discuss it, but apparently either you didn't visit this issue, or no decision was taken. Would it be possible to have transparency here? Did you visit the issue? Do you need more time to take a decision? What can be done? Since @gpshead asked to get SC to take a decision, I'm now stuck at waiting for the SC (decision).

Are you going to visit (discuss) this topic tonight?

@brettcannon
Copy link
Member

We didn't visit the issue, thus we need more time (PEP 703 is not exactly an easy PEP 😉).

@encukou
Copy link
Member

encukou commented Jul 20, 2023

Anecdotal evidence against solving a single problem (borrowed refs) en masse:

The new PyImport_AddModuleRef works like PyImport_AddModule, it just returns a strong reference.
But both have questionable behaviour: they either return an already imported module (from sys.modules), or a freshly created (empty) one -- and they don't indicate which of those cases happened.
We will want to yet another function to fix that wart, and if no one gets to it in time, the *Ref iteration will be hard to get rid of.

(I raised this on the issue, which is now closed: python/cpython#105922 (comment). I'm not sure what I should do next. And I don't really want to spend much time on one function I happened to notice.)

(Edit, after Victor's comment: to be clear, I don't care much about PyImport_AddModule itself, it's just one more weird old API you probably shouldn't use. This issue is about the process: if the borrowed issue needs to be solved, are core devs expected to spend time thinking about the API they're touching?)

@vstinner
Copy link
Member

We will want to yet another function to fix that wart, and if no one gets to it in time, the *Ref iteration will be hard to get rid of.

Is there any user request for such feature? Can't you get the behavior you want with existing functions, like checking if the name is in sys.modules?

One minor change would be to provide the information if the module was imported or created (add an optional parameter for that). You can open an issue if you want that.

@vstinner
Copy link
Member

vstinner commented Jul 21, 2023

It seems like the Steering Council is busy with the nogil PEP. I decided to move on and merge python/cpython#106005 since my PR was approved (well, it had 3 approvals, but it lost 2 in the bumpy discussion). If the Steering Council is in disagreement, the function can be reverted before Python 3.13 final: we have plently of time to consider such revert.

@gpshead
Copy link
Member Author

gpshead commented Jul 25, 2023

The steering council chatted about non-borrowed-ref and naming conventions today. We want to delegate this to the C API working group to come back with a broader recommendation. @iritkatriel has put together the initial draft of https://github.com/capi-workgroup/problems/blob/main/capi_problems.rst for example.

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

No branches or pull requests

5 participants