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

[IPU] support dy2static for IPU merge code #43770

Merged

Conversation

gglin001
Copy link
Contributor

@gglin001 gglin001 commented Jun 22, 2022

PR types

Others

PR changes

Others

Describe

clone of #42952

fix pre-commit CI

文档 PR PaddlePaddle/docs#4932

文档预览链接:http://10.136.157.23:8090/documentation/docs/zh/faq/index_cn.html?reviewVersion=jenkins-doc-review-2374

image

@paddle-bot-old paddle-bot-old bot added contributor External developers status: proposed labels Jun 22, 2022
@paddle-bot-old
Copy link

你的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.

@qili93 qili93 marked this pull request as ready for review June 23, 2022 06:31
qili93
qili93 previously approved these changes Jun 23, 2022
Copy link
Contributor

@qili93 qili93 left a comment

Choose a reason for hiding this comment

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

LGTM, code review in #42952

@gglin001 gglin001 changed the title [NO MERGE] [IPU] support dy2static for IPU [IPU] support dy2static for IPU merge code Jun 23, 2022
def identity_loss(x, reduction="none"):
r"""Marks a tensor as being part of the loss calculation for IPU.

This function should be called on the (final) loss of a model so that
Copy link
Contributor

Choose a reason for hiding this comment

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

This function

Copy link
Contributor

Choose a reason for hiding this comment

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

请删除 This function,文档里不这么描述。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

抱歉, 之前以为这里是取消这里的 comment, will fix later


Parameters:
x (Variable): The calculated loss ``Tensor``.
reduction(str|int): Reduce the loss output. The default value is "none". Supported values are:
Copy link
Contributor

Choose a reason for hiding this comment

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

reduction(str|int, optional)

请添加 reduction 为 int 时的说明。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed,

            * ``"sum"``: Sum the losses.

to

            * ``"sum"`` or ``1``: Sum the losses.


This function should be called on the (final) loss of a model so that
it is used as the start of backpropagation. This is equivalent to calling
``x.backward()`` on a tensor ``x`` when running on the CPU.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is equivalent to calling x.backward() on a tensor x when running on the CPU. 中文文档没有;
此外,关于 reduction ,英文版在参数中进行说明,而中文在描述中与参数中都说明了;建议按中文版,在功能描述中说明 reduction 为 string/int 时的行为,在参数描述中,保留 指定应用于输出结果的计算方式,可选的string值有: 'none', 'mean', 'sum' ,对应的int值分别为2,1,0 。默认为 'none'。 即可

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed;

去除了

This is equivalent to calling
    ``x.backward()`` on a tensor ``x`` when running on the CPU.

reduction 部分已应用建议的修改, 麻烦再看下还有没有其他的问题

@gglin001
Copy link
Contributor Author

gglin001 commented Jun 27, 2022

CI PR-CI-CINN 的 fix 见

Out = SUM(Out)

Parameters:
x (Variable): The calculated loss ``Tensor``.
Copy link
Contributor

Choose a reason for hiding this comment

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

这里需要与中文语义保持一致

Copy link
Contributor

Choose a reason for hiding this comment

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

更正为:

        x (Variable): The input tensor. The shapes is [N, *], where N is batch size and `*` means any number of
             additional dimensions. It's data type should be float32, float64 on CPU and float16, float32 on IPU.

def identity_loss(x, reduction="none"):
r"""Marks a tensor as being part of the loss calculation for IPU.

This function should be called on the (final) loss of a model so that
Copy link
Contributor

Choose a reason for hiding this comment

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

请删除 This function,文档里不这么描述。

@gglin001
Copy link
Contributor Author

@TCChenlong hi, 麻烦有空再看下还有其他要修改的地方吗?

关联的 doc
PaddlePaddle/docs#4932

Parameters:
x (Variable): The input tensor. The shapes is [N, *], where N is batch size and `*` means any number of
additional dimensions. It's data type should be float32, float64 on CPU and float16, float32 on IPU.
reduction(str|int): Reduce the loss output. Supported string values are: 'sum', 'mean', 'none'
Copy link
Contributor

Choose a reason for hiding this comment

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

reduction(str|int) -> reduction(str|int, optional)

Copy link
Contributor

Choose a reason for hiding this comment

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

已更改为 reduction(str|int, optional):

TCChenlong
TCChenlong previously approved these changes Jun 29, 2022
Copy link
Contributor

@TCChenlong TCChenlong 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

@qili93 qili93 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

@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.

LGMT

@qili93 qili93 merged commit 6984fbc into PaddlePaddle:develop Jul 7, 2022
@gglin001 gglin001 deleted the ipu_commit/dy2static_ipu_fix_format branch July 7, 2022 05:17
@paddle-bot-old paddle-bot-old bot removed the contributor External developers label Oct 17, 2022
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.

5 participants