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.

Sign up for GitHub

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

Jump to bottom

[ONNX]: add support for microsoft.range operator for ONNX frontend #28202

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

[ONNX]: add support for microsoft.range operator for ONNX frontend #28202

wants to merge 3 commits into from
+174 −0

Conversation

11happy
Copy link
Contributor

@11happy 11happy commented Dec 25, 2024

Overview:
This pull request fixes #17575 .

Dependencies:

  • No dependencies on other pull requests.

CC:

@11happy 11happy requested review from a team as code owners December 25, 2024 09:17
@github-actions github-actions bot added category: GPU OpenVINO GPU plugin category: ONNX FE OpenVINO ONNX FrontEnd labels Dec 25, 2024
@sys-openvino-ci sys-openvino-ci added the ExternalPR External contributor label Dec 25, 2024
@11happy
Copy link
Contributor Author

11happy commented Dec 28, 2024

Humble Ping @mlukasze , @p-wysocki to please review this PR.
Thank you

@sshlyapn
Copy link
Contributor

@11happy could you please remove GPU's onednn submodule update from the changes?

@11happy 11happy requested a review from a team as a code owner December 30, 2024 17:43
@github-actions github-actions bot added the category: dependency_changes Pull requests that update a dependency file label Dec 30, 2024
@11happy
Copy link
Contributor Author

11happy commented Dec 30, 2024

@sshlyapn I have removed this submodule

@github-actions github-actions bot removed the category: dependency_changes Pull requests that update a dependency file label Dec 30, 2024
@11happy
Copy link
Contributor Author

11happy commented Jan 2, 2025

@sshlyapn can you please review this

@p-durandin
Copy link
Contributor

build_jenkins

@11happy
Copy link
Contributor Author

11happy commented Jan 4, 2025

I ran clang format to resolve some of the build issues.
Thank you

@p-durandin
Copy link
Contributor

build_jenkins

@p-durandin
Copy link
Contributor

@11happy please check if oneDNN changes are required. Looks not

@11happy
correct & remove submodule
dee0205 
Signed-off-by: 11happy <soni5happy@gmail.com>
@11happy 11happy force-pushed the onnx.microsoft.range branch from 03a8081 to dee0205 Compare January 6, 2025 17:40
@github-actions github-actions bot removed the category: GPU OpenVINO GPU plugin label Jan 6, 2025
@11happy
Copy link
Contributor Author

11happy commented Jan 6, 2025 •
edited
Loading

@p-durandin done.
thanks

@p-durandin
Copy link
Contributor

p-durandin commented Jan 7, 2025 •
edited
Loading

@11happy, thanks. GPU review is not required for this PR, please fix code style

@11happy
correct code style & size checks
41bbd42 
Signed-off-by: 11happy <soni5happy@gmail.com>
@11happy
Copy link
Contributor Author

11happy commented Jan 7, 2025

I have corrected the code style & also corrected the checks related to size check.
thanks

gkrivor
gkrivor requested changes Jan 13, 2025
View reviewed changes
Copy link
Contributor

@gkrivor gkrivor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, could you apply a few comments, please? And as I may see - PR is mostly ready for merge.

src/frontends/onnx/frontend/src/op/com.microsoft/range.cpp Outdated
@@ -0,0 +1,31 @@
// Copyright (C) 2018-2024 Intel Corporation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, use 2025 for now :) Happy new year!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.
Happy new year :)

src/frontends/onnx/frontend/src/op/com.microsoft/range.cpp Outdated
namespace opset_1 {
ov::OutputVector range(const ov::frontend::onnx::Node& node) {
auto nodes = node.get_ov_inputs();
FRONT_END_GENERAL_CHECK(nodes.size() >= 2 && nodes.size() <= 3,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, use commom::default_op_checks procedure from "utils/common.hpp". Like here:

openvino/src/frontends/onnx/frontend/src/op/com.microsoft/simplified_layer_normalization.cpp

Line 28 in 0db7f8e

common::default_op_checks(node, 2);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

src/frontends/onnx/frontend/src/op/com.microsoft/range.cpp Outdated
auto nodes = node.get_ov_inputs();
FRONT_END_GENERAL_CHECK(nodes.size() >= 2 && nodes.size() <= 3,
"Range takes 2 or 3 inputs. Provided " + std::to_string(nodes.size()));
auto start = nodes.at(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And use nodes[0] and nodes[1] for inputs which availability was checked before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

src/frontends/onnx/frontend/src/op/com.microsoft/range.cpp Outdated
"Range takes 2 or 3 inputs. Provided " + std::to_string(nodes.size()));
auto start = nodes.at(0);
auto limit = nodes.at(1);
auto step =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/microsoft/onnxruntime/blob/main/docs/ContribOperators.md#com.microsoft.Range

Documentation requires same input tensors for all inputs. Could you add a check for element types?

Like here:

openvino/src/frontends/onnx/frontend/src/op/com.microsoft/simplified_layer_normalization.cpp

Line 34 in 0db7f8e

CHECK_VALID_NODE(node,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added this , but this is causing repeated code which I think is not preferred , so should I put this in a loop or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also I have changed step to delta to keep it consistent with documentation.
thank you

@11happy
refactor: add suggested changes
72dbf59 
Signed-off-by: 11happy <soni5happy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gkrivor gkrivor gkrivor requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
category: ONNX FE OpenVINO ONNX FrontEnd ExternalPR External contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extend ONNX Frontend with com.microsoft.Range operator
5 participants
@11happy @sshlyapn @p-durandin @gkrivor @sys-openvino-ci