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

[Eager] fix set_value logic when input's dtype is different #48519

Merged
merged 3 commits into from
Dec 4, 2022

Conversation

veyron95
Copy link
Contributor

PR types

Bug fixes

PR changes

Others

Describe

fix set_value logic when input's dtype is different

@paddle-bot
Copy link

paddle-bot bot commented Nov 29, 2022

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

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.

some comments

@@ -1243,6 +1242,10 @@ static PyObject* tensor_method__setitem_eager_tensor(TensorObject* self,
value_tensor = egr::EagerAmpAutoCast(
value_tensor.name(), value_tensor, amp_dtype, "set_value");
}
if (value_tensor.initialized() &&
Copy link
Contributor

Choose a reason for hiding this comment

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

why not merge this into branch above.

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 this into branch above now.

@@ -1243,6 +1242,10 @@ static PyObject* tensor_method__setitem_eager_tensor(TensorObject* self,
value_tensor = egr::EagerAmpAutoCast(
value_tensor.name(), value_tensor, amp_dtype, "set_value");
}
if (value_tensor.initialized() &&
self->tensor.dtype() != value_tensor.dtype()) {
value_tensor = cast_ad_func(value_tensor, self->tensor.dtype());
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 not a safe operants, check dtype first maybe

Copy link
Contributor

Choose a reason for hiding this comment

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

Checkout the EagerAmpAutoCast && GetAmpDestDtype system

Copy link
Contributor Author

@veyron95 veyron95 Dec 2, 2022

Choose a reason for hiding this comment

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

后续的set_value__dygraph_function的两个输入必须相同dtype, 而先前的 EagerAmpAutoCast 并不能保证两个输入相同 dtype, 比如tensor A float16, tensor B float64, 经过 EagerAmpAutoCast 处理,tensor A 为 float32, 而 tensor B 依然 float64,调用 set_value__dygraph_function 会报错的。所以需要调用 cast_ad_func 进行对齐。

self->tensor = set_value__dygraph_function(
          self->tensor, value_tensor, {}, {}, {}, attrs);

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

Copy link
Contributor

@zhangbo9674 zhangbo9674 left a comment

Choose a reason for hiding this comment

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

LGTM

@veyron95 veyron95 merged commit 46371c5 into PaddlePaddle:develop Dec 4, 2022
lxsbupt pushed a commit to lxsbupt/Paddle that referenced this pull request Dec 17, 2022
…ddle#48519)

* [Eager] fix set_value logic when input's dtype is different

* value_tensor

* fix set_value logic when input's dtype is different
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.

3 participants