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 operator ReduceMean-11, 13, 18 with original framework #20556

Closed
gkrivor opened this issue Oct 18, 2023 · 12 comments · Fixed by #23148
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 ReduceMean 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. Operator 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
@mlukasze mlukasze added the ONNX Related to support for ONNX standard. label Oct 26, 2023
@rghvsh
Copy link
Contributor

rghvsh commented Dec 26, 2023

.take

Copy link
Contributor

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

@rghvsh
Copy link
Contributor

rghvsh commented Dec 27, 2023

Hey! @gkrivor just wanted to make sure that I got this right:

I need to change the properties of ReduceMean such that they are same in each opset. For example ReduceMean-18 has an extra attribute noop_with_empty_axes and 2 inputs instead of 1 unlike in other opsets.So I need to add this to other or remove from this?

Could you also point me to the original framework tried searching but there were too many with same name didn't know which to pick.

Thanks
Raghav

@p-wysocki
Copy link
Contributor

Hello @rghvsh, sorry for the late reply, the holiday season just ended. :) @gkrivor could you please take a look?

Also, I am happy to announce that we have created a channel dedicated to Good First Issues support on our Intel DevHub Discord server! Join it to receive support, engage in discussions, ask questions and talk to OpenVINO developers.

@rghvsh
Copy link
Contributor

rghvsh commented Jan 17, 2024

Hey! @gkrivor @p-wysocki which of the these should I consider as the original framework?

@mlukasze
Copy link
Contributor

https://github.com/onnx/onnx/blob/main/docs/Operators.md#ReduceMean

@gkrivor
Copy link
Contributor Author

gkrivor commented Jan 17, 2024

@rghvsh hope you are doing great!

Yeah, you a right, if documentation says we have a difference in inputs (input changed to an attribute, or opposite way) - need to implement correct translation function which will do it. But keeping old implementation too, because models which implements older nodes (for example, in a range 13-18 they will use implementation for opset 13) should be also functional.

I've added a link to an issue with similar discussion: #20553

I suggest to identify difference and define it as a table here (would be easier to review later).

You should get something like https://github.com/openvinotoolkit/openvino/blob/master/src/frontends/onnx/frontend/src/op/softmax.cpp which will implement a found differences.

As you remember, you will need to add a tests for exact implementations like here: #21825

I mean a prototxt file and corresponing unit test to verify a correctness.

@p-wysocki
Copy link
Contributor

Hello @rghvsh, are you still working on that issue?

@rghvsh
Copy link
Contributor

rghvsh commented Jan 31, 2024

Hey! @p-wysocki yes

@rghvsh
Copy link
Contributor

rghvsh commented Feb 3, 2024

Image

@gkrivor
Copy link
Contributor Author

gkrivor commented Feb 4, 2024

@rghvsh great! as you can see most important change is in opset-18. opset-11 and -13 has a different set of supported input types.
Now you can re-use existing logic to prepare a PR with changes. Be notified some tests may unexpectedly fail on CPU due to broken shape inference, I'll fix it soon.

@rghvsh
Copy link
Contributor

rghvsh commented Feb 28, 2024

PR #23148

github-merge-queue bot pushed a commit that referenced this issue Jun 5, 2024
… with original framework (#23148)

### Details:
- Align behavior of ONNX Frontend operator ReduceMean-11, 13, 18 with
original framework

### Tickets:
 - Closes #20556

---------

Co-authored-by: Georgy Krivoruchko <georgy.krivoruchko@intel.com>
@mlukasze mlukasze added this to the 2024.3 milestone Jun 6, 2024
mory91 pushed a commit to mory91/openvino that referenced this issue Jun 10, 2024
… with original framework (openvinotoolkit#23148)

### Details:
- Align behavior of ONNX Frontend operator ReduceMean-11, 13, 18 with
original framework

### Tickets:
 - Closes openvinotoolkit#20556

---------

Co-authored-by: Georgy Krivoruchko <georgy.krivoruchko@intel.com>
alexandruenache1111 pushed a commit to alexandruenache1111/openvino that referenced this issue Jun 13, 2024
… with original framework (openvinotoolkit#23148)

### Details:
- Align behavior of ONNX Frontend operator ReduceMean-11, 13, 18 with
original framework

### Tickets:
 - Closes openvinotoolkit#20556

---------

Co-authored-by: Georgy Krivoruchko <georgy.krivoruchko@intel.com>
allnes pushed a commit to allnes/openvino that referenced this issue Jun 27, 2024
… with original framework (openvinotoolkit#23148)

### Details:
- Align behavior of ONNX Frontend operator ReduceMean-11, 13, 18 with
original framework

### Tickets:
 - Closes openvinotoolkit#20556

---------

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
4 participants