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 ReduceL2-11, 13, 18 with original framework #20560

Closed
gkrivor opened this issue Oct 18, 2023 · 7 comments · Fixed by #22741
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 ReduceL2 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
@LucaTamSapienza
Copy link
Contributor

.take

Copy link
Contributor

github-actions bot commented Feb 4, 2024

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

@LucaTamSapienza
Copy link
Contributor

Version Changes
ReduceL2-1 initial version
ReduceL2-11 axes range is now [-r, -r-1], r = rank(data)
ReduceL2-13 added support for T: tensor(bfloat16)
ReduceL2-18 axes attributes is now noop_with_empty_axes : int (default is 0)
now the inputs can be non-differentiable axes (optional, non-differentiable) : tensor(int64)

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

Hi, currently I'm adding namespaces set_11, 13, 18 inside reduce.cpp and reduce.hpp. I have registered the functions inside ops_bridge.cpp. Before creating the tests, I would like to have some feedback to know if I am proceeding in the right way. I have some questions about how to implement the changes that I've mentioned above.

@mlukasze
Copy link
Contributor

mlukasze commented Feb 6, 2024

you can prepare draft PR to show us changes you've prepared - it may make discussion easier :)

@LucaTamSapienza
Copy link
Contributor

LucaTamSapienza commented Feb 8, 2024

I apologize for the delay; I had some work to do. Here is my draft PR #22741

@mlukasze
Copy link
Contributor

mlukasze commented Feb 9, 2024

No worries, thanks for the draft.

@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 Apr 24, 2024
…22741)

### Details:
- I've aligned the ReduceL2 operation with opset 11, 13, and 18. I have
some doubts about how to implement support for the tensor bfloat16 for
opset 13 and also some doubts about opset 18. I've registered the
function inside the ops_bridge.cpp, created test models, and added them
inside onnx_import.in.cpp.
### Tickets:
 - Closes #20560

---------

Co-authored-by: Przemyslaw Wysocki <przemyslaw.wysocki@intel.com>
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 Apr 24, 2024
@mlukasze mlukasze added this to the 2024.2 milestone Apr 24, 2024
alvoron pushed a commit to alvoron/openvino that referenced this issue Apr 29, 2024
…penvinotoolkit#22741)

### Details:
- I've aligned the ReduceL2 operation with opset 11, 13, and 18. I have
some doubts about how to implement support for the tensor bfloat16 for
opset 13 and also some doubts about opset 18. I've registered the
function inside the ops_bridge.cpp, created test models, and added them
inside onnx_import.in.cpp.
### Tickets:
 - Closes openvinotoolkit#20560

---------

Co-authored-by: Przemyslaw Wysocki <przemyslaw.wysocki@intel.com>
Co-authored-by: Georgy Krivoruchko <georgy.krivoruchko@intel.com>
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
Development

Successfully merging a pull request may close this issue.

3 participants