Avoid using python bindings for absl::Status provided by pybind11_abseil

ODR violations were occasionally observed in presubmit test runs. Our investigation revealed that the issue stems from the Python bindings for absl::Status in pybind11_abseil (see the reported issue: pybind/pybind11_abseil#20).

Note: We have tested that this change helps by manually increasing the number of iterations in //py/arolla/py_utils:py_utils_test to 10**6.
PiperOrigin-RevId: 629017121
Change-Id: Iab0ff1f460971ef69e6a9653c10b7e2dddccfdcc

py/arolla/abc/BUILD

@@ -25,9 +25,7 @@ licenses(["notice"])
 
 py_library(
     name = "abc",
-    srcs = [
-        "abc.py",
-    ],
+    srcs = ["abc.py"],
     visibility = ["//arolla:internal"],
     deps = [
         ":lib",
@@ -60,27 +58,18 @@ py_library(
 
 py_library(
     name = "utils",
-    srcs = [
-        "utils.py",
-    ],
-    data = ["@pybind11_abseil//pybind11_abseil:status.so"],
-    deps = [
-        ":clib",
-        ":dummy_numpy",
-    ],
+    srcs = ["utils.py"],
+    deps = [":dummy_numpy"],
 )
 
 py_library(
     name = "dummy_numpy",
-    srcs = [
-        "dummy_numpy.py",
-    ],
+    srcs = ["dummy_numpy.py"],
 )
 
 arolla_pybind_extension(
     name = "clib",
     srcs = ["clib.cc"],
-    pytype_deps = ["//third_party/pybind11_abseil:status"],
     pytype_srcs = ["clib.pyi"],
     deps = [
         ":py_abc",
@@ -447,11 +436,8 @@ py_test(
     srcs = [
         "utils_test.py",
     ],
-    data = ["@pybind11_abseil//pybind11_abseil:status.so"],
     deps = [
-        ":clib",
         ":lib",
-        ":testing_clib",
         ":utils",
         requirement("numpy"),  # buildcleaner: keep
         "@com_google_absl_py//absl/testing:absltest",

py/arolla/abc/abc.py

@@ -187,7 +187,6 @@
 get_numpy_module_or_dummy = _rl_abc_utils.get_numpy_module_or_dummy
 get_type_name = _rl_abc_utils.get_type_name
 import_numpy = _rl_abc_utils.import_numpy
-process_status_not_ok = _rl_abc_utils.process_status_not_ok
 # go/keep-sorted end
 
 

py/arolla/abc/clib.cc

@@ -281,15 +281,6 @@ PYBIND11_MODULE(clib, m) {
               "--\n\n"
               "Returns the expression's nodes in DFS post-order."));
 
-  m.def(
-      "raise_if_error",
-      [](const absl::Status& status) { pybind11_throw_if_error(status); },
-      py::arg("status"), py::pos_only(),
-      py::doc(
-          "raise_if_error(status, /)\n"
-          "--\n\n"
-          "(deprecated) Raises a python exception if the status is not ok."));
-
   m.def(
       "register_aux_binding_policy_methods",
       [](absl::string_view aux_policy, py::handle make_python_signature_fn,

py/arolla/abc/clib.pyi

@@ -18,7 +18,6 @@ from __future__ import annotations
 
 import inspect
 from typing import Any, Callable, Iterable, Mapping, Protocol, Self
-from pybind11_abseil import status as pybind11_abseil_status
 
 class Fingerprint: ...
 
@@ -251,8 +250,6 @@ def placeholder(placeholder_key: str, /) -> Expr: ...
 
 def post_order(expr: Expr, /) -> list[Expr]: ...
 
-def raise_if_error(status: pybind11_abseil_status.Status, /) -> None: ...
-
 def register_default_expr_view_member(
     member_name: str, expr_view_member: Any, /
 ) -> None: ...

py/arolla/abc/testing_clib.cc

@@ -19,7 +19,6 @@
 #include "absl/status/status.h"
 #include "py/arolla/abc/py_signature.h"
 #include "py/arolla/abc/pybind11_utils.h"
-#include "py/arolla/py_utils/py_object_as_status_payload.h"
 #include "py/arolla/py_utils/py_utils.h"
 #include "pybind11/pybind11.h"
 #include "pybind11/pytypes.h"
@@ -90,16 +89,6 @@ PYBIND11_MODULE(testing_clib, m) {
   m.def("pybind11_type_caster_cast_load_operator_signature",
         [](ExprOperatorSignature x) { return x; });
 
-  m.def("set_py_exception_payload", [](absl::Status* status, py::object ex) {
-    return pybind11_throw_if_error(WritePyObjectToStatusPayload(
-        status, kPyException, PyObjectGILSafePtr::NewRef(ex.ptr())));
-  });
-  m.def("set_py_exception_cause_payload",
-        [](absl::Status* status, py::object ex) {
-          return pybind11_throw_if_error(WritePyObjectToStatusPayload(
-              status, kPyExceptionCause, PyObjectGILSafePtr::NewRef(ex.ptr())));
-        });
-
   // Register a `test.fail` operator.
   pybind11_throw_if_error(
       RegisterOperator<StdFunctionOperator>(

py/arolla/abc/utils.py

@@ -18,28 +18,8 @@
 import types
 import typing
 
-from arolla.abc import clib
 from arolla.abc import dummy_numpy
 
-from pybind11_abseil import status as absl_status
-
-
-def process_status_not_ok(status_not_ok: absl_status.StatusNotOk):
-  """Raises appropriate Python exception from the given not ok status."""
-  if not isinstance(status_not_ok, absl_status.StatusNotOk):
-    raise TypeError(
-        f"status must be StatusNotOk, got {get_type_name(type(status_not_ok))}"
-    )
-  # We want to drop the current context that raised StatusNotOK and show
-  # the original context if there was any.
-  try:
-    clib.raise_if_error(status_not_ok.status)
-  except Exception as ex:
-    ex.__context__ = status_not_ok.__context__
-    raise ex
-  # Mark code as unreachable to make pytype work correctly.
-  assert False, "unreachable"
-
 
 def get_type_name(t: typing.Type[typing.Any]) -> str:
   """Returns a meaningful type name."""

py/arolla/abc/utils_test.py

@@ -20,13 +20,9 @@
 
 from absl.testing import absltest
 from absl.testing import parameterized
-from arolla.abc import clib
 from arolla.abc import qtype as abc_qtype
-from arolla.abc import testing_clib
 from arolla.abc import utils as abc_utils
 
-from pybind11_abseil import status as absl_status
-
 
 class UtilsTest(parameterized.TestCase):
 
@@ -61,80 +57,5 @@ def test_numpy_import(self):
     self.assertIs(np, sys.modules['numpy'])
 
 
-class ProcessNotOkStatusTest(parameterized.TestCase):
-
-  def test_pure_status(self):
-    status = absl_status.Status(
-        absl_status.StatusCode.ABORTED, 'This is Status error.'
-    )
-    status = absl_status.StatusNotOk(status)
-    with self.assertRaisesRegex(ValueError, 'This is Status error.'):
-      abc_utils.process_status_not_ok(status)
-
-  def test_suppress_context(self):
-    raised_exception = None
-    try:
-      try:
-        raise absl_status.StatusNotOk(
-            absl_status.Status(absl_status.StatusCode.INTERNAL, 'Some error')
-        )
-      except absl_status.StatusNotOk as status:
-        abc_utils.process_status_not_ok(status)
-    except ValueError as ex:
-      raised_exception = ex
-    self.assertIsNotNone(raised_exception)
-    self.assertIsNone(raised_exception.__context__)
-
-  def test_suppress_context_chained(self):
-    context_exception = ValueError('Some context')
-    raised_exception = None
-    try:
-      try:
-        raise context_exception
-      except ValueError:
-        try:
-          raise absl_status.StatusNotOk(  # pylint: disable=raise-missing-from
-              absl_status.Status(absl_status.StatusCode.INTERNAL, 'Some error')
-          )
-        except absl_status.StatusNotOk as status:
-          abc_utils.process_status_not_ok(status)
-    except ValueError as ex:
-      raised_exception = ex
-    self.assertIsNotNone(raised_exception)
-    self.assertIs(raised_exception.__context__, context_exception)
-
-  def test_with_py_exception(self):
-    status = absl_status.Status(
-        absl_status.StatusCode.ABORTED, 'This is Status error.'
-    )
-    cause = AssertionError('This is the exception.')
-    testing_clib.set_py_exception_payload(status, cause)
-    status = absl_status.StatusNotOk(status)
-    with self.assertRaisesRegex(AssertionError, 'This is the exception.'):
-      abc_utils.process_status_not_ok(status)
-
-  def test_with_py_exception_cause(self):
-    status = absl_status.Status(
-        absl_status.StatusCode.ABORTED, 'This is Status error.'
-    )
-    cause = AssertionError('This is the cause.')
-    testing_clib.set_py_exception_cause_payload(status, cause)
-    status = absl_status.StatusNotOk(status)
-    raised_exception = None
-    try:
-      abc_utils.process_status_not_ok(status)
-    except ValueError as e:
-      raised_exception = e
-    self.assertIsNotNone(raised_exception)
-    self.assertIsNotNone(raised_exception.__cause__)
-    self.assertIs(raised_exception.__cause__, cause)
-
-  def test_ok_status(self):
-    with self.assertRaisesRegex(TypeError, 'status must be StatusNotOk'):
-      abc_utils.process_status_not_ok(absl_status.Status.OkStatus())  # pytype: disable=wrong-arg-types
-    # Extra check for the pure clib wrapper to not crash with ok status.
-    clib.raise_if_error(absl_status.Status.OkStatus())
-
-
 if __name__ == '__main__':
   absltest.main()

py/arolla/py_utils/BUILD

@@ -14,8 +14,8 @@
 
 # C++ helpers for Python C API.
 
+load("@pybind11_bazel//:build_defs.bzl", "pybind_extension")
 load("@rules_python//python:defs.bzl", "py_test")
-load("//py/arolla/dynamic_deps:build_defs.bzl", "arolla_pybind_extension")
 
 package(default_visibility = ["//visibility:private"])
 
@@ -34,7 +34,6 @@ cc_library(
     visibility = ["//visibility:public"],
     deps = [
         ":py_object_ptr_impl",
-        "//arolla/util:status_backport",
         "@com_google_absl//absl/base:core_headers",
         "@com_google_absl//absl/log:check",
         "@com_google_absl//absl/random",
@@ -49,9 +48,7 @@ cc_library(
 
 cc_library(
     name = "py_object_ptr_impl",
-    hdrs = [
-        "py_object_ptr_impl.h",
-    ],
+    hdrs = ["py_object_ptr_impl.h"],
     deps = [
         "@com_google_absl//absl/base:core_headers",
         "@rules_python//python/cc:current_py_cc_headers",  # buildcleaner: keep
@@ -60,32 +57,28 @@ cc_library(
 
 cc_test(
     name = "py_object_ptr_impl_test",
-    srcs = [
-        "py_object_ptr_impl_test.cc",
-    ],
+    srcs = ["py_object_ptr_impl_test.cc"],
     deps = [
         ":py_object_ptr_impl",
         "@com_google_googletest//:gtest_main",
         "@rules_python//python/cc:current_py_cc_headers",  # buildcleaner: keep
     ],
 )
 
-arolla_pybind_extension(
+pybind_extension(
     name = "testing_clib",
     testonly = True,
     srcs = ["testing_clib.cc"],
     deps = [
         ":py_utils",
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/strings:string_view",
-        "@pybind11_abseil//pybind11_abseil:status_casters",
     ],
 )
 
 py_test(
     name = "py_utils_test",
     srcs = ["py_utils_test.py"],
-    data = ["@pybind11_abseil//pybind11_abseil:status.so"],
     deps = [
         ":testing_clib",
         "//py:python_path",  # Adds //py to the path to allow convenient imports.

py/arolla/py_utils/py_object_as_status_payload.cc

@@ -27,7 +27,6 @@
 #include "absl/strings/cord.h"
 #include "absl/strings/string_view.h"
 #include "py/arolla/py_utils/py_utils.h"
-#include "arolla/util/status_macros_backport.h"
 
 // absl::MakeCordFromExternal creates an absl::Cord that owns an external
 // memory. We use this mechanism to make the cord own a ref-counted PyObject.
@@ -107,8 +106,11 @@ absl::Status WritePyObjectToStatusPayload(absl::Status* status,
     status->ErasePayload(type_url);
     return absl::OkStatus();
   }
-  ASSIGN_OR_RETURN(auto token, WrapPyObjectToCord(std::move(obj)));
-  status->SetPayload(type_url, std::move(token));
+  auto token = WrapPyObjectToCord(std::move(obj));
+  if (!token.ok()) {
+    return token.status();
+  }
+  status->SetPayload(type_url, *std::move(token));
   return absl::OkStatus();
 }