From ad3d627a685f88a6098b3fc9bc44e1b3f7fc01e3 Mon Sep 17 00:00:00 2001 From: David Gardner Date: Mon, 11 Mar 2024 12:15:44 -0700 Subject: [PATCH 1/6] Add test for issue #450 --- python/mrc/_pymrc/tests/test_utils.cpp | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/python/mrc/_pymrc/tests/test_utils.cpp b/python/mrc/_pymrc/tests/test_utils.cpp index 713bdc5f4..a40bc1a21 100644 --- a/python/mrc/_pymrc/tests/test_utils.cpp +++ b/python/mrc/_pymrc/tests/test_utils.cpp @@ -34,6 +34,7 @@ #include #include #include +#include #include #include #include @@ -143,6 +144,25 @@ TEST_F(TestUtils, CastFromPyObject) } } +TEST_F(TestUtils, CastFromPyObjectSerializeErrors) +{ + // Test to verify that cast_from_pyobject throws a python TypeError when encountering something that is not json + // serializable issue #450 + { + // Exceptions are not serializable. Keep in mind we aren't throwins a `std::runtime_error`, we are just trying + // to serialize it + py::object o = py::cast(std::runtime_error("test error")); + EXPECT_THROW(pymrc::cast_from_pyobject(o), py::type_error); + } + + { + // decimal.Decimal is not serializable + py::object Decimal = py::module_::import("decimal").attr("Decimal"); + py::object o = Decimal("1.0"); + EXPECT_THROW(pymrc::cast_from_pyobject(o), py::type_error); + } +} + TEST_F(TestUtils, PyObjectWrapper) { py::list test_list; From d2064747c5c0c8927f2564e10506b7a4761a7daf Mon Sep 17 00:00:00 2001 From: David Gardner Date: Mon, 11 Mar 2024 12:51:31 -0700 Subject: [PATCH 2/6] Throw a type error for unsupported types --- python/mrc/_pymrc/src/utils.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/python/mrc/_pymrc/src/utils.cpp b/python/mrc/_pymrc/src/utils.cpp index ba6a70584..1b9df1d93 100644 --- a/python/mrc/_pymrc/src/utils.cpp +++ b/python/mrc/_pymrc/src/utils.cpp @@ -17,6 +17,8 @@ #include "pymrc/utils.hpp" +#include "pymrc/utilities/acquire_gil.hpp" + #include #include #include @@ -170,8 +172,11 @@ json cast_from_pyobject(const py::object& source) return json(py::cast(source)); } - // else unsupported return null - return json(); + // else unsupported return throw a type error + { + AcquireGIL gil; + throw py::type_error("Object is not JSON serializable"); + } // NOLINTEND(modernize-return-braced-init-list) } From 7aaa6fea12303b1a2ac54e2ecb93de802e6e5cc5 Mon Sep 17 00:00:00 2001 From: David Gardner Date: Mon, 11 Mar 2024 13:34:37 -0700 Subject: [PATCH 3/6] Update CR year --- python/mrc/_pymrc/src/utils.cpp | 2 +- python/mrc/_pymrc/tests/test_utils.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/python/mrc/_pymrc/src/utils.cpp b/python/mrc/_pymrc/src/utils.cpp index 1b9df1d93..e084c939c 100644 --- a/python/mrc/_pymrc/src/utils.cpp +++ b/python/mrc/_pymrc/src/utils.cpp @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: Copyright (c) 2021-2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-FileCopyrightText: Copyright (c) 2021-2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved. * SPDX-License-Identifier: Apache-2.0 * * Licensed under the Apache License, Version 2.0 (the "License"); diff --git a/python/mrc/_pymrc/tests/test_utils.cpp b/python/mrc/_pymrc/tests/test_utils.cpp index a40bc1a21..e63793b6d 100644 --- a/python/mrc/_pymrc/tests/test_utils.cpp +++ b/python/mrc/_pymrc/tests/test_utils.cpp @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: Copyright (c) 2021-2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-FileCopyrightText: Copyright (c) 2021-2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved. * SPDX-License-Identifier: Apache-2.0 * * Licensed under the Apache License, Version 2.0 (the "License"); From e2d86cbf85d39014db647cb1153cc68bcf8c6d4e Mon Sep 17 00:00:00 2001 From: David Gardner Date: Tue, 12 Mar 2024 10:25:20 -0700 Subject: [PATCH 4/6] Add str repr, type name and json path to error message --- python/mrc/_pymrc/include/pymrc/utils.hpp | 15 ++++++ python/mrc/_pymrc/src/utils.cpp | 62 +++++++++++++++++++++-- python/mrc/_pymrc/tests/test_utils.cpp | 21 ++++---- 3 files changed, 82 insertions(+), 16 deletions(-) diff --git a/python/mrc/_pymrc/include/pymrc/utils.hpp b/python/mrc/_pymrc/include/pymrc/utils.hpp index f80838c3d..466889012 100644 --- a/python/mrc/_pymrc/include/pymrc/utils.hpp +++ b/python/mrc/_pymrc/include/pymrc/utils.hpp @@ -54,6 +54,21 @@ void from_import_as(pybind11::module_& dest, const std::string& from, const std: */ const std::type_info* cpptype_info_from_object(pybind11::object& obj); +/** + * @brief Given a pybind11 object, return the Python type name essentially the same as `str(type(obj))` + * @param obj : pybind11 object + * @return std::string. + */ +std::string get_py_type_name(const pybind11::object& obj, bool ignore_exceptions = true); + +/** + * @brief Given a pybind11 object, return the Python string representation essentially the same as `str(obj)` + * @param obj : pybind11 object + * @param ignore_exceptions : if true, exceptions will be caught and the return value will be an empty string. + * @return std::string. + */ +std::string as_string(const pybind11::object& obj, bool ignore_exceptions = true); + void show_deprecation_warning(const std::string& deprecation_message, ssize_t stack_level = 1); #pragma GCC visibility pop diff --git a/python/mrc/_pymrc/src/utils.cpp b/python/mrc/_pymrc/src/utils.cpp index e084c939c..28e49fa0a 100644 --- a/python/mrc/_pymrc/src/utils.cpp +++ b/python/mrc/_pymrc/src/utils.cpp @@ -19,6 +19,7 @@ #include "pymrc/utilities/acquire_gil.hpp" +#include #include #include #include @@ -27,6 +28,7 @@ #include #include +#include #include #include @@ -74,6 +76,39 @@ const std::type_info* cpptype_info_from_object(py::object& obj) return nullptr; } +std::string get_py_type_name(const pybind11::object& obj, bool ignore_exceptions) +{ + try + { + const auto py_type = py::type::of(obj); + return py_type.attr("__name__").cast(); + } catch (...) + { + if (!ignore_exceptions) + { + throw; + } + } + + return ""; +} + +std::string as_string(const pybind11::object& obj, bool ignore_exceptions) +{ + try + { + return py::str(obj).cast(); + } catch (...) + { + if (!ignore_exceptions) + { + throw; + } + } + + return ""; +} + py::object cast_from_json(const json& source) { if (source.is_null()) @@ -125,7 +160,7 @@ py::object cast_from_json(const json& source) // throw std::runtime_error("Unsupported conversion type."); } -json cast_from_pyobject(const py::object& source) +json cast_from_pyobject_impl(const py::object& source, const std::string& parent_path = "") { // Dont return via initializer list with JSON. It performs type deduction and gives different results // NOLINTBEGIN(modernize-return-braced-init-list) @@ -139,7 +174,9 @@ json cast_from_pyobject(const py::object& source) auto json_obj = json::object(); for (const auto& p : py_dict) { - json_obj[py::cast(p.first)] = cast_from_pyobject(p.second.cast()); + std::string key{p.first.cast()}; + std::string path{parent_path + "/" + key}; + json_obj[key] = cast_from_pyobject_impl(p.second.cast(), path); } return json_obj; @@ -150,7 +187,7 @@ json cast_from_pyobject(const py::object& source) auto json_arr = json::array(); for (const auto& p : py_list) { - json_arr.push_back(cast_from_pyobject(p.cast())); + json_arr.push_back(cast_from_pyobject_impl(p.cast(), parent_path)); } return json_arr; @@ -175,11 +212,28 @@ json cast_from_pyobject(const py::object& source) // else unsupported return throw a type error { AcquireGIL gil; - throw py::type_error("Object is not JSON serializable"); + std::ostringstream error_message; + std::string path{parent_path}; + if (path.empty()) + { + path = "/"; + } + + error_message << "Object (" << as_string(source, true) << ") of type: " << get_py_type_name(source, true) + << " at path: " << path << " is not JSON serializable"; + + DVLOG(5) << error_message.str(); + throw py::type_error(error_message.str()); } + // NOLINTEND(modernize-return-braced-init-list) } +json cast_from_pyobject(const py::object& source) +{ + return cast_from_pyobject_impl(source); +} + void show_deprecation_warning(const std::string& deprecation_message, ssize_t stack_level) { PyErr_WarnEx(PyExc_DeprecationWarning, deprecation_message.c_str(), stack_level); diff --git a/python/mrc/_pymrc/tests/test_utils.cpp b/python/mrc/_pymrc/tests/test_utils.cpp index e63793b6d..8ff6b4c40 100644 --- a/python/mrc/_pymrc/tests/test_utils.cpp +++ b/python/mrc/_pymrc/tests/test_utils.cpp @@ -42,6 +42,7 @@ namespace py = pybind11; namespace pymrc = mrc::pymrc; using namespace std::string_literals; +using namespace pybind11::literals; // to bring in the `_a` literal // Create values too big to fit in int & float types to ensure we can pass // long & double types to both nlohmann/json and python @@ -148,19 +149,15 @@ TEST_F(TestUtils, CastFromPyObjectSerializeErrors) { // Test to verify that cast_from_pyobject throws a python TypeError when encountering something that is not json // serializable issue #450 - { - // Exceptions are not serializable. Keep in mind we aren't throwins a `std::runtime_error`, we are just trying - // to serialize it - py::object o = py::cast(std::runtime_error("test error")); - EXPECT_THROW(pymrc::cast_from_pyobject(o), py::type_error); - } - { - // decimal.Decimal is not serializable - py::object Decimal = py::module_::import("decimal").attr("Decimal"); - py::object o = Decimal("1.0"); - EXPECT_THROW(pymrc::cast_from_pyobject(o), py::type_error); - } + // decimal.Decimal is not serializable + py::object Decimal = py::module_::import("decimal").attr("Decimal"); + py::object o = Decimal("1.0"); + EXPECT_THROW(pymrc::cast_from_pyobject(o), py::type_error); + + // Test with object in a nested dict + py::dict d("a"_a = py::dict("b"_a = py::dict("c"_a = py::dict("d"_a = o))), "other"_a = 2); + EXPECT_THROW(pymrc::cast_from_pyobject(d), py::type_error); } TEST_F(TestUtils, PyObjectWrapper) From 4922ad8726729fb756a498590b1533fbb33ebfe1 Mon Sep 17 00:00:00 2001 From: David Gardner Date: Tue, 12 Mar 2024 10:56:29 -0700 Subject: [PATCH 5/6] Remove ignore_exceptions since the error was an abort not a thrown exception Remove uneeded as_string method --- python/mrc/_pymrc/include/pymrc/utils.hpp | 10 +------ python/mrc/_pymrc/src/utils.cpp | 35 +++++------------------ python/mrc/_pymrc/tests/test_utils.cpp | 12 +++++++- 3 files changed, 19 insertions(+), 38 deletions(-) diff --git a/python/mrc/_pymrc/include/pymrc/utils.hpp b/python/mrc/_pymrc/include/pymrc/utils.hpp index 466889012..8ac5edc10 100644 --- a/python/mrc/_pymrc/include/pymrc/utils.hpp +++ b/python/mrc/_pymrc/include/pymrc/utils.hpp @@ -59,15 +59,7 @@ const std::type_info* cpptype_info_from_object(pybind11::object& obj); * @param obj : pybind11 object * @return std::string. */ -std::string get_py_type_name(const pybind11::object& obj, bool ignore_exceptions = true); - -/** - * @brief Given a pybind11 object, return the Python string representation essentially the same as `str(obj)` - * @param obj : pybind11 object - * @param ignore_exceptions : if true, exceptions will be caught and the return value will be an empty string. - * @return std::string. - */ -std::string as_string(const pybind11::object& obj, bool ignore_exceptions = true); +std::string get_py_type_name(const pybind11::object& obj); void show_deprecation_warning(const std::string& deprecation_message, ssize_t stack_level = 1); diff --git a/python/mrc/_pymrc/src/utils.cpp b/python/mrc/_pymrc/src/utils.cpp index 28e49fa0a..02b94a269 100644 --- a/python/mrc/_pymrc/src/utils.cpp +++ b/python/mrc/_pymrc/src/utils.cpp @@ -76,37 +76,16 @@ const std::type_info* cpptype_info_from_object(py::object& obj) return nullptr; } -std::string get_py_type_name(const pybind11::object& obj, bool ignore_exceptions) +std::string get_py_type_name(const pybind11::object& obj) { - try + if (!obj) { - const auto py_type = py::type::of(obj); - return py_type.attr("__name__").cast(); - } catch (...) - { - if (!ignore_exceptions) - { - throw; - } - } - - return ""; -} - -std::string as_string(const pybind11::object& obj, bool ignore_exceptions) -{ - try - { - return py::str(obj).cast(); - } catch (...) - { - if (!ignore_exceptions) - { - throw; - } + // calling py::type::of on a null object will trigger an abort + return ""; } - return ""; + const auto py_type = py::type::of(obj); + return py_type.attr("__name__").cast(); } py::object cast_from_json(const json& source) @@ -219,7 +198,7 @@ json cast_from_pyobject_impl(const py::object& source, const std::string& parent path = "/"; } - error_message << "Object (" << as_string(source, true) << ") of type: " << get_py_type_name(source, true) + error_message << "Object (" << py::str(source).cast() << ") of type: " << get_py_type_name(source) << " at path: " << path << " is not JSON serializable"; DVLOG(5) << error_message.str(); diff --git a/python/mrc/_pymrc/tests/test_utils.cpp b/python/mrc/_pymrc/tests/test_utils.cpp index 8ff6b4c40..e518bbd87 100644 --- a/python/mrc/_pymrc/tests/test_utils.cpp +++ b/python/mrc/_pymrc/tests/test_utils.cpp @@ -34,7 +34,6 @@ #include #include #include -#include #include #include #include @@ -160,6 +159,17 @@ TEST_F(TestUtils, CastFromPyObjectSerializeErrors) EXPECT_THROW(pymrc::cast_from_pyobject(d), py::type_error); } +TEST_F(TestUtils, GetTypeName) +{ + // invalid objects should return an empty string + EXPECT_EQ(pymrc::get_py_type_name(py::object()), ""); + EXPECT_EQ(pymrc::get_py_type_name(py::none()), "NoneType"); + + py::object Decimal = py::module_::import("decimal").attr("Decimal"); + py::object o = Decimal("1.0"); + EXPECT_EQ(pymrc::get_py_type_name(o), "Decimal"); +} + TEST_F(TestUtils, PyObjectWrapper) { py::list test_list; From 610c58c24297f0ae9599d61a621c2613a4f9537e Mon Sep 17 00:00:00 2001 From: David Gardner Date: Tue, 12 Mar 2024 10:58:39 -0700 Subject: [PATCH 6/6] Update CR year --- python/mrc/_pymrc/include/pymrc/utils.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/mrc/_pymrc/include/pymrc/utils.hpp b/python/mrc/_pymrc/include/pymrc/utils.hpp index 8ac5edc10..fbfe2e02f 100644 --- a/python/mrc/_pymrc/include/pymrc/utils.hpp +++ b/python/mrc/_pymrc/include/pymrc/utils.hpp @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: Copyright (c) 2021-2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-FileCopyrightText: Copyright (c) 2021-2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved. * SPDX-License-Identifier: Apache-2.0 * * Licensed under the Apache License, Version 2.0 (the "License");