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

[ONNX FE] Add SoftmaxCrossEntropyLoss operator to ONNX Frontend #28682

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

AJThePro99
Copy link

@AJThePro99 AJThePro99 commented Jan 26, 2025

This pull request implements the SoftmaxCrossEntropyLoss operation for the ONNX frontend in OpenVINO, supporting opset 12 and opset 13. It includes new header and source files to define and implement the operation.

Ticket #20547

New operation implementation:


TODO

  • 1. Create softmax_cross_entropy_loss.cpp & softmax_cross_entropy_loss.hpp
  • 2. Add implementation of SoftMaxCrossEntropyLoss for opset_12 & opset_13
  • 3. Register the function in ops_bridge.cpp
  • 4. Creating test models within ONNX Models directory
  • 5. Added test cases covering all use cases here
  • 6. Check Python xfailed tests for added functionality
  • 7. Final code formatting to adhere to OpenVINO codebase standards

P.S.: I'm very new to contributing to large codebases, and I want to know if my work meets the standards and most of all, is what's expected!

As a new contributor, I would really appreciate your guidance on the following:

  1. Does the implementation meet OpenVINO standards?
  2. Are there additional edge cases or test scenarios I should include? (Excluding TODOs)
  3. How can I improve code readability, efficiency, or maintainability?

@github-actions github-actions bot added the category: ONNX FE OpenVINO ONNX FrontEnd label Jan 26, 2025
@sys-openvino-ci sys-openvino-ci added the ExternalPR External contributor label Jan 26, 2025
@AJThePro99
Copy link
Author

Hi @gkrivor
I’ve created a draft pull request for the implementation of SoftmaxCrossEntropyLoss. Since I’m new to contributing to OpenVINO, I’d appreciate it if you could take a look and let me know if I’m on the right track. Let me know if there are any changes or additional requirements I should address. Thanks for your time!

@AJThePro99 AJThePro99 marked this pull request as ready for review January 26, 2025 16:21
@AJThePro99 AJThePro99 requested a review from a team as a code owner January 26, 2025 16:21
@AJThePro99
Copy link
Author

Hello!
I'd like to know if there exists a way to build the components of only the onnx frontend instead of the entire project src/frontends/onnx

It does have a CMakeLists.txt file, so I'm wondering if I could save time by doing that.
Or, does it not work that way?

@AJThePro99 AJThePro99 marked this pull request as draft January 27, 2025 06:13
@AJThePro99
Copy link
Author

Please bear with me for a while. My laptop's number keys just gave up and do not function anymore. And I have no external keyboard at the moment. It's going to take a while for my next update, after the servicing is over.
I apologize for the delay

@AJThePro99 AJThePro99 force-pushed the softmax_cross_entropy_loss_for_onnx_fe branch from 7021ae0 to 451d0cb Compare February 1, 2025 07:18
@AJThePro99 AJThePro99 force-pushed the softmax_cross_entropy_loss_for_onnx_fe branch from 451d0cb to d4ea9f1 Compare February 1, 2025 07:26
@AJThePro99
Copy link
Author

Hello @rkazants !
I've successfully completed the implementation of the SoftmaxCrossentropyLoss function within frontend/src/op
And, I've started working on the test cases.
My main question is how do you 'test' the functionality yourself?

@AJThePro99 AJThePro99 force-pushed the softmax_cross_entropy_loss_for_onnx_fe branch from 4242b3c to c19673e Compare February 8, 2025 04:39
@AJThePro99 AJThePro99 marked this pull request as ready for review February 8, 2025 04:40
@AJThePro99
Copy link
Author

AJThePro99 commented Feb 8, 2025

This pull request introduces the implementation of the Softmax Cross Entropy Loss operation for the ONNX frontend. The changes include adding new header and source files to define and implement the operation for two different opsets (12 and 13). The most important changes are as follows:

Addition of new header and source files

Implementation details

  • The impl_softmax_cross_entropy function handles the core logic of the Softmax Cross Entropy Loss, including support for optional weights and the ignore index attribute.
  • The function supports reduction modes ("mean" and "sum") and includes error handling for dynamic rank scenarios.

I have a question about registering the operator registration.
According to how_to_add_op.md, the registration looks like this within ops_bridge.cpp:

#include "op/org.openvinotoolkit/custom_add.hpp"
...
REGISTER_OPERATOR_WITH_DOMAIN(OPENVINO_ONNX_DOMAIN, "CustomAdd", 1, custom_add);

But, ops_bridge.cpp looks different and leads to a dead end, quite different from the docs.

But, when I compared softmax_cross_entropy_loss.cpp to other operators in the onnx/frontend/src/op directory, All of the operators are registered using a macro ONNX_OP(), hence - I used the same. This is just speculation for now, and I do not know if I properly completed the operator registration.
I'd like for you to guide me in the right direction if there are more things to be done.

And, I'd really appreciate it if there is a resource that will help me understand how to write the .prototxt test model files . If you know any, please redirect me to them . It'll help me out a lot.

Thanks a bunch!

@AJThePro99 AJThePro99 requested a review from rkazants February 8, 2025 04:48
@AJThePro99 AJThePro99 force-pushed the softmax_cross_entropy_loss_for_onnx_fe branch 3 times, most recently from 093e1ac to a0019fa Compare February 8, 2025 05:22
@AJThePro99 AJThePro99 force-pushed the softmax_cross_entropy_loss_for_onnx_fe branch from a0019fa to d95757d Compare February 8, 2025 05:23
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 ExternalPR External contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants