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

[PT FE] Add aten::rrelu #27528

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

[PT FE] Add aten::rrelu #27528

wants to merge 11 commits into from

Conversation

AdamPSU
Copy link

@AdamPSU AdamPSU commented Nov 13, 2024

Details:

  • Add support for aten::rrelu

Tickets:

@github-actions github-actions bot added category: build OpenVINO cmake script / infra category: PyTorch FE OpenVINO PyTorch Frontend labels Nov 13, 2024
@sys-openvino-ci sys-openvino-ci added the ExternalPR External contributor label Nov 13, 2024
@AdamPSU AdamPSU marked this pull request as ready for review November 13, 2024 04:10
@AdamPSU AdamPSU requested review from a team as code owners November 13, 2024 04:10
@AdamPSU
Copy link
Author

AdamPSU commented Nov 13, 2024

Feedback? This is my first contribution to anything, really, so apologies if things are a bit messy. A few points:

I accidentally pushed changes to cmake/developer_package/frontends/frontends.cmake. I take it I'm supposed to remove those, correct?

Also, I’m encountering the following error:

CMake Error at cmake/developer_package/frontends/frontends.cmake:195 (add_library):
Cannot find source file: ../../../src/frontends/pytorch/src/op/rrelu.cpp

Does this indicate an issue with my build setup?

Am I on the right track with this PR? I realize I’m missing aten::rrelu_ and tests, but I’m hoping to get feedback before proceeding further.

Thanks for your help!

@AdamPSU AdamPSU changed the title [PT FE] Add aten::relu [PT FE] Add aten::rrelu Nov 13, 2024
@AdamPSU AdamPSU closed this Nov 19, 2024
@mvafin
Copy link
Contributor

mvafin commented Nov 20, 2024

@AdamPSU Sorry for delayed response. You were going in the right direction, why did you close the PR? Do you to continue it?

@AdamPSU
Copy link
Author

AdamPSU commented Nov 20, 2024

I got cold feet and feared I was doing something wrong... But it's reassuring hearing you say this. I'm willing to open the PR again, though confirmation to the following would be great:

1.) Is something wrong with my build (refer to the above discussion post)
2.) Is there a simple way to test my code? It felt like I was coding blindholded, haha...

@AdamPSU AdamPSU reopened this Nov 20, 2024
@mvafin
Copy link
Contributor

mvafin commented Nov 20, 2024

I got cold feet and feared I was doing something wrong... But it's reassuring hearing you say this. I'm willing to open the PR again, though confirmation to the following would be great:

1.) Is something wrong with my build (refer to the above discussion post) 2.) Is there a simple way to test my code? It felt like I was coding blindholded, haha...

  1. You are right, you do not need to modify .cmake file. It should pick up when you run cmake automatically.
  2. Yes, you can test your code. First you need to add your operation in the list of supported operations here: https://github.com/openvinotoolkit/openvino/blob/master/src/frontends/pytorch/src/op_table.cpp
    After that please add test for rrelu here https://github.com/openvinotoolkit/openvino/tree/master/tests/layer_tests/pytorch_tests
    You can use a similar test as reference, for example: https://github.com/openvinotoolkit/openvino/blob/master/tests/layer_tests/pytorch_tests/test_clamp.py
    To run the test locally you can refer here: https://github.com/openvinotoolkit/openvino/blob/master/tests/layer_tests/README.md
    It is only required to run test on CPU with FP32 precision.

You do not need to add aten::rrelu_ for now, it should work automatically now, but you need to add tests for it.

Comment on lines 23 to 24
Output<Node> lower = ov::op::v0::Constant::create(element::f32, Shape{1}, {1.0f / 8.0f});
Output<Node> upper = ov::op::v0::Constant::create(element::f32, Shape{1}, {1.0f / 3.0f});
Copy link
Contributor

Choose a reason for hiding this comment

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

lower and upper are the inputs, so please use context.get_input to get them.

Copy link
Author

Choose a reason for hiding this comment

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

Will there always be inputs? According to the documentation for rrelu, if the inputs are omitted, lower is set to 1/8 and upper is 1/3.

Copy link
Contributor

@mvafin mvafin Nov 21, 2024

Choose a reason for hiding this comment

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

No, it is not guaranteed, so you need to check if they are present using input_is_none

@@ -193,7 +193,7 @@ macro(ov_add_frontend)

# Create library
add_library(${TARGET_NAME} ${LIBRARY_SRC} ${LIBRARY_HEADERS} ${LIBRARY_PUBLIC_HEADERS}
${PROTO_SRCS} ${PROTO_HDRS} ${flatbuffers_schema_files} ${proto_files})
${PROTO_SRCS} ${PROTO_HDRS} ${flatbuffers_schema_files} ${proto_files} "../../../src/frontends/pytorch/src/op/rrelu.cpp")
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this change please

@AdamPSU
Copy link
Author

AdamPSU commented Nov 20, 2024

Thanks -- a bit busy because of exams, but I'll get to this on Thursday. May take me a couple of days, but I'll try my best. Thanks for the patience, and I'll be sure to write those test cases. :)

@mvafin
Copy link
Contributor

mvafin commented Nov 20, 2024

Thanks -- a bit busy because of exams, but I'll get to this on Thursday. May take me a couple of days, but I'll try my best. Thanks for the patience, and I'll be sure to write those test cases. :)

Okay, no worries

@github-actions github-actions bot removed the category: build OpenVINO cmake script / infra label Nov 22, 2024
@AdamPSU
Copy link
Author

AdamPSU commented Nov 22, 2024

@mvafin Feel free to take a look at some of the changes; I believe I'm done with aten::rrelu. I intend to experiment with the test cases later tonight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: PyTorch FE OpenVINO PyTorch Frontend ExternalPR External contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants