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

[Dy2St]Add backend for to_static API #52596

Merged
merged 6 commits into from
Apr 11, 2023

Conversation

0x45f
Copy link
Contributor

@0x45f 0x45f commented Apr 6, 2023

PR types

New features

PR changes

APIs

Describe

Add backend for to_static API
中文文档修改PR:PaddlePaddle/docs#5793
PCard-66972

@paddle-bot
Copy link

paddle-bot bot commented Apr 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.

@0x45f 0x45f changed the title Add backend for to_static API [Dy2St]Add backend for to_static API Apr 7, 2023
Aurelius84
Aurelius84 previously approved these changes Apr 10, 2023
Copy link
Contributor

@Aurelius84 Aurelius84 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@JiabinYang JiabinYang left a comment

Choose a reason for hiding this comment

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

two more comments



@signature_safe_contextmanager
def dy2st_prim_guard(backend):
Copy link
Contributor

Choose a reason for hiding this comment

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

backend_guard is more self explained name

Copy link
Contributor Author

@0x45f 0x45f Apr 10, 2023

Choose a reason for hiding this comment

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

现在这个guard只是在切换prim的状态,现在的dy2st_prim_guard是不是更具体一些?

Copy link
Contributor

Choose a reason for hiding this comment

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

我是你这个部分本意是根据backend修改状态,其实不应该叫dy2st_prim_guard,而应该叫backend_grad

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已修改,感谢~

PrimHooker(concrete_program.main_program)
)
with dy2st_prim_guard(backend):
if core._is_fwd_prim_enabled() and not _in_amp_guard():
Copy link
Contributor

Choose a reason for hiding this comment

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

remove _in_amp_guard since, AMP O1 is supported in pr PR 52598

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这里已经merge develop,解决了冲突,感谢~

Copy link
Contributor

@JiabinYang JiabinYang left a comment

Choose a reason for hiding this comment

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

LGTM

@jeff41404
Copy link
Contributor

Please note that Chinese documents(for official website use) should also be modified.

@jzhang533 jzhang533 self-requested a review April 10, 2023 12:10
Copy link
Contributor

@XiaoguangHu01 XiaoguangHu01 left a comment

Choose a reason for hiding this comment

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

LGTM

@0x45f
Copy link
Contributor Author

0x45f commented Apr 10, 2023

Please note that Chinese documents(for official website use) should also be modified.

已提PR修改中文文档,文档的PR链接已经在PR描述中,感谢

out2 = self.forward(x, None)
np.testing.assert_allclose(out1, out2, rtol=1e-6)

def forward(self, x, beckend=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我下面提个PR再修改,感谢~

input_spec=None,
build_strategy=None,
backend=None,
**kwargs,
Copy link
Contributor

Choose a reason for hiding this comment

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

property 参数从positional/keyword arg 皆可, 改成了 keyword-only arg,是不兼容升级了。
检查过其影响吗?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

property参数目前只有语音套件会使用,已经和语音侧同步,语音侧使用方式都是通过k=v的方式传入该参数,所以对主框架以及套件不会产生其他影响。不兼容升级的邮件已经发送,评委已经通过

@0x45f 0x45f requested a review from jzhang533 April 11, 2023 02:50
Copy link
Contributor

@jzhang533 jzhang533 left a comment

Choose a reason for hiding this comment

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

LGTM

@0x45f 0x45f merged commit 3e66845 into PaddlePaddle:develop Apr 11, 2023
@0x45f 0x45f deleted the add-backend-simple branch April 11, 2023 03:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants