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

Subgraph extraction in ONNX models #4107

Merged
merged 34 commits into from
Mar 4, 2021
Merged

Subgraph extraction in ONNX models #4107

merged 34 commits into from
Mar 4, 2021

Conversation

tomdol
Copy link
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
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

pelszkow
pelszkow reviewed Feb 1, 2021
View reviewed changes
Copy link
Contributor

@pelszkow pelszkow 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.

...

ngraph/frontend/onnx_import/src/editor/detail/subgraph_extraction.cpp Show resolved Hide resolved
ngraph/frontend/onnx_import/src/editor/detail/subgraph_extraction.cpp Outdated Show resolved Hide resolved
ngraph/frontend/onnx_import/src/editor/detail/subgraph_extraction.cpp Outdated Show resolved Hide resolved
ngraph/test/util/onnx_test_util.cpp Outdated Show resolved Hide resolved
ngraph/frontend/onnx_import/src/editor/detail/subgraph_extraction.cpp Outdated Show resolved Hide resolved
@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
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

ngraph/frontend/onnx_import/src/editor/detail/subgraph_extraction.cpp Outdated Show resolved Hide resolved
ngraph/frontend/onnx_import/include/onnx_import/editor/detail/subgraph_extraction.hpp Show resolved Hide resolved
@tomdol tomdol requested a review from pelszkow February 3, 2021 11:20
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
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 use here the reversed order?

Copy link
Contributor Author

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
Contributor

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
Contributor Author

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
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
Contributor Author

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
Contributor

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
Contributor Author

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
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
Contributor

@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
KodiaqQ pushed a commit to KodiaqQ/openvino that referenced this pull request Mar 10, 2021
@KodiaqQ
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

@slyalin slyalin slyalin left review comments

@mbencer mbencer mbencer approved these changes

@ilyachur ilyachur ilyachur left review comments

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

@jiwaszki jiwaszki jiwaszki approved these changes

@tsocha tsocha tsocha approved these changes

@postrational postrational postrational approved these changes

@mitruska mitruska Awaiting requested review from mitruska

@lazarevevgeny lazarevevgeny Awaiting requested review from lazarevevgeny

@jane-intel jane-intel Awaiting requested review from jane-intel

@pelszkow pelszkow Awaiting requested review from pelszkow

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 @pelszkow @jiwaszki @t-jankowski