Repository navigation
MRO: Behavior change from 3.10 to 3.11: multiple inheritance issue with C-API (pybind11) #92678
Description
Activity
Do you have a minimal example that reproduces the issue, or failing that an example of a piece of pybind11 code demonstrating the problem?
You say that the issue first appeared in 3.11a3. Are you able to bisect further to a specific commit?
@JelleZijlstra The failing test can be found here: pybind/pybind11#3923
PyTypeReady failed
Do you get an exception? What is the exception?
@JelleZijlstra The failing test can be found here: pybind/pybind11#3923
Ah, the "Upstream / 🐍 3.11 dev • ubuntu-latest • x64 (pull_request)" failed with:
ImportError while loading conftest '/home/runner/work/pybind11/pybind11/tests/conftest.py'. conftest.py:16: in <module> import pybind11_tests # noqa: F401 E ImportError: VanillaDictMix1: PyType_Ready failed (TypeError: mro() returned base with unsuitable layout ('pybind11_tests.multiple_inheritance.WithDict'))!This error message comes from the mro_check() function of typeobject.c:
if (!PyType_IsSubtype(solid, solid_base(base))) { PyErr_Format( PyExc_TypeError, "mro() returned base with unsuitable layout ('%.500s')", base->tp_name); return -1; }with:
static PyTypeObject * solid_base(PyTypeObject *type) { PyTypeObject *base; if (type->tp_base) base = solid_base(type->tp_base); else base = &PyBaseObject_Type; if (extra_ivars(type, base)) return type; else return base; }Reacted by Aaron Gokaslanextra_ivars() used by solid_base() is different in Python 3.11, the following code path was removed in 3.11:
if (type->tp_dictoffset && base->tp_dictoffset == 0 && type->tp_dictoffset + sizeof(PyObject *) == t_size && type->tp_flags & Py_TPFLAGS_HEAPTYPE) t_size -= sizeof(PyObject *);Python 3.11 changes:
I don't understand well the purpose of the extra_ivars(type, base) function.
this issue occurs when the first base does not have dynamic attributes enabled, but another base class does.
pybind11 code:
/// Give instances of this type a `__dict__` and opt into garbage collection. inline void enable_dynamic_attributes(PyHeapTypeObject *heap_type) { auto *type = &heap_type->ht_type; type->tp_flags |= Py_TPFLAGS_HAVE_GC; type->tp_dictoffset = type->tp_basicsize; // place dict at the end type->tp_basicsize += (ssize_t) sizeof(PyObject *); // and allocate enough space for it type->tp_traverse = pybind11_traverse; type->tp_clear = pybind11_clear; static PyGetSetDef getset[] = { {const_cast<char *>("__dict__"), pybind11_get_dict, pybind11_set_dict, nullptr, nullptr}, {nullptr, nullptr, nullptr, nullptr, nullptr}}; type->tp_getset = getset; }It sets
type->tp_dictoffset: it may change extra_ivars() result.@vstinner Hmm, any idea how to ensure the ivars is happy then?
@JelleZijlstra I have a failing portion of PyBind11 test suite now. Any ideas how to debug this further?
Sorry, I am not familiar with this area of the code so I don't think I can be of more help than Victor. As Victor said, it would be helpful to have a self-contained example that doesn't rely on pybind11.
Also, would be good to know how big the impact is on pybind11: is this something a lot of users are likely to run into, or just an obscure edge case that your test suite happens to cover?
Here's a pybind11 reproducer:
#include <pybind11/pybind11.h> namespace py = pybind11; struct Vanilla {}; struct WithDict {}; struct VanillaDictMix2 : WithDict, Vanilla {}; struct VanillaDictMix1 : Vanilla, WithDict {}; PYBIND11_MODULE(example, m) { py::class_<Vanilla>(m, "Vanilla").def(py::init<>()); py::class_<WithDict>(m, "WithDict", py::dynamic_attr()).def(py::init<>()); py::class_<VanillaDictMix2, WithDict, Vanilla>(m, "VanillaDictMix2").def(py::init<>()); // OK py::class_<VanillaDictMix1, Vanilla, WithDict>(m, "VanillaDictMix1").def(py::init<>()); // Broken in 3.11 }
(Names chosen to match the test suite, which is why 2 comes before 1 - 2 works, 1 doesn't)
Compile and run (fish syntax):
$ clang++ -Wall -shared -std=c++14 -undefined dynamic_lookup (pipx run --python=~/.pyenv/versions/3.11.0b1/bin/python pybind11 --includes | string split " ") -g example.cpp -o example(~/.pyenv/versions/3.11.0b1/bin/python3-config --extension-suffix) $ ~/.pyenv/versions/3.11.0b1/bin/python -c "import example" Traceback (most recent call last): File "<string>", line 1, in <module> ImportError: VanillaDictMix1: PyType_Ready failed (TypeError: mro() returned base with unsuitable layout ('example.WithDict'))!
Note that WithDict has a
__dict__, andVanilladoes not. Both DictMix's should.Reacted by Aaron Gokaslan@markshannon: You made the two commits changing extra_ivars(). Do you have any idea why Python 3.11 behaves differently? Is it a deliberate choice?
63 remaining items
@Skylion007 could you confirm that #95596 is enough for pybind11 to work with 3.11 before the better API is provided in 3.12?
TL;DR: Yes, we're good (pybind11).
Details:
I built Python 3.11 from scratch (configure; make install) for two versions:
- "latest" = 5ac3d0f (happened to be HEAD of 3.11 when I ran
git pull) - "back" = d8df7e0 (just before
GH-92678: Fix tp_dictoffset inheritance. (GH-95596) (GH-95604))
I also backed out the
Py_TPFLAGS_MANAGED_DICTchanges from pybind11 (based on pybind11 master @ pybind/pybind11@ba5ccd8), diff below.With that:
- "latest": all unit pybind11 unit tests pass.
- "back": the
PyType_Readyerror is back, see below.
For completeness:
- WithOUT the diff below, both "latest" and "back" pass the pybind11 unit tests.
- I'm seeing a
test_embedModuleNotFoundErrorfor both "latest" and "back", with and without the diff below, but that's clearly unrelated to this MRO issue. Need to drill down.
diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h index db7cd8ef..65e223a3 100644 --- a/include/pybind11/attr.h +++ b/include/pybind11/attr.h @@ -345,11 +345,7 @@ struct type_record { bases.append((PyObject *) base_info->type); -#if PY_VERSION_HEX < 0x030B0000 dynamic_attr |= base_info->type->tp_dictoffset != 0; -#else - dynamic_attr |= (base_info->type->tp_flags & Py_TPFLAGS_MANAGED_DICT) != 0; -#endif if (caster) { base_info->implicit_casts.emplace_back(type, caster); diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index 42720f84..3db8429b 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -549,12 +549,8 @@ extern "C" inline int pybind11_clear(PyObject *self) { inline void enable_dynamic_attributes(PyHeapTypeObject *heap_type) { auto *type = &heap_type->ht_type; type->tp_flags |= Py_TPFLAGS_HAVE_GC; -#if PY_VERSION_HEX < 0x030B0000 type->tp_dictoffset = type->tp_basicsize; // place dict at the end type->tp_basicsize += (ssize_t) sizeof(PyObject *); // and allocate enough space for it -#else - type->tp_flags |= Py_TPFLAGS_MANAGED_DICT; -#endif type->tp_traverse = pybind11_traverse; type->tp_clear = pybind11_clear;
With "back" version of 3.11:
Running tests in directory "/usr/local/google/home/rwgk/forked/pybind11/tests": ImportError while loading conftest '/usr/local/google/home/rwgk/forked/pybind11/tests/conftest.py'. conftest.py:16: in <module> import pybind11_tests E ImportError: VanillaDictMix1: PyType_Ready failed: TypeError: mro() returned base with unsuitable layout ('pybind11_tests.multiple_inheritance.WithDict')Reacted by Gregory P. SmithReacted by Gregory P. Smith- "latest" = 5ac3d0f (happened to be HEAD of 3.11 when I ran
Thanks a lot for checking!
I'm closing this issue, let's reopen if we discover that we missed anything
- added a commit that references this issue
on Aug 4, 2022 "latest": all unit pybind11 unit tests pass.
Yeah! That's cool! Thanks for fixing pybind11 ;-)
"latest": all unit pybind11 unit tests pass.
Yeah! That's cool! Thanks for fixing pybind11 ;-)
To clarify (especially for people looking here later):
- I don't have a full understanding, I only jumped in to help with the last minute testing.
- The diff under MRO: Behavior change from 3.10 to 3.11: multiple inheritance issue with C-API (pybind11) #92678 (comment) is not even in a PR yet (but we will take care of it).
- @Skylion007 is driving this in general: question for him, did we actually make a net change after applying that diff?
- I believe we still have to make changes for 3.12 compatibility, with fairly high priority, so that we don't have a last minute situation again next year. Is that correct @Skylion007?
Reacted by Gregory P. Smith- added a commit that references this issue
on Aug 5, 2022 - added a commit that references this issue
on Aug 17, 2022
Bug report
We have started testing the 3.11 beta branch in pybind11 and have found an issue where a class constructed using the C-API that inherits from two base classes that have different tpget_set implementations. Interestingly, this issue occurs when the first base does not have dynamic attributes enabled, but another base class does. I tried making the child class also have dynamic attributes (and therefore a larger size to store the dict etc.), but I still get a PyTypeReady failed. We've been encountering this issue since alpha 3, but I have not found any issues on BPO of other people having similar issues. I am wondering what changed and how we can fix our API usage to allow for full support of Python 3.11 as it enters beta.
I suspect it's something very subtle with how we are constructing our Python types, but there was nothing in the 3.11 migration guide that flags this issue. Any thoughts on how to fix issue? Is this known behavior or a bug? Or is it something that should be added to the migration guide?
@vstinner I know you are very familiar with the C-API and helped us deal with some of the other API changes, any thoughts?
Here is the failing test: pybind/pybind11#3923
Linked PRs