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

【Paddle Toolkit Development Competition No.4】 Paddle 适配 torch-scatter #1028

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

NKNaN
Copy link
Contributor

@NKNaN NKNaN commented Nov 24, 2024

PR types

New features

PR changes

APIs

Describe

scatter_min/scatter_max/segment类/gather类 API 由自定义算子实现,其他由组合API实现。

Copy link

paddle-bot bot commented Nov 24, 2024

Thanks for your contribution!

@NKNaN
Copy link
Contributor Author

NKNaN commented Nov 24, 2024

Paddle repo的clang-format要求版本是13.0.0,PaddleScience 是 3.8,能否改成 13.0.0
image

@HydrogenSulfate
Copy link
Collaborator

Paddle repo的clang-format要求版本是13.0.0,PaddleScience 是 3.8,能否改成 13.0.0 image

辛苦提交PR,我明天修改一下

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

HydrogenSulfate commented Nov 26, 2024

  1. 对于组合实现的 API,有一些基础 API 尚未支持 fp16 和 bf16,如 repeat_interleave,所以 fp16 和 bf16 的测试暂时关闭。是否需要对那些 API 支持 fp16 和 bf16?

这个是否可以提交一个PR到paddle呢?应该只要把这几个类型添加到算子的注册宏里面就行了吧?而且比如repeat_interleave这种不涉及具体数值计算的,应该更简单?其它涉及数值计算的,可能还需要注意一下kernel里计算时,float16/bfloat16特化的模板里要转成float32,算完转回原类型即可。

)

index = broadcast(index, src, dim)
eps = paddle.to_tensor(eps, dtype=src.dtype)
Copy link
Collaborator

Choose a reason for hiding this comment

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

定义常量Tensor,建议使用full代替to_tensor

Suggested change
eps = paddle.to_tensor(eps, dtype=src.dtype)
eps = paddle.full([], eps, dtype=src.dtype)


mask = ~res.isfinite()
res[mask] = orig_out[mask]
paddle.assign(res, out)
Copy link
Collaborator

Choose a reason for hiding this comment

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

有个问题,这里如果out为None的话,是不是不正确? paddle.assign(res, out)之后,out应该仍然是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.

out为None的话 orig_out 也为None,会走上面那个分支return
image

Comment on lines 63 to 67
if out is not None:
paddle.assign(res, out)
return out
else:
return res
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里这样写是对的,如果out是None,则返回res


#include "paddle/extension.h"

#define MAX_TENSORINFO_DIMS 25
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个25应该是torch的维数限制,paddle设置为7维吧

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的


#include "paddle/extension.h"

#define MAX_TENSORINFO_DIMS 25
Copy link
Collaborator

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.

好的

#else
#define SHFL_UP_SYNC __shfl_up_sync
#define SHFL_DOWN_SYNC __shfl_down_sync
#endif
Copy link
Collaborator

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.

好的

Comment on lines 5 to 13
from paddle import assign
from paddle import divide
from paddle import floor_divide
from paddle import full
from paddle import full_like
from paddle import ones
from paddle import put_along_axis
from paddle import where
from paddle import zeros
Copy link
Collaborator

Choose a reason for hiding this comment

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

这些函数通过paddle模块访问吧,直接从模块import方法不太好

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
Collaborator

Choose a reason for hiding this comment

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

segment_coo系列的算子是否可以用自定义算子实现?否则for循环效率会不会比较低?

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
Collaborator

Choose a reason for hiding this comment

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

问题同coo系列API,是否能用自定义算子实现?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

也可以,我再改一下

version="1.0",
author="NKNaN",
url="https://github.com/PaddlePaddle/PaddleScience/jointContribution/paddle_scatter",
description="Paddle extension of scatter and segment operators with min and max reduction methods",
Copy link
Collaborator

Choose a reason for hiding this comment

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

description可以补充一下原作者的仓库吧。

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
Collaborator

HydrogenSulfate commented Nov 26, 2024

另外这个PR能单独建立一个私人仓库吗(结构与原项目保持一致即可)?然后权限加我一下,这样我能review,后续移动至PFCCLab仓库在,我会通过submodule的形式把这个添加到PaddleScience的可选安装依赖里

