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.

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

[opset5] ngraph implementation of Loop op #2583

Merged
merged 18 commits into from
Oct 19, 2020

Conversation

itikhono
Copy link
Contributor

@itikhono itikhono commented Oct 8, 2020

ngraph implementation of Loop op
prototype for reading Loop op from IR in IE IR Reader
prototype for converting Loop to CNN layer
single layer tests in disabled state
shape inference and copy tests

ngraph/core/include/ngraph/op/loop.hpp Outdated Show resolved Hide resolved
ngraph/test/type_prop/loop.cpp Show resolved Hide resolved
ngraph/test/type_prop/loop.cpp Outdated Show resolved Hide resolved
ngraph/test/type_prop/loop.cpp Show resolved Hide resolved
ngraph/core/include/ngraph/op/loop.hpp Outdated Show resolved Hide resolved
@itikhono itikhono marked this pull request as ready for review October 15, 2020 04:41
@itikhono itikhono requested review from a team, GlebKazantaev and ilyachur October 15, 2020 04:41
@itikhono itikhono requested a review from mbencer October 15, 2020 09:27
@itikhono itikhono added this to the 2021.2 milestone Oct 15, 2020
@itikhono itikhono added category: inference OpenVINO Runtime library - Inference category: Core OpenVINO Core (aka ngraph) labels Oct 15, 2020
Copy link
Contributor

@lazarevevgeny lazarevevgeny left a comment

Choose a reason for hiding this comment

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

LGTM from the operation specification perspective

@@ -809,6 +810,7 @@ void convertFunctionToICNNNetwork(const std::shared_ptr<const ::ngraph::Function
std::make_shared<Builder::NodeConverter<::ngraph::op::TopKIE>>(),
std::make_shared<Builder::NodeConverter<::ngraph::op::Unsqueeze>>(),
std::make_shared<Builder::NodeConverter<::ngraph::op::TensorIterator>>(),
std::make_shared<Builder::NodeConverter<::ngraph::opset5::Loop>>(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why you add node converter? Could you use visitorAPI?

Copy link
Contributor Author

@itikhono itikhono Oct 15, 2020

Choose a reason for hiding this comment

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

For now, the old NodeConverter of TensorIterator is used for Loop op with small specific (NodeConverter < Loop > just sets the name of the Loop operation), but this code has not been fully tested for Loop yet due to lack of plugin implementation and unsupported Const-> Results subgraphs.
I guess it's not a blocker for this PR. NodeConverter logic can be moved to the new SpecificCreator for both TI and Loop ops in the next PR.

auto ngPrc = FuncTestUtils::PrecisionUtils::convertIE2nGraphPrc(netPrecision);

// That which we iterate over
std::vector<std::vector<size_t>> inputs_separate;
Copy link
Contributor

Choose a reason for hiding this comment

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

may be, we should move to the builders this large part of code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not yet clear how to do this if we want to test different ngraph functions inside the Loop and it's just a prototype, now these tests are disabled.
They will be enabled and finalized in the next PR

ngraph/core/include/ngraph/op/loop.hpp Outdated Show resolved Hide resolved
ngraph/core/include/ngraph/op/loop.hpp Outdated Show resolved Hide resolved
ngraph/core/src/graph_util.cpp Outdated Show resolved Hide resolved
ngraph/core/src/op/loop.cpp Outdated Show resolved Hide resolved
ngraph/core/src/op/loop.cpp Show resolved Hide resolved
ngraph/core/src/op/loop.cpp Outdated Show resolved Hide resolved
ngraph/core/src/op/loop.cpp Outdated Show resolved Hide resolved
ngraph/core/src/op/loop.cpp Outdated Show resolved Hide resolved
ngraph/core/src/op/loop.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@iefode iefode left a comment

Choose a reason for hiding this comment

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

Approve, but comments should be applied in next PRs

@mbencer mbencer self-requested a review October 16, 2020 13:49
@ilyachur ilyachur merged commit 84b5fc5 into openvinotoolkit:master Oct 19, 2020
mryzhov pushed a commit to mryzhov/openvino that referenced this pull request Dec 16, 2020
* Loop op ngraph implementation, update IE IR Reader and ngraph to cnn converter

* refactoring SubGraphOp class

* type prop unit tests

* ngraph code style

* update comment

* single layer tests for Loop operation

* fix file name

* Add SpecialBodyPorts attribute in Loop op, update single layer tests

* add several new tests cases, strict checks in Loop impl, temporary disable single layer tests

* ngraph codestyle, refactoring, clone_new_args test

* resolve review remarks

* fix build

* fix tests

* add a new constructor of Loop op, resolve review remarks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: Core OpenVINO Core (aka ngraph) category: inference OpenVINO Runtime library - Inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants