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

[NewIR]Remove compatible logic of ProgramTranslator #55453

Merged
merged 18 commits into from
Jul 27, 2023

Conversation

Asthestarsfalll
Copy link
Contributor

@Asthestarsfalll Asthestarsfalll commented Jul 15, 2023

PR types

Others

PR changes

Others

Description

Remove compatible logic of ProgramTranslator

@paddle-bot
Copy link

paddle-bot bot commented Jul 15, 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 Jul 15, 2023
@kangguangli kangguangli self-assigned this Jul 17, 2023
@luotao1 luotao1 self-assigned this Jul 17, 2023
@luotao1 luotao1 added the HappyOpenSource 快乐开源活动issue与PR label Jul 17, 2023
@kangguangli
Copy link
Contributor

kangguangli commented Jul 17, 2023

看了下报错,主要是fetch_v2fill_constant的问题,fill_constant需要等对映射的修改合入。
fetch_v2的主要问题可能是


应该修改为

 - op: fetch(fetch_v2)

原因是在op_compat_info.hGetLegacyArgName(const std::string& op_type, const std::string& arg_name)中,这里调用时op_type会传入旧IR中的名字fetch_v2,当前在op_compat.yaml中只规定了fetch如何进行参数映射,因此需要通过上面的修改明确下fetch_v2fetch的映射。

相关代码如果有兴趣了解,可以先看看paddle/fluid/ir_adaptor/translator/op_compat_gen.py中生成op_compat_info.cc的逻辑。再看看paddle/fluid/ir_adaptor/translator/op_translator.cc中对op_compat_info.h中各个接口的使用。

@Asthestarsfalll
Copy link
Contributor Author

看了下报错,主要是fetch_v2fill_constant的问题,fill_constant需要等对映射的修改合入。 fetch_v2的主要问题可能是

应该修改为

 - op: fetch(fetch_v2)

原因是在op_compat_info.hGetLegacyArgName(const std::string& op_type, const std::string& arg_name)中,这里调用时op_type会传入旧IR中的名字fetch_v2,当前在op_compat.yaml中只规定了fetch如何进行参数映射,因此需要通过上面的修改明确下fetch_v2fetch的映射。

相关代码如果有兴趣了解,可以先看看paddle/fluid/ir_adaptor/translator/op_compat_gen.py中生成op_compat_info.cc的逻辑。再看看paddle/fluid/ir_adaptor/translator/op_translator.cc中对op_compat_info.h中各个接口的使用。

好的,已经添加了,注意到op_compat_gen.py中有对fetch_v2的特殊处理,根据我的理解应该是可以去掉的吧?
另外请问应该在哪个ci流水线中看报错呢?

@kangguangli
Copy link
Contributor

看了下报错,主要是fetch_v2fill_constant的问题,fill_constant需要等对映射的修改合入。 fetch_v2的主要问题可能是

应该修改为

 - op: fetch(fetch_v2)

原因是在op_compat_info.hGetLegacyArgName(const std::string& op_type, const std::string& arg_name)中,这里调用时op_type会传入旧IR中的名字fetch_v2,当前在op_compat.yaml中只规定了fetch如何进行参数映射,因此需要通过上面的修改明确下fetch_v2fetch的映射。
相关代码如果有兴趣了解,可以先看看paddle/fluid/ir_adaptor/translator/op_compat_gen.py中生成op_compat_info.cc的逻辑。再看看paddle/fluid/ir_adaptor/translator/op_translator.cc中对op_compat_info.h中各个接口的使用。

好的,已经添加了,注意到op_compat_gen.py中有对fetch_v2的特殊处理,根据我的理解应该是可以去掉的吧? 另外请问应该在哪个ci流水线中看报错呢?

是的,可以去掉。
现在我们修改的部分,会影响的新IR相关测试主要在两条流水线 PR-CI-Mac-Python3 和 PR-CI-Py3,基本都与Op单测相关,先关注这两条流水线的失败情况即可。这两条过了其他的也就不太会有问题了。

@kangguangli
Copy link
Contributor

@Asthestarsfalll fill_constant的修改已经合入了,可以rerun下所有CI,看下还有哪些单测失败?

@Asthestarsfalll
Copy link
Contributor Author

@Asthestarsfalll fill_constant的修改已经合入了,可以rerun下所有CI,看下还有哪些单测失败?

好的

@kangguangli
Copy link
Contributor

@Asthestarsfalll 看了下py3流水线的报错,有以下几类问题:

缺少参数映射

logspace/rrelu/channel_shuffle/fill_constant_batch_size_like 这几个应该是缺少在op_compat.yaml中的映射

缺少反向映射