@luotao1
Copy link
Collaborator

luotao1 commented Nov 26, 2024

后续移动至PFCCLab仓库在

也可以现在就移动至PFCCLab仓库下的私人repo

Copy link
Collaborator

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

from xx import yy 建议全部改为 import xx,然后使用 xx.yy的方式调用,否则非常容易出现循环引用的问题。

]


@pytest.mark.skipif(not paddle.cuda.is_available(), reason="CUDA not available")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
@pytest.mark.skipif(not paddle.cuda.is_available(), reason="CUDA not available")
@pytest.mark.skipif(paddle.cuda.device_count() == 0, reason="CUDA not available")



@pytest.mark.skipif(not paddle.cuda.is_available(), reason="CUDA not available")
@pytest.mark.skipif(paddle.cuda.device_count() < 2, reason="No multiple GPUS")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
@pytest.mark.skipif(paddle.cuda.device_count() < 2, reason="No multiple GPUS")
@pytest.mark.skipif(paddle.device.cuda.device_count() < 2, reason="No multiple GPUS")

@NKNaN
Copy link
Contributor Author

NKNaN commented Dec 2, 2024

另外这个PR能单独建立一个私人仓库吗(结构与原项目保持一致即可)?然后权限加我一下,这样我能review,后续移动至PFCCLab仓库在,我会通过submodule的形式把这个添加到PaddleScience的可选安装依赖里

建好了

@HydrogenSulfate
Copy link
Collaborator

另外这个PR能单独建立一个私人仓库吗(结构与原项目保持一致即可)?然后权限加我一下,这样我能review,后续移动至PFCCLab仓库在,我会通过submodule的形式把这个添加到PaddleScience的可选安装依赖里

建好了

好的,收到

@co63oc
Copy link
Contributor

co63oc commented Dec 18, 2024

可以编译安装,但是运行单测失败,不知道是要怎么使用

pip install -e .
pytest tests

image
image

@NKNaN
Copy link
Contributor Author

NKNaN commented Dec 18, 2024

可以编译安装,但是运行单测失败,不知道是要怎么使用

目前 setup.py 只是将c++自定义算子的部分打了包,包名是paddle_scatter_ops,对整个paddle_scatter还没有打包,应该是这个原因。怎么样对整体打包呢?

@HydrogenSulfate
Copy link
Collaborator

HydrogenSulfate commented Dec 18, 2024

可以编译安装,但是运行单测失败,不知道是要怎么使用

目前 setup.py 只是将c++自定义算子的部分打了包,包名是paddle_scatter_ops,对整个paddle_scatter还没有打包,应该是这个原因。怎么样对整体打包呢?

整体的话应该使用setup.py/pyproject.toml两种方式?这样能支持pip install .或者pip install -e .,将整个项目作为package安装到sitepackages下

@NKNaN
Copy link
Contributor Author

NKNaN commented Dec 18, 2024

可以编译安装,但是运行单测失败,不知道是要怎么使用

目前 setup.py 只是将c++自定义算子的部分打了包,包名是paddle_scatter_ops,对整个paddle_scatter还没有打包,应该是这个原因。怎么样对整体打包呢?

整体的话应该使用setup.py/pyproject.toml两种方式?这样能支持pip install .或者pip install -e .,将整个项目作为package安装到sitepackages下

那通过setup.py整体打包之后,把整体包名设置为paddle_scatter的话 (也就是通过paddle.utils.cpp_extension.setup把name设置成paddle_scatter),custom_op应该如何调用呢,paddle_scatter.custom_xx好像不行

@HydrogenSulfate
Copy link
Collaborator

可以编译安装,但是运行单测失败,不知道是要怎么使用

目前 setup.py 只是将c++自定义算子的部分打了包,包名是paddle_scatter_ops,对整个paddle_scatter还没有打包,应该是这个原因。怎么样对整体打包呢?

整体的话应该使用setup.py/pyproject.toml两种方式?这样能支持pip install .或者pip install -e .,将整个项目作为package安装到sitepackages下

