Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use ONNX Frontend instead of ONNX Reader #7031

Merged
merged 40 commits into from
Aug 26, 2021
Merged

Use ONNX Frontend instead of ONNX Reader #7031

merged 40 commits into from
Aug 26, 2021

Conversation

mbencer
Copy link
Contributor

@mbencer mbencer commented Aug 11, 2021 •
edited
Loading

Ticket: 62297, 62730

  • support load_by_model for ONNX FE (from file and from stream + passing path needed for onnx external data feature)
  • removed ONNX Reader
  • moved onnx validator to onnx common (and using it in ONNX FE)
  • creating target for C++ ONNX FE unit tests
  • removed prototxt support from ONNX Reader (and onnx validator)

@mbencer mbencer added this to the 2022.1 milestone Aug 11, 2021
@mbencer mbencer self-assigned this Aug 11, 2021
@mbencer mbencer requested a review from a team August 11, 2021 14:48
@openvino-pushbot openvino-pushbot added the category: Core OpenVINO Core (aka ngraph) label Aug 11, 2021
@mbencer mbencer requested a review from ilya-lavrenov August 12, 2021 07:13
@mbencer mbencer requested a review from a team August 20, 2021 17:07
ilyachur
ilyachur reviewed Aug 24, 2021
View reviewed changes
inference-engine/src/inference_engine/src/ie_network_reader.cpp Outdated
@@ -282,6 +270,19 @@ CNNNetwork details::ReadNetwork(const std::string& model,
return reader->read(modelStream, exts);
}
}
// Try to load with FrontEndManager
// NOTE: weights argument is ignored
static ngraph::frontend::FrontEndManager manager;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use one instance of FrontEndManager for core object?
Now you have 2 static instances

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

inference-engine/src/inference_engine/src/ie_network_reader.cpp
@@ -259,7 +247,7 @@ CNNNetwork details::ReadNetwork(const std::string& modelPath,
}
if (inputModel) {
auto ngFunc = FE->convert(inputModel);
return CNNNetwork(ngFunc);
return CNNNetwork(ngFunc, exts);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have a scenario

  • add extension
  • read an ONNX model
  • serialize it to xml
  • read the model again as xml
    Passing exts is needed. It is tested by CustomOpUser_ONNXImporter unit test

ngraph/frontend/onnx/frontend/src/editor.cpp Show resolved Hide resolved
ngraph/frontend/onnx/frontend/src/frontend.cpp Outdated
@@ -49,3 +85,63 @@ std::shared_ptr<ngraph::Function> FrontEndONNX::decode(InputModel::Ptr model) co
NGRAPH_CHECK(model_onnx != nullptr, "Invalid input model");
return model_onnx->decode();
}

std::string FrontEndONNX::get_name() const {
return "onnx_experimental";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why "experimental"? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, we can change it to "onnx", but onnx frontend is still skipped in mo.py

ngraph/frontend/onnx/onnx_common/src/onnx_model_validator.cpp Show resolved Hide resolved
@postrational postrational added the ONNX Related to support for ONNX standard. label Aug 24, 2021
@mbencer mbencer removed the WIP work in progress label Aug 24, 2021
@GlebKazantaev GlebKazantaev self-requested a review August 26, 2021 08:17
ilyachur
ilyachur reviewed Aug 26, 2021
View reviewed changes
inference-engine/src/inference_engine/src/ie_network_reader.cpp
Comment on lines +99 to +102
static ngraph::frontend::FrontEndManager* get_frontend_manager() {
static ngraph::frontend::FrontEndManager manager;
return &manager;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static ngraph::frontend::FrontEndManager* get_frontend_manager() {
static ngraph::frontend::FrontEndManager manager;
return &manager;
}
static ngraph::frontend::FrontEndManager& get_frontend_manager() {
static ngraph::frontend::FrontEndManager manager;
return manager;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discussed offline to resolve it separately

@ilyachur
Copy link
Contributor

Please resolve comments in the separate PR

@ilyachur ilyachur merged commit 0e521a1 into openvinotoolkit:master Aug 26, 2021
@mbencer mbencer mentioned this pull request Aug 26, 2021
Merged
rnugmanx pushed a commit to rnugmanx/openvino that referenced this pull request Aug 26, 2021
@tsavina
Use ONNX Frontend instead of ONNX Reader (openvinotoolkit#7031)
9e7f59a 
* added get_name

* add support to supported_impl

* remove debug code

* review remarks

* changed name to onnx_experimental

* fixed test

* revert onnx_experimental name

* integrate reader and fe api

* add unit tests

* removed prototxt from model_validator

* reader refactor

* add supress

* Update inference-engine/src/readers/onnx_reader/ie_onnx_reader.cpp

Co-authored-by: Tomasz Dołbniak <[email protected]>

* fix segfaults

* removed onnx reader

* handle istringstream

* wstring support

* removed saving path

* styles applied

* changed name to onnx experimental

* Apply suggestions from code review

Co-authored-by: Tatiana Savina <[email protected]>

* skip onnx_experimental frontend in mo.py

* add support of wstring paths

* fix wstring ctor of InputModelONNX

* added NGRAPH_SUPPRESS

* make one instance of manager

* change onnx_experimental name to onnx

* creation frontend manager refactor

Co-authored-by: Tomasz Dołbniak <[email protected]>
Co-authored-by: Tatiana Savina <[email protected]>
akuporos pushed a commit to akuporos/openvino that referenced this pull request Sep 29, 2021
@tsavina @akuporos
Use ONNX Frontend instead of ONNX Reader (openvinotoolkit#7031)
628a34d 
* added get_name

* add support to supported_impl

* remove debug code

* review remarks

* changed name to onnx_experimental

* fixed test

* revert onnx_experimental name

* integrate reader and fe api

* add unit tests

* removed prototxt from model_validator

* reader refactor

* add supress

* Update inference-engine/src/readers/onnx_reader/ie_onnx_reader.cpp

Co-authored-by: Tomasz Dołbniak <[email protected]>

* fix segfaults

* removed onnx reader

* handle istringstream

* wstring support

* removed saving path

* styles applied

* changed name to onnx experimental

* Apply suggestions from code review

Co-authored-by: Tatiana Savina <[email protected]>

* skip onnx_experimental frontend in mo.py

* add support of wstring paths

* fix wstring ctor of InputModelONNX

* added NGRAPH_SUPPRESS

* make one instance of manager

* change onnx_experimental name to onnx

* creation frontend manager refactor

Co-authored-by: Tomasz Dołbniak <[email protected]>
Co-authored-by: Tatiana Savina <[email protected]>
@mbencer mbencer deleted the mbencer/LoadByModel branch January 23, 2023 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsavina tsavina tsavina left review comments

@ilyachur ilyachur ilyachur approved these changes

@tomdol tomdol tomdol approved these changes

@GlebKazantaev GlebKazantaev GlebKazantaev approved these changes

@postrational postrational postrational approved these changes

@slyalin slyalin Awaiting requested review from slyalin

@nosovmik nosovmik Awaiting requested review from nosovmik

@ilya-lavrenov ilya-lavrenov Awaiting requested review from ilya-lavrenov

@andrew-zaytsev andrew-zaytsev Awaiting requested review from andrew-zaytsev

@avladimi avladimi Awaiting requested review from avladimi

Assignees

@mbencer mbencer

Labels
category: Core OpenVINO Core (aka ngraph) ONNX Related to support for ONNX standard.
Projects
None yet
Milestone
2022.1
Development

Successfully merging this pull request may close these issues.
8 participants
@mbencer @ilyachur @ilya-lavrenov @postrational @GlebKazantaev @tomdol @tsavina @openvino-pushbot