capi-workgroup / decisions

Discussion and voting on specific issues
8 stars 1 forks source link

Any WG thoughts on the C API changes in PEP-667 (consistent view of namespaces)? #24

Closed gpshead closed 7 months ago

gpshead commented 7 months ago

The steering council has reviewed PEP-677 Consistent view of namespaces and is likely to accept it. Before we do so, the one thing we'd like before we do that is to know if the C API WG has any issues to be addressed on the C API side of the PEP.

As currently written it contains a set of new C APIs that return new references, and conservative timeline deprecations of existing borrow-semantics C APIs with backwards compatibility rational and suggested replacement C code.

gvanrossum commented 7 months ago

(Not yet speaking for the C API WG.)

The proposed C API changes look good to me in terms of naming.

There is a small issue with the proposed deprecation schedule: According to the docs the deprecated functions PyEval_GetLocals, PyEval_GetGlobals and PyEval_GetBuiltins are all part of the Stable ABI (and presumably of the Limited API?). (The other three, FastToLocals* and LocalsToFast are undocumented.)

We may have to support dummy versions of these for as long as Stable ABI 3.12 is supported, which may be longer than anticipated by the PEP. Eventually, if there is no other solution, the dummies for Locals and Globals could always return NULL (as the docs clearly state that these may return NULL). It looks like the dummy for Builtins will have to continue to return a dict. I don't foresee problems with this, we just need to be careful here.

gpshead commented 7 months ago

Over on the discuss thread it sounded like the ultimate removal of the old borrowing APIs would be fine to take out of the PEP to leave for a future decision.

gvanrossum commented 7 months ago

Okay, that’s even better.

encukou commented 7 months ago

They return NULL with an exception set. The new functions should ensure and explicitly document that exception part.

Given the tricky situation with stable ABI, we might want to emit a runtime warning for these functions, visible by people who don't recompile. I'd prefer not adding the runtime warning right now: we're quite late in 3.13 for something that affects projects that opted in long-term stability. (The regular build-time warning can go in now of course.)

I assume the locals cache will be on the full-fledged PyFrameObject, not the lightweight _PyInterpreterFrame; it seems to me that an extra pointer there isn't that much of a burden. As for creating a reference loop, it should be should be no problem at all if only deprecated legacy API does it. If it was up to me, I'd add runtime warning in 3.18 (when all supported versions have the new API, so users can switch to it), and push the removal to 3.20-3.23 (or “abi4” if that comes sooner).

iritkatriel commented 7 months ago

PyEval_GetFrameGlobals() and PyEval_GetFrameBuiltins() aren't really related to this PEP, it seems they are added for consistency with PyEval_GetFrameLocals(). But then we remain with PyEval_GetFrame() as the odd function in the reflection API that returns a borrowed ref. Should this be replaced as well?

gvanrossum commented 7 months ago

They return NULL with an exception set.

That isn't documented though, and according to the source code, PyEval_GetLocals sets an exception, but PyEval_GetGlobals does not. (PyEval_GetBuiltins never returns NULL.)

The new functions should ensure and explicitly document that exception part.

Yes.

Given the tricky situation with stable ABI, we might want to emit a runtime warning for these functions, visible by people who don't recompile. I'd prefer not adding the runtime warning right now: we're quite late in 3.13 for something that affects projects that opted in long-term stability. (The regular build-time warning can go in now of course.)

Sounds good.

I assume the locals cache will be on the full-fledged PyFrameObject, not the lightweight _PyInterpreterFrame; it seems to me that an extra pointer there isn't that much of a burden. As for creating a reference loop, it should be should be no problem at all if only deprecated legacy API does it. If it was up to me, I'd add runtime warning in 3.18 (when all supported versions have the new API, so users can switch to it), and push the removal to 3.20-3.23 (or “abi4” if that comes sooner).

I don't know the answer to this. @markshannon

encukou commented 7 months ago

then we remain with PyEval_GetFrame() as the odd function in the reflection API that returns a borrowed ref. Should this be replaced as well?

Yes.


Oh, I just noticed the SC already asked to take the planned removal dates out of the PEP.

I think we're discussing details that should not block the PEP, does everyone agree?

The PEP is fine; some details of the API surface might need a regular C-API WG discussion

gvanrossum commented 7 months ago

Waiting only for @vstinner now.

encukou commented 7 months ago

The vote is in. Keeping this open for the additional details.

ncoghlan commented 7 months ago

Very cool to see this moving forward.

It would be good if the PEP was explicit regarding the documentation and language specification impact of the proposed changes.

It should be possible to adopt the relevant sections from PEP 558 wholesale (potentially just by reference rather than by making PEP 667 itself longer), as the Python level semantics haven't changed since the two PEPs converged in that regard back in December 2021 (the PEP 667 changes since then have just been improvements to the ergonomics of the updated C API and introducing a backwards compatibility period for the legacy C API).

ncoghlan commented 7 months ago

PR to withdraw PEP 558 in favour of PEP 667: https://github.com/python/peps/pull/3762

encukou commented 7 months ago

It would be good if the PEP was explicit regarding the documentation and language specification impact of the proposed changes.

Yes, but that doesn't really touch the C API surface, so it's something to take to Discourse (and ultimately the SC). As you did :)

gpshead commented 7 months ago

closing per https://discuss.python.org/t/pep-667-consistent-views-of-namespaces/46631/15 :) thanks!