Skip to content

Fix output names for group of Strided Slices to Variadic Split optimization#4378

Merged
GlebKazantaev merged 5 commits intoopenvinotoolkit:masterfrom
bsledz:VariadicSplitOutputNames
Feb 25, 2021
Merged

Fix output names for group of Strided Slices to Variadic Split optimization#4378
GlebKazantaev merged 5 commits intoopenvinotoolkit:masterfrom
bsledz:VariadicSplitOutputNames

Conversation

@bsledz
Copy link
Copy Markdown
Contributor

@bsledz bsledz commented Feb 17, 2021

Details:

Group of StridedSlice operators is optimized to single VariadicSplit operator with multiple outputs, but output names are not retained.

This can cause problems when these outputs are also the outputs of the entire model.

Tickets:

43277

@bsledz bsledz requested a review from a team February 17, 2021 09:35
@openvino-pushbot openvino-pushbot added the category: Core OpenVINO Core (aka ngraph) label Feb 17, 2021
@GlebKazantaev GlebKazantaev self-assigned this Feb 17, 2021
@bsledz bsledz requested a review from tsocha February 18, 2021 05:37
if (record.first != fake_output) {
const auto out_name = record.first.get_node_shared_ptr()->get_friendly_name();
NGRAPH_SUPPRESS_DEPRECATED_START
variadic_split->get_output_tensor(i).set_name(out_name);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is a WA.
Can we use the new API?

It should allow to map framework names to OV names without work arounds on the transformations side.

Copy link
Copy Markdown
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.

We used the same WA for the TI at the current moment, but honesty I highly recommend to use the new API because we have a plan to remove get/set_name().

@bsledz bsledz requested review from mbencer and tomdol February 18, 2021 09:10
Copy link
Copy Markdown
Contributor

@tomdol tomdol left a comment

Choose a reason for hiding this comment

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

LGTM but please test the new API as suggested above. It can be done separately and used in a separate PR.

@GlebKazantaev
Copy link
Copy Markdown
Contributor

Please do not merge, I still have some concerns that I wan to discuss with @ilyachur today.

@GlebKazantaev GlebKazantaev merged commit 6c98171 into openvinotoolkit:master Feb 25, 2021
@GlebKazantaev GlebKazantaev added this to the 2021.3 milestone Feb 25, 2021
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)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants