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

【PaddlePaddle Hackathon 3】Add Paddle box_coder operator #12394

Merged
merged 3 commits into from
Nov 22, 2022
Merged

【PaddlePaddle Hackathon 3】Add Paddle box_coder operator #12394

merged 3 commits into from
Nov 22, 2022

Conversation

Asthestarsfalll
Copy link
Contributor

Details:

add box_coder operation in Paddle front end

Reference:

https://github.com/PaddlePaddle/Paddle/blob/98f8fa4cb3d7b9c14c1897f2716249a54c44f8be/paddle/phi/kernels/cpu/box_coder.cc
The paddle documents are different with source code:
https://www.paddlepaddle.org.cn/documentation/docs/en/1.8/api/layers/box_coder.html

uni-test passed

image

@Asthestarsfalll
Copy link
Contributor Author

@OpenVINO-dev-contest
Could you please take a look? Thanks!

@openvino-dev-samples
Copy link
Contributor

@OpenVINO-dev-contest Could you please take a look? Thanks!

Thanks for your contribution. We will review it ASAP.

@yuxu42
Copy link
Contributor

yuxu42 commented Aug 4, 2022

@ceciliapeng2011 Could you please review?

@ceciliapeng2011
Copy link
Contributor

Please fix clang format error.

It could automatically fixed with steps -

  1. apt install clang-format-9
  2. enable clang with cmake option: -DENABLE_CLANG_FORMAT=ON
  3. make clang_format_fix_all

Then submit PR again.

@Asthestarsfalll
Copy link
Contributor Author

Asthestarsfalll commented Aug 4, 2022

@ceciliapeng2011 How can I trigger the code style check?

src/frontends/paddle/src/op/box_coder.cpp Outdated Show resolved Hide resolved
src/frontends/paddle/src/op/box_coder.cpp Outdated Show resolved Hide resolved
src/frontends/paddle/src/op/box_coder.cpp Outdated Show resolved Hide resolved
src/frontends/paddle/src/op/box_coder.cpp Outdated Show resolved Hide resolved
src/frontends/paddle/src/op/box_coder.cpp Outdated Show resolved Hide resolved
src/frontends/paddle/src/op/box_coder.cpp Outdated Show resolved Hide resolved
@ceciliapeng2011
Copy link
Contributor

@ceciliapeng2011 How can I trigger the code style check?

I did it for you.

@Asthestarsfalll
Copy link
Contributor Author

@ceciliapeng2011
Hi, I'm wondering which version of paddle used in ci test.

@ceciliapeng2011
Copy link
Contributor

@ceciliapeng2011 Hi, I'm wondering which version of paddle used in ci test.

paddle 2.1.0

@ceciliapeng2011 ceciliapeng2011 added PaddlePaddle Hackathon a Intel and Baidu joint Hackathon event category: PDPD FE OpenVINO PaddlePaddle FrontEnd labels Sep 1, 2022
@Asthestarsfalll Asthestarsfalll requested a review from a team as a code owner September 8, 2022 10:11
@Asthestarsfalll
Copy link
Contributor Author

@ceciliapeng2011
Hi, I find out that the following dygraph tests after the first one would fail, even though all the test settings are same.

Copy link
Contributor

@ceciliapeng2011 ceciliapeng2011 left a comment

Choose a reason for hiding this comment

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

I would suggest to submit the static shape support in this PR. And for dynamic shape situations, if the problem is still there and cannot catch up the deadline, we could solve it in a separate PR. So just comment the code lines and leave it there.

@Asthestarsfalll
Copy link
Contributor Author

I would suggest to submit the static shape support in this PR. And for dynamic shape situations, if the problem is still there and cannot catch up the deadline, we could solve it in a separate PR. So just comment the code lines and leave it there.

I find that the openvino-lin and openvino-win passed, it seems the problem is deu to my local develop environment?

@ceciliapeng2011
Copy link
Contributor

I would suggest to submit the static shape support in this PR. And for dynamic shape situations, if the problem is still there and cannot catch up the deadline, we could solve it in a separate PR. So just comment the code lines and leave it there.

I find that the openvino-lin and openvino-win passed, it seems the problem is deu to my local develop environment?

They are just building pass. Paddle FE UT is under "ci/jenkins". Please -

  1. comment out dynamic test cases. let's make sure static shape cases pass first.
  2. fix clang-format errors.

Then I will trigger ci/jenkins again.

@Asthestarsfalll
Copy link
Contributor Author

