-
Notifications
You must be signed in to change notification settings - Fork 3k
[core] Make the internal implementation of Core.compile_model(fs::path) the base implementation #33740
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
[core] Make the internal implementation of Core.compile_model(fs::path) the base implementation #33740
Conversation
| CompiledModel compile_model(const std::string& model_path, const AnyMap& properties = {}); | ||
|
|
||
| template <class Path, std::enable_if_t<std::is_same_v<Path, std::filesystem::path>>* = nullptr> | ||
| auto compile_model(const Path& model_path, const AnyMap& properties = {}) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead remove it make if constexpr for string or wstring to call correct resolve overload use
| const std::string& device_name, | ||
| const AnyMap& properties = {}); | ||
|
|
||
| template <class Path, std::enable_if_t<std::is_constructible_v<std::wstring, Path>>* = nullptr> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is possible to remove this enable_if and resolve it by single template compile_model(const Path& model_path,
…into almilosz/compile_model_path
This reverts commit 31f50f5.
| CompiledModel compile_model(const std::wstring& model_path, const AnyMap& properties = {}); | ||
| #endif | ||
| template <class Path, std::enable_if_t<std::is_constructible_v<std::string, Path>>* = nullptr> | ||
| CompiledModel compile_model(const Path& model_path, const AnyMap& properties = {}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is wstring version is handled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After I moved the implementation to std::filesystem::path, the "wstring version" was just explicitly calling "std::filesystem::version version" without any helpers like wstring_to_string. Now it's done implicitly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wstring -> path conversion may not work correctly in some cases, the test should be created to confirm it (see file_path.cpp) where there is dedicated constructor.
It safer to keep wstring version and make call by usingov::util::make_path conversion.
| template <typename... Properties> | ||
| util::EnableIfAllStringAny<CompiledModel, Properties...> compile_model(const std::string& model_path, | ||
| template <class Path, class... Properties, std::enable_if_t<std::is_constructible_v<std::string, Path>>* = nullptr> | ||
| util::EnableIfAllStringAny<CompiledModel, Properties...> compile_model(const Path& model_path, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about wstring version?
…losz/openvino into almilosz/compile_model_path
| } | ||
|
|
||
| TEST_F(CoreBaseTest, compile_model) { | ||
| generate_test_model_files("model2"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What special unicode characters are created in path?
why not use similar patter from test TEST_P(FileUtilTestP, create_directories) the test values can be re-used in other tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to reuse model_files_name_w, weight_files_name_w
| std::vector<std::wstring> model_files_name_w, weight_files_name_w; |
| const auto model = core.compile_model(model_path, devices.at(0), ov::AnyMap{}); | ||
| EXPECT_TRUE(model); | ||
| } | ||
| #ifdef OPENVINO_ENABLE_UNICODE_PATH_SUPPORT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be avoided similar like in TEST_P(FileUtilTestP, create_directories) test
| const auto model_path = model_file_name; | ||
| const auto weight_path = weight_file_name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why copy is required?
| ov::SoPtr<ov::ICompiledModel> ov::Plugin::compile_model(const std::filesystem::path& model_path, | ||
| const ov::AnyMap& properties) const { | ||
| OV_PLUGIN_CALL_STATEMENT(return {m_ptr->compile_model(model_path, properties), m_so}); | ||
| OV_PLUGIN_CALL_STATEMENT(return {m_ptr->compile_model(ov::util::path_to_string(model_path), properties), m_so}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this path conversion will be remove when plugin interface will be updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
Details:
openvino/src/inference/src/dev/plugin.cpp
Line 59 in 626f1c6
openvino/src/inference/src/dev/core_impl.cpp
Line 1731 in 626f1c6
Tickets: