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

【SCU】【Paddle Tensor No.11】新增 Tensor.__dlpack__() #69781

Merged
merged 15 commits into from
Dec 5, 2024

Conversation

SCUcookie
Copy link
Contributor

PR Category

User Experience

PR Types

New features

Description

Paddle Tensor 规范化:新增 Tensor.__dlpack__

Copy link

paddle-bot bot commented Nov 28, 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 Nov 28, 2024
@HydrogenSulfate
Copy link
Contributor

@SCUcookie 解决一下冲突

@SCUcookie
Copy link
Contributor Author

@SCUcookie 解决一下冲突

已解决

Comment on lines 1380 to 1384
if "gpu" not in str(self.place):
raise AttributeError(
"Can't get __dlpack__ on non-CUDA tensor. "
"Use tensor.cuda() to move the tensor to device memory."
)
Copy link
Contributor

Choose a reason for hiding this comment

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

为什么只能支持"gpu"类型?这里应该不用这个判断吧?

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 on lines 1394 to 1396
"Can't get __dlpack__ on Tensor that requires gradients. "
"If gradients aren't required, use tensor.detach() to get a tensor without gradient."
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Can't get __dlpack__ on Tensor that requires gradients. "
"If gradients aren't required, use tensor.detach() to get a tensor without gradient."
)
"Can't get __dlpack__ from a Tensor that requires gradients, "
"use tensor.detach() if gradients are not required."
)