Asthestarsfalll commented Sep 19, 2022

I would suggest to submit the static shape support in this PR. And for dynamic shape situations, if the problem is still there and cannot catch up the deadline, we could solve it in a separate PR. So just comment the code lines and leave it there.

I find that the openvino-lin and openvino-win passed, it seems the problem is deu to my local develop environment?

They are just building pass. Paddle FE UT is under "ci/jenkins". Please -

  1. comment out dynamic test cases. let's make sure static shape cases pass first.
  2. fix clang-format errors.

Then I will trigger ci/jenkins again.

done.

I use vscode for code formatting, but it still can not pass.

@Asthestarsfalll
Copy link
Contributor Author

@ceciliapeng2011
Can you trigger ci/jenkins first to see if the static tests can pass.
I will resovle code style problem later.

@ceciliapeng2011
Copy link
Contributor

I use vscode for code formatting, but it still can not pass.

diff --git a/src/frontends/paddle/src/op_table.cpp b/src/frontends/paddle/src/op_table.cpp
index 50608492..530d4165 100644
--- a/src/frontends/paddle/src/op_table.cpp
+++ b/src/frontends/paddle/src/op_table.cpp
@@ -111,7 +111,7 @@ std::map<std::string, CreatorFunction> get_supported_ops() {
             {"bilinear_interp_v2", op::bilinear_interp_v2},
             {"bilinear_interp", op::bilinear_interp_v2},
             {"bmm", op::matmul},
-            {"box_coder", op::box_coder}, **<**a invisible space here!**>**
+            {"box_coder", op::box_coder},
             {"cast", op::cast},
             {"clip", op::clip},
             {"concat", op::concat},

@openvino-dev-samples
Copy link
Contributor

Hi @Asthestarsfalll can you help to fix the format issue ?

@Asthestarsfalll
Copy link
Contributor Author

Hi @Asthestarsfalll can you help to fix the format issue ?

Done.

@Asthestarsfalll
Copy link
Contributor Author

@ceciliapeng2011
HI, can you trigger ci/jenkins for me? Thanks!

@ilyachur ilyachur added this to the 2022.3 milestone Sep 23, 2022
src/core/tests/frontend/paddle/op_fuzzy.cpp Outdated Show resolved Hide resolved
src/frontends/paddle/src/op/box_coder.cpp Outdated Show resolved Hide resolved
@ceciliapeng2011 ceciliapeng2011 self-assigned this Sep 30, 2022
@ilyachur ilyachur enabled auto-merge (squash) September 30, 2022 06:21
@Asthestarsfalll
Copy link
Contributor Author

@ceciliapeng2011
Hi, I'm wondering that why the ci can not pass. I can't see details about ci/jenkins.

@ceciliapeng2011
Copy link
Contributor

Can you please rebase and resolve the conflicts? Thanks. @Asthestarsfalll

auto-merge was automatically disabled November 6, 2022 04:38

Head branch was pushed to by a user without write access

auto-merge was automatically disabled November 6, 2022 04:38

Pull request was closed

@Asthestarsfalll
Copy link
Contributor Author

Can you please rebase and resolve the conflicts? Thanks. @Asthestarsfalll

Done

@Asthestarsfalll
Copy link
Contributor Author

@ceciliapeng2011
HI, can you trigger ci/jenkins for me? Thanks!

@Asthestarsfalll
Copy link
Contributor Author

Since this PR has been suspended for nearly two months, I really want to figure out the reason of CI failure.Could you please give me some prompts in this situation? Thanks!
@ceciliapeng2011 @ilyachur

@ilyachur ilyachur enabled auto-merge (squash) November 16, 2022 06:24
@ceciliapeng2011
Copy link
Contributor

Since this PR has been suspended for nearly two months, I really want to figure out the reason of CI failure.Could you please give me some prompts in this situation? Thanks!

I am keeping an eye on it. Should be okay after manually triggering the private CI.

@ilyachur ilyachur merged commit 2edb9f7 into openvinotoolkit:master Nov 22, 2022
Lyamin-Roman pushed a commit to Lyamin-Roman/openvino that referenced this pull request Nov 22, 2022
Co-authored-by: Ilya Churaev <ilya.churaev@intel.com>
@ilya-lavrenov ilya-lavrenov added the ExternalPR External contributor label Mar 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: PDPD FE OpenVINO PaddlePaddle FrontEnd ExternalPR External contributor PaddlePaddle Hackathon a Intel and Baidu joint Hackathon event
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants