Compiled Python extension does not work with Python 3.12

[musicinmybrain]

What version of protobuf and what language are you using?

Version: 3.19.6 – but I searched the source code and verified that the current release (22.1) and main should also be affected
Language: Python

What operating system (Linux, Windows, ...) and version?

Fedora Linux Rawhide/39, plus experimental Python 3.12: https://copr.fedorainfracloud.org/coprs/g/python/python3.12/

What runtime / compiler are you using (e.g., python version or gcc version)

Python 3.12.0a5, GCC 13.0.1

What did you do?
Steps to reproduce the behavior:

1. Build the C++ version of the Python protobuf extension.
2. from google.protobuf.pyext import _message

This import occurred in the generated proto bindings in googleapis-common-protos; see downstream bug https://bugzilla.redhat.com/show_bug.cgi?id=2176158.

What did you expect to see

Successful import with no output.

What did you see instead?

[…]
  File "/builddir/build/BUILDROOT/python-googleapis-common-protos-1.58.0-2.fc39~bootstrap.x86_64/usr/lib/python3.12/site-packages/google/api/annotations_pb2.py", line 20, in <module>
    from google.protobuf import descriptor as _descriptor
  File "/usr/lib64/python3.12/site-packages/google/protobuf/descriptor.py", line 47, in <module>
    from google.protobuf.pyext import _message
TypeError: Metaclasses with custom tp_new are not supported.

Added the new limited C API function PyType_FromMetaclass(), which generalizes the existing PyType_FromModuleAndSpec() using an additional metaclass argument. (Contributed by Wenzel Jakob in gh-93012.)

The metaclass is used to construct the resulting type object. When metaclass is NULL, the metaclass is derived from bases (or Py_tp_base[s] slots if bases is NULL, see below). Note that metaclasses that override tp_new are not supported.

https://github.com/python/cpython/issues/93012
https://docs.python.org/3.12/whatsnew/3.12.html

Make sure you include information that can help us debug (full error message, exception listing, stack trace, logs).

Anything else we should know about your project / environment

See python/cpython#93012.

Grepping for tp_new in main gives:

python/google/protobuf/pyext/descriptor.cc
1600:    nullptr,                             // tp_new

python/google/protobuf/pyext/unknown_field_set.cc
197:    unknown_field_set::New,         //  tp_new

python/google/protobuf/pyext/message_factory.cc
293:    message_factory::New,         // tp_new

python/google/protobuf/pyext/field.cc
126:    nullptr,                        // tp_new

python/google/protobuf/pyext/message.cc
275:  ScopedPyObjectPtr result(PyType_Type.tp_new(type, new_args.get(), nullptr));
502:    message_meta::New,         // tp_new
2762:    cmessage::New,                       //  tp_new

python/google/protobuf/pyext/descriptor_pool.cc
750:    cdescriptor_pool::New,                    // tp_new

python/google/protobuf/pyext/descriptor_containers.cc
596:    nullptr,                   // tp_new
783:    nullptr,                   // tp_new
929:    nullptr,                      // tp_new

[haberman]

It appears that this applies to both the old C++ native extension and the new upb one.

[gpshead]

That TypeError probably turns into a DeprecationWarning if you update to a more recent 3.12 that contains python/cpython#103972. it'll become an actual error in Python 3.14. The metaclasses tp_new is ignored.

[encukou]

IMO, the DeprecationWarning is correct, because the behaviour in Python 3.11 is buggy -- though the bug might be harmless in protobuf's case.

protobuf uses PyType_FromSpecWithBases to create ScalarMapContainer and MessageMapContainer, which derive from collections.abc.MutableMapping, so their metaclass is abc.ABCMeta.

I believe (but didn't check, compiling/testing protobuf is out of my league at the moment) that abc.ABCMeta.__new__ was not called on these container types, even in Python 3.11.
They seem to be abstract bases that are never instantiated, so the setup that abc.ABCMeta.__new__ does could turn to to not be necessary -- but that would be a fragile assumption to make.
So, the current (3.11) behaviour is unsafe, hence, a DeprecationWarning for it.

What to do? I wish I had a good answer.
I guess the correct workaround for protobuf is to derive a ScalarMapContainerBase_Type from object (using PyType_FromSpecWithBases), and derive then ScalarMapContainer_Type from that and MutableMapping (with a call to the metaclass, that is, Py_TYPE(mutable_mapping)). That should work correctly (that is, set the ABC metadata) even in Python 3.11.

Or, if you're sure you're not skipping anything important, temporarily set Py_TYPE(mutable_mapping)->tp_new = Py_TYPE(&PyType_Type)->tp_new;. That's a nasty hack, but... if you do want the previous nasty behaviour, it rather clearly shows what that is.

You might want to follow python/cpython#103968 for a bit before implementing my suggestions.

[anandolee]

Thank you both Gregory and Petr

From python/cpython#103968 , Petr mentioned:

I looked at protobuf.
...
I don't know what the correct general solution is, but IMO, the current behaviour is worth deprecating.

And as the TypeError has turned into DeprecationWarning in more recent 3.12. We may want to wait until python/cpython#103968 is resolved and have a correct general solution before get rid of the Warning. Please correct me if I was wrong, @encukou

[encukou]

That's right.
Unfortunately, it looks like the general solution is pretty vague, with several options that all have downsides :(
The draft is here: https://github.com/python/cpython/pull/105698/files

I didn't include the ideas from above, one is specific to protobuf (ABCs) and the other is too hacky. But they should work.

[anandolee]

I tried the Py_TYPE(mutable_mapping.get())->tp_new = Py_TYPE(&PyType_Type)->tp_new; But got following error:

google/protobuf/internal/type_checkers.py", line 49, in <module>
    import numbers
  File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1006, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 688, in _load_unlocked
  File "/<embedded stdlib>/numbers.py", line 144, in <module>
  File "/<embedded stdlib>/abc.py", line 115, in register
  File "/<embedded stdlib>/abc.py", line 123, in __subclasscheck__
AttributeError: type object 'Complex' has no attribute '_abc_impl'

Wondering if you can help us with a fixing PR Petr?

[encukou]

I intend to. Unfortunately I didn't get to it this week.

[haberman]

I left some thoughts about this in python/cpython#103968 (comment)

TL;DR: I don't think we want to actually run tp_new from abc.ABCMeta on {Scalar,Message}MapContainer, because we do not want our classes to have a register() method.

We just want to mix in MutableMapping without adding any class methods. We may just want to remove the inheritance and do something like:

from collections.abc import MutableMapping

class ScalarMapContainer:
    # ...

MutableMapping.register(ScalarMapContainer)

# Mixin the methods manually.
ScalarMapContainer.__contains__ = MutableMapping.__contains__
ScalarMapContainer.keys = MutableMapping.keys
ScalarMapContainer.items = MutableMapping.items
ScalarMapContainer.values = MutableMapping.values
# ...

[encukou]

Wondering if you can help us with a fixing PR Petr?

I've spent some time trying to build protobuf, so I can send a tested PR, but didn't succeed.
I've installed bazelisk, and, according to these instructions I ran bazel build @upb//python/dist:binary_wheel. But that seems to not use map_container.cc (which I checked crudely: when I add a syntax error, a new build is still successful).
I'm stumped, and can't seem to find clear instructions.

I can run a Linux container. How do I build and test this code? Preferably with non-system install of Python.

We just want to mix in MutableMapping without adding any class methods.

That sounds like a great idea.

[phillipuniverse]

Just to make it easier to find, this is the latest stack trace in 3.12.0:

../../Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.12/lib/python3.12/site-packages/opentelemetry/exporter/otlp/proto/common/_internal/__init__.py:21: in <module>
    from opentelemetry.proto.common.v1.common_pb2 import (
../../Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.12/lib/python3.12/site-packages/opentelemetry/proto/common/v1/common_pb2.py:5: in <module>
    from google.protobuf import descriptor as _descriptor
../../Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.12/lib/python3.12/site-packages/google/protobuf/descriptor.py:40: in <module>
    from google.protobuf.internal import api_implementation
../../Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.12/lib/python3.12/site-packages/google/protobuf/internal/api_implementation.py:74: in <module>
    if _CanImport('google._upb._message'):
../../Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.12/lib/python3.12/site-packages/google/protobuf/internal/api_implementation.py:64: in _CanImport
    mod = importlib.import_module(mod_name)
../../.pyenv/versions/3.12.0/lib/python3.12/importlib/__init__.py:90: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
E   SystemError: <class 'DeprecationWarning'> returned a result with an exception set

As @gpshead alluded to, the TypeError did indeed change to DeprecationWarning but still for some reason results in a SystemError

EDIT - I think there is a misunderstanding somewhere (maybe by me). While there is something about a DeprecationWarning there is some sort of SystemError happening too, and this code crashes.

[phillipuniverse]

Actually, my issue is when I turn all warnings (including DeprecationWarning) into errors in pytest. Example from my pyproject.toml:

[tool.pytest.ini_options]
filterwarnings = [
    "error",# treat all warnings as errors]
]

But if I ignore all DeprecationWarning, then things are ok:

[tool.pytest.ini_options]
filterwarnings = [
    "error", # treat all warnings as errors
    "ignore::DeprecationWarning"
]

And a more targeted exclusion also works:

[tool.pytest.ini_options]
filterwarnings = [
    "error", # treat all warnings as errors
    "ignore:.*custom tp_new.*in Python 3.14.*:DeprecationWarning",
]

[github-actions]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

[github-actions]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please reopen it.

This issue was closed and archived because there has been no new activity in the 14 days since the inactive label was added.