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

[Good First Issue]: Align behavior of ONNX Frontend function ReduceSumSquare-11, 13, 18 with original framework #20563

Closed
gkrivor opened this issue Oct 18, 2023 · 8 comments · Fixed by #23798
Assignees
Labels
category: ONNX FE OpenVINO ONNX FrontEnd good first issue Good for newcomers no_stale Do not mark as stale ONNX Related to support for ONNX standard.
Milestone

Comments

@gkrivor
Copy link
Contributor

gkrivor commented Oct 18, 2023

Context

Neural networks are graphs consisting of nodes called operators. Each operator corresponds to a mathematical function, usually described in framework's documentation or an AI standard, such as ONNX.
OpenVINO ONNX Frontend is a component responsible for working with ONNX graphs and requires implementation of different ONNX operators in order to use ONNX models.
This task requires alignment between OpenVINO ONNX Frontend and original framework implementations of ReduceSumSquare for next list of opsets: opset 11, opset 13, opset 18
Necessary help will be provided by ONNX Fronted team.

What needs to be done?

First of all, please, take a look on ReduceMax PR for a reference.

Operator details can be found in ONNX Operators
More details can be found in ONNX Changelog: opset 11, opset 13, opset 18

  1. Function already has a common implementation in OpenVINO. First of all, you need to review a documentation and prepare a table with differences between versions. It could be, for instance, a missing property, extended/reduced coverage of existing property, etc...
  2. Copy existing implementation here to make it aligned with original framework (extend or reduce coverage of a common implementation). Copy of modified implementation should be in a defined opset, or in opset 1 in case it implements oldest implementation. Example of multi-opset operation.
  3. Register the function in ops_bridge.cpp while keeping alphabetical order
  4. Create test model(s) in ONNX models directory. OpenVINO test infrastructure then converts prototxt files to ONNX models - you will use those models later in tests
  5. Add tests covering all use cases here
  6. Check Python xfailed tests to find a test marked as a xfailed for added functionality. If any exist - remove corresponding lines and try to verify by using cmdline "python -m pytest -k name_of_test".
    More details in adding operators to ONNX Frontend guide

Example Pull Requests

No response

Resources

Contact points

@gkrivor

Ticket

No response

@gkrivor gkrivor added good first issue Good for newcomers category: ONNX FE OpenVINO ONNX FrontEnd no_stale Do not mark as stale labels Oct 18, 2023
@github-project-automation github-project-automation bot moved this to Contrubutors needed in Good first issues Oct 18, 2023
@mlukasze mlukasze added the ONNX Related to support for ONNX standard. label Oct 26, 2023
@inbasperu
Copy link
Contributor

.take

Copy link
Contributor

github-actions bot commented Feb 5, 2024

Thank you for looking into this issue! Please let us know if you have any questions or require any help.

@mlukasze mlukasze moved this from Contributors Needed to Assigned in Good first issues Feb 5, 2024
@inbasperu
Copy link
Contributor

Hello OpenVINO maintainers,

I've set up and built the OpenVINO repository locally and have been exploring potential solutions for the issue at hand. Below is my proposed approach, and I would greatly appreciate any feedback or guidance on whether I'm heading in the right direction.

Proposed Approach

  1. Differences Table: I've reviewed the changelogs and compiled a table outlining the differences between the three specified versions.
Feature/Property Opset 11 Opset 13 Opset 18
Attributes - axes: list of ints
- keepdims: int (default 1)
- axes: list of ints
- keepdims: int (default 1)
- keepdims: int (default 1)
- noop_with_empty_axes: int (default 0)
Axes Attribute A list of integers to reduce along range:[-r, r-1] . Default is all dimensions. A list of integers to reduce along range:[-r, r-1] . Default is all dimensions. Not present
Keepdims Attribute Keep the reduced dimension (default is to keep). Keep the reduced dimension (default is to keep). Keep the reduced dimension (default is to keep).
Noop_with_empty_axes Attribute Not present Not present Defines behavior if 'axes' is empty. Reduces all axes by default if false, or acts as an identity op if true.
Inputs - data: T - data (differentiable): T - data (differentiable): T
- axes (optional, non-differentiable): tensor(int64)
Input Tensors of Rank Zero Not specified Valid input tensors Valid input tensors
Reduction over Empty Set Not specified Yields 0 Yields 0
Outputs - reduced: T - reduced (differentiable): T - reduced (differentiable): T
Type Constraints - T: high-precision numeric tensors (uint32, uint64, int32, int64, float16, float, double) - T: numeric tensors (uint32, uint64, int32, int64, float16, float, double, bfloat16) - T: numeric tensors (uint32, uint64, int32, int64, float16, float, double, bfloat16)

Key differences between versions:

  • The noop_with_empty_axes attribute is introduced in Opset 18, which specifies the behavior when the axes attribute is empty.
  • Opset 13 started to mention that reduction over an empty set of values yields 0 and that input tensors of rank zero are valid, which was not specified in Opset 11.
  • The type constraint was extended in Opset 13 to include tensor(bfloat16), which was not supported in earlier versions.
  • In Opset 18, the axes attribute becomes an optional input, whereas in Opsets 11 and 13, it is a required attribute with a default value.
  1. Common Implementation for Multi Opset:

    • Files to Modify:
    • Goal: Align behavior across the specified versions.
  2. Function Registration:

  3. Test Models:

  4. Use Case Coverage:

  5. Python xfailed Tests:

    • Review test_backend.py for any tests marked as xfailed for the added functionality, remove the corresponding lines, and validate using the command python -m pytest -k name_of_test.

Additional Context

I've also identified similar issues that could offer insights into this problem:

For further understanding of multi-opset operations, I studied the softmax function implementation and have compiled my notes here.

I'm eager to contribute to the resolution of this issue and look forward to your feedback to ensure my efforts are aligned with the project's needs.

@p-wysocki
Copy link
Contributor

Hello @inbasperu, the work you've done so far is very impressive, looks good to me. I think we could start with the implementation. :)

@inbasperu
Copy link
Contributor

Hello,
During testing in my development environment, the test suite failed to execute successfully, despite no recent changes being made to the repository. The tests are attempting to access specific ONNX model files, such as quantize_linear_const.onnx, quantize_linear_u16.onnx, and quantize_linear_zero_point.onnx, but cannot locate them at the specified paths.

The following error message was encountered:

unknown file: Failure
C++ exception with description "Exception from src/frontends/onnx/onnx_common/src/parser.cpp:24:
Could not open the file: /Users/inbasekaranperumal/Developer/OpenSource/openvino/bin/arm64/Release/./test_model_zoo/onnx/quantize_linear_const.onnx"

image

Are these ONNX files supposed to be generated during the build or setup process?

Build Instructions

cmake -G "Ninja Multi-Config" \
      -DCMAKE_BUILD_TYPE=Debug \
      -DENABLE_PYTHON=ON \
      -DENABLE_TESTS=ON \
      -DENABLE_FUNCTIONAL_TESTS=ON \
      -DENABLE_DEBUG_CAPS=ON \
      -DENABLE_CPU_DEBUG_CAPS=ON \
      -DENABLE_NCC_STYLE=ON \
      -DENABLE_SYSTEM_PUGIXML=ON \
      -DENABLE_SYSTEM_SNAPPY=ON \
      -DENABLE_SYSTEM_PROTOBUF=ON \
      -DPython3_EXECUTABLE=.venv/bin/python3 ..

cmake --build . --parallel $(sysctl -n hw.ncpu)

Please provide guidance on resolving this issue.

System Information

  • Device: M3 Pro
  • Operating System: macOS Sonoma 14.2.1

@inbasperu
Copy link
Contributor

Linking Error with libtbb_debug.12.2.dylib on macOS in Debug Configuration

While attempting to build a project in Debug configuration on macOS, I encountered a linker error concerning libtbb_debug.12.2.dylib. The error suggests a problem with the __LINKEDIT segment of the TBB library file, preventing successful compilation.

I configure the project with CMake for a Debug build with the following execute the build command: cmake --build . --config Debug --parallel $(sysctl -n hw.ncpu)

Error Message

The build process fails with the following linker error:
image
image

ld: segment '__LINKEDIT' load command content extends beyond end of file in '/Users/inbasekaranperumal/Developer/OpenSource/openvino/temp/tbb/lib/libtbb_debug.12.2.dylib'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Environment

  • Device: M3 Pro

  • Operating System: macOS Sonoma 14.2.1

  • Compiler: Apple clang version 15.0.0

  • TBB Version: 12.2 (debug version)

  • The issue specifically occurs with the debug version of the TBB library.

  • I have confirmed the existence of the libtbb_debug.12.2.dylib file and attempted to rebuild it, yet the problem persists.

I'm seeking advice to resolve this linking issue. Any input would be greatly appreciated. Thanks in advance for your time and help.

@inbasperu
Copy link
Contributor

Hello @p-wysocki,

I have started the implementation and encountered some uncertainties along the way. I have made a PR with the changes I've added. It would be great if you could review it and guide me on how to proceed further.

@p-wysocki
Copy link
Contributor

Thank you for the PR! Let's continue the discussion there.

@mlukasze mlukasze moved this from Assigned to In Review in Good first issues Feb 27, 2024
github-merge-queue bot pushed a commit that referenced this issue May 23, 2024
…ith original framework (#23798)

Hello maintainers,

Since my previous PR
(#22993 (comment))
had diverged significantly from the master branch, and also included
additional changes, I thought it would be best to create a new PR.
@gkrivor, could you please take a look?

This closes
#20563 (comment).

---------

Co-authored-by: Georgy Krivoruchko <georgy.krivoruchko@intel.com>
@github-project-automation github-project-automation bot moved this from In Review to Closed in Good first issues May 23, 2024
@mlukasze mlukasze added this to the 2024.2 milestone May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: ONNX FE OpenVINO ONNX FrontEnd good first issue Good for newcomers no_stale Do not mark as stale ONNX Related to support for ONNX standard.
Projects
Archived in project
4 participants