[Cpp API Compatibility] Align resize api#78554
[Cpp API Compatibility] Align resize api#78554SigureMo merged 3 commits intoPaddlePaddle:developfrom
Conversation
|
你的PR提交成功,感谢你对开源项目的贡献! |
There was a problem hiding this comment.
Pull request overview
Updates the ATen compatibility layer’s Tensor::resize_ behavior to better match PyTorch semantics when resizing changes the total number of elements, and adds regression tests to cover shrink/grow scenarios.
Changes:
- Implement
Tensor::resize_to usereshapewhennumelis unchanged, otherwise fall back to a storage-changing path. - Add C++ compat tests for shrinking/growing resize cases to validate shape and prefix data preservation.
- Update test commentary to reflect the new compat behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/cpp/compat/ATen_resize_test.cc | Adds tests for resize_ with different numel (shrink/grow) and updates test notes. |
| paddle/phi/api/include/compat/ATen/ops/resize.h | Adjusts compat Tensor::resize_ implementation to handle both same-numel reshapes and storage-changing resizes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (memory_format.has_value()) { | ||
| TORCH_CHECK(*memory_format == at::MemoryFormat::Contiguous, | ||
| "resize_ only supports contiguous memory format, but got ", | ||
| static_cast<int>(*memory_format)); |
There was a problem hiding this comment.
The memory_format rejection message reports static_cast<int>(*memory_format), which is hard to interpret and inconsistent with other compat ops that report unsupported MemoryFormat values more directly. Prefer emitting a clearer value (e.g., the enum name) and/or reusing the same phrasing used in other ATen compat ops (like empty/empty_like) so downstream users get consistent diagnostics.
| static_cast<int>(*memory_format)); | |
| *memory_format); |
| TEST(TensorResizeTest, ResizeShrinkDifferentNumel) { | ||
| at::Tensor t = at::arange(24, at::kFloat).reshape({2, 3, 4}); | ||
|
|
||
| t.resize_({4, 5}); | ||
|
|
There was a problem hiding this comment.
New behavior was added for the memory_format argument (only contiguous is accepted), but the test suite here doesn't cover that path. Adding a small gtest that passes a non-contiguous memory format and asserts the expected failure would lock in the intended compatibility behavior.
| std::vector<int64_t> dims(size.begin(), size.end()); | ||
| int64_t new_numel = 1; | ||
| for (auto dim : dims) { | ||
| new_numel *= dim; | ||
| } |
There was a problem hiding this comment.
new_numel is computed by multiplying dims without any checks for negative dimensions or int64 overflow. If a caller passes an invalid/very large size, new_numel can wrap and the code may take the reshape path or pass sizes into set_ in a surprising way. Consider validating dim >= 0 and using overflow-safe multiplication (or an existing checked helper) before comparing against tensor_.numel().
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #78554 +/- ##
==========================================
Coverage ? 95.55%
==========================================
Files ? 1
Lines ? 45
Branches ? 0
==========================================
Hits ? 43
Misses ? 2
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/re-run all-failed |
1 similar comment
|
/re-run all-failed |
ShigureNyako
left a comment
There was a problem hiding this comment.
这版把 resize_ 从“只能 reshape”推进到了“numel 变化时也能工作”,方向是对的,但我这边暂时不能给过,主要有两处和 upstream torch.resize_ 语义仍不一致:
- 现在只要
numel变化就会copy_to + set_,这会无条件替换底层 storage;而 PyTorch 的 shrink 语义是不改 underlying storage,只在容量不够时扩容。这样shrink -> grow back的 round-trip 行为会不一致,也属于兼容层里的行为级 breaking change。 memory_format目前被限制为Contiguous,会把 upstream 允许的channels_last / channels_last_3d调用直接变成异常;相比现状,这也是新的兼容风险。
建议先把这两点补齐,并补上对应的回归测试(至少包含 shrink-grow round-trip 和非 contiguous memory_format 场景)。另外,现在线上 thread 里提到的负尺寸 / 溢出校验我也建议一并补上,避免异常路径继续和 PyTorch 偏离。
| paddle::experimental::reshape(tensor_, size._PD_ToPaddleIntArray()); | ||
| const_cast<Tensor*>(this)->tensor_ = result; | ||
| if (memory_format.has_value()) { | ||
| TORCH_CHECK(*memory_format == at::MemoryFormat::Contiguous, |
There was a problem hiding this comment.
这里把 memory_format 限死为 Contiguous,和 upstream resize_ 也还没对齐。
我查了 PyTorch 的实现,resize_ 会把 memory_format 继续传给 empty_tensor_restride,也就是至少允许 Contiguous / ChannelsLast / ChannelsLast3d,只会拒绝 Preserve 这类不支持的值。当前 compat 实现则会把所有非 Contiguous 的调用直接变成异常。
这不仅是兼容性缺口,还是行为级 breaking change:这个 compat 层原来对 memory_format 参数是“静默接受但不生效”,而这版会让 channels_last 调用直接报错。既然 PR 标题和描述都在强调“Align resize api”,这里最好补到和 upstream 一致,并加一组对应的回归测试。
There was a problem hiding this comment.
嗯?相比于之前不是 breaking 吧?之前应该 Paddle 本身也没支持,会有更不友好的报错,这里报错是合理的
There was a problem hiding this comment.
嗯?相比于之前不是 breaking 吧?之前应该 Paddle 本身也没支持,会有更不友好的报错,这里报错是合理的
你这个提醒有道理:如果只从 Paddle 内核能力看,这里直接报错不算离谱。
我这里说的兼容风险,主要是 compat 层相对于当前已有行为的回退:旧实现这里是直接 reshape,memory_format 参数虽然没有真正生效,但在 same-numel 场景下不会在 compat 层同步抛错;这版新增 guard 之后,会把原来能走通的 channels_last / channels_last_3d 调用直接变成异常。
所以我补了一个更保守的 inline suggestion:如果这次 split PR 想先控风险,最小改法就是先别在这里新增 hard error,保持旧行为不退化;真正要和 upstream 对齐到位,则需要后续把 ChannelsLast/ChannelsLast3d 的 restride 也补上,复杂性会比这次拆分略高一些。
| } | ||
|
|
||
| auto source = tensor_.copy_to(tensor_.place(), /*blocking=*/true); | ||
| paddle::experimental::set_(const_cast<Tensor*>(this)->tensor_, source, dims); |
There was a problem hiding this comment.
这里有一个和 upstream torch.resize_ 语义不一致的点,建议在合入前修正。
当前实现一旦 numel 变化,就会先 copy_to 再 set_,这会无条件换掉底层 storage。仓内 as_strided_scatter 也是用 copy_to(..., true) 来显式做“independent copy”,所以这里不是单纯改 shape,而是把 storage 换成了一份新副本。
但 PyTorch 的 resize_ 在 shrink 场景下不会改 underlying storage,只会在容量不够时扩容;因此 shrink -> grow back 时,旧 capacity 内被隐藏的 tail 数据应该还能重新可见。按这里的实现,tail 数据会在 copy_to 时直接丢掉,行为会变成“只保留当前可见前缀”。
建议至少补一个 round-trip 用例来锁定这点,比如 24 -> 20 -> 24 后检查 20..23 是否仍然可见;实现上也尽量改成“仅在需要扩容时变更 storage”,这样才能和 upstream 对齐。
There was a problem hiding this comment.
@ShigureNyako 给点详细的 suggestion 吧,通过 code suggestion 提出来,这里具体在兼容性的风险是什么?是否会增加很多复杂性?如果在可控范围内可以按照你说的改
There was a problem hiding this comment.
@ShigureNyako 给点详细的 suggestion 吧,通过 code suggestion 提出来,这里具体在兼容性的风险是什么?是否会增加很多复杂性?如果在可控范围内可以按照你说的改
收到,我刚补了两个更具体的 inline suggestion。
这里这个 storage 语义的风险点是:当前实现对所有 numel 变化都先 copy_to,这一步会把底层 storage 换成一份 independent copy。这样在 shrink -> grow back 的 round-trip 场景里,原来 hidden tail 那段数据会被直接截掉;而 upstream torch.resize_ 的 shrink 语义是不换 storage,只在容量不够时扩容,所以 tail 数据应当还能重新可见。
我给的最小改法是把这里改成直接复用当前 tensor 自己做 source,也就是和 Paddle Python 侧 resize_ 的 x.set_(x, shape) 保持一致:这样复杂性比较可控,不需要先引入完整的新 storage 管理逻辑。除此之外,再补一个 24 -> 20 -> 24 的 round-trip 用例,基本就能把这个兼容风险锁住。
ShigureNyako
left a comment
There was a problem hiding this comment.
按 @SigureMo 的追问,我补了更具体的 suggestion 和风险说明,主要是两点:
memory_format:如果这次 split PR 先以“不要引入新的兼容回退”为目标,建议先不要把原来基本忽略的参数升级成 hard error;真正补齐到 upstream 语义,可以后续单独做完整 restride 支持。- storage 语义:建议避免
copy_to + set_这种会先做 independent copy 的路径,尽量改成直接复用当前 tensor 自己作为set_的 source,并补一个24 -> 20 -> 24的 round-trip 测试锁定 shrink-grow-back 行为。
这样改动范围相对可控,也更接近当前 Paddle Python resize_ 和 upstream torch.resize_ 的预期语义。
| auto source = tensor_.copy_to(tensor_.place(), /*blocking=*/true); | ||
| paddle::experimental::set_(const_cast<Tensor*>(this)->tensor_, source, dims); |
There was a problem hiding this comment.
这里可以给一个更具体的最小改法:不要先 copy_to 再 set_,而是直接把当前 tensor 自己作为 set_ 的 source。
Paddle Python 侧 resize_ 本身就是 x.set_(x, shape);按这个思路改,至少不会先做一次 independent copy,语义上也更接近 upstream resize_ 的“shrink 不换 storage,只在需要时扩容”。另外建议再补一个 24 -> 20 -> 24 的 round-trip 测试,锁定 tail 数据是否还能重新可见。
| auto source = tensor_.copy_to(tensor_.place(), /*blocking=*/true); | |
| paddle::experimental::set_(const_cast<Tensor*>(this)->tensor_, source, dims); | |
| paddle::experimental::set_(const_cast<Tensor*>(this)->tensor_, tensor_, dims); |
| if (memory_format.has_value()) { | ||
| TORCH_CHECK(*memory_format == at::MemoryFormat::Contiguous, | ||
| "resize_ only supports contiguous memory format, but got ", | ||
| static_cast<int>(*memory_format)); | ||
| } |
There was a problem hiding this comment.
如果这次拆分 PR 先以“不要引入新的兼容回退”为目标,这里建议先不要把 memory_format 从“原来基本忽略”升级成 hard error。
具体风险在于:旧实现这里直接 reshape,memory_format 参数虽然没真正生效,但同-numel 场景至少不会在 compat 层同步报错;这版则会把 channels_last/channels_last_3d 调用直接变成异常。完整补齐 upstream 语义需要额外的 restride 支持,复杂性比这个 split PR 大一些;如果想先控风险,最小改法可以是先删掉这个 guard,后续再单独补真正的 memory-format 对齐。
| if (memory_format.has_value()) { | |
| TORCH_CHECK(*memory_format == at::MemoryFormat::Contiguous, | |
| "resize_ only supports contiguous memory format, but got ", | |
| static_cast<int>(*memory_format)); | |
| } | |
| // Keep old compat behavior for memory_format in this split PR. | |
| // TODO: add real ChannelsLast/ChannelsLast3d restride support in a follow-up. |
| paddle::experimental::reshape(tensor_, size._PD_ToPaddleIntArray()); | ||
| const_cast<Tensor*>(this)->tensor_ = result; | ||
| if (memory_format.has_value()) { | ||
| TORCH_CHECK(*memory_format == at::MemoryFormat::Contiguous, |
There was a problem hiding this comment.
嗯?相比于之前不是 breaking 吧?之前应该 Paddle 本身也没支持,会有更不友好的报错,这里报错是合理的
你这个提醒有道理:如果只从 Paddle 内核能力看,这里直接报错不算离谱。
我这里说的兼容风险,主要是 compat 层相对于当前已有行为的回退:旧实现这里是直接 reshape,memory_format 参数虽然没有真正生效,但在 same-numel 场景下不会在 compat 层同步抛错;这版新增 guard 之后,会把原来能走通的 channels_last / channels_last_3d 调用直接变成异常。
所以我补了一个更保守的 inline suggestion:如果这次 split PR 想先控风险,最小改法就是先别在这里新增 hard error,保持旧行为不退化;真正要和 upstream 对齐到位,则需要后续把 ChannelsLast/ChannelsLast3d 的 restride 也补上,复杂性会比这次拆分略高一些。
| } | ||
|
|
||
| auto source = tensor_.copy_to(tensor_.place(), /*blocking=*/true); | ||
| paddle::experimental::set_(const_cast<Tensor*>(this)->tensor_, source, dims); |
There was a problem hiding this comment.
@ShigureNyako 给点详细的 suggestion 吧,通过 code suggestion 提出来,这里具体在兼容性的风险是什么?是否会增加很多复杂性?如果在可控范围内可以按照你说的改
收到,我刚补了两个更具体的 inline suggestion。
这里这个 storage 语义的风险点是:当前实现对所有 numel 变化都先 copy_to,这一步会把底层 storage 换成一份 independent copy。这样在 shrink -> grow back 的 round-trip 场景里,原来 hidden tail 那段数据会被直接截掉;而 upstream torch.resize_ 的 shrink 语义是不换 storage,只在容量不够时扩容,所以 tail 数据应当还能重新可见。
我给的最小改法是把这里改成直接复用当前 tensor 自己做 source,也就是和 Paddle Python 侧 resize_ 的 x.set_(x, shape) 保持一致:这样复杂性比较可控,不需要先引入完整的新 storage 管理逻辑。除此之外,再补一个 24 -> 20 -> 24 的 round-trip 用例,基本就能把这个兼容风险锁住。
ShigureNyako
left a comment
There was a problem hiding this comment.
我重新看了最新提交,之前我这边卡住的两个核心问题已经处理到可接受范围:
- 不再把
memory_format新增成 hard error,避免了这次 split PR 引入 compat 行为回退; resize_改成直接操作底层DenseTensor,并补了24 -> 20 -> 24round-trip 测试,之前担心的 shrink/grow-back tail 数据丢失问题也被覆盖到了。
另外,负尺寸 / numel 溢出校验和对应测试也已经补上。就这次拆分 PR 的范围看,我这里没有新的 blocking issue 了;更完整的 memory_format restride 语义可以后续再单独补齐。
PR Category
Execute Infrastructure
PR Types
Bug fixes
Description
拆分自 #78484
对齐
resize_API,用于 DeepGEMM 等场景中的内存对齐。变更详情
变更内容 (
ATen/ops/resize.h)支持多种调用方式:
关键修复:
resize_对IntArrayRef参数的处理memory_format参数回归测试补充 (
test/cpp/compat/ATen_resize_test.cc)ResizeBasic: 基础 resize 操作ResizeWithMemoryFormat: 带内存格式的 resizeResizeException: 异常路径测试相关文档
是否引起精度变化
否