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

Support metaclass parameter in HPyType_FromSpec #296

Closed
steve-s opened this issue Feb 22, 2022 · 8 comments · Fixed by #335
Closed

Support metaclass parameter in HPyType_FromSpec #296

steve-s opened this issue Feb 22, 2022 · 8 comments · Fixed by #335

Comments

@steve-s
Copy link
Contributor

steve-s commented Feb 22, 2022

This necessary for the numpy port. Numpy defines a metaclass of dtype class, with its own C struct, the gist is:

NPY_NO_EXPORT PyTypeObject PyArrayDTypeMeta_Type = {
    PyVarObject_HEAD_INIT(NULL, 0)
    .tp_name = "numpy._DTypeMeta",
    .tp_basicsize = sizeof(PyArray_DTypeMeta),
    // ...
}

typedef struct {
    PyHeapTypeObject super;
    // Numpy specific data:
    PyArray_Descr *singleton;
    int type_num;
    // ...
} PyArray_DTypeMeta;

NPY_NO_EXPORT PyArray_DTypeMeta PyArrayDescr_Type = {
    {{
        /* NULL represents `type`, this is set to DTypeMeta at import time */
        PyVarObject_HEAD_INIT(NULL, 0)
        .tp_name = "numpy.dtype",
        .tp_basicsize = sizeof(PyArray_Descr),
        // ...
    },},
    // values for the numpy specific fields:
    .type_num = -1,
    .singleton = NULL,
    // ...
};

// ... in module init:
Py_SET_TYPE(&PyArrayDescr_Type, &PyArrayDTypeMeta_Type);
if (PyType_Ready(&PyArrayDescr_Type) < 0) {
    goto err;
}

In order to support this, HPyType_FromSpec should take metaclass as an additional parameter. There is even a TODO for this in HPy. The HPy side of things seems to be clear.

However, the issue is that PyType_FromSpecWithBases does not support metaclasses (https://bugs.python.org/issue15870). I think that we will have to copy the logic of PyType_FromSpecWithBases from CPython and delegate back to CPython only at the point where the original code calls PyType_Ready.

The problematic bit is that PyType_FromSpecWithBases calls PyType_GenericAlloc(&PyType_Type, nmembers), it should call tp_alloc of the metaclass as suggested in https://bugs.python.org/issue15870.

@seberg
Copy link

seberg commented Feb 22, 2022

Also xref https://bugs.python.org/issue45383, I think CPython is open to adding API to make this work reasonable without static types (I was confused about Python 3.11 support, I guess there is still some chance we can manage that).

@steve-s
Copy link
Contributor Author

steve-s commented Feb 23, 2022

I've hacked CPython to support metaclasses in PyType_FromSpecWithBases and added a support for metaclasses also to HPy using this API. The plot thickens as there is more to this in the context of HPy:

  • pure HPy types cannot inherit from other builtins than object, because the way we compute the struct size in ctx_Type_FromSpec is ~ sizeof(PyObject) + spec->basicsize. Maybe HPy could use base->tp_basicsize + spect->basicsize, but it will be tricky with alignment...
  • the metaclass needs to inherit from PyType_Type, i.e., its first struct field should be PyHeapType super => we currently cannot do that for HPy pure types
  • Support metaclass parameter in HPyType_FromSpec #296 (comment)

@seberg
Copy link

seberg commented Feb 23, 2022

I don't think I have any input other than linking that CPython issue :). I hope you can figure it out, but if there is something I may have insight on, let me know.
It might be quite a bit of churn to find a different solution for NumPy, but I am not generally opposed to changing NumPy (I am not sure we can do without metaclasses, but maybe we can solve the additional storage problem in another – fast enough – way). I just never really figured out what a half decent alternative would be, and while CPython support is a shady it seemed more like a bug then intent.

@steve-s
Copy link
Contributor Author

steve-s commented May 24, 2022

Some more notes regarding the issue described in my previous comment:

The issue is that:

  1. Types without custom metatype take a fast-path in mro_invoke, which omits a check in extra_ivars (called from mro_internal->mro_invoke->mro_check->solid_base). The check is this:
    size_t t_size = type->tp_basicsize;
    size_t b_size = base->tp_basicsize;

    assert(t_size >= b_size); /* Else type smaller than base! */
  1. Specifying basicsize=0 normally instructs PyType_Ready (or PyType_FromSpec, which internally delegates to PyType_Ready) to set the basicsize to the basicsize of the base type. This would fix the assertion in 1), but 1) gets executed before 2) inside PyType_Ready.

git with an example using PyType_Ready

Practical implication of this is that if one specifies metatype one also has to set basicsize to at least sizeof(PyObject) if there is no extra space needed. I.e., setting basicsize=0, which normally tells Python to set the basicsize automatically, does not work if the type has a custom metatype.

@steve-s
Copy link
Contributor Author

steve-s commented May 24, 2022

Support for metatypes in PyType_FromSpec in CPython: python/cpython#93012

@hodgestar
Copy link
Contributor

Could we add the metaclass to the HPyType_Spec rather than adding it as an extra parameter to HPyType_FromSpec?

@encukou
Copy link

encukou commented May 24, 2022

Could we add the metaclass to the HPyType_Spec rather than adding it as an extra parameter to HPyType_FromSpec?

Hi! De-facto maintainer of this API in CPython here. I assume we want our APIs to be similar, if possible.

The reason for the argument in CPython is the same as the bases in PyType_FromSpecWithBases:

  • if the metaclass is a heap type, it can't be part of a static spec
  • even for static types, on Windows/MSVC a pointer to a variable loaded from another DLL (e.g. NumPy's metaclass used in a different extension) can't be part of a static spec either.

Another reason is that CPython's PyType_Spec can't change, and its extension mechanism -- slots -- takes function pointers rather than data pointers. (Some slots are historically data anyway, but I'd rather not add more.)

@hodgestar
Copy link
Contributor

* if the metaclass is a heap type, it can't be part of a static spec

* even for static types, on Windows/MSVC a pointer to a variable loaded from another DLL (e.g. NumPy's metaclass used in a different extension) can't be part of a static spec either.

Those are two very good reasons. Thanks @encukou!

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 a pull request may close this issue.

4 participants