[ONNX] Add ONNX specific FrontEnd classes #6615

mateusztabaka · 2021-07-13T05:36:10Z

This change implements a subset of FrontEnd API for ONNX:
Changes:

move onnx_common, onnx_editor and onnx_import to ngraph/frontend/onnx
introduce new target 'onnx_frontend_ngraph' located in ngraph/frontend/onnx/frontend
new target implements subset of FrontEnd, InputModel and Place interfaces

Ticket: 51678

This change implements a subset of FrontEnd API for ONNX: Changes: - move onnx_common, onnx_editor and onnx_import to ngraph/frontend/onnx - introduce new target 'onnx_frontend_ngraph' located in ngraph/frontend/onnx/frontend - new target implements subset of FrontEnd, InputModel and Place interfaces

ngraph/python/tests/test_ngraph/test_frontend_onnx.py

ngraph/frontend/onnx/CMakeLists.txt

ngraph/CMakeLists.txt

nosovmik

Please rebase to latest master, I see other PR for static protobuf is already merged

ngraph/frontend/onnx/onnx_editor/include/onnx_editor/editor.hpp

ngraph/python/tests/test_frontend/test_frontend_onnx.py

ngraph/frontend/onnx/frontend/include/onnx_frontend/input_model.hpp

ngraph/frontend/onnx/frontend/src/frontend.cpp

nosovmik · 2021-07-19T17:43:35Z

ngraph/frontend/onnx/frontend/src/input_model.cpp

+    return ret;
+}
+
+Place::Ptr InputModelONNX::get_place_by_tensor_name(const std::string& tensor_name) const


Will these methods implement some use case for Model Optimizer conversion? I can't find Place::is_equal implementation which is mandatory for options like '--input'

It will be done in separate PR

nosovmik · 2021-07-19T17:54:17Z

cmake/features.cmake

@@ -121,7 +121,7 @@ else()
 endif()

 ie_dependent_option(NGRAPH_ONNX_IMPORT_ENABLE "Enable ONNX importer" ON "protoc_available" OFF)
-ie_dependent_option(NGRAPH_ONNX_EDITOR_ENABLE "Enable ONNX Editor" ON "NGRAPH_ONNX_IMPORT_ENABLE" OFF)
+ie_dependent_option(NGRAPH_ONNX_FRONTEND_ENABLE "Enable ONNX FrontEnd" OFF "NGRAPH_ONNX_IMPORT_ENABLE" OFF)


Can frontend be enabled by default everywhere when NGRAPH_ONNX_IMPORT_ENABLE is ON? This is to make sure that 'frontend' can be correctly built on all supported platforms

Agree, don't see a reason how have multiple options about the similar stuff

Can frontend be enabled by default everywhere when NGRAPH_ONNX_IMPORT_ENABLE is ON?
That'd make MO to use ONNX frontend by default, which I don't think we want it yet

In this case, maybe set name for onnx frontend as something temporary, like "onnx_experimental"? So that it will not conflict with legacy 'onnx' name in MO. I just want to be sure that there are no build issues of onnx frontend on supported platforms

Can frontend be enabled by default everywhere when NGRAPH_ONNX_IMPORT_ENABLE is ON? This is to make sure that 'frontend' can be correctly built on all supported platforms

The feature is currently under development. We have separate PRs for testing against CI with the feature turned ON and we'll merge them after we complete the feature. For now the feature will be turned OFF be defautlt.

How can we merge changes when we don't know if they are even compilable?

We manually enable NGRAPH_ONNX_FRONTEND_ENABLE=ON in a number of CI environments. We are just not ready to turn it on by default, especially in the context of Model Optimizer.

slyalin

Can we measure what is the binary size for all ONNX related libraries modified by this PR in comparison to master version?

ngraph/frontend/frontend_manager/include/frontend_manager/frontend.hpp

ngraph/frontend/onnx/frontend/include/onnx_frontend/place.hpp

ngraph/frontend/onnx/frontend/include/onnx_frontend/input_model.hpp

slyalin · 2021-07-20T10:50:04Z

ngraph/frontend/onnx/onnx_import/include/onnx_import/core/model.hpp

+        std::int64_t get_opset_version(const ONNX_NAMESPACE::ModelProto& model_proto,
+                                       const std::string& domain);
+
+        class Model


Can we merge a separate onnx_import directly to ONNX FE and prepare for onnx_import API deprecation having ONNX FE as a replacement that is aligned with other FWs? In this case this Model class as well as others become a kind of internal API inside ONNX FE.

ngraph/frontend/onnx/onnx_import/include/onnx_import/utils/onnx_internal.hpp

nosovmik · 2021-07-20T12:00:11Z

ngraph/frontend/onnx/frontend/src/frontend.cpp

