[API Compatibility] Align paddle.Tensor.is_sparse, paddle.Tensor.type, paddle.Tensor.size api, paddle.nn.PReLU and paddle.distributions.categorical.Categorical -part - #79550
Conversation
|
你的PR提交成功,感谢你对开源项目的贡献! |
There was a problem hiding this comment.
| 序号 | 位置 | 优先级 | 状态 |
|---|---|---|---|
| 1 | python/paddle/utils/decorator_utils.py |
🚧 | |
| 2 | python/paddle/compat/proxy.py |
🚧 |
点击展开 Review 规则
PR 评审规则:- P0、P1 级别的评审必须提交新的 commit 进行修改;
- P2、P3 级别的评审可以通过评论进行修改。
状态说明:
| 已解决 | 无需修复 | 待修复 |
|---|---|---|
| ✅ | 🟡 | 🚧 |
| is_torch_device = weight_attr is None or ( | ||
| isinstance(weight_attr, str) | ||
| and weight_attr.split(':', 1)[0].lower() | ||
| in {'cpu', 'cuda', 'gpu', 'xpu', 'mps', 'meta'} | ||
| ) | ||
| if is_torch_device: | ||
| if not isinstance(data_format, str): | ||
| device, dtype = weight_attr, data_format | ||
| weight_attr, data_format = None, "NCHW" | ||
| elif weight_attr is not None and data_format == "NCHW": | ||
| device, weight_attr = weight_attr, None |
There was a problem hiding this comment.
问题: 这里把第三个位置参数为 "cpu"/"cuda" 等字符串且 data_format 仍为默认值的调用重解释成 PyTorch 的 device。但 weight_attr: ParamAttrLike 旧 API 明确支持字符串参数名,ParamAttr._to_attr("cpu") 会创建名为 "cpu" 的参数;现在 paddle.nn.PReLU(2, 0.5, "cpu") 会丢掉 weight_attr 并创建匿名参数。
影响: 这是原生 paddle.nn.PReLU 构造函数的全局行为变化,不需要开启 enable_compat(level=2) 就会触发,会破坏依赖参数名的旧模型代码、静态图或 checkpoint/state_dict 兼容性。
处理要求:请针对该评论修复并提交新的 commit。
期望: 保留旧的字符串 weight_attr 语义;PyTorch 的无 dtype 位置参数 device 形态建议放到 paddle.compat.nn.PReLU/level=2 alias 中处理,或至少只在不会与旧 weight_attr 字符串冲突的形态下重解释。例如可以按下面的形态收窄 native 构造函数里的自动重解释:
# 伪代码:仅处理第四个位置参数明确是 dtype/device pair 的无冲突形态
if not isinstance(data_format, str):
device, dtype = weight_attr, data_format
weight_attr, data_format = None, "NCHW"
# PReLU(..., "cpu") 这种无 dtype 的 PyTorch 形态不要在 native paddle.nn.PReLU
# 中覆盖字符串 weight_attr;如需支持,请放到 compat-only wrapper 中。… three_Tensor_and_PReLU
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #79550 +/- ##
==========================================
Coverage ? 96.35%
==========================================
Files ? 6
Lines ? 137
Branches ? 0
==========================================
Hits ? 132
Misses ? 5
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/re-run all-failed |
|
/re-run all-failed |
risemeup1111
left a comment
There was a problem hiding this comment.
已复查当前 head,先前指出的 PReLU(2, 0.5, "cpu") 后向兼容问题在代码上已经修复。本次新增一个非阻塞的行级建议,细节见 inline comment;另外 CI 里 Static-Check / Slice 仍有红灯,合入前还需要继续跟进。
| return f"{prefix}.{tensor_type}" | ||
|
|
||
| device = None | ||
| if isinstance(dtype, type): |
There was a problem hiding this comment.
问题: 这里用 isinstance(dtype, type) 捕获所有 Python type,然后只按 FloatTensor/DoubleTensor 这类 tensor factory 名称识别。DTypeLike 里包含 np.float32、np.float64、np.int64 等 numpy scalar type;这些对象也是 type,会在这里因为 dtype.__name__(例如 "float64")不在 _TENSOR_TYPE_DTYPES 而直接 ValueError,无法走后面的 input.to(dtype=dtype)。
影响: enable_compat(level=2) 后,t.type(np.float64) 这类 Paddle 其他 dtype 参数常见写法会被误判为非法 tensor class。与当前签名里的 DTypeLike | str | type 不一致,也缺少测试覆盖。
处理要求:请针对该评论进行回复(同意并已修改请回复 Done,不同意请说明理由)。
期望: 只把已知 tensor factory class 当作 PyTorch-style class conversion,其他 DTypeLike 继续交给 Tensor.to 校验/转换,并补一个 np.float64 之类的回归测试。可以先把该行收窄为:
| if isinstance(dtype, type): | |
| if isinstance(dtype, type) and dtype.__name__ in _TENSOR_TYPE_DTYPES: |
There was a problem hiding this comment.
补充复查:新提交的 test_tensor_type_edge_cases 覆盖了 t.type(paddle.float64, **{"async": True}),但这个参数不是 Python type,不会进入当前 if isinstance(dtype, type): 分支;原问题里的 np.float64 / np.int64 等 numpy scalar type 仍会进入该分支并因为 "float64" 不在 _TENSOR_TYPE_DTYPES 里抛 ValueError。
处理要求:请针对该评论进行回复(同意并已修改请回复 Done,不同意请说明理由)。
建议仍按上面 suggestion 收窄 tensor factory class 的判断,并把回归测试补到 numpy dtype 路径,例如:
import numpy as np
self.assertEqual(t.type(np.float64).dtype, paddle.float64)There was a problem hiding this comment.
补充复查:这个提交把原来的判断改成了 dtype.__name__ in _TENSOR_TYPE_NAMES,但 _TENSOR_TYPE_NAMES 的 key 是 "float32"/"float64" 这类 dtype 名;paddle.DoubleTensor 来自 dtype_tensor_factory('float64', 'DoubleTensor'),__name__ 是 "DoubleTensor"。因此 t.type(paddle.DoubleTensor) 不会进入这里的 tensor factory class 分支,会继续把 class 对象传给 input.to(dtype=...),这不是 DTypeLike 支持的输入,新增的 self.assertEqual(t.type(paddle.DoubleTensor).dtype, paddle.float64) 也会失败。
处理要求:请针对该评论修复并提交新的 commit。
请把 class 分支的 membership 查 _TENSOR_TYPE_DTYPES(反查表)而不是 _TENSOR_TYPE_NAMES,并保留这次补上的 np.float64 回归测试。修复形态可以是:
if isinstance(dtype, type) and dtype.__name__ in _TENSOR_TYPE_DTYPES:
tensor_type = dtype.__name__
dtype = _TENSOR_TYPE_DTYPES[tensor_type]
device = "cpu"
risemeup1111
left a comment
There was a problem hiding this comment.
已复查最新提交。先前的 PReLU(2, 0.5, "cpu") 后向兼容问题在当前代码里仍是修复状态。
不过 _tensor_type 的 follow-up 修复把 tensor factory class 的查表对象写反了,当前仍会影响 t.type(paddle.DoubleTensor) 这类兼容路径;我已在既有行级线程里补充了具体证据和修复形态。这个问题需要再提交一个修复 commit 后才能通过。当前还有部分 CI 在运行中,请合入前继续确认结果。
risemeup1111
left a comment
There was a problem hiding this comment.
已复查最新提交,先前关于 PReLU 后向兼容和 Tensor.type dtype/class 处理的意见在当前代码中都已修复,未发现新的需要阻塞合入的问题。当前仍有部分 CI 在运行中,合入前请继续确认最终结果。
|
/re-run all-failed |
1 similar comment
|
/re-run all-failed |
risemeup1111
left a comment
There was a problem hiding this comment.
已复查最新提交。此前关于 PReLU 和 Tensor.type 的问题在当前代码中仍是修复状态。
这次新增的 Categorical 参数校验还有一个需要修正的兼容性问题,具体见行级评论。当前仍有部分 CI 在运行中,合入前请继续确认最终结果。
|
/re-run all-failed |
risemeup1111
left a comment
There was a problem hiding this comment.
已复查你在 Categorical 线程里的说明,确认当前实现与 PyTorch 的 probs 归一化/校验顺序一致;此前这条阻塞意见已撤回。先前关于 PReLU 和 Tensor.type 的问题在当前 head 中也仍是修复状态,未发现新的需要阻塞合入的问题。
当前仍有部分 CI 未完成/有红灯,请合入前继续确认最终结果。
risemeup1111
left a comment
There was a problem hiding this comment.
已复查标题更新,当前标题已覆盖这次新增的 Categorical 兼容范围;代码 head 未变化,先前评审结论不变,未发现新的需要阻塞合入的问题。
当前仍有部分 CI 未完成/有红灯,请合入前继续确认最终结果。
|
/re-run all-failed |
|
@zhwesky20 review this PR, Thx |
| return str(input.dtype) | ||
|
|
||
| device = None | ||
| if isinstance(dtype, type) and dtype.__name__ in _TENSOR_TYPE_DTYPES: |
There was a problem hiding this comment.
这里是否考虑了全部情况且能正确执行:
paddle.float32
np.float32
'float32'
'paddle.FloatTensor'
'torch.FloatTensor'
| compat_fn = getattr(compat_root, attr_name) | ||
| _PADDLE_NAMESPACE_SAVED[(paddle.Tensor, attr_name)] = native_method | ||
| _PADDLE_NAMESPACE_SAVED[(paddle.Tensor, attr_name)] = native_attr | ||
| if isinstance(compat_attr, property) or inspect.isdatadescriptor( |
|
/re-run all-failed |
… three_Tensor_and_PReLU
| return proxy | ||
|
|
||
|
|
||
| def dispatch_property( |
There was a problem hiding this comment.
这里有点奇怪,property->func、func->property、property->property 三个分支可以共享这一个dispatch吗?
如果能共享,那后面三个分支也可以合并了
| dispatcher = dispatch_property(native_attr, compat_attr) | ||
| else: | ||
| # native function -> compat property | ||
| dispatcher = dispatch_property(native_attr, compat_attr) |
|
@SigureMo 需要review下enable_compat的接口变动。由于paconvert测试会同时跑 torch+paddle代码来比较结果,原设计会出现很多地方无法跑的问题,因此对enable_compat的功能进行了解耦设计:
|
|
@Manfredss CI看下有没有问题 |
感觉像是卡了,我多试试 |
|
/re-run all-failed |
| level |= 1 | ||
| if _PADDLE_NAMESPACE_SAVED: | ||
| level |= 2 | ||
| return level |
There was a problem hiding this comment.
这里是按位运算的逻辑吧?那是否可以用 enum.Flag 来维护下?数字直接 |= 会让人感到困惑
There was a problem hiding this comment.
import enum
class CompatLevel(enum.Flag):
MODULE_PROXY = 1
API_ALIAS = 2
LEVEL_MODULE_PROXY = CompatLevel.MODULE_PROXY
LEVEL_API_ALIAS = CompatLevel.API_ALIAS
LEVEL_ALL = CompatLevel.MODULE_PROXY | CompatLevel.API_ALIAS
VALID_COMPAT_LEVELS = {LEVEL_MODULE_PROXY, LEVEL_API_ALIAS, LEVEL_ALL}
def is_module_proxy_enabled(level: CompatLevel) -> bool:
return bool(level & CompatLevel.MODULE_PROXY)
def is_api_alias_enabled(level: CompatLevel) -> bool:
return bool(level & CompatLevel.API_ALIAS)
def is_valid_compat_level(level: CompatLevel) -> bool:
return level in VALID_COMPAT_LEVELS接口层不需要感知这些(仍然是数字),仅仅实现层是这样
| if native_method is None: | ||
| tensor_apis = { | ||
| attr_name: getattr(compat_root, attr_name) | ||
| for attr_name in getattr(compat_root, "__all__", ()) |
There was a problem hiding this comment.
paddle.compat.__all__ 里的所有函数都是 tensor 上的 API 么?总感觉不太对?
虽然下面有过滤,但是这里的命名 -> method 的对应关系总感觉不应该按照这个假设?
There was a problem hiding this comment.
呃 确实不是全部 __all__ 里的 seed 就不是 Tensor API,它靠 inspect.getattr_static(paddle.Tensor, name, None) is None 过滤掉;其余 10 个在 Tensor 上同名同义、首参对应描述符传入的 tensor,能对上
我改一下 更鲁棒一点
| if compat_is_property: | ||
| if native_is_property: | ||
| # native property -> compat property | ||
| dispatcher = dispatch_property(native_attr, compat_attr) | ||
| else: | ||
| # native function -> compat property | ||
| dispatcher = dispatch_property(native_attr, compat_attr) | ||
| elif native_is_property: | ||
| # native property -> compat function | ||
| dispatcher = dispatch_property(native_attr, compat_attr) |
There was a problem hiding this comment.
这几个分支不都是 dispatcher = dispatch_property(native_attr, compat_attr) 吗?为啥要这么多 if?
| attr = native_attr | ||
| else: | ||
| attr = compat_attr | ||
| return attr.__get__(instance, owner) |
There was a problem hiding this comment.
这里能保证 attr 是 function 么?不然应该不能 .__get__ 吧
There was a problem hiding this comment.
不用一定是 function,只要是描述符就可以了吧
dispatch_property 只有在 _patch_tensor_methods 里才用到,native 这边来自 inspect.getattr_static(paddle.Tensor, name),compat 这边来自 paddle.compat.__all__(func)或 _TENSOR_API_OVERRIDES(func 或 property)。
现在只有两个 API 会走到这里:type(native 是 getset_descriptor,compat 是 func)、is_sparse(native 是 method_descriptor,compat 是 property)。这几个类型都实现了 __get__,所以我认为应该是没有问题的
There was a problem hiding this comment.
是 descriptor 还是 function 不重要,关键是能不能 bind object(__get__)
算了,反正大概率也没人会在这两处注册 int 之类的 normal object,大概率也没啥问题,有问题再说吧
另外,其实 dispatch_property 已经能取代 dispatch_function 了吧?当然两者本质上有一点区别是,前者在 bind object 时候或者说 getattr 时候就已经确定 dispatch 的结果,而后者则是在实际函数调用时候才确定,但在实际调用过程中我觉得应该感知不到这点细微差异;还有一点差异就是之前可以用 paddle.Tensor.fn(obj) 而 descriptor 就不能 paddle.Tensor.des(obj) 了哈哈,但我觉得这倒没什么不至于有人这么去用,不过不重要了
|
/re-run all-failed |
|
/re-run all-failed |
| device = None | ||
| if isinstance(dtype, type) and dtype.__name__ in _TENSOR_TYPE_DTYPES: | ||
| tensor_type = dtype.__name__ | ||
| dtype = _TENSOR_TYPE_DTYPES[tensor_type] |
There was a problem hiding this comment.
np.float32触发这个分支吗?看起来有点问题
直接判断np.dtype、paddle.dtype吧,鲁棒些。
这个isinstance(dtype, type) 比较奇怪
| ) | ||
| elif tensor_type in _TENSOR_TYPE_DTYPES.values(): | ||
| dtype = tensor_type | ||
| if dtype_string.startswith(("torch.cuda.", "paddle.cuda.")): |
There was a problem hiding this comment.
这个是什么case?有 'torch.cuda.float32' 吗
| else: | ||
| raise ValueError(f"invalid type: {dtype_string!r}") | ||
|
|
||
| dtype_name = str(input.dtype).removeprefix("paddle.") |
There was a problem hiding this comment.
直接判断:str(input.dtype) == dtype 就可以了吧
dtype本身就是字符串
There was a problem hiding this comment.
dtype 走到这里不一定是字符串好像,像t.type(paddle.float32)、t.type(np.float32)、t.type(np.dtype('float32')) 这些都不进上面两个分支,会直接传下来,而且即使是字符串,前面也已经把 'torch.DoubleTensor'、'paddle.float64' 这些统一成了 'float64',和带前缀的 str(input.dtype)('paddle.float64')比的话是不一致的 我还是想保留现在的写法
| return input.is_sparse_coo() | ||
|
|
||
|
|
||
| _TENSOR_API_OVERRIDES = { |
There was a problem hiding this comment.
我建议这个是不是和上面的字典合并起来,统一成一个字典:
_TENSOR_API_OVERRIDES = (
'allclose': allclose,
'equal': equal
'numel': _tensor_numel,
)
risemeup1111
left a comment
There was a problem hiding this comment.
当前 head 仍未修复前次指出的两处 P1:paddle.nn.PReLU 仍会把字符串位置参数当作 device 重解释,enable_compat() / use_compat_guard(enable=False) 的 finder 恢复也还没有改成幂等插入或完整恢复。因此我仍然请求修改。
PR Category
User Experience
PR Types
Improvements
Description
Align the following Paddle APIs with their PyTorch counterparts:
paddle.Tensor.numel()to return a Pythonintunderenable_compat(level=2).paddle.Tensor.is_sparseas a property and returnTrueonly for sparse COO tensors.paddle.Tensor.type()behavior, including type queries, dtype/class/string conversions, device handling, and same-type identity preservation.paddle.nn.PReLUto accept PyTorch-styledeviceanddtypepositional and keyword arguments.paddle.distributions.categorical.CategoricalPaddle-internal callers retain native Tensor semantics, and all patched attributes are restored after disabling compat.
Also refine the compat level semantics and make
use_compat_guardrestore the originalcompatstate. Add lifecycle and real-torch regression tests. This is related to是否引起精度变化
否