openvinotoolkit / openvino

OpenVINO™ is an open-source toolkit for optimizing and deploying AI inference
https://docs.openvino.ai
Apache License 2.0
7.23k stars 2.26k forks source link

[Good First Issue]: Align behavior of ONNX Frontend operator ReduceMin-11, 12, 13, 18, 20 with original framework #20557

Closed gkrivor closed 7 months ago

gkrivor commented 1 year ago

Context

eural 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 ReduceMin for next list of opsets: opset 11, opset 12, opset 13, opset 18, opset 20 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 12, opset 13, opset 18, opset 20

  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

salmanra commented 10 months ago

.take

github-actions[bot] commented 10 months ago

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

salmanra commented 10 months ago

@gkrivor

  1. I have set up a table detailing the difference between the different ONNX Opsets' ReduceMin operators.
  2. It seems that reduce.cpp has most of the necessary logic supporting the differences between each version of this operator (barring varied support for tensor types). Therefore, should I define namespaces for each of the opsets 11, 12, 13, 18, and 20, and make sure they call make_ng_reduction_op with the proper parameters? Additionally, how do we specify supported types for the input and output tensors? Right now, the common impl of ReduceMin uses a set defined in element_type.hpp. Do we want to define different sets of element types corresponding to those supported by each ONNX opset?
  3. I haven't figured out how to register a function in ops_bridge.cpp. Do we register each version of ReduceMin? Or do we register ReduceMin exactly once regardless of how many different opsets it appears in?
p-wysocki commented 10 months ago

Hello @salmanra, 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.

gkrivor commented 9 months ago

@salmanra sorry for the delay.

  1. Great! You've identified difference correct, especially important one - axes as an input instead of attribute
  2. Yep, you are right, most of logic is available and should be re-used. Yes, translators should be placed in a separate namespaces. No, you don't need to define some additional types, just use existing list.
  3. Yes, in should be registered for each opset.
p-wysocki commented 8 months ago

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

salmanra commented 8 months ago

@p-wysocki Hi, apologies, work has been busy, and I am transitioning from work to starting a PhD later this Summer.

Best for me would be to drop this issue and pick up a new one sometime in April or May.

mlukasze commented 8 months ago

No problem at all! Have a nice time and we will wait for you in April :)

YaritaiKoto commented 8 months ago

.take

github-actions[bot] commented 8 months ago

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