Repository navigation
[BugFix] Fix SyncBatchNorm.convert_sync_batchnorm dropping use_global_stats=True - #79944
sibisrinivasb wants to merge 2 commits into
Conversation
…_stats=True A BatchNorm*d layer created with use_global_stats=True is kept unchanged by the conversion: it always normalizes with its stored mean and variance, so it has no batch statistics to synchronize, and SyncBatchNorm does not support use_global_stats.
|
|
|
|
Paddle-Bot Review Board （review完成)
可维护性建议（总分 89.8 / 100，只给建议，不影响评审结论）
各维度得分（仅评本 PR 改动，点击展开）
Powered by Nyanpasu claude with Opus 5.5 默认推理级别, please check the suggestions carefully. |
| # its stored mean and variance and never updates them, so it has no | ||
| # batch statistics to synchronize. SyncBatchNorm does not support | ||
| # use_global_stats, so such a layer is kept as it is. | ||
| if isinstance(layer, _BatchNormBase) and not layer._use_global_stats: |
There was a problem hiding this comment.
paddle.sparse.nn.SyncBatchNorm.convert_sync_batchnorm（python/paddle/sparse/nn/layer/norm.py:376）是本方法的近似拷贝，仍只判断 isinstance(layer, _BatchNormBase)。它同样会转换 dense BatchNorm*d（else 分支构造 paddle.nn.SyncBatchNorm），而 sparse BatchNorm 也保存 use_global_stats（同文件 117/145 行）。因此用户改用 sparse 入口转换含 BatchNorm2D(use_global_stats=True) 或 paddle.sparse.nn.BatchNorm(use_global_stats=True) 的模型时，仍会被替换为 SyncBatchNorm，训练时改用 batch 统计量并覆盖 mean/variance，与本 PR 的修复和新增 Note 不一致。
建议在 sparse 版的判断上同步加 and not layer._use_global_stats，补同样的 docstring Note，并在 test/legacy_test/test_sparse_norm_op.py 增加对应用例。
There was a problem hiding this comment.
Done for dense layers, in 4723bca: paddle.sparse.nn.SyncBatchNorm.convert_sync_batchnorm now keeps a dense BatchNorm*d(use_global_stats=True) layer, with the same Note and a test in test_sparse_norm_op.py (it fails before the change and passes after).
I did not apply the condition to paddle.sparse.nn.BatchNorm layers, for this reason: its forward writes self._use_global_stats = not self.training on the first call (python/paddle/sparse/nn/layer/norm.py, lines 145-146). On 3.3.1, a layer built without the option reads None when new, True after one forward in eval mode and False after one forward in train mode. So at conversion time the attribute does not tell whether the layer was created with use_global_stats=True, and the same condition would also skip layers that only ran once in eval mode. The Note states that sparse BatchNorm layers are always converted (unchanged behaviour).
That side effect looks like a separate problem: a sparse BatchNorm whose first forward was in eval mode no longer updates its statistics in train mode (stored mean moved by 0.0, against 0.0121 for a layer whose first forward was in train mode). I can open a separate issue / PR for it, after which the sparse case can use the same condition. If you prefer it in this PR, tell me.
There was a problem hiding this comment.
已确认 4723bca 的修改：sparse 入口现在会保留 dense BatchNorm*d(use_global_stats=True)，并有对应用例。关于 paddle.sparse.nn.BatchNorm，forward 在 _use_global_stats is None 时会改写该属性（python/paddle/sparse/nn/layer/norm.py:145-146），转换时确实无法区分构造参数，保持转换并在 Note 中说明是合理的。这个副作用可以单独开 issue/PR 跟进。此问题标记为已解决。
| Helper function to convert :class: `paddle.nn.BatchNorm*d` layers in the model to :class: `paddle.nn.SyncBatchNorm` layers. | ||
|
|
||
| Note: | ||
| A `BatchNorm*d` layer created with ``use_global_stats=True`` is kept unchanged. It always |
There was a problem hiding this comment.
💡 可维护性建议（契约一致性，不阻塞合入）：新增 Note 说明 use_global_stats=True 的层保持不变，但同一 docstring 的 Returns（1812 行）仍写 “If BatchNorm*d layer in the model, use SyncBatchNorm layer instead”，即无条件替换，二者矛盾。建议同步改写 Returns，例如：“The original model with BatchNorm*d layers converted to SyncBatchNorm layers, except layers created with use_global_stats=True, which are returned unchanged.”
There was a problem hiding this comment.
Done in 4723bca, with your wording. The Returns section of the sparse entry is updated the same way.
There was a problem hiding this comment.
已确认 dense 与 sparse 两个入口的 Returns 都已与 Note 一致，标记为已解决。
…_sync_batchnorm; update the Returns docs Review follow-up. The Returns section of paddle.nn.SyncBatchNorm.convert_sync_batchnorm now states the exception. paddle.sparse.nn.SyncBatchNorm.convert_sync_batchnorm keeps a dense BatchNorm*d layer created with use_global_stats=True. Sparse BatchNorm layers are still always converted: their forward rewrites _use_global_stats on the first call, so the attribute does not tell whether the layer was created with use_global_stats=True.
PR Category
User Experience
PR Types
Bug fixes
Description
Fixes #79943
SyncBatchNorm.convert_sync_batchnormreplaced everyBatchNorm*dlayer, including layers created withuse_global_stats=True.SyncBatchNormdoes not supportuse_global_stats, so after the conversion such a layer normalized with batch statistics in train mode and overwrote its stored mean / variance on every forward pass, with no warning. A partially frozen model silently changed behaviour.This PR keeps a layer with
use_global_stats=Trueunchanged in the conversion. It always normalizes with its stored statistics, so it has no batch statistics to synchronize. All other layers are converted as before.Tests added to
test_sync_batch_norm_op_convert.py:BatchNorm2D(use_global_stats=True)is the same layer after the conversion, while a plainBatchNorm2Din the same model becomes aSyncBatchNorm;Both tests fail before the change and pass after it. They do not run a
SyncBatchNormforward, so they also pass on CPU.An alternative is to support
use_global_statsinsideSyncBatchNormand carry it through the conversion. I chose the smaller change; I can switch to the other one if you prefer.Review follow-up (4723bca):
paddle.sparse.nn.SyncBatchNorm.convert_sync_batchnormalso keeps a denseBatchNorm*d(use_global_stats=True)layer (test added totest_sparse_norm_op.py). SparseBatchNormlayers are still always converted, because theirforwardrewrites_use_global_statson the first call; that needs a separate fix.是否引起精度变化
否 - no operator or kernel computation is changed. The only behaviour change is the bug path described above: a
BatchNorm*d(use_global_stats=True)layer is no longer converted, so it keeps using its stored statistics.