Comment on lines 1387 to 1390
raise AttributeError(
"Can't get __dlpack__ on sparse tensor. "
"Use Tensor.to_dense() to convert to a dense tensor first."
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
raise AttributeError(
"Can't get __dlpack__ on sparse tensor. "
"Use Tensor.to_dense() to convert to a dense tensor first."
)
raise AttributeError(
"Can't get __dlpack__ from sparse tensor. "
"Use Tensor.to_dense() to convert to a dense tensor first."
)

Comment on lines 1403 to 1404
if stream == 0:
stream = paddle.device.cuda.default_stream()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if stream == 0:
stream = paddle.device.cuda.default_stream()
if stream == 1 and not paddle.device.is_compiled_with_rocm():
stream = paddle.device.cuda.default_stream()
elif stream == 0 and paddle.device.is_compiled_with_rocm():
stream = paddle.device.cuda.default_stream()

event.record(current_stream)
stream.wait_event(event)

return paddle.utils.dlpack.to_dlpack(self)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return paddle.utils.dlpack.to_dlpack(self)
return paddle.to_dlpack(self)

Comment on lines 328 to 338
def test_dlpack_basic(self):
tensor_cpu = paddle.to_tensor([1.0, 2.0, 3.0], place=paddle.CPUPlace())
if paddle.device.is_compiled_with_cuda():
tensor_gpu = paddle.to_tensor(
[1.0, 2.0, 3.0], place=paddle.CUDAPlace(0)
)
dlpack_capsule_gpu = tensor_gpu.__dlpack__()
self.assertIsNotNone(dlpack_capsule_gpu)

converted_tensor_gpu = paddle.from_dlpack(dlpack_capsule_gpu)
self.assertTrue(paddle.equal_all(tensor_gpu, converted_tensor_gpu))
Copy link
Contributor

@HydrogenSulfate HydrogenSulfate Nov 28, 2024

Choose a reason for hiding this comment

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

不同设备的测试代码可以合并成同一个,他们只有place的区别,参考:

self.places = []
if (
os.environ.get('FLAGS_CI_both_cpu_and_gpu', 'False').lower()
in ['1', 'true', 'on']
or not base.core.is_compiled_with_cuda()
):
self.places.append(base.CPUPlace())
if base.core.is_compiled_with_cuda():
self.places.append(base.CUDAPlace(0))
self.places.append(base.CUDAPinnedPlace())

places = [...]

for place in places:
    x = paddle.to_tensor(xxx, place=place)
    // write test code below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的

HydrogenSulfate

This comment was marked as resolved.

@SCUcookie
Copy link
Contributor Author

感觉单测可以直接使用:Paddle/test/legacy_test/test_dlpack.py,把里面的paddle.to_dlpack换成Tensor.__dlpack__()即可

test_dlpack里所有的都要改吗

@HydrogenSulfate
Copy link
Contributor

HydrogenSulfate commented Nov 28, 2024

感觉单测可以直接使用:Paddle/test/legacy_test/test_dlpack.py,把里面的paddle.to_dlpack换成Tensor.__dlpack__()即可

test_dlpack里所有的都要改吗

哦可能我没说清楚,我的意思是不修改test_dlpack.py,而是可以直接复制test_dlpack里的所有单元测试到你这个PR的单测里,并且把全部to_dlpack的调用改成__dlpack__调用就行

@luotao1 luotao1 added the HappyOpenSource Pro 进阶版快乐开源活动,更具挑战性的任务 label Nov 28, 2024
@SCUcookie
Copy link
Contributor Author

感觉单测可以直接使用:Paddle/test/legacy_test/test_dlpack.py,把里面的paddle.to_dlpack换成Tensor.__dlpack__()即可

test_dlpack里所有的都要改吗

哦可能我没说清楚,我的意思是不修改test_dlpack.py,而是可以直接复制test_dlpack里的所有单元测试到你这个PR的单测里,并且把全部to_dlpack的调用改成__dlpack__调用就行

好的

@SCUcookie
Copy link
Contributor Author

感觉单测可以直接使用:Paddle/test/legacy_test/test_dlpack.py,把里面的paddle.to_dlpack换成Tensor.__dlpack__()即可

test_dlpack里所有的都要改吗

哦可能我没说清楚,我的意思是不修改test_dlpack.py,而是可以直接复制test_dlpack里的所有单元测试到你这个PR的单测里,并且把全部to_dlpack的调用改成__dlpack__调用就行

这个所有都提示找不到Tensor

@HydrogenSulfate
Copy link
Contributor

test_dlpack.py

能看一下具体报错信息吗?test_dlpack.py

@SCUcookie
Copy link
Contributor Author

test_dlpack.py

能看一下具体报错信息吗?test_dlpack.py

就是如果把test_dlpack的内容复制到新的test文件里然后改成Tensor.dlpack()每一个都会报错:
Traceback (most recent call last):
File "/root/Paddle/test/legacy_test/test___dlpack__.py", line 255, in test_to_dlpack_strides_consistency
dlpack_v2 = Tensor.dlpack()(x_strided)
NameError: name 'Tensor' is not defined

@HydrogenSulfate
Copy link
Contributor

test_dlpack.py

能看一下具体报错信息吗?test_dlpack.py

就是如果把test_dlpack的内容复制到新的test文件里然后改成Tensor.dlpack()每一个都会报错: Traceback (most recent call last): File "/root/Paddle/test/legacy_test/test___dlpack__.py", line 255, in test_to_dlpack_strides_consistency dlpack_v2 = Tensor.dlpack()(x_strided) NameError: name 'Tensor' is not defined

不好意思我没表述清楚,Tensor需要改成你实际的tensor名字,而不是把Tensor这六个字符也复制过去 😢

@SCUcookie
Copy link
Contributor Author

test_dlpack.py

能看一下具体报错信息吗?test_dlpack.py

就是如果把test_dlpack的内容复制到新的test文件里然后改成Tensor.dlpack()每一个都会报错: Traceback (most recent call last): File "/root/Paddle/test/legacy_test/test___dlpack__.py", line 255, in test_to_dlpack_strides_consistency dlpack_v2 = Tensor.dlpack()(x_strided) NameError: name 'Tensor' is not defined

不好意思我没表述清楚,Tensor需要改成你实际的tensor名字,而不是把Tensor这六个字符也复制过去 😢

哦哦哦我理解错了

@HydrogenSulfate
Copy link
Contributor

test_dlpack.py

能看一下具体报错信息吗?test_dlpack.py

就是如果把test_dlpack的内容复制到新的test文件里然后改成Tensor.dlpack()每一个都会报错: Traceback (most recent call last): File "/root/Paddle/test/legacy_test/test___dlpack__.py", line 255, in test_to_dlpack_strides_consistency dlpack_v2 = Tensor.dlpack()(x_strided) NameError: name 'Tensor' is not defined

不好意思我没表述清楚,Tensor需要改成你实际的tensor名字,而不是把Tensor这六个字符也复制过去 😢

举个例子,比如你复制过来的写法是: dl_pack = paddle.utils.dlpack.to_dlpack(x),那么需要将其改成dl_pack = x.__dlpack__(),这样才能调用Tensor.__dlpack__这个方法

@SCUcookie
Copy link
Contributor Author

test_dlpack.py

能看一下具体报错信息吗?test_dlpack.py

就是如果把test_dlpack的内容复制到新的test文件里然后改成Tensor.dlpack()每一个都会报错: Traceback (most recent call last): File "/root/Paddle/test/legacy_test/test___dlpack__.py", line 255, in test_to_dlpack_strides_consistency dlpack_v2 = Tensor.dlpack()(x_strided) NameError: name 'Tensor' is not defined

不好意思我没表述清楚,Tensor需要改成你实际的tensor名字,而不是把Tensor这六个字符也复制过去 😢

举个例子,比如你复制过来的写法是: dl_pack = paddle.utils.dlpack.to_dlpack(x),那么需要将其改成dl_pack = x.__dlpack__(),这样才能调用Tensor.__dlpack__这个方法

我以为是要求实现Tensor.__dlpack__那种,忘了这个了,谢谢老师

@HydrogenSulfate
Copy link
Contributor

提交代码一定要用pre-commit,否则还得重新过CI:
image

Copy link
Contributor

@HydrogenSulfate HydrogenSulfate left a comment

Choose a reason for hiding this comment

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

覆盖率没通过,看起来需要再加几个单测
image
这些报错也需要通过单测触发来覆盖,伪代码如下:

with self.assertRaises(错误类型如TypeError):
    此处触发对应类型的报错代码

image

这样能测试报错能否正常触发

@SCUcookie
Copy link
Contributor Author

覆盖率没通过,看起来需要再加几个单测 image 这些报错也需要通过单测触发来覆盖,伪代码如下:

with self.assertRaises(错误类型如TypeError):
    此处触发对应类型的报错代码

image

这样能测试报错能否正常触发

好的老师

Copy link
Contributor

@HydrogenSulfate HydrogenSulfate Nov 29, 2024

Choose a reason for hiding this comment

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

哦忘了dlpack不支持XPU了,还麻烦每个单测类的上方加上下面这个装饰器(装饰器里的reason要改一下),否则Kunlun-bxcheck会挂

@unittest.skipIf(
paddle.core.is_compiled_with_xpu(),
"xpu does not support complex cast temporarily",
)
class TestComplexCast(TestCastBase):
def prepare(self):
self.input_shape = (8, 16)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的

@luotao1 luotao1 changed the title 【SCU】【Paddle Tensor No.3】新增 Tensor.__dlpack__ 【SCU】【Paddle Tensor No.11】新增 Tensor.__dlpack__ Dec 2, 2024
@@ -129,6 +129,7 @@ def _to_static_var(self, to_parameter=False, **kwargs):
'strides',
'offset',
'__cuda_array_interface__',
'__dlpack__',
'__dlpack_device__',
Copy link
Member

Choose a reason for hiding this comment

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

__dlpack____dlpack_device__ 都删掉,property 才加到这里

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的

Copy link
Contributor Author

Choose a reason for hiding this comment

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

但是__dlpack_device__也要删吗

Copy link
Member

Choose a reason for hiding this comment

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

删,__dlpack_device__ 不是方法吗?

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 on lines 1396 to 1408
if stream is not None and stream != -1:
if self.place.is_gpu_place():
if stream == 1 and not paddle.device.is_compiled_with_rocm():
stream = paddle.device.cuda.default_stream()
elif stream == 0 and paddle.device.is_compiled_with_rocm():
stream = paddle.device.cuda.default_stream()
else:
stream = paddle.device.cuda.ExternalStream(stream)
current_stream = paddle.device.cuda.current_stream()
if stream != current_stream:
event = paddle.device.cuda.Event()
event.record(current_stream)
stream.wait_event(event)
Copy link
Contributor

@HydrogenSulfate HydrogenSulfate Dec 3, 2024

Choose a reason for hiding this comment

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

这里还麻烦补充一个stream的单测吧,新建stream可以参考:paddle.device.Stream: https://www.paddlepaddle.org.cn/documentation/docs/zh/develop/api/paddle/device/Stream_cn.html#stream,当paddle.is_compiled_with_gpu()为True时进行测试。

Comment on lines +237 to +259
def test_to_dlpack_from_zero_size(self):
with dygraph_guard():
places = [base.CPUPlace()]
if paddle.is_compiled_with_cuda():
places.append(base.CUDAPlace(0))
for place in places:
for _ in range(4):
x = paddle.zeros([0, 10]).to(device=place)
dlpack_v1 = paddle.utils.dlpack.to_dlpack(x)
dlpack_v2 = x.__dlpack__()
y1 = paddle.utils.dlpack.from_dlpack(dlpack_v1)
y2 = paddle.from_dlpack(dlpack_v2)
self.assertEqual(x.data_ptr(), y1.data_ptr())
self.assertEqual(x.data_ptr(), y2.data_ptr())
self.assertEqual(str(x.place), str(y1.place))
self.assertEqual(str(x.place), str(y2.place))
self.assertEqual(y1.shape, [0, 10])
self.assertEqual(y2.shape, [0, 10])
self.assertEqual(y1.numel().item(), 0)
self.assertEqual(y2.numel().item(), 0)
np.testing.assert_array_equal(x.numpy(), y1.numpy())
np.testing.assert_array_equal(x.numpy(), y2.numpy())

Copy link
Contributor

@HydrogenSulfate HydrogenSulfate Dec 3, 2024

Choose a reason for hiding this comment

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

此处添加一个单测,不显式调用__dlpack__()进行转换,在paddle和numpy两个框架上验证

...
x = paddle.randn([10, 10]).cpu()
pd_y = paddle.from_dlpack(x)
np_y = np.from_dlpack(x)

# pd_y和np_y一致性检查

...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个单测一直遇到tensor找不到__dlpack_device__,但是我查看代码也是最新的
image

Copy link
Contributor

@HydrogenSulfate HydrogenSulfate Dec 3, 2024

Choose a reason for hiding this comment

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

这个单测一直遇到tensor找不到__dlpack_device__,但是我查看代码也是最新的 image

先确认下本地代码是没问题的,我这里是能正常调用的,如果有改了源码记得改回去
image

@HydrogenSulfate HydrogenSulfate changed the title 【SCU】【Paddle Tensor No.11】新增 Tensor.__dlpack__ 【SCU】【Paddle Tensor No.11】新增 Tensor.__dlpack__() Dec 3, 2024
Comment on lines 132 to 133
'__dlpack__',
'__dlpack_device__',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'__dlpack__',
'__dlpack_device__',

Copy link
Contributor

Choose a reason for hiding this comment

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

这里得添加上__dlpack__

for method_name, method in (
("__bool__", __bool__),
("__nonzero__", __nonzero__),
("_to_static_var", _to_static_var),
("set_value", set_value),
("block", block),
("backward", backward),
("clear_grad", clear_grad),
("inplace_version", inplace_version),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的

@HydrogenSulfate
Copy link
Contributor

改完可以本地编包安装测试一下,不然提交的PR的测试也会挂掉

@SCUcookie
Copy link
Contributor Author

改完可以本地编包安装测试一下,不然提交的PR的测试也会挂掉

好的老师我之前测试过了可能不全

@HydrogenSulfate
Copy link
Contributor

改完可以本地编包安装测试一下,不然提交的PR的测试也会挂掉

好的老师我之前测试过了可能不全

如果只是python的修改,可以直接改源码(site_packages/paddle/下的对应文件的代码)进行测试,不用重新编包安装。

@SCUcookie
Copy link
Contributor Author

改完可以本地编包安装测试一下,不然提交的PR的测试也会挂掉

好的老师我之前测试过了可能不全

如果只是python的修改,可以直接改源码(site_packages/paddle/下的对应文件的代码)进行测试,不用重新编包安装。

好的老师

Comment on lines 260 to 276
def test_dlpack_with_custom_stream(self):
if not paddle.is_compiled_with_cuda():
self.skipTest("Test requires CUDA support.")
with dygraph_guard():
paddle.set_device('gpu:0')
s1 = paddle.device.Stream()
s2 = paddle.device.Stream()
e = paddle.device.Event()
s1.record_event(e)
s2.wait_event(e)
x = paddle.to_tensor([1, 2, 3], dtype='float32')
s1.synchronize()
dlpack_capsule = x.__dlpack__()
y = paddle.from_dlpack(dlpack_capsule)
np.testing.assert_array_equal(x.numpy(), y.numpy())
self.assertTrue(s1.query(), "Stream s1 did not complete all tasks.")
self.assertTrue(s2.query(), "Stream s2 did not complete all tasks.")
Copy link
Contributor

@HydrogenSulfate HydrogenSulfate Dec 4, 2024

Choose a reason for hiding this comment

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

  1. 这个单测不对,并没有测试到__dlpack__接收一个stream参数的case,所以没有覆盖到相应的代码。
    image
    应该这样调用:dlpack_capsule = x.__dlpack__(s1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的老师

Copy link
Member

@SigureMo SigureMo left a comment

Choose a reason for hiding this comment

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

LGTMeow 🐾

@HydrogenSulfate HydrogenSulfate self-requested a review December 5, 2024 02:54
Copy link
Contributor

@HydrogenSulfate HydrogenSulfate left a comment

Choose a reason for hiding this comment

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

LGTM

@HydrogenSulfate HydrogenSulfate merged commit 973dadd into PaddlePaddle:develop Dec 5, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor External developers HappyOpenSource Pro 进阶版快乐开源活动,更具挑战性的任务
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants