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

Enable specifyingpy::metaclass(PyType_Type) without a C/C++ cast. #5015

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Feb 1, 2024

Description

py::metaclass(PyType_Type) is useful to resolve these problems:

This PR makes it possible to simply specify

py::metaclass(PyType_Type) instead of

py::metaclass((PyObject *) &PyType_Type).

This PR also adds comments regarding abc.ABCMeta compatibility.

Note for completeness: the added tests are a partial backport from google/pywrapcc#30094.

Suggested changelog entry:

It is now possible to specify ``py::metaclass(PyType_Type)`` without a C/C++ cast.

@wjakob
Copy link
Member

wjakob commented Feb 1, 2024

This seems quite benign as a change. But I think it could be useful to add somewhere in the docs that types can be specified this way, and that the basic PyType_Type metaclass has some friction points but offers compatibility to ABCMeta.

@rwgk
Copy link
Collaborator Author

rwgk commented Feb 1, 2024

But I think it could be useful to add somewhere in the docs that types can be specified this way, and that the basic PyType_Type metaclass has some friction points but offers compatibility to ABCMeta.

Done: cf230fb

@wjakob
Copy link
Member

wjakob commented Feb 2, 2024

Is "and can also lead to other surprising issues" appropriate given that is text that users will read in the documentation? It seems a bit like fear-mongering that pybind11 is doing something bad by default. Whereas the default metaclass works perfectly fine except in fairly narrow niche cases that you seem to be running into in the Google ecosystem (nobody had ever brought up the issue you raised before). And there are good technical reasons why it this custom metaclass is used.

I think it's also worth pointing out in the docstring that if compatibility with ABCMeta is desired via the proposed workaround, then this will interfere with the behavior of static properties bound on those classes.

@rwgk
Copy link
Collaborator Author

rwgk commented Feb 2, 2024

Is "and can also lead to other surprising issues" appropriate given that is text that users will read in the documentation?

I replaced "issues" (which I understood as something that simply needs attention) with "side effects" (the evidence is in the PR description; I'm not mentioning #1359 because I don't understand it enough, but there is a good chance that it is also related).

I think it's also worth pointing out in the docstring that if compatibility with ABCMeta is desired via the proposed workaround, then this will interfere with the behavior of static properties bound on those classes.

I did already (although I had accidentally left out one /; fixed now):

    /// Example usage: py::metaclass(PyType_Type)
    /// PyType_Type is recommended if compatibility with abc.ABCMeta is required.
    /// The only potential downside is that static properties behave differently
    /// (see pybind/pybind11#679 for background).

@rwgk rwgk marked this pull request as draft February 2, 2024 16:38
@rwgk
Copy link
Collaborator Author

rwgk commented Feb 2, 2024

I just converted this PR back to Draft mode because we (@Yhg1s and myself) just decided that PyCLIF-pybind11 will use the default pybind11 metaclass, and that we'll change client code to make that possible (sprawling changes around the Google code base). How exactly I'm still figuring out. The main idea is to use a metaclass that has both ABCMeta and the default pybind11 metaclass as parents. Holding off with this PR because I want to mention the final solution as an alternative to using py::metaclass(PyType_Type).

@rwgk
Copy link
Collaborator Author

rwgk commented Feb 2, 2024

It seems a bit like ... pybind11 is doing something bad by default.

The default pybind11 metaclass pretends that it lives in module pybind11_builtins and has the name pybind11_type.

A module with the name pybind11_builtins does not actually exist.

I wouldn't call that bad, but it does not look right either, especially because it is why MATLAB is falling over:

We need do consider: there may be multiple ABI-incompatible metaclasses in a given process. We cannot simply create a pybind11_builtins module and put one reference to a pybind11_type there.

Or could we? — By ensuring that pybind11_type does not use C++ (only C)? EDIT: See next comment.

Or could/should we add the ABI key to the name? (e.g. pybind11_type_v4_gcc_libstdcpp_cxxabi1018)

@rwgk
Copy link
Collaborator Author

rwgk commented Feb 2, 2024

Or could we? — By ensuring that pybind11_type does not use C++ (only C)?

Pretty certain: we cannot.

Because pybind11_meta_setattro() and pybind11_meta_dealloc() both call get_internals().

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

Successfully merging this pull request may close these issues.

None yet

2 participants