Skip to content

【PIR OpTest Fix No.27】 fix test_softmax_mask_fuse_op #62767

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 19, 2024
Merged

【PIR OpTest Fix No.27】 fix test_softmax_mask_fuse_op #62767

merged 3 commits into from
Mar 19, 2024

Conversation

Eddie-Wang1120
Copy link
Contributor

@Eddie-Wang1120 Eddie-Wang1120 commented Mar 16, 2024 •
edited
Loading

PR types

Others

PR changes

Others

Description

改动过程：

  • 观察到paddle中前向和反向均已有对应的infermeta实现，但op_compact.yaml中无实现，所以在op_compact.yaml中补充代码，补充后编译完成运行单侧时报错：
    fcfeedad566714728e735d20bae3a9d
  • 打开全量日志后发现是反向时未找到Out输入，怀疑是原本fused_softmax_mask_op.cc代码中命名错误（与已经写好的backward.yaml中的输入输出不对应）
    原本为：op->SetInput("Softmax", this->Output("Out"));
    观察多个op中都为：op->SetInput("Out", this->Output("Out"));
  • 将其改为Out后编译完成运行单侧时报错tensor not initialized，观察源码发现签名未修改，所以修改fused_softmax_mask_sig.cc中签名
  • 签名修改完成后执行单侧使用旧IR和新IR均全部通过。

麻烦帮忙看一下这种修改方式是否符合规范~

PIR Op单测修复
修复单测 test_softmax_mask_fuse_op
修复后打开FLAGS_enable_pir_in_executor单测是否通过：是

@paddle-bot Paddle-bot
Copy link

paddle-bot bot commented Mar 16, 2024

你的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 Mar 16, 2024
@luotao1 luotao1 mentioned this pull request Mar 18, 2024
Closed
@luotao1 luotao1 added the HappyOpenSource 快乐开源活动issue与PR label Mar 18, 2024
kangguangli
kangguangli reviewed Mar 18, 2024
View reviewed changes
Copy link
Contributor

@kangguangli kangguangli left a comment

Choose a reason for hiding this comment

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

@Eddie-Wang1120 Good job! 但是不推荐修改OpMaker里的内容，虽然反向算子没有兼容性问题，但是我们并不确定有没有历史代码依赖这种行为。这种命名上的问题，可以通过op_compat的机制解决，只要将旧Op里的Softmax输入映射到新Op里的out输入就行了。之所以op_compat.yaml不能解决，是因为他原本是用来解决动静Op的不一致，只涉及前向算子。所以为了翻译的时候能处理反向，在paddle/fluid/ir_adaptor/translator/op_compat_gen.py中处理下即可，添加

op_arg_name_mappings['fused_softmax_mask_grad'].update(
        {"out": "Softmax"}
    )

理论上这样就可以解决，其他问题可以@我看下

@Eddie-Wang1120
Copy link
Contributor Author

@Eddie-Wang1120 Good job! 但是不推荐修改OpMaker里的内容，虽然反向算子没有兼容性问题，但是我们并不确定有没有历史代码依赖这种行为。这种命名上的问题，可以通过op_compat的机制解决，只要将旧Op里的Softmax输入映射到新Op里的out输入就行了。之所以op_compat.yaml不能解决，是因为他原本是用来解决动静Op的不一致，只涉及前向算子。所以为了翻译的时候能处理反向，在paddle/fluid/ir_adaptor/translator/op_compat_gen.py中处理下即可，添加

op_arg_name_mappings['fused_softmax_mask_grad'].update(
        {"out": "Softmax"}
    )

理论上这样就可以解决，其他问题可以@我看下

非常感谢！问题已经解决并且通过了单测，修改已更新。

@kangguangli kangguangli merged commit a29a754 into PaddlePaddle:develop Mar 19, 2024
@Eddie-Wang1120 Eddie-Wang1120 deleted the eddie-dev-softmax branch March 19, 2024 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zyfncg zyfncg zyfncg approved these changes

@kangguangli kangguangli kangguangli approved these changes

Assignees

@luotao1 luotao1

@kangguangli kangguangli

Labels
contributor External developers HappyOpenSource 快乐开源活动issue与PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Eddie-Wang1120 @zyfncg @kangguangli @luotao1