gh-107073: Make PyObject_VisitManagedDict() public #108763
Conversation
|
According to NEWS when these were added, they were unstable. Presumably because they mess with internals that might change. Then shouldn't they be named PyUnstable_...? Or, @markshannon have you changed your mind about their status? |
|
It's too late for 3.12 right? |
That's a question for @Yhg1s, Python 3.12 release manager. This change adds 2 functions to the public C API. I would prefer to fix the API in Python 3.12, but if it's too late, it's too late :-) |
|
If it's too late for Python 3.12, that's ok. The private functions can be used. But I will update my PR to document the change in What's new in Python 3.13 instead ;-) Just tell what you think about this one @Yhg1s ;-) |
|
No, I think it's too late for 3.12. |
What do you mean when you say that Are you referring to the What's New in Python 3.12 doc? The "does not fully support multiple inheritance" part is scary. The |
aa4eeb5 to
a13d2d5
Compare
Ok, the release manager spoke, I re-targeted my PR to Python 3.13 :-) It's ok to use the private API in Python 3.12. |
It's literally what the NEWS entry for 3.12a1 says: |
So @gvanrossum is worried by the Changelog entry adding these 2 APIs as "unstable". @markshannon: You added these functions 1 year ago in Python 3.12 as commit de388c0. Do you still consider these APIs as "unstable"? Are you ok to make them public? Well, see the issue and previous comments for the rationale ;-) |
|
For me, it's unfortunate that Python 3.12 broke type inheritance with the new "managed dict" change, but doesn't offer a public API to switch to it, while the documentation strongly advices using this new API. That's why I asked to make this API public in Python 3.12. But this issue is waiting for 1 month, and Python 3.12 release is following its process towards the final release in one month. It's unclear to me if inheritance was broken in Python 3.11 or 3.12. I recall that pybind11 got a regression related to that, and as I wrote. It now uses a the private Well, even if I dislike the implementation, it's good to know that there is a way to implement PyObject_VisitManagedDict() and PyObject_ClearManagedDict() on Python 3.11. It also means that there is a need for public API for that, since pybind11 already uses it :-) |
If we decide adding So I don't have to be pushy with our busy Python 3.12 release manager to get it merged after beta1 (after rc1!!!) ;-) |
|
@dean0x7d @wjakob: Hi, you modified pybin11 to travere and clear the dictionary in pybind11 (commit) with such code: I see that pybind11 uses Py_TPFLAGS_MANAGED_DICT on Python 3.11. Would it help you to have public function PyObject_VisitManagedDict() and PyObject_ClearManagedDict()? Do you have an opinion on the API? Is it good? |
|
The proposal looks fine to me. A tangent: a more general solution would be to expose the dictionary in a public (perhaps even stable) API interface, but not like the existing mechanism. /// dynamic_attr: Allow the garbage collector to traverse the internal instance `__dict__`.
extern "C" inline int pybind11_traverse(PyObject *self, visitproc visit, void *arg) {
PyObject *dict = PyObject_GetDict(self);
Py_VISIT(dict);
return 0;
}
/// dynamic_attr: break a reference cycle involving this object
extern "C" inline int pybind11_clear(PyObject *self) {
PyObject *dict = PyObject_GetDict(self);
if (dict) {
PyDict_Clear(dict);
}
return 0;
} |
|
In short, you ask to make /* Helper to get a pointer to an object's __dict__ slot, if any.
* Creates the dict from inline attributes if necessary.
* Does not set an exception.
*
* Note that the tp_dictoffset docs used to recommend this function,
* so it should be treated as part of the public API.
*/ |
|
@vstinner Ah, interesting—I did not know about this function. The fact that it creates the dictionary if non-existent makes it inappropriate for what I had suggested (for use in garbage collection hooks). |
a13d2d5 to
00af801
Compare
|
Oops, sorry for the noise :-( I tried a Git tool which messed up my PRs. I fixed my PR. |
Can we discuss exposing _PyObject_GetDict() separetly? Exposing just PyObject_VisitManagedDict() and PyObject_ClearManagedDict() already looks controversial :-) |
|
@markshannon @gvanrossum @encukou: this issue is languishing since July 22. I plan to merge my PR next Friday. If you need more time for review, please speak up. Once this PR will be merged, I consider adding a compatible layer for Python 3.11 and 3.12 in pythoncapi-compat. Later, I consider making _PyObject_GetDict() public, but apparently this function needs to be adjusted before being made public. |
|
I understand that exposing these functions would be helpful to fix garbage collection of managed dictionaries in extension libraries like pybind11. But unless there is a big rush to push this out for a release (which I don't think is the case here, right?), isn't it better to refine and have a perfect API? In this case, it somehow seems suboptimal to expose two highly specific public functions as public API if they are likely to be subsumed by a more general one later one. |
I don't think that anyone has a plan to replace these two convenient functions with something like _PyObject_GetDict(). _PyObject_GetDict() cannot be used directly in traverse and clear functions, IMO it's less convenient to use. See my Doc/c-api/typeobj.rst changes in this PR: PyObject_VisitManagedDict() and PyObject_ClearManagedDict() are convenient to use. For me, _PyObject_GetDict() fits other use cases, no? |
|
Both Mark and I are on vacation this week. Please wait. |
So, any update on these functions? Should they be exposed? Is it the new official way to have a dict in an object? Or should we burry them deeper in the internal C API? |
|
Pinging @markshannon |
|
I've commented on the issue on my thinking as to why we should add these to the public API. |
| @@ -1368,6 +1371,23 @@ and :c:data:`PyType_Type` effectively act as defaults.) | |||
| debugging aid you may want to visit it anyway just so the :mod:`gc` module's | |||
| :func:`~gc.get_referents` function will include it. | |||
|
|
|||
| Heap types (:c:macro:`Py_TPFLAGS_HEAPTYPE`) must visit their type with:: | |||
There was a problem hiding this comment.
This doesn't seem relevant PyObject_VisitManagedDict, but no harm in adding it.
There was a problem hiding this comment.
This doc was forgotten since Python 3.8, it requires to fix an important bug.
|
Thanks for the review @gvanrossum and @markshannon, I really wasn't sure about this one. Now we should visit the _PyObject_GetDict() case, hum. |
|
Oh, @markshannon replied that we should not expose it: #107073 (comment) @wjakob: If you want, you can now use the pythoncapi-project to use PyObject_VisitManagedDict() and PyObject_ClearManagedDict() on Python 3.11 and Python 3.12, implemented using _PyObject_GetDictPtr(), see: python/pythoncapi-compat@a594354 |
|
@vstinner — those two functions would be usable for pybind11 (@rwgk, @henryiii). For nanobind, I'm still using old-style dictionaries built into each instance :-/. I don't this functionality is sufficiently exposed through limited API / stable ABI interfaces to be able to switch to managed dicts. Is it the plan that stable ABI interfaces can also use them? |
If you want a function to be exposed in the limited C API, please open a new issue for that. |
`PyObject_VisitManagedDict` and `PyObject_ClearManagedDict` were made public in python/cpython#108763. Both are available from `pythoncapi_compat.h`.
Make PyObject_VisitManagedDict() and PyObject_ClearManagedDict() functions public in Python 3.13 C API. * Rename _PyObject_VisitManagedDict() to PyObject_VisitManagedDict(). * Rename _PyObject_ClearManagedDict() to PyObject_ClearManagedDict(). * Document these functions.
Make PyObject_VisitManagedDict() and PyObject_ClearManagedDict() functions public in Python 3.13 C API.
EDIT: Target Python 3.13, not Python 3.12.
📚 Documentation preview 📚: https://cpython-previews--108763.org.readthedocs.build/