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

Support code generation for op fill_any #54378

Merged
merged 3 commits into from
Jun 14, 2023

Conversation

huangjiyi
Copy link
Member

@huangjiyi huangjiyi commented Jun 6, 2023

PR types

Others

PR changes

Others

Description

@paddle-bot
Copy link

paddle-bot bot commented Jun 6, 2023

你的PR提交成功,感谢你对开源项目的贡献!
请关注后续CI自动化测试结果,详情请参考Paddle-CI手册
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@paddle-bot paddle-bot bot added contributor External developers status: proposed labels Jun 6, 2023
Comment on lines -23 to +24
def fill_any_wrapper(x, value_float=0, value_int=0):
return paddle._legacy_C_ops.fill_any(
x, "value_float", value_float, "value_int", value_int
)
def fill_any_wrapper(x, value=0):
return paddle._legacy_C_ops.fill_any(x, "value", value)
Copy link
Member Author

@huangjiyi huangjiyi Jun 6, 2023

Choose a reason for hiding this comment

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

改这里是因为原来的 attr value 在 OpMaker 里有定义两个:value_floatvalue_int,但实际用到的只有 value_float(因为 fill_sig.cc 的映射只有 value_float,即传给 FillKernelvalue 只有 value_float),所以既然 value_int 没用到就删了,然后为了与 kernel 对齐,value_float 改成了 value

Copy link
Contributor

Choose a reason for hiding this comment

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

目前看fill_any仅仅用于旧动态图算子,但是不确定旧版本保存的模型是否含有该算子,所以这里删除op的attribute存在潜在的兼容性问题,这个PR需要其他人同意后才可以merge。

Copy link
Member Author

Choose a reason for hiding this comment

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

Get.

@huangjiyi huangjiyi changed the title Support code generation for op fill Support code generation for op fill_any Jun 6, 2023
@huangjiyi
Copy link
Member Author

huangjiyi commented Jun 6, 2023

本次 PR 自动生成的 op 是 fill_any 而不是 fill,因为 fill_any 的算子名会被映射成 fill,所以在 yaml 里配置的都是 fill,另外 fluid/operators 下还有一个 fill_op.cc,感觉这里定义的才是真正的 fill

@huangjiyi huangjiyi changed the title Support code generation for op fill_any Support code generation for op ~~fill~~ fill_any Jun 6, 2023
@huangjiyi huangjiyi changed the title Support code generation for op ~~fill~~ fill_any Support code generation for op fill_any Jun 6, 2023
@huangjiyi
Copy link
Member Author

@heavyrain-lzy,帮忙 Review 一下 ~

heavyrain-lzy
heavyrain-lzy previously approved these changes Jun 6, 2023
Copy link
Contributor

@heavyrain-lzy heavyrain-lzy left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines -23 to +24
def fill_any_wrapper(x, value_float=0, value_int=0):
return paddle._legacy_C_ops.fill_any(
x, "value_float", value_float, "value_int", value_int
)
def fill_any_wrapper(x, value=0):
return paddle._legacy_C_ops.fill_any(x, "value", value)
Copy link
Contributor

Choose a reason for hiding this comment

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

目前看fill_any仅仅用于旧动态图算子,但是不确定旧版本保存的模型是否含有该算子,所以这里删除op的attribute存在潜在的兼容性问题,这个PR需要其他人同意后才可以merge。

x, "value_float", value_float, "value_int", value_int
)
def fill_any_wrapper(x, value=0):
return paddle._legacy_C_ops.fill_any(x, "value", value)
Copy link
Contributor

Choose a reason for hiding this comment

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

这里的fill_any_wrapper是不可以直接去掉了?

Copy link
Member Author

@huangjiyi huangjiyi Jun 7, 2023

Choose a reason for hiding this comment

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

我删了之后跑单测会报下面的错误:

871: Traceback (most recent call last):
871:   File "/parallel/Paddle/build/test/legacy_test/test_fill_any_op.py", line 40, in test_check_output
871:     self.check_output()
871:   File "/parallel/Paddle/build/test/legacy_test/eager_op_test.py", line 2176, in check_output
871:     check_cinn=check_cinn,
871:   File "/parallel/Paddle/build/test/legacy_test/eager_op_test.py", line 2042, in check_output_with_place
871:     dygraph_checker.check()
871:   File "/parallel/Paddle/build/test/legacy_test/eager_op_test.py", line 1806, in check
871:     self.calculate_output()
871:   File "/parallel/Paddle/build/test/legacy_test/eager_op_test.py", line 1878, in calculate_output
871:     dygraph_outs = self.op_test._calc_python_api_output(place)
871:   File "/parallel/Paddle/build/test/legacy_test/eager_op_test.py", line 1068, in _calc_python_api_output
871:     % self.op_type
871: AssertionError: Detect there is KernelSignature for `fill_any` op, please set the `self.python_api` if you set check_dygraph = True

