From fccb7887a65c133ad3cf4428c3815976a9a794b6 Mon Sep 17 00:00:00 2001 From: rwgk Date: Mon, 17 Jun 2024 01:24:51 -0700 Subject: [PATCH] Make `//third_party/clif/pybind11` independent from `//util/task/python`. Straightforward untangling of dependencies. The current situation arose from very large-scale, multi-year work on the Python bindings for `absl::Status` and `absl::StatusOr` across PyCLIF and pybind11. Trigger for this cleanup: Support experimental work on going back to deploying pybind11 from the smart_holder branch. However, this cleanup is useful regardless. For easy future reference, here is a snapshot of the fig graph from which this CL is submitted: https://rwgk.users.x20web.corp.google.com/hg_xl_archive/hgx_back2sh_fig_as_of_2024-06-16%2B2357.txt PiperOrigin-RevId: 643912636 --- clif/pybind11/runtime.h | 4 +- clif/pybind11/status_return_override.cc | 16 ++++ clif/pybind11/status_return_override.h | 97 +++++++++++++++++++++++++ clif/pybind11/type_casters.h | 2 +- 4 files changed, 117 insertions(+), 2 deletions(-) create mode 100644 clif/pybind11/status_return_override.cc create mode 100644 clif/pybind11/status_return_override.h diff --git a/clif/pybind11/runtime.h b/clif/pybind11/runtime.h index 52b35e98..43bd0772 100644 --- a/clif/pybind11/runtime.h +++ b/clif/pybind11/runtime.h @@ -15,12 +15,14 @@ #ifndef CLIF_PYBIND11_RUNTIME_H_ #define CLIF_PYBIND11_RUNTIME_H_ +// pybind11 includes have to be at the very top, even before Python.h #include "third_party/pybind11/include/pybind11/smart_holder.h" #include "third_party/pybind11/include/pybind11/type_caster_pyobject_ptr.h" +// Must be after pybind11 include. +#include "clif/pybind11/status_return_override.h" #include "clif/python/stltypes.h" #include "third_party/pybind11_abseil/absl_casters.h" -#include "util/task/python/clif/status_return_override.h" namespace clif { diff --git a/clif/pybind11/status_return_override.cc b/clif/pybind11/status_return_override.cc new file mode 100644 index 00000000..5e44bc88 --- /dev/null +++ b/clif/pybind11/status_return_override.cc @@ -0,0 +1,16 @@ +#include "clif/pybind11/status_return_override.h" + +#include "absl/log/check.h" +#include "absl/status/status.h" +#include "third_party/pybind11/include/pybind11/pybind11.h" +#include "third_party/pybind11_abseil/compat/status_from_py_exc.h" + +namespace clif_pybind11 { + +absl::Status StatusFromErrorAlreadySet(pybind11::error_already_set& e) { + CHECK(PyGILState_Check()); + e.restore(); + return pybind11_abseil::compat::StatusFromPyExcGivenErrOccurred(); +} + +} // namespace clif_pybind11 diff --git a/clif/pybind11/status_return_override.h b/clif/pybind11/status_return_override.h new file mode 100644 index 00000000..f4b364a8 --- /dev/null +++ b/clif/pybind11/status_return_override.h @@ -0,0 +1,97 @@ +#ifndef CLIF_PYBIND11_STATUS_RETURN_OVERRIDE_H_ +#define CLIF_PYBIND11_STATUS_RETURN_OVERRIDE_H_ + +// pybind11 includes have to be at the very top, even before Python.h +#include "third_party/pybind11/include/pybind11/detail/common.h" +#include "third_party/pybind11/include/pybind11/gil.h" +#include "third_party/pybind11/include/pybind11/pybind11.h" // NOLINT(build/include_order) + +// Must be after pybind11 include. +#include // NOLINT(build/include_order) + +#include "absl/status/status.h" +#include "absl/status/statusor.h" +#include "third_party/pybind11/include/pybind11/pytypes.h" + +namespace clif_pybind11 { + +absl::Status StatusFromErrorAlreadySet(pybind11::error_already_set& e); + +template +pybind11::function GetOverload( + const U* this_ptr, const std::string& function_name) { + pybind11::gil_scoped_acquire gil; + return pybind11::get_overload(static_cast(this_ptr), + function_name.c_str()); +} + +template +absl::Status CatchErrorAlreadySetAndReturnStatus( + const U* this_ptr, const std::string& function_name, + const pybind11::return_value_policy_pack rvpp, Ts... args) { + try { + pybind11::function overload = GetOverload(this_ptr, function_name); + if (!overload) { + return absl::Status(absl::StatusCode::kUnimplemented, + "No Python overload is defined for " + function_name + + "."); + } + overload.call_with_policies(rvpp, args...); /* Ignoring return value. */ + return absl::OkStatus(); + } catch (pybind11::error_already_set &e) { + return StatusFromErrorAlreadySet(e); + } +} + +template +StatusOrPayload CatchErrorAlreadySetAndReturnStatusOr( + const U* this_ptr, const std::string& function_name, + const pybind11::return_value_policy_pack rvpp, Ts... args) { + try { + pybind11::function overload = GetOverload(this_ptr, function_name); + if (!overload) { + return absl::Status(absl::StatusCode::kUnimplemented, + "No Python overload is defined for " + function_name + + "."); + } + auto o = overload.call_with_policies(rvpp, args...); + return o.template cast(); + } catch (pybind11::error_already_set &e) { + return StatusFromErrorAlreadySet(e); + } +} + +} // namespace clif_pybind11 + +// Similar with macro `PYBIND11_OVERLOAD_PURE` defined in pybind11/pybind11.h, +// but catches all exceptions derived from pybind11::error_already_set and +// converts the exception to a StatusNotOk return. +#define PYBIND11_OVERRIDE_PURE_STATUS_RETURN(cname, name, fn, rvpp, ...) \ + return ::clif_pybind11::CatchErrorAlreadySetAndReturnStatus( \ + this, name, rvpp, ##__VA_ARGS__); + +#define PYBIND11_OVERRIDE_PURE_STATUSOR_RETURN(statusor_payload_type, cname, \ + name, fn, rvpp, ...) \ + return ::clif_pybind11::CatchErrorAlreadySetAndReturnStatusOr< \ + statusor_payload_type>(this, name, rvpp, ##__VA_ARGS__); + +#define PYBIND11_OVERRIDE_STATUS_RETURN(cname, name, fn, rvpp, ...) \ + pybind11::function overload = ::clif_pybind11::GetOverload(this, name); \ + if (overload) { \ + return ::clif_pybind11::CatchErrorAlreadySetAndReturnStatus( \ + this, name, rvpp, ##__VA_ARGS__); \ + } else { \ + return cname::fn(__VA_ARGS__); \ + } + +#define PYBIND11_OVERRIDE_STATUSOR_RETURN(statusor_payload_type, cname, name, \ + fn, rvpp, ...) \ + pybind11::function overload = ::clif_pybind11::GetOverload(this, name); \ + if (overload) { \ + return ::clif_pybind11::CatchErrorAlreadySetAndReturnStatusOr< \ + statusor_payload_type>(this, name, rvpp, ##__VA_ARGS__); \ + } else { \ + return cname::fn(__VA_ARGS__); \ + } + +#endif // CLIF_PYBIND11_STATUS_RETURN_OVERRIDE_H_ diff --git a/clif/pybind11/type_casters.h b/clif/pybind11/type_casters.h index f7593795..b5ca764f 100644 --- a/clif/pybind11/type_casters.h +++ b/clif/pybind11/type_casters.h @@ -29,11 +29,11 @@ #include "absl/container/flat_hash_map.h" #include "absl/container/flat_hash_set.h" +#include "clif/pybind11/status_return_override.h" #include "clif/python/types.h" #include "third_party/pybind11/include/pybind11/detail/common.h" #include "third_party/pybind11_abseil/status_caster.h" #include "third_party/pybind11_abseil/statusor_caster.h" -#include "util/task/python/clif/status_return_override.h" namespace pybind11 { namespace detail {