+extern "C" FRONTEND_API void* GetFrontEndData()
+{
+    FrontEndPluginInfo* res = new FrontEndPluginInfo();
+    res->m_name = "onnx";


My proposal is to change this name to 'onnx_experimental' so that MO will not try to use it for existing 'onnx' model conversion.
And also it will be possible to enable this by default in CI, and it would be possible to remove 'onnx_reader' from build to verify that ONNX models are converted via frontends path

mateusztabaka · 2021-07-22T11:22:56Z

Can we measure what is the binary size for all ONNX related libraries modified by this PR in comparison to master version?

onnx_frontend branch:
$ ls -sh libonnx*
4.6M libonnx_importer.so 64K libonnx_ngraph_frontend.so
on master:
$ ls -sh libonnx*
4.6M libonnx_importer.so

azhogov · 2021-07-22T14:23:58Z

You can add into onnx CI such test that total size of libs <= NNN

We will address your idea about package refactoring in a separate PR.

This change implements a subset of FrontEnd API for ONNX: Changes: - move onnx_common, onnx_editor and onnx_import to ngraph/frontend/onnx - introduce new target 'onnx_frontend_ngraph' located in ngraph/frontend/onnx/frontend - new target implements subset of FrontEnd, InputModel and Place interfaces

mateusztabaka requested review from postrational and mbencer July 13, 2021 05:36

postrational requested a review from tomdol July 13, 2021 09:49

postrational reviewed Jul 13, 2021

View reviewed changes

ngraph/python/tests/test_ngraph/test_frontend_onnx.py Outdated Show resolved Hide resolved

postrational reviewed Jul 13, 2021

View reviewed changes

ngraph/frontend/onnx/CMakeLists.txt Show resolved Hide resolved

mateusztabaka marked this pull request as ready for review July 14, 2021 06:18

mateusztabaka requested a review from a team July 14, 2021 06:18

postrational requested a review from slyalin July 14, 2021 09:47

postrational mentioned this pull request Jul 14, 2021

[ONNX] Add ONNX specific FrontEnd classes #6550

Closed

slyalin requested a review from nosovmik July 15, 2021 07:44

rename NGRAPH_ONNX_EDITOR_ENABLE to NGRAPH_ONNX_FRONTEND_ENABLE

01b5179

mateusztabaka requested review from a team and postrational July 15, 2021 13:01

postrational approved these changes Jul 15, 2021

View reviewed changes

nosovmik requested review from ilyachur and ilya-lavrenov July 15, 2021 13:57

nosovmik reviewed Jul 15, 2021

View reviewed changes

ngraph/CMakeLists.txt Outdated Show resolved Hide resolved

mbencer approved these changes Jul 15, 2021

View reviewed changes

fix error message

8d79baf

mateusztabaka requested a review from nosovmik July 15, 2021 14:09

nosovmik reviewed Jul 16, 2021

View reviewed changes

ngraph/frontend/onnx/onnx_editor/include/onnx_editor/editor.hpp Outdated Show resolved Hide resolved

ngraph/python/tests/test_frontend/test_frontend_onnx.py Outdated Show resolved Hide resolved

tomdol approved these changes Jul 16, 2021

View reviewed changes

nosovmik reviewed Jul 16, 2021

View reviewed changes

ngraph/frontend/onnx/frontend/include/onnx_frontend/input_model.hpp Outdated Show resolved Hide resolved

ilyachur mentioned this pull request Jul 16, 2021

Merged ONNX editor and ONNX importer to the one folder #6672

Merged

mateusztabaka requested review from a team and nosovmik July 19, 2021 14:07

Merge branch 'master' into onnx_frontend

15df53a

mateusztabaka force-pushed the onnx_frontend branch from f066db1 to 15df53a Compare July 19, 2021 14:20

mateusztabaka added 2 commits July 19, 2021 14:20

apply code style

0b9c298

disable ONNX frontend by default

c7c41aa

nosovmik reviewed Jul 19, 2021

View reviewed changes

ngraph/frontend/onnx/frontend/src/frontend.cpp Outdated Show resolved Hide resolved

nosovmik reviewed Jul 19, 2021

View reviewed changes

Merge branch 'master' into onnx_frontend

08fdfe8

slyalin previously requested changes Jul 20, 2021

View reviewed changes

apply code style

f74740f

ilya-lavrenov reviewed Jul 20, 2021

View reviewed changes

ngraph/frontend/onnx/onnx_import/include/onnx_import/utils/onnx_internal.hpp Show resolved Hide resolved

nosovmik reviewed Jul 20, 2021

View reviewed changes

apply review comments

6b1add7

mateusztabaka requested a review from a team July 20, 2021 13:27

mateusztabaka added 6 commits July 20, 2021 19:57

add ONNX_FRONTEND_API macro

fe3ddab

Merge branch 'master' into onnx_frontend

f3f7e38

apply code style

1b8c82c

fix flake style

92552d8

Merge branch 'master' into onnx_frontend

7fd0b18

Merge branch 'master' into onnx_frontend

4907023

azhogov approved these changes Jul 22, 2021

View reviewed changes

postrational merged commit 22273f8 into openvinotoolkit:master Jul 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ONNX] Add ONNX specific FrontEnd classes #6615

[ONNX] Add ONNX specific FrontEnd classes #6615

mateusztabaka commented Jul 13, 2021

nosovmik left a comment

nosovmik Jul 19, 2021

mbencer Jul 20, 2021

nosovmik Jul 19, 2021

ilya-lavrenov Jul 19, 2021

mateusztabaka Jul 20, 2021

nosovmik Jul 20, 2021

postrational Jul 20, 2021

ilya-lavrenov Jul 20, 2021

postrational Jul 20, 2021

slyalin left a comment

slyalin Jul 20, 2021

nosovmik Jul 20, 2021 •

edited

Loading

mateusztabaka commented Jul 22, 2021

azhogov commented Jul 22, 2021

[ONNX] Add ONNX specific FrontEnd classes #6615

[ONNX] Add ONNX specific FrontEnd classes #6615

Conversation

mateusztabaka commented Jul 13, 2021

nosovmik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slyalin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nosovmik Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

mateusztabaka commented Jul 22, 2021

azhogov commented Jul 22, 2021

nosovmik Jul 20, 2021 •

edited

Loading