scalar :
value :
data_type : float
support_tensor : true
Copy link
Contributor

Choose a reason for hiding this comment

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

fill_any的value之前没有配置support_tensor,感觉这里也可以不加support_tensor

Copy link
Member Author

@huangjiyi huangjiyi Jun 7, 2023

Choose a reason for hiding this comment

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

if attr_item['is_support_tensor'] is False:
attr_item['tensor_name'] = scalar_config['tensor_name']

这里如果不加 support_tensorgenerate_op.py 会报错说需要配置 tensor_name,如果把 tensor_name 也配置成 value 的话,生成的 OpMaker 会多一个 Input,然后编译的时候会报 input 数量有问题

class FillAnyOpMaker : public framework::OpProtoAndCheckerMaker {
 public:
  void Make() override {
    AddInput("X", "(Tensor), input 0 of fill_any op.");
    AddOutput("Out", "(Tensor), output 0 of fill_any op.");
    AddInput("value", "attribute 0 for fill_any op from 0D Tensor.")
        .AsDispensable();
    AddAttr<float>("value", "(float), attribute 0 for fill_any op.")
        .SetDefault(0);

Copy link
Contributor

Choose a reason for hiding this comment

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

if attr_item['is_support_tensor'] is False:
attr_item['tensor_name'] = scalar_config['tensor_name']

这里如果不加 support_tensorgenerate_op.py 会报错说需要配置 tensor_name,如果把 tensor_name 也配置成 value 的话,生成的 OpMaker 会多一个 Input,然后编译的时候会报 input 数量有问题

class FillAnyOpMaker : public framework::OpProtoAndCheckerMaker {
 public:
  void Make() override {
    AddInput("X", "(Tensor), input 0 of fill_any op.");
    AddOutput("Out", "(Tensor), output 0 of fill_any op.");
    AddInput("value", "attribute 0 for fill_any op from 0D Tensor.")
        .AsDispensable();
    AddAttr<float>("value", "(float), attribute 0 for fill_any op.")
        .SetDefault(0);

尝试修改一下这里的逻辑:

if attr_item['is_support_tensor'] is False and 'tensor_name' in scalar_config: 
     attr_item['tensor_name'] = scalar_config['tensor_name'] 

Copy link
Member Author

Choose a reason for hiding this comment

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

尝试修改一下这里的逻辑:

if attr_item['is_support_tensor'] is False and 'tensor_name' in scalar_config: 
     attr_item['tensor_name'] = scalar_config['tensor_name'] 

这样虽然不会报错,但是生成的 OpMaker 中依然会多一个 Input:

class FillAnyOpMaker : public framework::OpProtoAndCheckerMaker {
 public:
  void Make() override {
    AddInput("X", "(Tensor), input 0 of fill_any op.");
    AddOutput("Out", "(Tensor), output 0 of fill_any op.");
    AddInput("ValueTensor", "attribute 0 for fill_any op from 0D Tensor.")
        .AsDispensable();
    AddAttr<float>("value", "(float), attribute 0 for fill_any op.")
        .SetDefault(0);

好像是因为下面的代码:

{% if typename is scalar and
("is_support_tensor" not in attr or attr["is_support_tensor"] is false) %}
AddInput("{{attr | to_scalar_tensor_name}}", "attribute {{i}} for {{op_name}} op from 0D Tensor.")
.AsDispensable();
AddAttr<{{attr["data_type"]}}>("{{name}}", "({{attr["data_type"]}}), attribute {{i}} for {{op_name}} op.")

这里只要 is_support_tensor 设置 false(包括不设置)就会生成一个 Input

Copy link
Contributor

Choose a reason for hiding this comment

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

确实存在问题,那暂时保持support_tensor : true

@huangjiyi
Copy link
Member Author

@zyfncg,Review 意见都回复了,帮忙再 Review 一下 ~

@luotao1 luotao1 added the HappyOpenSource 快乐开源活动issue与PR label Jun 7, 2023
@huangjiyi huangjiyi requested a review from zyfncg June 9, 2023 05:48
zyfncg
zyfncg previously approved these changes Jun 13, 2023
@heavyrain-lzy
Copy link
Contributor

麻烦解决一下conflict

@huangjiyi huangjiyi dismissed stale reviews from zyfncg and heavyrain-lzy via 918998f June 13, 2023 12:00
@huangjiyi
Copy link
Member Author

@heavyrain-lzy,帮忙再 Review 一下 ~

Copy link
Contributor

@heavyrain-lzy heavyrain-lzy left a comment

Choose a reason for hiding this comment

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

LGTM

@huangjiyi
Copy link
Member Author

@luotao1,帮忙豁免一下 CI ~

@luotao1 luotao1 merged commit 4277f61 into PaddlePaddle:develop Jun 14, 2023
24 of 25 checks passed
@huangjiyi huangjiyi deleted the autogen_fill branch January 9, 2024 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor External developers HappyOpenSource 快乐开源活动issue与PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants