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

Open
gkrivor opened this issue Oct 18, 2023 · 17 comments
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.

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 ReduceL1 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
@mlukasze mlukasze added the ONNX Related to support for ONNX standard. label Oct 26, 2023
@Gabriellgpc
Copy link

.take

Copy link
Contributor

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

@p-wysocki
Copy link
Contributor

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.

@p-wysocki
Copy link
Contributor

I'm reopening the issue due to current assignee's inactivity. If you with to repick the issue please let me know.

@RitikaxShakya
Copy link
Contributor

.take

Copy link
Contributor

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

@RitikaxShakya
Copy link
Contributor

RitikaxShakya commented Jan 13, 2024

@gkrivor Hello! I have created the table of differences and changes made on versions of ReduceL1 image
image

@gkrivor
Copy link
Contributor Author

gkrivor commented Jan 17, 2024

@RitikaxShakya awesome! I'll add link to a issue with similar discussion: #20553

Now I'm expecting to achieve something like https://github.com/openvinotoolkit/openvino/blob/master/src/frontends/onnx/frontend/src/op/softmax.cpp which will implement a found differences.

Also 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 @RitikaxShakya, are you still working on that issue?

@RitikaxShakya
Copy link
Contributor

Yes, I am still working on it, I have made the changes so now working on running tests.

@gkrivor
Copy link
Contributor Author

gkrivor commented Feb 4, 2024

@RitikaxShakya hi, I had a chance take a look on set of Reduce*-18 operations. All of them could have issues with tests due to problems with a shape inference, because of newly added axes-input. I'll fix it soon.

@RitikaxShakya
Copy link
Contributor

Thank you

@p-wysocki
Copy link
Contributor

Helo @RitikaxShakya, are you still working on that issue?

@RitikaxShakya
Copy link
Contributor

Yes I am working on it as the issues with tests due to problems with a shape inference, because of newly added axes-input is fixed.

@RitikaxShakya
Copy link
Contributor

RitikaxShakya commented Mar 3, 2024

@gkrivor @p-wysocki Hello! I have completed all 6 steps mentioned in the issue description and here's the PR

@RitikaxShakya
Copy link
Contributor

RitikaxShakya commented Mar 22, 2024

Hello, I would like if someone @gkrivor @p-wysocki can review my PR. I have updated it with new changes while taking ReduceMax as reference. Had some closed PRs (sorry about this) because i got into some issue while resolving merging conflicts so created another pr for this. Thank you!

@mlukasze mlukasze linked a pull request Jun 18, 2024 that will close this issue
github-merge-queue bot pushed a commit that referenced this issue Aug 9, 2024
### Details:
-I've aligned the ReduceL1 operation with opset 11, 13, and 18. created
test models, and added them inside onnx_import.in.cpp.

### Tickets:
 - **#20559

---------

Co-authored-by: Georgy Krivoruchko <georgy.krivoruchko@intel.com>
mory91 pushed a commit to mory91/openvino that referenced this issue Aug 13, 2024
…it#25909)

### Details:
-I've aligned the ReduceL1 operation with opset 11, 13, and 18. created
test models, and added them inside onnx_import.in.cpp.

### Tickets:
 - **openvinotoolkit#20559

---------

Co-authored-by: Georgy Krivoruchko <georgy.krivoruchko@intel.com>
@hub-bla
Copy link
Contributor

hub-bla commented Sep 26, 2024

Hey @mlukasze, @gkrivor, I think it can be closed due to #25909.

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
Status: Contributors Needed
Development

Successfully merging a pull request may close this issue.

6 participants