Skip to content

Subgraph extraction in ONNX models#4107

Merged
postrational merged 34 commits intoopenvinotoolkit:masterfrom
tomdol:onnx_model_cutting
Mar 4, 2021
Merged

Subgraph extraction in ONNX models#4107
postrational merged 34 commits intoopenvinotoolkit:masterfrom
tomdol:onnx_model_cutting

Conversation

@tomdol
Copy link
Copy Markdown
Contributor

@tomdol tomdol commented Feb 1, 2021 •
edited
Loading

Related ticket number: 45414

@tomdol tomdol added ONNX Related to support for ONNX standard. category: onnx importer labels Feb 1, 2021
@tomdol
Copy link
Copy Markdown
Contributor Author

tomdol commented Feb 1, 2021

The subgraph extraction flow:

ONNXModelEditor::cut_graph_fragment
    SubgraphExtractor::add_new_inputs
    SubgraphExtractor::add_new_outputs
    SubgraphExtractor::extract_subgraph
        SubgraphExtractor::discover_output_contributors // called for each output
        SubgraphExtractor::extract_subgraph_from_onnx_model

elszkowski
elszkowski reviewed Feb 1, 2021
View reviewed changes
Copy link
Copy Markdown

@elszkowski elszkowski left a comment •
edited
Loading

Choose a reason for hiding this comment

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

...

@GlebKazantaev GlebKazantaev removed the request for review from a team February 1, 2021 15:59
mbencer
mbencer approved these changes Feb 3, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@mbencer mbencer left a comment

Choose a reason for hiding this comment

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

Very readable and well-tested implementation IMO. Great

@tomdol tomdol requested a review from elszkowski February 3, 2021 11:20
ilyachur
ilyachur reviewed Feb 16, 2021
View reviewed changes
ilyachur
ilyachur reviewed Feb 18, 2021
View reviewed changes
ngraph/frontend/onnx_import/src/editor/detail/subgraph_extraction.cpp
const int current_node_idx,
const std::string& input_name)
{
for (int i = current_node_idx - 1; i >= 0; --i)
Copy link
Copy Markdown
Contributor

@ilyachur ilyachur Feb 18, 2021

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 use here the reversed order?

Copy link
Copy Markdown
Contributor Author

@tomdol tomdol Feb 18, 2021

Choose a reason for hiding this comment

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

We're iterating over a topological order of nodes. This means that the node producing input to a given node has to be located before. This way we can start looking at the position current_node_idx - 1 and go towards the beginning of the container.

...s/onnx/model_editor/reference/subgraph__initializer_without_matching_input_tail_cut.prototxt Outdated
@@ -0,0 +1,120 @@
ir_version: 3
producer_name: "tomdol"
Copy link
Copy Markdown
Contributor

@ilyachur ilyachur Feb 18, 2021

Choose a reason for hiding this comment

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

@tomdol Why do we need to have this field?

Copy link
Copy Markdown
Contributor Author

@tomdol tomdol Feb 18, 2021

Choose a reason for hiding this comment

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

We don't, it's optional. I can remove this field but it will require a full run of CI.

slyalin
slyalin reviewed Feb 18, 2021
View reviewed changes
ngraph/frontend/onnx_import/include/onnx_import/editor/detail/subgraph_extraction.hpp
{
}

const int m_node_idx;
Copy link
Copy Markdown
Contributor

@slyalin slyalin Feb 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

How can one obtain a valid value for m_node_idx? Briefly looking in the code I see that a valid value is really required but there is no way for user to deduce this index based on some user-visible name without parsing model proto file. Even Netron doesn't give this index.

Can we make it at least optional? In this case it would mean cutting by a tensor name and providing a single input for all the consumers.

Copy link
Copy Markdown
Contributor Author

@tomdol tomdol Feb 18, 2021

Choose a reason for hiding this comment

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

Similarily to the comment below - TBD in 47578

slyalin
slyalin reviewed Feb 18, 2021
View reviewed changes
ngraph/frontend/onnx_import/include/onnx_import/editor/detail/subgraph_extraction.hpp
/// there are 3 possible valid instances of this struct:
/// InputEdge(5, "in_A")
/// InputEdge(5, "in_B")
/// InputEdge(5, "in_C")
Copy link
Copy Markdown
Contributor

@slyalin slyalin Feb 18, 2021

Choose a reason for hiding this comment

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

Assuming that node 5 is an operation and (in_A) etc. are tensors, this documentation doesn't provide rationale why we are specifying node index, because there is only one consumer for each of the tensors on the schematics. In this case the tensor name should be enough to specify the cutting point.

Copy link
Copy Markdown
Contributor Author

@tomdol tomdol Feb 18, 2021

Choose a reason for hiding this comment

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

As discussed offline, this will be addressed in a separate ticket: 47578

@tomdol
Copy link
Copy Markdown
Contributor Author

tomdol commented Feb 19, 2021

@openvinotoolkit/openvino-ngraph-maintainers all feedback addressed. Can we merge it or should we wait for anyone else's review?

@ilyachur
Copy link
Copy Markdown
Contributor

ilyachur commented Feb 19, 2021

@tomdol I propose to wait for release branch creation. Because the feature freeze happened and we don't want to have this feature in the 2021.3 release.
So my suggestion is to wait for release branch creation.

@tomdol tomdol added this to the 2021.4 milestone Feb 25, 2021
@postrational postrational merged commit a085b68 into openvinotoolkit:master Mar 4, 2021
@tomdol tomdol deleted the onnx_model_cutting branch March 4, 2021 09:22
nikita-malininn pushed a commit to nikita-malininn/openvino that referenced this pull request Mar 10, 2021
@nikita-malininn
Subgraph extraction in ONNX model editor (openvinotoolkit#4107)
2b6787a
@tomdol tomdol restored the onnx_model_cutting branch May 26, 2021 07:24
@tomdol tomdol deleted the onnx_model_cutting branch August 4, 2021 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@t-jankowski t-jankowski t-jankowski approved these changes

@mitruska mitruska Awaiting requested review from mitruska

@lazarevevgeny lazarevevgeny Awaiting requested review from lazarevevgeny

@e-nugmanova e-nugmanova Awaiting requested review from e-nugmanova

@elszkowski elszkowski Awaiting requested review from elszkowski

+6 more reviewers

@ilyachur ilyachur ilyachur left review comments

@mbencer mbencer mbencer approved these changes

@slyalin slyalin slyalin left review comments

@postrational postrational postrational approved these changes

@tsocha tsocha tsocha approved these changes

@jiwaszki jiwaszki jiwaszki approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ONNX Related to support for ONNX standard.

Projects

None yet

Milestone

2021.4

Development

Successfully merging this pull request may close these issues.

9 participants

@tomdol @ilyachur @postrational @mbencer @tsocha @slyalin @elszkowski @jiwaszki @t-jankowski