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

MultiSubgraph in nGraph #6621

Merged
merged 34 commits into from
Jul 23, 2021
Merged

MultiSubgraph in nGraph #6621

merged 34 commits into from
Jul 23, 2021

Conversation

evolosen
Copy link
Contributor

Ticket #50047
Added new base class for operations with subgraphs

evolosen added 12 commits December 3, 2020 11:44
@evolosen
Merge pull request #1 from openvinotoolkit/master
56b755e 
update my fork
@evolosen
Merge pull request #2 from openvinotoolkit/master
fbfe44c 
Update forked branch
@evolosen
Merge pull request #3 from openvinotoolkit/master
0cf84d4 
Update forked branch
@evolosen
Merge pull request #4 from openvinotoolkit/master
0df8350 
Update forked branch
@evolosen
Merge pull request #5 from openvinotoolkit/master
c299e1d 
Update forked branch
@evolosen
Merge pull request #6 from openvinotoolkit/master
f4f54ac 
Update forked branch
@evolosen
Update forked branch
8feeba4 
Update forked branch
@evolosen
Merge pull request #8 from openvinotoolkit/master
fb93329 
Update forked branch
@evolosen
Merge pull request #9 from openvinotoolkit/master
2a19318 
Update forked branch
@evolosen
Update forked branch
c09041d 
Update forked branch
@evolosen
Merge remote-tracking branch 'upstream/master'
e9f256e
@evolosen
Add multisubgraph
c0f3a78
@evolosen evolosen requested a review from itikhono July 13, 2021 09:25
@evolosen evolosen marked this pull request as ready for review July 13, 2021 12:38
@evolosen evolosen requested review from a team, rkazants, sadolini and vgavrilo and removed request for a team July 13, 2021 12:38
@itikhono itikhono requested a review from ilyachur July 14, 2021 07:03
@evolosen evolosen requested a review from sadolini July 14, 2021 11:24
@evolosen evolosen requested a review from itikhono July 16, 2021 09:47
itikhono
itikhono approved these changes Jul 16, 2021
View reviewed changes
Copy link
Contributor

@itikhono itikhono left a comment

Choose a reason for hiding this comment

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

in general LGTM

ilyachur
ilyachur reviewed Jul 19, 2021
View reviewed changes
ngraph/core/include/ngraph/op/tensor_iterator.hpp Show resolved Hide resolved
ngraph/core/include/ngraph/op/util/multi_subgraph_base.hpp Outdated Show resolved Hide resolved
ngraph/core/include/ngraph/op/util/multi_subgraph_base.hpp Outdated Show resolved Hide resolved
ngraph/core/include/ngraph/op/util/multi_subgraph_base.hpp Outdated Show resolved Hide resolved
ngraph/core/include/ngraph/op/util/multi_subgraph_base.hpp Outdated Show resolved Hide resolved
ngraph/core/include/ngraph/op/util/multi_subgraph_base.hpp Outdated Show resolved Hide resolved
ngraph/core/include/ngraph/op/util/multi_subgraph_base.hpp Outdated Show resolved Hide resolved
ngraph/core/include/ngraph/op/util/multi_subgraph_base.hpp Outdated Show resolved Hide resolved
ngraph/core/include/ngraph/op/util/multi_subgraph_base.hpp Outdated Show resolved Hide resolved
@evolosen evolosen requested a review from ilyachur July 19, 2021 06:11
ilyachur
ilyachur reviewed Jul 20, 2021
View reviewed changes
Copy link
Contributor

@ilyachur ilyachur left a comment

Choose a reason for hiding this comment

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

I don't see any new tests, can we add new tests to cover subgraph base?

ngraph/core/include/ngraph/op/util/sub_graph_base.hpp Outdated
/// SubGraphOp.
///
class NGRAPH_API SliceInputDescription : public InputDescription
virtual std::shared_ptr<Function> get_function() { return m_bodies[0]; };
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
virtual std::shared_ptr<Function> get_function() { return m_bodies[0]; };
virtual const std::shared_ptr<Function>& get_function() { return m_bodies[0]; };

ngraph/core/include/ngraph/op/util/sub_graph_base.hpp Outdated
///
class NGRAPH_API SliceInputDescription : public InputDescription
virtual std::shared_ptr<Function> get_function() { return m_bodies[0]; };
virtual std::shared_ptr<const Function> get_function() const
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
virtual std::shared_ptr<const Function> get_function() const
virtual const std::shared_ptr<const Function>& get_function() const

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need to have both methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its old code, i dont fix it, in ti and loop sometimes are used theese methods,

Copy link
Contributor

@itikhono itikhono Jul 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It happened historically :) I don't remember exactly the whole story. We had get_function() without const qualifier in TensorIterator and SubGraph classes (it was an error), but Gleb G. wanted to use this function in const context, so he added a new function with const qualifier.
I guess we can remain only
virtual const std::shared_ptr<const Function>& get_function() const
But I'm not sure it's backward compatible change or not, @ilyachur what do you think should leave both functions or it's better to remain only one?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't modify the function I think we can have only one function. In any way, we can decide it in the separate PR.

@evolosen evolosen requested a review from ilyachur July 20, 2021 10:54
@ilyachur
Copy link
Contributor

Please fix CI

@ilyachur ilyachur merged commit f328eec into openvinotoolkit:master Jul 23, 2021
rnugmanx pushed a commit to rnugmanx/openvino that referenced this pull request Aug 26, 2021
@evolosen
MultiSubgraph in nGraph (openvinotoolkit#6621)
98bb91e 
* Add multisubgraph

* Fix format

* Fix clang format

* Fix TensorIterator RTT

* Fix subgraph

* Fix codestyle

* Fix comments

* Fix comments

* Fix coments

* Fix comments

* delete get function

* fix methods

* fix ci

* Fix ci

* fix bugs

* Fix cmake

* Fix ci

* delete virtual function

* delete virtual function

* fix ci

* Fix ci
andrei-cv pushed a commit to andrei-cv/openvino that referenced this pull request Aug 30, 2021
@evolosen @andrei-cv
MultiSubgraph in nGraph (openvinotoolkit#6621)
000e19f 
* Add multisubgraph

* Fix format

* Fix clang format

* Fix TensorIterator RTT

* Fix subgraph

* Fix codestyle

* Fix comments

* Fix comments

* Fix coments

* Fix comments

* delete get function

* fix methods

* fix ci

* Fix ci

* fix bugs

* Fix cmake

* Fix ci

* delete virtual function

* delete virtual function

* fix ci

* Fix ci
akuporos pushed a commit to akuporos/openvino that referenced this pull request Sep 29, 2021
@evolosen @akuporos
MultiSubgraph in nGraph (openvinotoolkit#6621)
a2a9b3e 
* Add multisubgraph

* Fix format

* Fix clang format

* Fix TensorIterator RTT

* Fix subgraph

* Fix codestyle

* Fix comments

* Fix comments

* Fix coments

* Fix comments

* delete get function

* fix methods

* fix ci

* Fix ci

* fix bugs

* Fix cmake

* Fix ci

* delete virtual function

* delete virtual function

* fix ci

* Fix ci
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itikhono itikhono itikhono approved these changes

@vgavrilo vgavrilo vgavrilo approved these changes

@ilyachur ilyachur ilyachur approved these changes

@rkazants rkazants Awaiting requested review from rkazants

@sadolini sadolini Awaiting requested review from sadolini

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@evolosen @ilyachur @itikhono @sadolini @vgavrilo