bilinear_tensor_product的问题要复杂些,它在op_compat.yaml里是有映射的,出问题的是它的反向 bilinear_grad的参数映射。这里有两种修改方式:

  1. 短期方案,在op_compat.yaml里添加对反向op的映射,例如:
- op : yolo_loss (yolov3_loss)
  backward: yolo_loss_grad (yolov3_loss_grad)

这样的话前反向都会配置下面的参数映射。
2. 长期方案,修改op_compat_info.h中的GetLegacyArgName中对反向Op的处理,这样不添加上面的映射也可以正确完成参数的映射。

其他问题

  1. program_translator_test是因为它应当把merge_momentum翻译为pd.mermerge_momentum_而非pd.mermerge_momentum,前者在新IR下有定义而后者没有。你可以试着排查下为什么这里翻译错了。这里对于inplace op就是应该在名字后面加上下划线才对。

复现方式

program_translator_test以外的单测,设置环境变量FLAGS_NEW_IR_OPTEST=1即可。program_translator_test不需要设置环境变量。

@Asthestarsfalll
Copy link
Contributor Author

  1. 长期方案,修改op_compat_info.h中的GetLegacyArgName中对反向Op的处理,这样不添加上面的映射也可以正确完成参数的映射。

看了一下,似乎在op_compat_gen.py里添加对反向的处理也行?如果op本身有别名,且没有声明反向的别名,就将前向的别名拿过来在末尾加上_grad

@kangguangli
Copy link
Contributor

kangguangli commented Jul 21, 2023

@Asthestarsfalll 看了下Mac和py3流水线的情况:

  1. test_bilinear_interp_v2_op 应该是随机挂,应该与这个PR的改动无关,我会分析下,你遇到这个问题rerun CI就行了
  2. program_translator_test跟之前分析的不同,原因是在 op_compat.yaml里,我们添加了如下映射:
- op : merged_momentum_
  inputs :
    {param : Param, grad : Grad, velocity : Velocity, learning_rate : LearningRate, master_param : MasterParam}
  outputs :
    {param_out : ParamOut, velocity_out : VelocityOut, master_param_out : MasterParamOut}

但是在原本的静态图算子定义下,inplace语义不会通过名字中的下划线表示,因此它叫merged_momentum,进而查找不到参数映射。一个可能的解决方案是:

- op : merged_momentum_(merged_momentum)

但是这对其他算子生成部分会不会有影响,我需要跟 @zyfncg @heavyrain-lzy 讨论下。

@kangguangli
Copy link
Contributor

kangguangli commented Jul 24, 2023

@Asthestarsfalll 看了下Mac和py3流水线的情况:

  1. test_bilinear_interp_v2_op 应该是随机挂,应该与这个PR的改动无关,我会分析下,你遇到这个问题rerun CI就行了
  2. program_translator_test跟之前分析的不同,原因是在 op_compat.yaml里,我们添加了如下映射:
- op : merged_momentum_
  inputs :
    {param : Param, grad : Grad, velocity : Velocity, learning_rate : LearningRate, master_param : MasterParam}
  outputs :
    {param_out : ParamOut, velocity_out : VelocityOut, master_param_out : MasterParamOut}

但是在原本的静态图算子定义下,inplace语义不会通过名字中的下划线表示,因此它叫merged_momentum,进而查找不到参数映射。一个可能的解决方案是:

- op : merged_momentum_(merged_momentum)

但是这对其他算子生成部分会不会有影响,我需要跟 @zyfncg @heavyrain-lzy 讨论下。

@Asthestarsfalll 应该不会有影响,不过这里需要做一些修改,可以拉 #55636 试试,应该可以修复

@Asthestarsfalll
Copy link
Contributor Author

@kangguangli py3和mac-python3 都通过了

@Asthestarsfalll Asthestarsfalll changed the title [WIP][NewIR]Remove compatible logic of ProgramTranslator [NewIR]Remove compatible logic of ProgramTranslator Jul 24, 2023
kangguangli
kangguangli previously approved these changes Jul 24, 2023
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.

LGTM

@Asthestarsfalll
Copy link
Contributor Author

@kangguangli ci-coverage也通过了

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.

LGTM

@kangguangli kangguangli requested a review from zyfncg July 27, 2023 03:45
@luotao1 luotao1 merged commit cbbd940 into PaddlePaddle:develop Jul 27, 2023
wz1qqx pushed a commit to wz1qqx/Paddle that referenced this pull request Jul 31, 2023
@Asthestarsfalll Asthestarsfalll deleted the remove_compat branch August 19, 2023 06:38
jinjidejinmuyan pushed a commit to jinjidejinmuyan/Paddle that referenced this pull request Aug 30, 2023
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.

4 participants