Repository navigation
Conversation
Indexing an axis of size 0 is always out of range, but the bound checks in funcs::ScatterAssign / ScatterAssignAdd and their GPU counterparts were unreachable because ScatterKernel returned early when out had no elements. A non-empty index into an x whose first dimension is 0 was therefore silently accepted and every update was dropped. Reject it in ScatterInferMeta rather than in the kernels, the same way gather/take_along_axis/index_sample do since PaddlePaddle#79912: the check is device agnostic, so it covers XPU and the static graph as well, and scatter only ever indexes axis 0. An empty index still returns a copy of x, and an index whose shape is not fully known is left to the runtime. scatter_grad also returned garbage for an empty index: dUpdates was left uninitialized on CPU and mis-shaped on GPU, because gather writes only the rows the index covers. Handle the empty index explicitly as dX = dOut and dUpdates = 0.
Add dygraph tests over get_places() asserting IndexError for a non-empty index into x whose first axis is 0, with either overwrite setting. The test also pins the empty-index gradients to dX = dOut and dUpdates = 0. TestScatterOp_ZeroSize2 used a 4-element index into x of shape (0, 1) and expected the op to silently return an empty output, which is exactly the missing bound check; its index is now empty so the case stays in range.
SwiGLUGradKernel returned as soon as dx or dy had no elements, before any of its shape checks ran. dx shares x's meta, so whenever x was 0-size a dz with the wrong rank or shape was silently accepted, while the same mismatch on a non-empty x raised InvalidArgument. Run the shape checks first, then take the 0-size early returns. When y is given, the checks mirror SwiGLUInferMeta: x and y may differ if either is 0-size, and dz must match the forward output shape, which is y's shape only when x is the non-empty one.
Paddle-Bot Review Board （review完成)
可维护性建议（总分 87.9 / 100，只给建议，不影响评审结论）
各维度得分（仅评本 PR 改动，点击展开）
Powered by Nyanpasu claude with Opus 5.5 默认推理级别, please check the suggestions carefully. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #79922 +/- ##
===========================================
Coverage ? 100.00%
===========================================
Files ? 2
Lines ? 19
Branches ? 0
===========================================
Hits ? 19
Misses ? 0
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // Returns true only when the rank of `dims` is known and every dim is known and | ||
| // positive, i.e. the tensor is statically known to hold at least one element. | ||
| // A 0-D tensor holds exactly one element, so it returns true. | ||
| static bool IsKnownNonEmpty(const DDim& dims) { |
There was a problem hiding this comment.
💡 可维护性建议（抽象适配度，不阻塞合入）：IsKnownNonEmpty 与 paddle/phi/infermeta/binary.cc:2244 的同名静态函数逐字相同，下方 2945 行的“空轴 + 非空 index 即越界”判定也与 CheckIndexOnEmptyAxis（binary.cc:2258，已被 gather、index_sample、take_along_axis 复用）等价。同一契约现在在 infermeta 包内维护两份，后续调整判据或错误类型时容易只改一处而静默分叉。建议把这两个 helper 提取到 infermeta 共享头，或直接调用 CheckIndexOnEmptyAxis(ref_dims, 0, index_dims) 并说明 axis 0 的语义来源。
| // updates may have more rows than index; rows past the index length never | ||
| // reach Out, so their grad is 0. Gather only into the leading rows, which | ||
| // also keeps the gather from resizing updates_grad to the index length. | ||
| DenseTensor updates_grad_head = *updates_grad; |
There was a problem hiding this comment.
💡 可维护性建议（中层优雅度，不阻塞合入）：空 index 早退（42-51 行）与这里 updates_grad 头部切片、尾部清零的准备逻辑，和 paddle/phi/kernels/gpu/scatter_grad_kernel.cu（43-52、76-87 行）逐字重复，仅 Gather 函数名不同，且这些操作（Copy/Full/Slice/Alloc）都与设备无关。建议收敛到两端共享的 scatter_grad_kernel.h 模板辅助函数（完成 Alloc、按 len(index) 切出 head、尾部 Full 置零），两个后端只保留 CPUGather/GPUGather 分派，避免后续一侧修好另一侧漏改。
|
|
||
| if (updates_grad) { | ||
| dev_ctx.template Alloc<T>(updates_grad); | ||
| // updates may have more rows than index; rows past the index length never |
There was a problem hiding this comment.
💡 可维护性建议（约定偏离，不阻塞合入）：这里的“updates 行数多于 index 时尾部梯度置 0”目前只在 CPU/GPU 生效；paddle/phi/kernels/xpu/scatter_grad_kernel.cc 的同构实现只有 index 为空的分支，仍把整个 updates_grad 交给 xpu::scatter_grad。本 PR 合并后同一算子在 XPU 上对 len(index) < updates.shape[0] 仍可能得到未初始化或被改写的 dUpdates，跨后端行为开始分叉。建议补上同样的尾部置零与头部切片，或在本 PR 中明确 XPU 的后续处理计划与跟踪项。
| """updates must match x in every dim but the first, on every device.""" | ||
|
|
||
| def set_device(self, place): | ||
| paddle.set_device("cpu" if isinstance(place, base.CPUPlace) else "gpu") |
There was a problem hiding this comment.
💡 可维护性建议（测试策略，不阻塞合入）：这里把 get_places() 的结果映射为 isinstance(place, base.CPUPlace) ? "cpu" : "gpu"，但 op_test.get_places() 在 is_custom_device() 构建下返回 base.CustomPlace，会被映射成 "gpu"：未编译 CUDA 的 MLU/NPU 构建上 paddle.set_device("gpu") 会直接报错，也就无法在这些设备上覆盖新增校验。建议直接 paddle.set_device(place)（该接口接受 Place），并与本文件已有的 get_device_place()/self.places 写法保持一致；同时把重复三处的内联映射统一到 set_device 辅助方法。
| y_dims, | ||
| dims)); | ||
| const auto &y_dims = y.get().dims(); | ||
| // Mirror SwiGLUInferMeta: x and y may differ when either is 0-size, and |
There was a problem hiding this comment.
💡 可维护性建议（高层优雅度，不阻塞合入）：内核在这里重新推导前向输出形状（out_dims = (!x_empty && y_empty) ? y_dims : dims），与 SwiGLUInferMeta（paddle/phi/infermeta/binary.cc:5053）逐句对应，注释也写明 “Mirror”。同一形状契约因此有两个来源：前向 InferMeta 调整 0-size 语义时，这里既不会编译失败也不会立刻有测试失败。建议把“由 x/y 推导 out dims”抽成可复用的 inline 函数，供 InferMeta 与 kernel 共用；顺带把为跨分支传递而提升到函数作用域的 m/n（44 行）收回 y 为 None 的分支内局部计算，避免隐式状态。
…'s length scatter writes one row of updates per index value, so the two must be as long. ScatterInferMeta only required index.shape[0] <= updates.shape[0] and the trailing rows of updates were silently dropped, but that rule was only honoured in three places: this check, the docstring, and a zero-padding branch in the two prim scatter_grad decompositions. The kernels and the SPMD rule were written for equal lengths all along: - cpu/gpu scatter_grad derive dUpdates as gather(dOut, index), which writes exactly index.shape[0] rows. With a longer updates the trailing rows were left uninitialized on CPU, and GPUGather resized dUpdates to the index length on GPU, so for x=[8], index=[0,1], updates=[5] and dOut all 3 the expected [3,3,0,0,0] came back as [3,3,nan,nan,nan] and as [3,3]. - ScatterGradInferSpmd takes updates_grad's dist attr straight from GatherInferSpmd, i.e. it assumes the same thing. Rather than teach the kernels the padding the prim rules do, require the lengths to match: that is what most of the implementation already assumed, and what torch's index_copy_ / index_add_ enforce. The message follows torch's wording. The padding branches in composite_backward_api.h and decomp_vjp/details.h are now unreachable and are removed; the forward scatter that the x_grad path builds passes a zero_tensor shaped like updates, which satisfies the new check for any program the forward accepts. Also check the trailing dims of updates here, skipping unknown (-1) dims, for a 1-D index and for a 0-D index (where updates must equal one row of x). InferMeta runs before the kernels on every backend and in the static graph, so this closes two gaps: both kernels return a copy of x on an empty index before reaching the CPU check, and the GPU kernel never checked the shape at all, reading updates with x's row stride. TestScatterOp_ZeroSize, TestScatterOp_ZeroSize2 and the two origin-signature cases in test_scatter_compatible.py paired an index with a longer updates; their updates are shortened to the index length. The expected values in test_scatter_compatible.py do not change, because the rows it dropped were the trailing ones, so it still pins the behaviour PaddlePaddle#75068 promised to keep.
77c85bc to
d4102ea
Compare
|
/re-run all-failed |
op_test.get_places() drops CPUPlace on a CUDA build unless
FLAGS_CI_both_cpu_and_gpu is set, so TestZeroSizeAxisOutOfRangeError and
TestScatterUpdatesShapeError only ever ran on GPU and left the empty-index
branch of cpu/scatter_grad_kernel.cc uncovered.
Iterate over CPU plus the current backend device instead, and pass the place
straight to paddle.set_device, which accepts a Place. The three inlined
"cpu" if isinstance(place, base.CPUPlace) else "gpu" mappings this replaces
would also have picked "gpu" for the CustomPlace that get_places() returns on
an MLU/NPU build, where set_device("gpu") fails outright. CPU is skipped on a
custom-device build, mirroring get_places()'s own exclusion.
omoYang
changed the title
|
|
|
/re-run all-failed |
1 similar comment
|
/re-run all-failed |
|
@Paddle-Review-Bot 重新review |
|
@Paddle-Review-Bot 求review🥺 |
|
@Paddle-Review-Bot /new-session |
| // One index selects one row of updates, so the two must have the same | ||
| // length. The kernels and the SPMD rule already assume this: both derive | ||
| // dUpdates as gather(dOut, index), whose row count is the index length. | ||
| PADDLE_ENFORCE_EQ( |
There was a problem hiding this comment.
这里把 PADDLE_ENFORCE_LE 改成 PADDLE_ENFORCE_EQ 后，paddle.scatter 会拒绝此前合法的输入：len(index) < updates.shape[0]（典型写法是让 updates 复用 x 的形状，前向只使用前 len(index) 行）。这属于公共 API 的可见行为收紧，而不是单纯的参数校验补强：
python/paddle/tensor/manipulation.py:4748的updates说明仍写着 “the updates shape should be the same as input”，描述的正是这种短index用法，本次没有同步修改；test/legacy_test/test_scatter_compatible.py中原先index长 2、updates为[3, 4]的兼容性用例，本次被改成[2, 4]才能通过，说明旧语义确实被该用例覆盖。
仓库规则要求公共 API 变更保持向后兼容或提供明确迁移方案，PR 描述中的说明不能替代迁移方案。建议二选一：
- 保留
<=校验，并在 CPU/GPU 的scatter_grad中把index未覆盖的updates行梯度置零（这些行不参与前向输出）； - 明确接受该不兼容变更，同时同步更新
manipulation.py的updates说明与 API 变更记录，让升级用户看到明确指引。
There was a problem hiding this comment.
这个变更要保留，len(index) < updates.shape[0] 不再接受，在manipulation.py:line4747里已经进行了说明，updates和index的长度必须一致
There was a problem hiding this comment.
明白，该行为变更保留，我不再要求回退。关于文档：4747 行的 index 说明确实已写明长度必须等于 updates 的第 0 维，但紧邻的 4748 行 updates 说明仍是旧规则（“the updates shape should be the same as input”），示例上方 4724-4725 行的注释也写着 “shape of updates should be the same as x”。当 len(index) != x.shape[0] 时，这两处与新约束及实现矛盾。建议同步为：1-D index 时 updates 除第 0 维外应与 input 相同，第 0 维等于 index 长度。完成这处文档同步后，该 P1 即可关闭。
There was a problem hiding this comment.
“the updates shape should be the same as input”的意思是scatter的公式是x[index[i]] = updates[i]，这里是让updates除最高维以外和input保持一致（最高维是updates.shape[0]，表示updates这个数组的长度，和index一样；最高维以外就是updates.shape[1...N-1]要和input.shape[1...N-1]一样），不一致的话x[index[i]] = updates[i]会出错的
There was a problem hiding this comment.
收到，理解了 4748 行的意图：updates 第 0 维等于 index 长度，其余维度与 input 一致，这与实现相符。仍有两处示例注释与示例本身矛盾：4724 行 “shape of updates should be the same as x”，而该示例 x 为 [3,2]、updates 为 [4,2]；4725 行的 “dim > 1” 在 2-D 示例中没有对应维度，应为 “dim > 0”。同步这两行后，该 P1 即可关闭。
There was a problem hiding this comment.
而该示例 x 为 [3,2]、updates 为 [4,2]不矛盾啊。公式是x[index[i]] = updates[i]，x.shape[1]和updates.shape[1]是一样的就没问题了啊；4724行的意思是shape of updates with dim > 1 should be the same as input（with dim >1)
| """A 0-size x must not let a mismatched dz/y skip the shape checks.""" | ||
|
|
||
| def setUp(self): | ||
| self.devices = ["cpu"] |
There was a problem hiding this comment.
💡 可维护性建议（约定偏离，不阻塞合入）：这里用 core.is_compiled_with_cuda() 枚举设备，且切换后没有还原设备。同一文件的其他用例（约 135-150 行的 check_dygraph）统一使用 paddle.is_compiled_with_cuda() or is_custom_device()，并在结束后 paddle.set_device(origin_device) 还原。当前写法在 XPU/NPU 等自定义设备构建上会让这两组 0-size 形状校验只在 CPU 上执行，而本 PR 的校验位于 CPU/GPU/XPU 共用的 swiglu_grad_kernel.h，加速卡上同样需要覆盖；设备不还原也会影响后续用例。建议改为本文件既有写法。
|
@liuhao2638 已按指令处理：
本轮结论：REQUEST_CHANGES。发现 1 个 P1 —— |
The two device loops in TestSwigluGrad0SizeShapeCheck called paddle.set_device() without restoring, leaking the last device (gpu on CUDA builds) into later test classes in the same process (the class runs 3rd by unittest name order, with 7 classes after it). Capture and restore the original device per loop, matching the existing check_dygraph pattern. Device enumeration is intentionally kept CUDA-only; this PR's dz-shape validation lives in the shared swiglu_grad_kernel.h used by CPU and GPU, while the XPU kernel is a separate implementation left untouched here.
|
/re-run all-failed |
2 similar comments
|
/re-run all-failed |
|
/re-run all-failed |
PR Category
Operator Mechanism
PR Types
Bug fixes
Description
本 PR 把
paddle.scatter的入参校验集中到ScatterInferMeta，给scatter_grad的 CPU/GPU kernel 补上空 index 的梯度分支，并让swiglu_grad在 0-size 提前返回之前先做形状检查。1.
ScatterInferMeta中增加对scatter 的输入参数校验paddle/phi/infermeta/ternary.cc的ScatterInferMeta原先只校验 index 的 rank 和index.shape[0] <= updates.shape[0]，其余约束散落在各后端 kernel 里。本 PR 在这里补齐三项，让它成为唯一的校验入口。(a)对x的第 0 维为 0 且 index 非空时进行拦截
x 的第 0 维为 0 时没有任何合法下标，所以非空 index 必然越界。
IsKnownNonEmpty的语义和命名沿用 #79912 给gather/take_along_axis/index_sample加的同名辅助函数（它在binary.cc里是static，跨 translation unit 不可见，因此在ternary.cc另置一份）；scatter 只索引第 0 轴，不需要CheckIndexOnEmptyAxis的轴规范化。index 形状未完全已知（含-1）时不做判断，交给运行期。(b) index 长度必须等于 updates 的第 0 维，对不满足的进行拦截
一个 index 值对应 updates 的一行，两者本应等长。这是不兼容变更，单独在下一节说明。
受影响的现存单测共 4 处，updates 缩短到 index 的长度：
TestScatterOp_ZeroSize、TestScatterOp_ZeroSize2，以及test_scatter_compatible.py的test_non_inplace_origin_scatter和test_inplace_origin_scatter。后两者是 #75068 为"新增 torch 风格签名后旧签名行为不变"加的回归守卫，期望值和梯度断言一字未改——它们原先丢弃的恰好是尾部行，缩短 updates 后写入的行完全相同，所以 #75068 的兼容性承诺仍然成立。(c) updates 的尾部维度必须和 x 一致
1 维 index 时校验
updates.shape[i] == x.shape[i]（i >= 1）；0 维 index 时 updates 应当正好是 x 的一行，校验updates.shape[i] == x.shape[i+1]。两者都跳过值为-1的未知维度。依据是updates[i]整行覆盖x[index[i]],尾部维度不一致时按 x 的行步长去读 updates 会读错位置。2. 为
scatter_grad的 CPU/GPU kernel 增加空 index 的处理分支cpu/scatter_grad_kernel.cc和gpu/scatter_grad_kernel.cu在out_grad.numel() == 0的提前返回之后新增一段：反向的主体是
dUpdates = gather(dOut, index)。index 为空时CPUGather一行都不写，dUpdates会保持未初始化（实测为nan） ；GPUGather内部的GatherV2CUDAFunction会把输出Resize成 index 的长度，dUpdates形状变成[0],和updates对不上。空 index 时Out == X，梯度有确定答案：dX = dOut，dUpdates按 updates 的形状全 0，所以单独处理。3. 删除两处 prim
scatter_grad分解里的补零分支paddle/fluid/prim/api/composite_backward/composite_backward_api.h和paddle/fluid/primitive/decomp_rule/decomp_vjp/details.h里各有一段：加上 1(b) 的等长约束后，
updates_dims[0] > index_dims[0]不再可能成立，这两段变为不可达，删除。两条 prim 路径都会经过ScatterInferMeta,静态图 prim 在static_prim_api.cc里显式调用op->InferShape()，而scatter这个 fluid op 的 InferShape 通过PD_INFER_META(phi::ScatterInferMeta)桥接；动态图 prim 转发给scatter_ad_func；PIR 侧由ops.yaml的infer_meta: ScatterInferMeta在建 op 时执行。x_grad路径构造的前向scatter<T>(out_grad, index, zero_tensor, false)传的zero_tensor用的是updates的形状，对任何能通过前向的程序都满足新约束，前反向自洽。4.
swiglu_grad先做形状检查，再走 0-size 提前返回paddle/phi/kernels/swiglu_grad_kernel.h里SwiGLUGradKernel原先只要dx或dy没有元素就立刻 return，所有形状检查都在其后。本 PR 把检查顺序调到前面。传入y时检查逻辑与SwiGLUInferMeta对齐：x 和 y 在其中一个为 0-size 时允许不同；dz必须匹配前向输出形状，而该形状只有在 x 是非空的那一侧时才等于 y 的形状。调整的原因是
dx复用x的 meta,x 为 0-size 时dx也是 0-size，于是一个秩或形状错误的dz会被静默接受，而同样的不匹配在非空 x 上会报InvalidArgument。5. docstring
python/paddle/tensor/manipulation.py里scatter的 index 参数说明，从 "The length of index cannot exceed updates's length" 改为 "When the index is a 1-D tensor, its length must be equal to updates's first dimension size"。测试
test/legacy_test/test_scatter_op.py新增两个类，均为 dygraph，遍历 CPU 与当前后端设备（见 get_scatter_check_places）：TestZeroSizeAxisOutOfRangeError覆盖 1(a) 和 2：x为[0, 1]且 index 非空时断言IndexError，overwrite两种取值都测；index 为空时核对dX = dOut且dUpdates保持updates的形状。TestScatterUpdatesShapeError覆盖 1(b) 和 1(c)：9 组非法输入断言ValueError，按破坏的约束分组——尾部维度 4 组（空 index 2 组、非空 index 2 组）、0 维 index 1 组、长度不等 4 组（含 updates 更长和更短两个方向）；另有 4 组合法输入断言正常返回。p.s op_test.get_places() 在 CUDA 构建下不返回 CPUPlace（除非设 FLAGS_CI_both_cpu_and_gpu），所以这里get_scatter_check_places手动构造place列表，避免coverage测试中CPU kernel 的新增分支漏测。
test/legacy_test/test_swiglu.py为 4 新增 0-size 下dz形状不匹配的用例。本地实测（单卡，用
python <file>运行以执行__main__里的paddle.enable_static()）：被skipped的 1 个是 bf16 支持探测。
test_prim_sub_graph_pqrst_backward_dynamic_shape.py用于确认删除 prim 补零分支后分解路径仍然正确；test_lu_op.py/test_lu_unpack_op.py/test_inplace.py用于确认它们的 scatter 调用本来就是等长的。是否引起精度变化
否