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

【Hackathon 5th No.112】move read_file to phi - part #59359

Merged
merged 8 commits into from
Dec 6, 2023

Conversation

zeroRains
Copy link
Contributor

@zeroRains zeroRains commented Nov 24, 2023

PR types

Others

PR changes

Others

Description

move read_file to phi
#57262

Copy link

paddle-bot bot commented Nov 24, 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 the contributor External developers label Nov 24, 2023
@zeroRains zeroRains changed the title 【Hackathon 5th No.103】read_file、fused_gemm_epilogue to phi -part 【Hackathon 5th No.103】move read_file、fused_gemm_epilogue to phi Nov 24, 2023
Comment on lines 2059 to 2070
- op : read_file
args : (str filename = "", DataType dtype = DataType::UINT8, Place place = CPUPlace())
output : Tensor(out)
infer_meta :
func : ReadFileInferMeta
param : [filename, dtype]
kernel :
func : read_file
param : [filename, dtype]
data_type : dtype
backend : place

Copy link
Contributor Author

@zeroRains zeroRains Nov 24, 2023

Choose a reason for hiding this comment

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

这个算子参数很少,只有一个filename的attr和一个输出out,cmake要求必须含有dtype和place两个参数才能编译。感觉我这样写应该没有问题,但是在进行单测时,竟然不进到read_file_kernel里。。。

这个Segmentation Fault应该是没找到这个kernel才导致越界的,参数这么简单的kernel不应该找不到啊。

可以麻烦老师看看是哪里写得不合理么~

a9929edd184eb53b1ffa9b1e0f76af2

@zeroRains zeroRains changed the title 【Hackathon 5th No.103】move read_file、fused_gemm_epilogue to phi 【Hackathon 5th No.103】move read_file to phi - part Nov 25, 2023
@zeroRains zeroRains changed the title 【Hackathon 5th No.103】move read_file to phi - part 【Hackathon 5th No.112】move read_file to phi - part Nov 25, 2023
@@ -82,6 +82,7 @@
'layer_norm_act_xpu',
'multi_encoder_xpu',
'multihead_matmul',
'read_file',
Copy link
Contributor

Choose a reason for hiding this comment

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

read_file不加到这个list里是会编译报错吗?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好像不会,这就删掉

Copy link
Contributor

@YuanRisheng YuanRisheng left a comment

Choose a reason for hiding this comment

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

这里可以参考#59363 的回复,不删除read_file_op.cc文件里的算子定义,这里之所以不建议删除的原因是,删除算子定义后,在ops.yaml里配置并添加额外参数会对op定义造成不兼容修改(ops.yaml会自动生成op定义,相当于把之前删除的算子定义自动生成出来了)。
这里依然建议是保留Op定义,配置legacy_ops.yaml文件(不会生成op定义,为动态图生成api),同时配置pir目录下的ops.yaml文件(paddle/fluid/pir/dialect/operator/ir/ops.yaml,为新IR生成api)

Comment on lines 41 to 43
PD_REGISTER_KERNEL(read_file, CPU, ALL_LAYOUT, phi::ReadFileKernel, uint8_t) {
kernel->OutputAt(0).SetDataType(phi::DataType::UINT8);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

这个看起来只有cpu的kernel,建议按照phi规范将该文件挪到phi/kernels/cpu目录下

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

@YuanRisheng YuanRisheng merged commit 57e79db into PaddlePaddle:develop Dec 6, 2023
@zeroRains zeroRains deleted the 112 branch January 30, 2024 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants