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 group_norm operator #12329

Merged
merged 6 commits into from
Sep 13, 2022
Merged

【PaddlePaddle Hackathon 3】Add Paddle group_norm operator #12329

merged 6 commits into from
Sep 13, 2022

Conversation

Copy link
Contributor

@openvino-dev-samples openvino-dev-samples left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, could you provide a screenshot for unitest results?


auto shape = ov::opset6::Constant::create(ngraph::element::i64, Shape{3}, {0, num_groups, -1});
auto reshape_input = std::make_shared<ov::opset6::Reshape>(data, shape);
auto scale_ = ov::opset6::Constant::create(dtype, {1}, {1.0 * num_groups});
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 the default_opset , not ov::opset6

auto dtype = data.get_element_type();
auto num_groups = node.get_attribute<int32_t>("Groups");
auto epsilon = node.get_attribute<float>("Epsilon");
auto data_layout = node.get_attribute<std::string>("Data_layout");
Copy link
Contributor

Choose a reason for hiding this comment

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

better use the default value for 'get_attribute()' function in case it throw error for models which doesn't set this attributes

@Patrick-Star125
Copy link
Contributor Author

I faced this problem and I don't know how to fix it. @OpenVINO-dev-contest
image
I think this led to the error described below
image

  1. I can't find the error log called "unable_build_paddle_models.txt".
  2. It's made by wrong code? or compilation mistaken? I want to figure out the potential factors.

@openvino-dev-samples
Copy link
Contributor

I faced this problem and I don't know how to fix it. @OpenVINO-dev-contest image I think this led to the error described below image

  1. I can't find the error log called "unable_build_paddle_models.txt".
  2. It's made by wrong code? or compilation mistaken? I want to figure out the potential factors.
  1. Have you installed Paddle ? If you haven't Paddle environment, the model generation will not run.
  2. Try run the model generation script separately.

@Patrick-Star125
Copy link
Contributor Author

Thank you, I fixed that problem by adding python path to the cmake command line parameters
But I ran into another tricky problem (oh..
image
I think it is some kind of type conversion error. But due to lacking of information, I still have no clue about how to deal with it.

@openvino-dev-samples
Copy link
Contributor

Thank you, I fixed that problem by adding python path to the cmake command line parameters But I ran into another tricky problem (oh.. image I think it is some kind of type conversion error. But due to lacking of information, I still have no clue about how to deal with it.

Did you try other OP's unitest ? will the same error happen?

@Patrick-Star125
Copy link
Contributor Author

  1. most of other Op's unittests passed successfully
    image
  2. And I retried to recompile and test my code, it showed some different information this time.
    image
  3. It seems like there is some type conversion problem in my code indeed, but the information provided is quite fuzzy. I combined many OPs to implement group_norm function, is there any way I can locate this problematic conversion?

@openvino-dev-samples
Copy link
Contributor

openvino-dev-samples commented Jul 30, 2022

  1. most of other Op's unittests passed successfully
    image

    1. And I retried to recompile and test my code, it showed some different information this time.
      image

    2. It seems like there is some type conversion problem in my code indeed, but the information provided is quite fuzzy. I combined many OPs to implement group_norm function, is there any way I can locate this problematic conversion?

  1. most of other Op's unittests passed successfully.
    image

    1. And I retried to recompile and test my code, it showed some different information this time.
      image

    2. It seems like there is some type conversion problem in my code indeed, but the information provided is quite fuzzy. I combined many OPs to implement group_norm function, is there any way I can locate this problematic conversion?

Hi
I think the error is from variable cast from int to long. you may switch the type of some parameter

@Patrick-Star125
Copy link
Contributor Author

the CI of Code Style didn't passed. Is that means I should reformat my code with Google C++ standard?

@openvino-dev-samples
Copy link
Contributor

the CI of Code Style didn't passed. Is that means I should reformat my code with Google C++ standard?

you can format your code (for VScode click "format selection") and retrigger the CI.

@openvino-dev-samples
Copy link
Contributor

Since your mapping is complicated, could you help to explain why you select these OPs to build 'group_norm' ?
Can it be simplified? Do you have more reference sample, like ONNX2IR ? It will helpful for us to speed-up the review process.
Thank you

@Patrick-Star125
Copy link
Contributor Author

Patrick-Star125 commented Aug 1, 2022

Hi, I want to illustrate my implementation point by point.

  1. Why my mapping is so comlicated.
    Unlike most of the other paddle mapping OPs, I didn't not find the function can be called directly, for example <default_opset::GroupNorm>. Therefore, I thought I need to implement this operator on my own, which lead to my mapping comlicated because it's computation process is not quite simple indeed.
  2. Why select these OPs to build 'group_norm'.(Reference updated)
    The main structure of my code is refer to the group norm implemented by openvino. In addition, I chang some of the OPs' structure and add some OPs inspired by onnx's implementation.
  3. Whether it can be simplified.
    Although this mapping is kind of comlicated, I think it's sematicaly clear(still I can add more annotation), and replace these OPs by other comprehensive operators may bring unnecessary coupling.

@openvino-dev-samples
Copy link
Contributor

Hi, I want to illustrate my implementation point by point.

  1. Why my mapping is so comlicated.
    Unlike most of the other paddle mapping OPs, I didn't not find the function can be called directly, for example <default_opset::GroupNorm>. Therefore, I thought I need to implement this operator on my own, which lead to my mapping comlicated because it's computation process is not quite simple indeed.
  2. Why select these OPs to build 'group_norm'.(Reference updated)
    The main structure of my code is refer to the group norm implemented by openvino. In addition, I chang some of the OPs' structure and add some OPs inspired by onnx's implementation.
  3. Whether it can be simplified.
    Although this mapping is kind of comlicated, I think it's sematicaly clear(still I can add more annotation), and replace these OPs by other comprehensive operators may bring unnecessary coupling.

Thanks for your illustration. Could you help to put this link on your Reference which is the document for the OP you try to enable: https://www.paddlepaddle.org.cn/documentation/docs/zh/api/paddle/static/nn/group_norm_cn.html#group-norm

Thanks

@Patrick-Star125
Copy link
Contributor Author

Reference updated

@Patrick-Star125
Copy link
Contributor Author

@OpenVINO-dev-contest Could you please review?

@openvino-dev-samples
Copy link
Contributor

@OpenVINO-dev-contest Could you please review?

Cloud you help to explain why you only delivery a single output mapping for this operation? and it seems has more one output ?
Thank you.
https://github.com/PaddlePaddle/Paddle/blob/release/2.3/python/paddle/fluid/layers/nn.py#L3790

@Patrick-Star125
Copy link
Contributor Author

Patrick-Star125 commented Aug 6, 2022

@OpenVINO-dev-contest Hi, I think the placeholder of outputs ["Mean", "Variance"] are just code aligment with other normalization operators.

  1. Unlike the other operators in same file, this python API only return "group_norm_out" instead of a collection of outputs.
    image
  2. the document of this op shows there is only single return indeed.
    image

@openvino-dev-samples
Copy link
Contributor

@OpenVINO-dev-contest Hi, I think the placeholder of outputs ["Mean", "Variance"] are just code aligment with other normalization operators.

  1. Unlike the other operators in same file, this python API only return "group_norm_out" instead of a collection of outputs.
    image
  2. the document of this op shows there is only single return indeed.
    image

Thanks for your illustration.

@Patrick-Star125
Copy link
Contributor Author

@OpenVINO-dev-contest could please you take a review?

@openvino-dev-samples
Copy link
Contributor

@OpenVINO-dev-contest could please you take a review?

Hi, seems scale_attr and bias_attr can be in bool, will your test case include it ?
https://www.paddlepaddle.org.cn/documentation/docs/zh/api/paddle/static/nn/group_norm_cn.html#group-norm

@Patrick-Star125
Copy link
Contributor Author

update: 1.python code of unit test 2.screenshot for unitest results

@yuxu42
Copy link
Contributor

yuxu42 commented Aug 15, 2022

@luo-cheng2021 could you please take a review? Thanks!

Copy link
Contributor

@luo-cheng2021 luo-cheng2021 left a comment

Choose a reason for hiding this comment

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

The mapping is quite complicated, could you please add some comments in the code to explain the ideas, thanks.

src/frontends/paddle/src/op/group_norm.cpp Outdated Show resolved Hide resolved
src/frontends/paddle/src/op/group_norm.cpp Outdated Show resolved Hide resolved
src/frontends/paddle/src/op/group_norm.cpp Outdated Show resolved Hide resolved
@Patrick-Star125 Patrick-Star125 requested a review from a team as a code owner August 25, 2022 03:42
@ceciliapeng2011 ceciliapeng2011 added PaddlePaddle Hackathon a Intel and Baidu joint Hackathon event category: PDPD FE OpenVINO PaddlePaddle FrontEnd labels Sep 5, 2022
size_t rank_size = pshape.rank().get_length();
PADDLE_OP_CHECK(node, rank_size >= 2, "2-D and above tensors supported only");

if (data_layout == "NHWC") {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if don't we have defined layout in the model?

Copy link
Contributor Author

@Patrick-Star125 Patrick-Star125 Sep 6, 2022

Choose a reason for hiding this comment

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

In line 29, the default value of data_layout is "NCHW", which is same as Paddle doc description.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ilyachur Some Paddle operations have this data_layout attribute, while some are not. For NHWC layout, it is okay to use this layout conversion for the op mapper, I think. We could apply a transformation from model perspective to eliminate the conversions between operations later.

@ilyachur ilyachur merged commit 089955f into openvinotoolkit:master Sep 13, 2022
@ilyachur ilyachur added this to the 2022.3 milestone Sep 13, 2022
@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.

7 participants