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

[MO] StridedSlice improvements #4139

Merged
merged 34 commits into from
Feb 16, 2021

Conversation

pavel-esir
Copy link
Contributor

@pavel-esir pavel-esir commented Feb 2, 2021

Description: Fixed some issues with StridedSlice permutation and processing ellipsis masks. Now infer function do not change attributes and inputs of StridedSlice, moved normalization into a separate transformation, also reused general existing permute mechanism. Now complex StridedSlices are correctly inferred in ShapeOf subgraphs.

tickets 40372, 43072, 39311

Code:

  • Comments
  • Code style (PEP8)
  • Transformation generates reshape-able IR
  • Transformation preserves original framework node names

Validation:

  • Unit tests
  • Framework operation tests
  • Transformation tests
  • e2e model test with an update of ./tests/e2e_oss/utils/reshape_utils.py
  • Model Optimizer IR Reader check

Documentation:

  • Supported frameworks operations list: N/A
  • Supported public models list: N/A
  • New operations specification: N/A
  • Guide on how to convert the public model: N/A
  • User guide update: N/A

@pavel-esir pavel-esir changed the title StridedSlice [MO] StridedSlice improvements Feb 2, 2021
@pavel-esir pavel-esir marked this pull request as ready for review February 5, 2021 11:20
@pavel-esir pavel-esir requested review from a team, rkazants, sadolini and jane-intel February 5, 2021 11:20
@pavel-esir pavel-esir added the category: MO Model Optimizer label Feb 5, 2021
@pavel-esir pavel-esir added this to the 2021.3 milestone Feb 5, 2021
model-optimizer/extensions/back/CropToStridedSlice.py Outdated Show resolved Hide resolved
model-optimizer/extensions/middle/ApplyPermutations.py Outdated Show resolved Hide resolved
model-optimizer/extensions/middle/ApplyPermutations.py Outdated Show resolved Hide resolved
model-optimizer/mo/front/mxnet/extractors/slice_axis.py Outdated Show resolved Hide resolved
model-optimizer/mo/graph/perm_inputs.py Outdated Show resolved Hide resolved
model-optimizer/mo/graph/perm_inputs.py Outdated Show resolved Hide resolved
model-optimizer/mo/front/common/partial_infer/utils.py Outdated Show resolved Hide resolved
model-optimizer/mo/ops/strided_slice.py Outdated Show resolved Hide resolved
model-optimizer/mo/ops/strided_slice.py Outdated Show resolved Hide resolved
model-optimizer/mo/ops/strided_slice.py Outdated Show resolved Hide resolved
model-optimizer/mo/ops/strided_slice.py Outdated Show resolved Hide resolved
model-optimizer/mo/ops/strided_slice.py Outdated Show resolved Hide resolved
model-optimizer/mo/ops/strided_slice.py Outdated Show resolved Hide resolved
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.

In general looks good, but still have some comments.

Also, please, run unit test with code coverage to see how new functions are covered with unit tests: coverage run -m unittest discover -p "*_test.py"

model-optimizer/mo/ops/strided_slice.py Outdated Show resolved Hide resolved
model-optimizer/mo/ops/strided_slice.py Outdated Show resolved Hide resolved
@pavel-esir
Copy link
Contributor Author

@lazarevevgeny thanks for your comments! Resolved them all.
coverage run -m unittest discover -p "*_test.py" - successfully done

There are 3 sets of automatically unit-tests that cover almost all cases.
ShapeInfer tests with shrink_axis, newaxis and ellipsis placed on almost every possible positions of 2, 3, 4 rank slices with different input_ranks.

# automatically generated the whole range of 2d slices over 2d, 3d and 4d input tensors

The same ShapeInfer tests after Normalizer

# automatically generated the whole range of 2d slices over 2d, 3d and 4d input tensors

Permute tests

# automatically generated permutation tests

dl-benchmark finished without regressions on a previous run. @jane-intel, @sadolini, @rkazants please review

Copy link
Contributor

@jane-intel jane-intel left a comment

Choose a reason for hiding this comment

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

First part 14/20 files

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.

Overall looks good. But left several minor comments to be fixed.

@pavel-esir pavel-esir requested a review from a team as a code owner February 15, 2021 17:07
@lazarevevgeny lazarevevgeny merged commit 22169a0 into openvinotoolkit:master Feb 16, 2021
@likholat
Copy link
Contributor

According to the documentation, the stride input is optional: https://docs.openvinotoolkit.org/latest/openvino_docs_ops_movement_StridedSlice_1.html
But after these changes, the absence of the stride input causes the error:

[ ERROR ]  Traceback (most recent call last):
  File "/home/anna/projects/openvino/model-optimizer/mo/utils/class_registration.py", line 290, in apply_transform
    for_graph_and_each_sub_graph_recursively(graph, replacer.find_and_replace_pattern)
  File "/home/anna/projects/openvino/model-optimizer/mo/middle/pattern_match.py", line 59, in for_graph_and_each_sub_graph_recursively
    func(graph)
  File "/home/anna/projects/openvino/model-optimizer/extensions/middle/StridedSliceNormalizer.py", line 107, in find_and_replace_pattern
    StridedSliceNormalizer.normalize_strided_slice(graph, node)
  File "/home/anna/projects/openvino/model-optimizer/extensions/middle/StridedSliceNormalizer.py", line 129, in normalize_strided_slice
    StridedSliceNormalizer.normalize_slices_attr(node)
  File "/home/anna/projects/openvino/model-optimizer/extensions/middle/StridedSliceNormalizer.py", line 249, in normalize_slices_attr
    res_slices[-1] = slice(*res_slices[-1].indices(data_shape[in_idx]))  # convert negative begins/ends
TypeError: slice indices must be integers or None or have an __index__ method

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/anna/projects/openvino/model-optimizer/mo/main.py", line 305, in main
    ret_code = driver(argv)
  File "/home/anna/projects/openvino/model-optimizer/mo/main.py", line 269, in driver
    ret_res = emit_ir(prepare_ir(argv), argv)
  File "/home/anna/projects/openvino_contrib/modules/mo_pytorch/mo_pytorch.py", line 106, in _prepare_ir
    graph = unified_pipeline(argv)
  File "/home/anna/projects/openvino/model-optimizer/mo/pipeline/unified.py", line 29, in unified_pipeline
    class_registration.ClassType.BACK_REPLACER
  File "/home/anna/projects/openvino/model-optimizer/mo/utils/class_registration.py", line 340, in apply_replacements
    apply_replacements_list(graph, replacers_order)
  File "/home/anna/projects/openvino/model-optimizer/mo/utils/class_registration.py", line 330, in apply_replacements_list
    num_transforms=len(replacers_order))
  File "/home/anna/projects/openvino/model-optimizer/mo/utils/logger.py", line 124, in wrapper
    function(*args, **kwargs)
  File "/home/anna/projects/openvino/model-optimizer/mo/utils/class_registration.py", line 318, in apply_transform
    )) from err
Exception: Exception occurred during running replacer "REPLACEMENT_ID (<class 'extensions.middle.StridedSliceNormalizer.StridedSliceNormalizer'>)": slice indices must be integers or None or have an __index__ method

Is it a bug or the stride input is now required?

@pavel-esir
Copy link
Contributor Author

pavel-esir commented Feb 17, 2021

Is it a bug or the stride input is now required?

Specification haven't been changed. strides are still optional. Looks like a bug. I will take a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: MO Model Optimizer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants