diff --git a/BUILD b/BUILD index a3e94b1..272806e 100644 --- a/BUILD +++ b/BUILD @@ -90,8 +90,6 @@ pybind_extension( "@com_google_cel_spec//proto/cel/expr:syntax_cc_proto", "@com_google_protobuf//:protobuf", "@pybind11_abseil//pybind11_abseil:absl_casters", - "@pybind11_abseil//pybind11_abseil:import_status_module", - "@pybind11_abseil//pybind11_abseil:status_casters", ], ) diff --git a/py_cel_env.cc b/py_cel_env.cc index cd99ad6..871ac64 100644 --- a/py_cel_env.cc +++ b/py_cel_env.cc @@ -24,15 +24,14 @@ #include #include "absl/log/absl_check.h" -#include "absl/status/statusor.h" #include "py_cel_activation.h" #include "py_cel_arena.h" #include "py_cel_env_internal.h" #include "py_cel_expression.h" #include "py_cel_type.h" +#include "py_error_status.h" #include #include -#include "pybind11_abseil/status_casters.h" namespace cel_python { @@ -121,14 +120,13 @@ std::shared_ptr PyCelEnv::NewActivation( return std::make_shared(env_, data, functions, arena); } -absl::StatusOr PyCelEnv::Compile(const std::string& cel_expr, - bool disable_check) { - return PyCelExpression::Compile(env_, cel_expr, disable_check); +PyCelExpression PyCelEnv::Compile(const std::string& cel_expr, + bool disable_check) { + return ThrowIfError(PyCelExpression::Compile(env_, cel_expr, disable_check)); } -absl::StatusOr PyCelEnv::Deserialize( - const std::string& serialized_expr) { - return PyCelExpression::Deserialize(env_, serialized_expr); +PyCelExpression PyCelEnv::Deserialize(const std::string& serialized_expr) { + return ThrowIfError(PyCelExpression::Deserialize(env_, serialized_expr)); } } // namespace cel_python diff --git a/py_cel_env.h b/py_cel_env.h index 7ba0444..73133a4 100644 --- a/py_cel_env.h +++ b/py_cel_env.h @@ -23,7 +23,6 @@ #include #include -#include "absl/status/statusor.h" #include "py_cel_activation.h" #include "py_cel_arena.h" #include "py_cel_expression.h" @@ -52,10 +51,11 @@ class PyCelEnv { ~PyCelEnv(); - absl::StatusOr Compile(const std::string& cel_expr, - bool disable_check = false); - absl::StatusOr Deserialize( - const std::string& serialized_expr); + // May throw exceptions. + PyCelExpression Compile(const std::string& cel_expr, + bool disable_check = false); + // May throw exceptions. + PyCelExpression Deserialize(const std::string& serialized_expr); std::shared_ptr NewActivation( const std::unordered_map& data, const std::vector>& functions, diff --git a/py_cel_expression.cc b/py_cel_expression.cc index c1d9bc3..1bf7c9c 100644 --- a/py_cel_expression.cc +++ b/py_cel_expression.cc @@ -53,7 +53,6 @@ #include "status_macros.h" #include #include -#include "pybind11_abseil/status_casters.h" namespace cel_python { @@ -79,14 +78,13 @@ void PyCelExpression::DefinePythonBindings(py::module& m) { data, const std::optional>>& functions, - const std::shared_ptr& arena = - nullptr) -> absl::StatusOr { + const std::shared_ptr& arena = nullptr) -> PyCelValue { if (activation) { if (data || functions || arena) { - return absl::InvalidArgumentError( - "Cannot provide both activation and any other arguments."); + throw StatusToException(absl::InvalidArgumentError( + "Cannot provide both activation and any other arguments.")); } - return self.Eval(**activation); + return ThrowIfError(self.Eval(**activation)); } std::unordered_map data_ptrs; if (data) { @@ -94,11 +92,11 @@ void PyCelExpression::DefinePythonBindings(py::module& m) { data_ptrs[key] = val.ptr(); } } - return self.Eval(PyCelActivation( + return ThrowIfError(self.Eval(PyCelActivation( self.env_, data_ptrs, functions.value_or( std::vector>{}), - arena ? arena : NewArena())); + arena ? arena : NewArena()))); }, py::arg("activation") = py::none(), py::arg("data") = py::none(), py::arg("functions") = py::none(), py::arg("arena") = nullptr); diff --git a/py_cel_function.cc b/py_cel_function.cc index f168ce4..8710686 100644 --- a/py_cel_function.cc +++ b/py_cel_function.cc @@ -37,7 +37,6 @@ #include "status_macros.h" #include #include -#include "pybind11_abseil/status_casters.h" namespace cel_python { @@ -56,7 +55,7 @@ static std::shared_ptr GetEnvFromContext( void PyCelFunction::DefinePythonBindings(pybind11::module& m) { py::class_>(m, "Function") - .def(py::init, bool, PyObject*, + .def(py::init, bool, py::object, PyCelType>(), py::arg("function_name"), py::arg("parameters"), py::arg("is_member"), py::arg("impl"), @@ -65,36 +64,21 @@ void PyCelFunction::DefinePythonBindings(pybind11::module& m) { PyCelFunction::PyCelFunction(std::string function_name, std::vector parameters, bool is_member, - PyObject* impl, PyCelType return_type) + py::object impl, PyCelType return_type) : function_name_(std::move(function_name)), return_type_(std::move(return_type)), parameters_(std::move(parameters)), is_member_(is_member), - impl_(impl) { + impl_(std::move(impl)) { ABSL_CHECK(PyGILState_Check()); - Py_XINCREF(impl_); } -PyCelFunction::~PyCelFunction() { - auto gil_state = PyGILState_Ensure(); - Py_XDECREF(impl_); - PyGILState_Release(gil_state); -}; - PyCelFunctionAdapter::PyCelFunctionAdapter(std::string function_name, PyCelType return_type, - PyObject* py_function) + py::object py_function) : function_name_(std::move(function_name)), return_type_(std::move(return_type)), - py_function_(py_function) { - Py_XINCREF(py_function_); -} - -PyCelFunctionAdapter::~PyCelFunctionAdapter() { - auto gil_state = PyGILState_Ensure(); - Py_XDECREF(py_function_); - PyGILState_Release(gil_state); -} + py_function_(std::move(py_function)) {} absl::StatusOr PyCelFunctionAdapter::Invoke( absl::Span args, @@ -110,7 +94,7 @@ absl::StatusOr PyCelFunctionAdapter::Invoke( CelValueToPyObject(args[i], env, py_arena, /*plain_value=*/true)); } - PyObject* result = PyObject_CallObject(py_function_, py_args); + PyObject* result = PyObject_CallObject(py_function_.ptr(), py_args); absl::Status status = PyErr_toStatus(); if (!status.ok()) { return cel::ErrorValue(status); @@ -119,8 +103,9 @@ absl::StatusOr PyCelFunctionAdapter::Invoke( return PyObjectToCelValue( result, return_type_, [this]() { - return absl::StrFormat("Python function '%s'", - PyUnicode_AsUTF8(PyObject_Repr(py_function_))); + return absl::StrFormat( + "Python function '%s'", + PyUnicode_AsUTF8(PyObject_Repr(py_function_.ptr()))); }, env, context.arena()); }; diff --git a/py_cel_function.h b/py_cel_function.h index dea6a57..2394d83 100644 --- a/py_cel_function.h +++ b/py_cel_function.h @@ -29,6 +29,8 @@ namespace cel_python { +namespace py = ::pybind11; + class PyCelEnvInternal; // Wrapper for a Python function that implements a CEL late-bound function. @@ -38,13 +40,12 @@ class PyCelFunction { static void DefinePythonBindings(pybind11::module& m); PyCelFunction(std::string function_name, std::vector parameters, - bool is_member, PyObject* impl, PyCelType return_type); - ~PyCelFunction(); + bool is_member, py::object impl, PyCelType return_type); std::string function_name() const { return function_name_; } const std::vector& parameters() const { return parameters_; } bool is_member() const { return is_member_; } - PyObject* impl() const { return impl_; } + py::object impl() const { return impl_; } const PyCelType& return_type() const { return return_type_; } private: @@ -52,7 +53,7 @@ class PyCelFunction { PyCelType return_type_; std::vector parameters_; bool is_member_; - PyObject* impl_; + py::object impl_; }; // Internal wrapper for a Python function that implements a CEL extension @@ -60,8 +61,7 @@ class PyCelFunction { class PyCelFunctionAdapter : public cel::Function { public: PyCelFunctionAdapter(std::string function_name, PyCelType return_type, - PyObject* py_function); - ~PyCelFunctionAdapter() override; + py::object py_function); absl::StatusOr Invoke( absl::Span args, @@ -70,7 +70,7 @@ class PyCelFunctionAdapter : public cel::Function { private: std::string function_name_; PyCelType return_type_; - PyObject* py_function_; + py::object py_function_; }; } // namespace cel_python diff --git a/py_cel_function_decl.cc b/py_cel_function_decl.cc index 273e006..f775124 100644 --- a/py_cel_function_decl.cc +++ b/py_cel_function_decl.cc @@ -21,8 +21,6 @@ #include "py_cel_overload.h" #include #include -#include "pybind11_abseil/absl_casters.h" -#include "pybind11_abseil/status_casters.h" namespace cel_python { diff --git a/py_cel_module.cc b/py_cel_module.cc index 776ca98..11bbd26 100644 --- a/py_cel_module.cc +++ b/py_cel_module.cc @@ -23,12 +23,10 @@ #include "py_cel_type.h" #include "py_cel_value.h" #include -#include "pybind11_abseil/import_status_module.h" namespace cel_python { PYBIND11_MODULE(py_cel, m) { - pybind11::google::ImportStatusModule(); m.doc() = "Python bindings for CEL."; PyCelArena::DefinePythonBindings(m); diff --git a/py_cel_overload.cc b/py_cel_overload.cc index 575159b..aa305b2 100644 --- a/py_cel_overload.cc +++ b/py_cel_overload.cc @@ -22,7 +22,6 @@ #include "py_cel_type.h" #include #include -#include "pybind11_abseil/status_casters.h" namespace cel_python { @@ -33,50 +32,23 @@ void PyCelOverload::DefinePythonBindings(py::module_& m) { .def(py::init([](const std::string& overload_id, const PyCelType& return_type, const std::vector& parameters, bool is_member, - PyObject* impl) { + py::object impl) { return PyCelOverload(overload_id, return_type, parameters, - is_member, impl != Py_None ? impl : nullptr); + is_member, std::move(impl)); }), py::arg("overload_id"), py::arg("return_type"), py::arg("parameters"), py::arg("is_member") = false, - py::arg("impl") = nullptr); + py::arg("impl") = py::none()); } PyCelOverload::PyCelOverload(std::string overload_id, const PyCelType& return_type, std::vector parameters, bool is_member, - PyObject* py_function) + py::object py_function) : overload_id_(std::move(overload_id)), return_type_(return_type), parameters_(std::move(parameters)), is_member_(is_member), - py_function_(py_function) { - Py_XINCREF(py_function_); -} - -PyCelOverload::PyCelOverload(const PyCelOverload& other) { - overload_id_ = other.overload_id_; - return_type_ = other.return_type_; - parameters_ = other.parameters_; - is_member_ = other.is_member_; - py_function_ = other.py_function_; - Py_XINCREF(py_function_); -}; - -PyCelOverload& PyCelOverload::operator=(const PyCelOverload& other) { - overload_id_ = other.overload_id_; - return_type_ = other.return_type_; - parameters_ = other.parameters_; - is_member_ = other.is_member_; - py_function_ = other.py_function_; - Py_XINCREF(py_function_); - return *this; -} - -PyCelOverload::~PyCelOverload() { - auto gil_state = PyGILState_Ensure(); - Py_XDECREF(py_function_); - PyGILState_Release(gil_state); -} + py_function_(std::move(py_function)) {} } // namespace cel_python diff --git a/py_cel_overload.h b/py_cel_overload.h index fd8c31f..0163179 100644 --- a/py_cel_overload.h +++ b/py_cel_overload.h @@ -25,30 +25,31 @@ namespace cel_python { +namespace py = ::pybind11; + class PyCelOverload { public: static void DefinePythonBindings(pybind11::module& m); - PyCelOverload(const PyCelOverload&); - PyCelOverload& operator=(const PyCelOverload&); + PyCelOverload(const PyCelOverload&) = default; + PyCelOverload& operator=(const PyCelOverload&) = default; PyCelOverload(std::string overload_id, const PyCelType& return_type, std::vector parameters, bool is_member = false, - PyObject* py_function = nullptr); - ~PyCelOverload(); + py::object py_function = py::none()); std::string overload_id() const { return overload_id_; } PyCelType return_type() const { return return_type_; } const std::vector& parameters() const { return parameters_; } bool is_member() const { return is_member_; } - PyObject* py_function() const { return py_function_; } + py::object py_function() const { return py_function_; } private: std::string overload_id_; PyCelType return_type_; std::vector parameters_; bool is_member_; - PyObject* py_function_ = nullptr; + py::object py_function_ = py::none(); }; } // namespace cel_python diff --git a/py_cel_python_extension.cc b/py_cel_python_extension.cc index a87036a..4026b2a 100644 --- a/py_cel_python_extension.cc +++ b/py_cel_python_extension.cc @@ -112,7 +112,7 @@ absl::Status PyCelPythonExtension::ConfigureRuntime( cel::FunctionDescriptor descriptor(function.name(), overload.is_member(), types, kFunctionDescriptorOptions); - if (overload.py_function()) { + if (!overload.py_function().is_none()) { PY_CEL_RETURN_IF_ERROR(runtime_builder.function_registry().Register( descriptor, std::make_unique( function.name(), overload.return_type(), diff --git a/py_cel_type.cc b/py_cel_type.cc index 4189d57..7fcb7e1 100644 --- a/py_cel_type.cc +++ b/py_cel_type.cc @@ -41,7 +41,6 @@ #include "google/protobuf/descriptor.h" #include #include -#include "pybind11_abseil/status_casters.h" namespace cel_python { @@ -106,15 +105,15 @@ PyCelType PyCelType::ListType(const PyCelType& element_type) { return PyCelType(cel::Kind::kList, "LIST", {element_type}); } -absl::StatusOr PyCelType::MapType(const PyCelType& key_type, - const PyCelType& value_type) { +PyCelType PyCelType::MapType(const PyCelType& key_type, + const PyCelType& value_type) { if (key_type.GetKind() != cel::Kind::kBool && key_type.GetKind() != cel::Kind::kInt && key_type.GetKind() != cel::Kind::kUint && key_type.GetKind() != cel::Kind::kString && key_type.GetKind() != cel::Kind::kDyn) { - return absl::InvalidArgumentError( - absl::StrFormat("Unsupported map key type: %s", key_type.GetName())); + throw StatusToException(absl::InvalidArgumentError( + absl::StrFormat("Unsupported map key type: %s", key_type.GetName()))); } return PyCelType(cel::Kind::kMap, "MAP", {key_type, value_type}); @@ -230,7 +229,7 @@ const PyCelType& PyCelType::List() { // cel.Type.Map(cel.Type.DYN, cel.Type.DYN) aka cel.Type.MAP const PyCelType& PyCelType::Map() { static const absl::NoDestructor kInternalType( - *PyCelType::MapType(PyCelType::Dyn(), PyCelType::Dyn())); + PyCelType::MapType(PyCelType::Dyn(), PyCelType::Dyn())); return *kInternalType; } diff --git a/py_cel_type.h b/py_cel_type.h index 9357c05..d8a58ec 100644 --- a/py_cel_type.h +++ b/py_cel_type.h @@ -53,8 +53,9 @@ class PyCelType { ~PyCelType() = default; static PyCelType ListType(const PyCelType& element_type); - static absl::StatusOr MapType(const PyCelType& key_type, - const PyCelType& value_type); + // May throw if the key type is not supported. + static PyCelType MapType(const PyCelType& key_type, + const PyCelType& value_type); static PyCelType TypeType(const PyCelType& parameter); static PyCelType AbstractType(const std::string& name, const std::vector& params = {}); diff --git a/py_cel_value.cc b/py_cel_value.cc index 9d3f378..fac377f 100644 --- a/py_cel_value.cc +++ b/py_cel_value.cc @@ -48,7 +48,6 @@ #include #include #include "pybind11_abseil/absl_casters.h" -#include "pybind11_abseil/status_casters.h" namespace cel_python { @@ -57,9 +56,14 @@ namespace py = ::pybind11; void PyCelValue::DefinePythonBindings(py::module& m) { py::class_(m, "Value") .def("type", &PyCelValue::Type) - .def("value", &PyCelValue::Value, py::return_value_policy::reference) - .def("plain_value", &PyCelValue::PlainValue, - py::return_value_policy::reference) + .def("value", + [](PyCelValue& self) { + return py::reinterpret_borrow(self.Value()); + }) + .def("plain_value", + [](PyCelValue& self) { + return py::reinterpret_borrow(self.PlainValue()); + }) .def("__repr__", &PyCelValue::ToString); py::class_(m, "_CelListItemAccessor"); diff --git a/py_cel_value.h b/py_cel_value.h index b32d557..aeeef8b 100644 --- a/py_cel_value.h +++ b/py_cel_value.h @@ -30,6 +30,8 @@ namespace cel_python { +namespace py = ::pybind11; + class PyCelArena; class PyCelEnvInternal; class PyMessageFactory; diff --git a/py_error_status.cc b/py_error_status.cc index 57452b6..78ec7b3 100644 --- a/py_error_status.cc +++ b/py_error_status.cc @@ -17,8 +17,11 @@ #include // IWYU pragma: keep - Needed for PyObject +#include + #include "absl/base/no_destructor.h" #include "absl/container/flat_hash_map.h" +#include "absl/log/absl_check.h" #include "absl/log/absl_log.h" // IWYU pragma: keep - Needed for ABSL_LOG #include "absl/status/status.h" @@ -51,6 +54,40 @@ static absl::Status PyErrorToStatus(PyObject* py_type, PyObject* py_error) { return absl::Status(it->second, message); } +// A custom exception type that can be thrown from C++ to Python. +// This allows us to propagate absl::Status through pybind11 without introducing +// a dependency on pybind11_abseil:status_casters. +class PyCelError : public pybind11::builtin_exception { + public: + explicit PyCelError(const absl::Status& status) + : builtin_exception(status.ToString() + " [" + + absl::StatusCodeToString(status.code()) + "]") { + switch (status.code()) { + case absl::StatusCode::kInvalidArgument: + case absl::StatusCode::kOutOfRange: + py_err_type_ = PyExc_ValueError; + break; + case absl::StatusCode::kNotFound: + py_err_type_ = PyExc_LookupError; + break; + default: + py_err_type_ = PyExc_RuntimeError; + break; + } + } + + void set_error() const override { PyErr_SetString(py_err_type_, what()); } + + private: + PyObject* py_err_type_; +}; + +std::runtime_error StatusToException(const absl::Status& status) { + ABSL_CHECK(!status.ok()); // Crash OK: all call sites are + // local to the library. + return PyCelError(status); +} + static absl::Status& PendingPyError() { static absl::NoDestructor pending_py_error(absl::OkStatus()); return *pending_py_error; diff --git a/py_error_status.h b/py_error_status.h index 42cc903..d4f7bdd 100644 --- a/py_error_status.h +++ b/py_error_status.h @@ -16,8 +16,14 @@ #ifndef THIRD_PARTY_CEL_PYTHON_PY_ERROR_STATUS_H_ #define THIRD_PARTY_CEL_PYTHON_PY_ERROR_STATUS_H_ +#include +#include +#include + #include "absl/status/status.h" +#include "absl/status/statusor.h" #include "status_macros.h" +#include #define CEL_PYTHON_ASSIGN_OR_RETURN(...) \ PY_CEL_RETURN_IF_ERROR(PyErr_toStatus()); \ @@ -26,6 +32,16 @@ namespace cel_python { +std::runtime_error StatusToException(const absl::Status& status); + +template +T ThrowIfError(absl::StatusOr status_or) { + if (!status_or.ok()) { + throw cel_python::StatusToException(status_or.status()); + } + return std::move(*status_or); +} + absl::Status PyErr_toStatus(); // Checks if there is a pending Python exception, and if so, stores it for a