那通过setup.py整体打包之后,把整体包名设置为paddle_scatter的话 (也就是通过paddle.utils.cpp_extension.setup把name设置成paddle_scatter),custom_op应该如何调用呢,paddle_scatter.custom_xx好像不行

假设自定义算子的包名为paddle_scater_core,它和paddle-scatter(假设包名为paddle_scatter)是两套代码,应该通过paddle-scatter内的python API去调用paddle_scater_core.*,然后对外公开的API应该是paddle_scatter.*下的封装好的python API

@NKNaN
Copy link
Contributor Author

NKNaN commented Dec 20, 2024

更新了一下,应该这样就可以了

  • Build
cd paddle_scatter
python setup_ops.py install   # 打包c++算子
pip install .              # 整体打包
  • Test
cd paddle_scatter
pytest tests

@co63oc
Copy link
Contributor

co63oc commented Dec 20, 2024

pip install . 

代码已更新最新代码,还是有错误

python setup_ops.py install
pip install .
pytest tests

image
image

@NKNaN
Copy link
Contributor Author

NKNaN commented Dec 20, 2024

已更新

@co63oc
Copy link
Contributor

co63oc commented Dec 20, 2024

已更新

可以了感谢,只有一个单测 tests/test_softmax.py::test_log_softmax,需要设置rtol=1e-3,不知道是不是问题
image

@co63oc
Copy link
Contributor

co63oc commented Dec 20, 2024

https://github.com/rusty1s/pytorch_scatter/
image
示例修改为paddle,运行失败

import paddle
from paddle_scatter import scatter_max

src = paddle.to_tensor([[2, 0, 1, 4, 3], [0, 2, 1, 3, 4]])
index = paddle.to_tensor([[4, 5, 4, 2, 3], [0, 0, 2, 2, 1]])

out, argmax = scatter_max(src, index, dim=-1)

print(out)
print(argmax)

image

@NKNaN
Copy link
Contributor Author

NKNaN commented Dec 20, 2024

已更新

可以了感谢,只有一个单测 tests/test_softmax.py::test_log_softmax,需要设置rtol=1e-3,不知道是不是问题 image

应该是得设置一下rtol。

重新改了一下文件夹的结构,麻烦再试一下。

@co63oc
Copy link
Contributor

co63oc commented Dec 20, 2024

已更新

可以了感谢,只有一个单测 tests/test_softmax.py::test_log_softmax,需要设置rtol=1e-3,不知道是不是问题 image

应该是得设置一下rtol。

重新改了一下文件夹的结构,麻烦再试一下。

pytest都可以跑通了

@co63oc
Copy link
Contributor

co63oc commented Dec 22, 2024

https://github.com/rusty1s/pytorch_scatter/
image
示例修改为paddle,运行失败

import paddle
from paddle_scatter import scatter_max

src = paddle.to_tensor([[2, 0, 1, 4, 3], [0, 2, 1, 3, 4]])
index = paddle.to_tensor([[4, 5, 4, 2, 3], [0, 0, 2, 2, 1]])

out, argmax = scatter_max(src, index, dim=-1)

print(out)
print(argmax)

image

@NKNaN

@NKNaN
Copy link
Contributor Author

NKNaN commented Dec 22, 2024

https://github.com/rusty1s/pytorch_scatter/ image 示例修改为paddle,运行失败

import paddle
from paddle_scatter import scatter_max

src = paddle.to_tensor([[2, 0, 1, 4, 3], [0, 2, 1, 3, 4]])
index = paddle.to_tensor([[4, 5, 4, 2, 3], [0, 0, 2, 2, 1]])

out, argmax = scatter_max(src, index, dim=-1)

print(out)
print(argmax)

image

@NKNaN

我在aistudio上打完包之后,试了一下在这三个地方都是可以运行的

image

@co63oc
Copy link
Contributor

co63oc commented Dec 22, 2024

可以了没问题了感谢 @NKNaN

@NKNaN
Copy link
Contributor Author

NKNaN commented Dec 22, 2024

可以了没问题了感谢 @NKNaN

好的,麻烦你了~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor HappyOpenSource Pro 进阶版快乐开源活动,更具挑战性的任务
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants