From fe406d16066b89f7facad9b89f9354917a7e25f5 Mon Sep 17 00:00:00 2001 From: Mateusz Bencer Date: Fri, 18 Mar 2022 14:29:46 +0100 Subject: [PATCH] Cpp fix of python segfault, reverted pybind workaround (#10749) * test fix of segfault * styles applied * added keep_alive to pybind * remove redundant code * fix json tests * review remarks * introduced correct path to dlls in CI * removing passing path via env variable * introduced cpp solution * remove keep alive * review remarks * remove explicit removing model * removed shared_objects from ir frontend * core test updated * unified approach to handle extensions by frontends * added nullptr check * Revert "added nullptr check" This reverts commit 666f5e4489c014c5d2d47eb83a3497ccfc7b4f5d. * Revert "unified approach to handle extensions by frontends" This reverts commit bf85ac24a62566990b5a7b52d369f050402ae70b. * m_extensions declaration in Frontend * added assert --- .../python/src/pyopenvino/frontend/frontend.cpp | 3 --- .../tests/test_inference_engine/test_core.py | 6 ++---- .../include/openvino/frontend/frontend.hpp | 2 ++ src/frontends/common/src/frontend.cpp | 17 ++++------------- src/frontends/common/src/manager.cpp | 8 ++++---- src/frontends/common/src/plugin_loader.hpp | 17 +++++++++++++++++ .../include/openvino/frontend/ir/frontend.hpp | 2 -- src/frontends/ir/src/frontend.cpp | 7 +++---- .../openvino/frontend/paddle/frontend.hpp | 4 ---- .../openvino/frontend/tensorflow/frontend.hpp | 4 ---- 10 files changed, 32 insertions(+), 38 deletions(-) diff --git a/src/bindings/python/src/pyopenvino/frontend/frontend.cpp b/src/bindings/python/src/pyopenvino/frontend/frontend.cpp index f6739881b21..48829a01794 100644 --- a/src/bindings/python/src/pyopenvino/frontend/frontend.cpp +++ b/src/bindings/python/src/pyopenvino/frontend/frontend.cpp @@ -38,7 +38,6 @@ void regclass_frontend_FrontEnd(py::module m) { fem.def("convert", static_cast (FrontEnd::*)(const InputModel::Ptr&) const>(&FrontEnd::convert), py::arg("model"), - py::keep_alive<0, 1>(), R"( Completely convert and normalize entire function, throws if it is not possible. @@ -63,7 +62,6 @@ void regclass_frontend_FrontEnd(py::module m) { fem.def("convert_partially", &FrontEnd::convert_partially, py::arg("model"), - py::keep_alive<0, 1>(), R"( Convert only those parts of the model that can be converted leaving others as-is. Converted parts are not normalized by additional transformations; normalize function or @@ -78,7 +76,6 @@ void regclass_frontend_FrontEnd(py::module m) { fem.def("decode", &FrontEnd::decode, py::arg("model"), - py::keep_alive<0, 1>(), R"( Convert operations with one-to-one mapping with decoding nodes. Each decoding node is an nGraph node representing a single FW operation node with diff --git a/src/bindings/python/tests/test_inference_engine/test_core.py b/src/bindings/python/tests/test_inference_engine/test_core.py index 0c23897a06b..6340e7c30c7 100644 --- a/src/bindings/python/tests/test_inference_engine/test_core.py +++ b/src/bindings/python/tests/test_inference_engine/test_core.py @@ -315,10 +315,8 @@ def test_add_extension_template_extension(device): new_shapes = {"in_data": after_reshape} assert model.input().partial_shape == before_reshape model.reshape(new_shapes) - assert model.input().partial_shape == after_reshape - - # CVS-74584 - del model + compiled = core.compile_model(model, device) + assert compiled.input().partial_shape == after_reshape def test_add_extension(): diff --git a/src/frontends/common/include/openvino/frontend/frontend.hpp b/src/frontends/common/include/openvino/frontend/frontend.hpp index d2b781e441d..6f76e978287 100644 --- a/src/frontends/common/include/openvino/frontend/frontend.hpp +++ b/src/frontends/common/include/openvino/frontend/frontend.hpp @@ -148,6 +148,8 @@ protected: virtual InputModel::Ptr load_impl(const std::vector& variants) const; + std::vector m_extensions; + private: static std::shared_ptr create_copy(const std::shared_ptr& ov_model, const std::shared_ptr& shared_object); diff --git a/src/frontends/common/src/frontend.cpp b/src/frontends/common/src/frontend.cpp index 9728783b4d0..cbc650ed1f4 100644 --- a/src/frontends/common/src/frontend.cpp +++ b/src/frontends/common/src/frontend.cpp @@ -9,6 +9,7 @@ #include "openvino/frontend/extension/op.hpp" #include "openvino/frontend/manager.hpp" #include "openvino/frontend/place.hpp" +#include "plugin_loader.hpp" #include "so_extension.hpp" #include "utils.hpp" @@ -74,6 +75,7 @@ void FrontEnd::normalize(const std::shared_ptr& model) const { void FrontEnd::add_extension(const std::shared_ptr& extension) { if (m_actual) { + add_extension_to_shared_data(m_shared_object, extension); m_actual->add_extension(extension); return; } @@ -82,28 +84,17 @@ void FrontEnd::add_extension(const std::shared_ptr& extension) { } void FrontEnd::add_extension(const std::vector>& extensions) { - if (m_actual) { - m_actual->add_extension(extensions); - return; - } - for (const auto& ext : extensions) + for (const auto& ext : extensions) { add_extension(ext); + } } void FrontEnd::add_extension(const std::string& library_path) { - if (m_actual) { - m_actual->add_extension(library_path); - return; - } add_extension(ov::detail::load_extensions(library_path)); } #ifdef OPENVINO_ENABLE_UNICODE_PATH_SUPPORT void FrontEnd::add_extension(const std::wstring& library_path) { - if (m_actual) { - m_actual->add_extension(library_path); - return; - } add_extension(ov::detail::load_extensions(library_path)); } #endif diff --git a/src/frontends/common/src/manager.cpp b/src/frontends/common/src/manager.cpp index e1cf28ac1f2..7630500434e 100644 --- a/src/frontends/common/src/manager.cpp +++ b/src/frontends/common/src/manager.cpp @@ -45,7 +45,7 @@ public: if (plugin_it != m_plugins.end()) { if (plugin_it->load()) { auto fe_obj = std::make_shared(); - fe_obj->m_shared_object = plugin_it->get_so_pointer(); + fe_obj->m_shared_object = std::make_shared(plugin_it->get_so_pointer()); fe_obj->m_actual = plugin_it->get_creator().m_creator(); return fe_obj; } @@ -56,7 +56,7 @@ public: OPENVINO_ASSERT(plugin.load(), "Cannot load frontend ", plugin.get_name_from_file()); if (plugin.get_creator().m_name == framework) { auto fe_obj = std::make_shared(); - fe_obj->m_shared_object = plugin.get_so_pointer(); + fe_obj->m_shared_object = std::make_shared(plugin.get_so_pointer()); fe_obj->m_actual = plugin.get_creator().m_creator(); return fe_obj; } @@ -93,7 +93,7 @@ public: OPENVINO_ASSERT(fe, "Frontend error: frontend '", plugin.get_creator().m_name, "' created null FrontEnd"); if (fe->supported(variants)) { auto fe_obj = std::make_shared(); - fe_obj->m_shared_object = plugin.get_so_pointer(); + fe_obj->m_shared_object = std::make_shared(plugin.get_so_pointer()); fe_obj->m_actual = fe; return fe_obj; } @@ -185,7 +185,7 @@ private: if (fe && fe->supported(variants)) { // Priority FE (e.g. IR) is found and is suitable auto fe_obj = std::make_shared(); - fe_obj->m_shared_object = plugin_info.get_so_pointer(); + fe_obj->m_shared_object = std::make_shared(plugin_it->get_so_pointer()); fe_obj->m_actual = fe; return fe_obj; } diff --git a/src/frontends/common/src/plugin_loader.hpp b/src/frontends/common/src/plugin_loader.hpp index 0e48b8f9ade..5762aefb63c 100644 --- a/src/frontends/common/src/plugin_loader.hpp +++ b/src/frontends/common/src/plugin_loader.hpp @@ -15,6 +15,23 @@ static const char PathSeparator[] = ":"; namespace ov { namespace frontend { +/// \brief Internal data structure holding by each frontend. Includes library handle and extensions. +class FrontEndSharedData { + friend inline void add_extension_to_shared_data(std::shared_ptr& obj, + const std::shared_ptr& ext); + std::shared_ptr m_so; + std::vector> m_loaded_extensions = {}; + +public: + explicit FrontEndSharedData(const std::shared_ptr& so) : m_so(so) {} +}; + +inline void add_extension_to_shared_data(std::shared_ptr& obj, const std::shared_ptr& ext) { + auto obj_data = std::static_pointer_cast(obj); + OPENVINO_ASSERT(obj_data, "internal error: not allowed type of shared data used"); + obj_data->m_loaded_extensions.push_back(ext); +} + /// \brief Internal data structure holding plugin information including library handle, file names and paths, etc. class PluginInfo { std::shared_ptr m_so; // Library shared object, must be first data member to be destroyed last diff --git a/src/frontends/ir/include/openvino/frontend/ir/frontend.hpp b/src/frontends/ir/include/openvino/frontend/ir/frontend.hpp index 0bb38a6a9a0..8f6b5025e15 100644 --- a/src/frontends/ir/include/openvino/frontend/ir/frontend.hpp +++ b/src/frontends/ir/include/openvino/frontend/ir/frontend.hpp @@ -45,8 +45,6 @@ protected: InputModel::Ptr load_impl(const std::vector& params) const override; private: - std::vector> shared_objects; - std::vector extensions; std::shared_ptr m_telemetry; }; diff --git a/src/frontends/ir/src/frontend.cpp b/src/frontends/ir/src/frontend.cpp index fd3725a1b10..967f9b7da99 100644 --- a/src/frontends/ir/src/frontend.cpp +++ b/src/frontends/ir/src/frontend.cpp @@ -105,11 +105,10 @@ void FrontEnd::add_extension(const ov::Extension::Ptr& ext) { m_telemetry = telemetry; } else if (auto so_ext = std::dynamic_pointer_cast(ext)) { if (std::dynamic_pointer_cast(so_ext->extension())) { - shared_objects.emplace_back(so_ext->shared_object()); - extensions.emplace_back(so_ext->extension()); + m_extensions.emplace_back(so_ext->extension()); } } else if (std::dynamic_pointer_cast(ext)) - extensions.emplace_back(ext); + m_extensions.emplace_back(ext); } InputModel::Ptr FrontEnd::load_impl(const std::vector& variants) const { @@ -119,7 +118,7 @@ InputModel::Ptr FrontEnd::load_impl(const std::vector& variants) const auto create_extensions_map = [&]() -> std::unordered_map { std::unordered_map exts; - for (const auto& ext : extensions) { + for (const auto& ext : m_extensions) { if (auto base_ext = std::dynamic_pointer_cast(ext)) exts.insert({base_ext->get_type_info(), base_ext}); } diff --git a/src/frontends/paddle/include/openvino/frontend/paddle/frontend.hpp b/src/frontends/paddle/include/openvino/frontend/paddle/frontend.hpp index b0d73094229..8b083fb8c3a 100644 --- a/src/frontends/paddle/include/openvino/frontend/paddle/frontend.hpp +++ b/src/frontends/paddle/include/openvino/frontend/paddle/frontend.hpp @@ -77,10 +77,6 @@ protected: std::function(const std::map>&, const std::shared_ptr&)> func); - // m_extensions should be the first member here, - // m_extensions can contain SO Extension (holder for other Extensions), - // so it should be released last. - std::vector m_extensions; TelemetryExtension::Ptr m_telemetry; std::vector m_transformation_extensions; std::vector m_conversion_extensions; diff --git a/src/frontends/tensorflow/include/openvino/frontend/tensorflow/frontend.hpp b/src/frontends/tensorflow/include/openvino/frontend/tensorflow/frontend.hpp index 296fd88daf7..d0bdd3ce201 100644 --- a/src/frontends/tensorflow/include/openvino/frontend/tensorflow/frontend.hpp +++ b/src/frontends/tensorflow/include/openvino/frontend/tensorflow/frontend.hpp @@ -73,10 +73,6 @@ protected: bool no_conversion, std::shared_ptr& ng_function) const; - // m_extensions should be the first member here, - // m_extensions can contain SO Extension (holder for other Extensions), - // so it should be released last. - std::vector m_extensions; TelemetryExtension::Ptr m_telemetry; std::vector m_transformation_extensions; std::vector m_conversion_extensions;