-
Notifications
You must be signed in to change notification settings - Fork 5.9k
[API Compatibility No.6] Sink bitwise_xor and bitwise_xor_ to cpp -part #76341
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
base: develop
Are you sure you want to change the base?
Conversation
|
你的PR提交成功,感谢你对开源项目的贡献! |
1ab24a5 to
ccab584
Compare
|
/re-run all-failed |
1 similar comment
|
/re-run all-failed |
faa6f44 to
17d6201
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #76341 +/- ##
===========================================
Coverage ? 100.00%
===========================================
Files ? 2
Lines ? 3
Branches ? 0
===========================================
Hits ? 3
Misses ? 0
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi, 我注意到有两个 CI 失败:
经过分析,这两个失败应该与本 PR 的改动无关,原因如下:
|
非required流水线不用管,目前还在调试中
等研发review过后,我会进行豁免 |
…stubs for bitwise_xor parameter alias support
|
/re-run all-failed |
| - op : bitwise_xor | ||
| name : [paddle.bitwise_xor, paddle.Tensor.bitwise_xor] | ||
| args_alias : | ||
| x : [input] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个用use_default_mapping : True 就可以
python/paddle/_paddle_docs.py
Outdated
|
|
||
| # other | ||
| add_doc_and_signature( | ||
| "bitwise_xor_", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个是bitwise_xor还是bitwise_xor_?
前者是非inplace的,后者是inplace的
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
修复列表里写的是 bitwise_xor_,所以这里写的 bitwise_xor_。这个 API 只能通过装饰器来修改
但 bitwise_xor 是能够 cpp 下沉的,所以有些疑惑 @zhwesky2010
python/paddle/tensor/logic.py
Outdated
| if TYPE_CHECKING: | ||
| from paddle import Tensor | ||
|
|
||
| # @overload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这些不需要
python/paddle/tensor/logic.py
Outdated
| ``paddle.bitwise_xor`` supports broadcasting. If you want know more about broadcasting, please refer to please refer to `Introduction to Tensor`_ . | ||
| .. _Introduction to Tensor: ../../guides/beginner/tensor_en.html#chapter5-broadcasting-of-tensor | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
下沉后原来的bitwise_xor需要删除掉,从_C_ops中导入
|
我觉得不能删去 self.python_api = paddle.tensor.logic.bitwise_xor框架会使用 ValueError: no signature found for builtin <built-in function bitwise_xor>
TypeError: unsupported callable
如果直接从 _C_op 导入,这些功能会无法实现。 最后需要确认一下,API 兼容性增强 No.6 针对的是 bitwise_xor 还是 bitwise_xor_?多谢解答! @luotao1 @zhwesky2010 |
5b6a282 to
eb48a1f
Compare
eb48a1f to
0f7b938
Compare
|
/re-run all-failed |
|
@Manfredss 以表格中的字符串为准。与表格一一对应就行。 |
python/paddle/tensor/logic.py
Outdated
| return _C_ops.bitwise_or_(x, y) | ||
|
|
||
|
|
||
| @param_two_alias(["x", "input"], ["y", "other"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
写了下沉就不要写装饰器了。
两者选其一就可,另外下沉inplace API(bitwise_xor_)之前需要先把非inplace原版(bitwise_xor)也下沉了,因为两者是复用的一个kernel。
|
@zhwesky2010 已根据意见完成修改,烦请再 review 一下,谢谢! |
python/paddle/tensor/logic.py
Outdated
|
|
||
|
|
||
| @inplace_apis_in_dygraph_only | ||
| @ParamAliasDecorator({"x": ["input"], "y": ["other"]}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个也下沉吧?
| args_alias: | ||
| use_default_mapping : True | ||
|
|
||
| - op : bitwise_xor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
下沉后原来的python 层API需要删掉,只有c++接口了
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#76341 (comment) 主要是这个问题,所以我还留着
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
把两个一起下沉了就行了
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
好的
7bad604 to
d5240bf
Compare
这些都不是问题。除非_C_ops前有很复杂的逻辑,或者组合实现的API,都可以下沉。 |
| if TYPE_CHECKING: | ||
| from paddle import Tensor | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
下沉后,需要移除之前的python api,就没有这一层逻辑了。直接走c++
| x: Tensor, | ||
| y: Tensor, | ||
| out: Tensor | None = None, | ||
| name: str | None = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
签名统一参照其他的:
(x, y, name, *, out)
| x_np = np.random.randint(-10, 10, []) | ||
| y_np = np.random.randint(-10, 10, []) | ||
| out_np = eval(f'np.{api.__name__}(x_np, y_np)') | ||
| out_np = eval(f'np.{api.__name__.lstrip("_")}(x_np, y_np)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zero_dim这里为啥需要改
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里的改动是因为测试会有报错
======================================================================
ERROR: test_dygraph_binary (__main__.TestBinaryAPI.test_dygraph_binary)
----------------------------------------------------------------------
Traceback (most recent call last):
File "D:\Xue\ML\PaddleDebug\test\legacy_test\test_zero_dim_binary_api.py", line 170, in test_dygraph_binary
out_np = eval(f'np.{api.__name__}(x_np, y_np)')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "<string>", line 1, in <module>
File "C:\Users\tzy12\anaconda3\envs\paddle\Lib\site-packages\numpy\__init__.py", line 808, in __getattr__
raise AttributeError(f"module {__name__!r} has no attribute {attr!r}")
AttributeError: module 'numpy' has no attribute '_bitwise_xor'. Did you mean: 'bitwise_xor'?
----------------------------------------------------------------------
Ran 6 tests in 0.508s| Inplace version of ``bitwise_xor`` API, the output Tensor will be inplaced with input ``x``. | ||
| Please refer to :ref:`api_paddle_bitwise_xor`. | ||
| """ | ||
| out_shape = broadcast_shape(x.shape, y.shape) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
下沉后,这个逻辑可能会丢掉了,需要测一下
| return paddle.bitwise_xor(var, self.y) | ||
|
|
||
|
|
||
| class TestDygraphInplacBitwiseXorAlias1(TestDygraphInplacBitwiseAnd): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
paddle.bitwise_xor测一下compat情况下的用法
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bitwise_xor_ 需要吗?好像 bitwise_xor_ 用了 compat 会有 segfault 的情况,我认为可能是 bitwise_xor_ 下沉了但是 ops.yaml 里没有 bitwise_xor_ 的定义。需要添加吗?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bitwise_xor_ 需要吗?好像 bitwise_xor_ 用了 compat 会有 segfault 的情况,我认为可能是 bitwise_xor_ 下沉了但是 ops.yaml 里没有 bitwise_xor_ 的定义。需要添加吗?
ops.yaml里不需要bitwise_xor_定义,会根据bitwise_xor的inplace字段,来生成bitwise_xor_
两者本质是共享一个kernel的,只在输出out处理这里有些区别。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bitwise_xor_ 需要吗?好像 bitwise_xor_ 用了 compat 会有 segfault 的情况,我认为可能是 bitwise_xor_ 下沉了但是 ops.yaml 里没有 bitwise_xor_ 的定义。需要添加吗?
先按标准来测试,不要绕过,先提前暴露问题。
这里的segfault是不是触发了什么底层机制的问题?因为目前还没有inplace的API下沉过。
|
/re-run all-failed |
PR Category
User Experience
PR Types
New Features
Description
任务:No. 6 bitwise_xor_ , No.42 bitwise_xor
修改内容
input作为x的别名other作为y的别名test/legacy_test/test_inplace.py中添加参数别名测试用例