Add weakref tests for python messages + fix for upb implementation

[bostikforever]

This exposes a regression with the upb implementation where the python message objects are not weakref-able.

See also: https://github.com/protocolbuffers/protobuf/issues/13727

[github-actions[bot]]

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

This PR is labeled inactive because the last activity was over 90 days ago. This PR will be closed and archived after 14 additional days without activity.

[bostikforever]

Pinging @anandolee

[bostikforever]

Pinging again @anandolee @haberman

[bostikforever]

@anandolee Branch seemed out of date and there were some (seemingly) random CI failures. I have now re-based. Can you please re-approve?

[bostikforever]

@anandolee Looks like the tests pass, apart from the copybara thing which I can't view as it's internal.

[anandolee]

Thank you Bolutife. Our internal tool is strict on the format, and here is the complain:

Error:
1 of 1 file(s) have formatting incompatible with clang-format.
Clang-format would introduce the following changes:
--- //depot/[google3/third_party/upb/python/message.c](http://google3/third_party/upb/python/message.c)
+++ //depot/[google3/third_party/upb/python/message.c](http://google3/third_party/upb/python/message.c) (formatted)
@@ -1787,7 +1787,8 @@
   }
 
   PyObject* ret = cpython_bits.type_new(state->message_meta_type, args, NULL);
-  ((PyTypeObject*)(ret))->tp_weaklistoffset = offsetof(PyUpb_Message, weakreflist);
+  ((PyTypeObject*)(ret))->tp_weaklistoffset =
+      offsetof(PyUpb_Message, weakreflist);
   Py_DECREF(args);
   if (!ret) return NULL;

[esrauchg]

Importing the PR to the Google internal codebase hit a couple different blocking issues:

• Apparent typo(?) on @_parameterized instead of @parameterized
• the 80 line limit violation that was noted above
• The wheels GHA tests are broken with this error:

python/message.c:1790:25: error: incomplete definition of type 'struct _typeobject' ((PyTypeObject*)(ret))->tp_weaklistoffset =

• a merge conflict with an internal-only test (don't worry about this one)

The first three should be visible in the external test runs, I'm unsure if you made those changes since the last time it ran the tests? Are you able to take a look at those issues to resolve them?

[bostikforever]

@esrauchg

Importing the PR to the Google internal codebase hit a couple different blocking issues:

* Apparent typo(?) on @_parameterized instead of @parameterized

Not sure this is a real issue, as the parameterized being referenced is the one with underscore (part of the bazel build config, see: https://github.com/protocolbuffers/protobuf/blob/71c41c329a88b9e33d24c44cbd6512e887ecbea0/python/google/protobuf/internal/message_test.py#L41, https://github.com/protocolbuffers/protobuf/blob/71c41c329a88b9e33d24c44cbd6512e887ecbea0/python/google/protobuf/internal/message_test.py#L51)

* the 80 line limit violation that was noted above

Fixed.

* The wheels GHA tests are broken with this error:

python/message.c:1790:25: error: incomplete definition of type 'struct _typeobject' ((PyTypeObject*)(ret))->tp_weaklistoffset =

Not sure how to reproduce this failure locally (or even in the external CI run). My local build does not flag this, can you please help with what I ought to run?

* a merge conflict with an internal-only test (don't worry about this one)

The first three should be visible in the external test runs, I'm unsure if you made those changes since the last time it ran the tests? Are you able to take a look at those issues to resolve them?

Actually none of these 3 seemed to be an issue in the external test runs, or in my local test runs. Do you have any advice on how to reproduce 1 and 3?

[haberman]

Not sure how to reproduce this failure locally (or even in the external CI run). My local build does not flag this, can you please help with what I ought to run?

I think I can offer some insight on this. I think you're running into an issue because we use the Python Limited API for our release wheels whenever possible. When the limited API is enabled, certain structures like PyTypeObject are only forward-declared (not defined), because their definition can change from version to version and we don't want our wheels to be version-specific.

Unfortunately it does not appear that tp_weaklistoffset is supported in the limited API. There is no slot definition for Py_tp_weaklistoffset in https://github.com/python/cpython/blob/main/Include/typeslots.h

That presents a significant challenge to adding support for weak references to the upb Python backend. It appears we would need to give up using the limited API to support weak references, at least until Python 3.12 which introduces support for Py_TPFLAGS_MANAGED_WEAKREF

As for why this didn't initially fail in the CI: We don't enable the limited API by default, because it won't work for a few versions of Python on Windows, see: https://github.com/protocolbuffers/protobuf/blob/413e7b10a098a1cd6b36b1835078454a2d1ccbe7/python/dist/BUILD.bazel#L433-L450

In other words, the limited API is only enabled currently when you build release wheels, not for regular tests. But our CI unfortunately cannot build the release wheels for external contributions, because they use an internal-only Docker image. This is a rare case where only the wheel-based tests would detect the error.

[github-actions[bot]]

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

This PR is labeled inactive because the last activity was over 90 days ago. This PR will be closed and archived after 14 additional days without activity.

[haberman]

I think the best way forward would be to use Py_TPFLAGS_MANAGED_WEAKREF for Python >=3.12, and skip the tests on Python <3.12.

Then weak refs would be supported on Python >=3.12.

[googleberg]

Waiting on response from @bostikforever

[bostikforever]

I think the best way forward would be to use Py_TPFLAGS_MANAGED_WEAKREF for Python >=3.12, and skip the tests on Python <3.12.

Then weak refs would be supported on Python >=3.12.

This seems good enough.

[bostikforever]

I think the best way forward would be to use Py_TPFLAGS_MANAGED_WEAKREF for Python >=3.12, and skip the tests on Python <3.12. Then weak refs would be supported on Python >=3.12.

This seems good enough.

Are you happy to make the necessary changes yourselves?

[haberman]

Are you happy to make the necessary changes yourselves?

I'm not able to prioritize this. If this is a priority for you, probably the best way to get it landed is via a contribution.

[github-actions[bot]]

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

This PR is labeled inactive because the last activity was over 90 days ago. This PR will be closed and archived after 14 additional days without activity.

[github-actions[bot]]

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

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