GeezerVison is showing github.com — MRO: Behavior change from 3.10 to 3.11: multiple inheritance issue with C-API (pybind11) · Issue #92678 · python/cpython
MRO: Behavior change from 3.10 to 3.11: multiple inheritance issue with C-API (pybind11) #92678Description
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
- CPython versions tested on: 3.11
- Operating system and architecture: Ubuntu-latest
Linked PRs
ActivityJelleZijlstra commented on May 11, 2022 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? Skylion007 commented on May 11, 2022 I have looked through the commit list, but was unable to find any suspicious commits. @henryiii @rwgk Any thoughts on which commit might breaking out test? Skylion007 commented on May 11, 2022 vstinner commented on May 11, 2022
PyTypeReady failed
Do you get an exception? What is the exception? vstinner commented on May 11, 2022
@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 Gokaslan vstinner commented on May 11, 2022 extra_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. vstinner commented on May 11, 2022
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. Skylion007 commented on May 12, 2022 @vstinner Hmm, any idea how to ensure the ivars is happy then? vstinner commented on May 12, 2022
@vstinner Hmm, any idea how to ensure the ivars is happy then?
I have no idea. It would help to have a simpler reproducer than pybind11 test suite. Skylion007 commented on May 14, 2022 @JelleZijlstra I have a failing portion of PyBind11 test suite now. Any ideas how to debug this further? JelleZijlstra commented on May 16, 2022 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? henryiii commented on May 16, 2022 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__, and Vanilla does not. Both DictMix's should. Reacted by Aaron Gokaslan vstinner commented on May 17, 2022 @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? rwgk commented on Aug 4, 2022
@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_DICT changes from pybind11 (based on pybind11 master @ pybind/pybind11@ba5ccd8), diff below.
With that:
- "latest": all unit pybind11 unit tests pass.
- "back": the
PyType_Ready error is back, see below.
For completeness:
- WithOUT the diff below, both "latest" and "back" pass the pybind11 unit tests.
- I'm seeing a
test_embed ModuleNotFoundError for 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. Smith Reacted by Gregory P. Smith pablogsal commented on Aug 4, 2022 Thanks a lot for checking!
I'm closing this issue, let's reopen if we discover that we missed anything vstinner commented on Aug 5, 2022
"latest": all unit pybind11 unit tests pass.
Yeah! That's cool! Thanks for fixing pybind11 ;-) rwgk commented on Aug 5, 2022
"latest": all unit pybind11 unit tests pass.
Yeah! That's cool! Thanks for fixing pybind11 ;-)
To clarify (especially for people looking here later):
Reacted by Gregory P. Smith added 2 commits that reference this issue on Aug 9, 2022GH-92678: Document that you shouldn't be doing your own dictionary of… GH-92678: Document that you shouldn't be doing your own dictionary of… vstinner commented on Aug 17, 2022 See also issue #96046 which might be related.
MetadataMetadataStatusShow more project fields
DevelopmentNo branches or pull requestsIssue actions
|