[Py OV] Unify system path handling in Python API#34195
[Py OV] Unify system path handling in Python API#34195almilosz wants to merge 14 commits intoopenvinotoolkit:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces improved path handling in the OpenVINO Python API to support pathlib.Path and bytes types in addition to strings. The implementation adds a new to_fs_path() helper function that converts Python path objects to std::filesystem::path, which properly handles Unicode paths across platforms.
Changes:
- Added
to_fs_path()helper to convert Python path objects (str, bytes, pathlib.Path) tostd::filesystem::path - Updated Core::compile_model, Core::read_model, and Core::add_extension to use the new path handling
- Updated FrontEndManager::register_front_end and FrontEnd::add_extension to accept flexible path types
- Updated documentation to reflect support for
Union[str, bytes, pathlib.Path]where applicable
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/bindings/python/src/pyopenvino/graph/node_factory.cpp | NodeFactory::add_extension has new to_fs_path() code commented out; still using old approach |
| src/bindings/python/src/pyopenvino/frontend/manager.cpp | FrontEndManager::register_front_end successfully migrated to to_fs_path(); load_by_model has new code commented out |
| src/bindings/python/src/pyopenvino/frontend/frontend.cpp | FrontEnd::add_extension migrated to to_fs_path(); load and supported methods have new code commented out |
| src/bindings/python/src/pyopenvino/core/core.cpp | Core::compile_model, read_model, and add_extension successfully migrated to to_fs_path() with minor issues |
Comments suppressed due to low confidence (2)
src/bindings/python/src/pyopenvino/frontend/frontend.cpp:52
- The commented-out code using
to_fs_path()should be uncommented and used to replace the manual string conversion and Windows-specific wstring handling. Thestd::filesystem::pathtype with pybind11's automatic conversion already handles Unicode paths correctly across platforms, including Windows. The current manual conversion and conditional wstring usage adds unnecessary complexity.
However, note that the current code has special handling for bytes inputs (line 52) where it passes enable_mmap as a second parameter to a different overload. If this is intentional behavior that should be preserved, the logic would need to be restructured to use to_fs_path() while still handling this case.
// return self.load(Common::utils::to_fs_path(py_obj));
// check if model path is either a string/pathlib.Path/bytes
std::string model_path = Common::utils::convert_path_to_string(py_obj);
if (py::isinstance(py_obj, py::module_::import("pathlib").attr("Path")) ||
py::isinstance<py::str>(py_obj)) {
// Fix unicode path
#if defined(OPENVINO_ENABLE_UNICODE_PATH_SUPPORT) && defined(_WIN32)
return self.load(ov::util::string_to_wstring(model_path.c_str()));
#else
return self.load(model_path.c_str());
#endif
}
return self.load(model_path, enable_mmap);
src/bindings/python/src/pyopenvino/frontend/frontend.cpp:95
- The commented-out code using
to_fs_path()should be uncommented and used to replace the manual string conversion and Windows-specific wstring handling. Thestd::filesystem::pathtype with pybind11's automatic conversion already handles Unicode paths correctly across platforms, including Windows. The current manual conversion and conditional wstring usage adds unnecessary complexity and is inconsistent with the approach used elsewhere in this PR.
// return self.supported(Common::utils::to_fs_path(model));
// check if model path is either a string/pathlib.Path/bytes
std::string model_path = Common::utils::convert_path_to_string(model);
if (py::isinstance(model, py::module_::import("pathlib").attr("Path")) ||
py::isinstance<py::str>(model)) {
// Fix unicode path
#if defined(OPENVINO_ENABLE_UNICODE_PATH_SUPPORT) && defined(_WIN32)
return self.supported(ov::util::string_to_wstring(model_path.c_str()));
#else
return self.supported(model_path.c_str());
#endif
}
…into almilosz/unify-python-api-path
…into almilosz/unify-python-api-path
p-wysocki
left a comment
There was a problem hiding this comment.
Discussed offline, LGTM
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/bindings/python/src/pyopenvino/core/core.cpp:687
Core.add_extension(library_path)is now bound tostd::filesystem::path, but the docstring still states:type library_path: str. Since the binding will acceptpathlib.Path(and likelybytesvia the filesystem caster), please update the documented type to match the actual accepted inputs (e.g.,Union[str, bytes, pathlib.Path]).
cls.def("add_extension",
static_cast<void (ov::Core::*)(const std::filesystem::path&)>(&ov::Core::add_extension),
py::arg("library_path"),
R"(
Registers an extension to a Core object.
:param library_path: Path to library with ov::Extension
:type library_path: str
)");
| void add_extension(const std::string& library_path); | ||
|
|
||
| void add_extension(const std::filesystem::path& library_path) { | ||
| add_extension(library_path.string()); | ||
| add_extension(library_path.native()); | ||
| } |
Details:
convert_path_to_string()in favour of to_fs_path(). Use to_fs_path() consistently across the codebase.pathlib.Pathto Core::add_extension and FrontEndManager::register_front_end. Add testsFrontEnd::load()andFrontEnd::supported()is tested e.g. intest_onnx.py::TestUnicodePathsONNXpr24801. Replace the usage ofCommon::utils::convert_path_to_string()and the extra Windows-specific fix with theCommon::utils::to_fs_path().enable_mmapparameter toFrontEnd::load(). It was previously omitted in a #if defined block.pyopenvino'sFrontEnd.add_extenstion to useto_fs_path, the testtest_frontend_onnx.py::test_add_extension_unicode_pathsfailed with the error:"No mapping for the Unicode character exists".
This issue was resolved by using
.native()in the C++ implementation of FrontEnd::add_extension()Tickets: