Repository navigation
Fix bias arithmetic in Conv2d BatchNorm fusion - #2528
Open
AaravMehta-07 wants to merge 1 commit into
Open
AaravMehta-07 wants to merge 1 commit into
AaravMehta-07 wants to merge 1 commit into
Conversation
Assisted-by: OpenAI Codex
Author
|
Hi maintainers, whenever you have a chance, I'd appreciate a review of this fix for preserving bias arithmetic in Conv2d/BatchNorm fusion. The eight focused numerical fusion tests passed locally, including float64 and non-affine cases. Please let me know if you'd prefer a different approach or additional focused coverage. Thank you for your time. AI-assisted follow-up. |
2 of 3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Preserve inference outputs when fusing an eval
BatchNorm2dintoConv2d.Two bias terms currently break computation preservation:
(conv_bias - running_mean) * gamma / sqrt(running_var + eps) + beta, rather than leavingconv_biasunscaled.affine=False, BatchNorm has no additive beta term, so its replacement must be zeros, not ones. The existing fallback adds an unintended constant offset to every output.Only those two production lines change. Linear/BatchNorm1d fusion, graph eligibility, training-mode behavior and dependency pins are untouched.
Validation
0df84f23replay: 6 failures, 2 passing controls in the new Conv2d tests.git diff --checkpasses.Local environment: Windows, Python 3.11.9, Torch 2.13.0+cpu, Transformers 5.19.0. No pretrained model downloads, mocks of inference/fusion, GPU benchmark or speed claim. Full legacy FX tests were attempted but could not collect because they import the removed
transformers.utils.fxmodule; the new tests use actualtorch.fx.symbolic_traceand run offline. This is not a full-suite claim.Additional isolated legacy compatibility run with Transformers 4.57.6, tokenizers 0.22.2 and Hub 0.36.2: the eight new cases plus the existing offline
test_get_parentpass (9 passed, 16 deselected). This overlay is not the current main dependency set and does not change repository pins; no pretrained-model tests were run.Live duplicate checks for
FuseBatchNorm,BatchNorm2d,bn2dand affine fusion found no matching active fix. The original batchnorm-folding PR #348 is already merged; this corrects its bias arithmetic, not a second implementation.Before submitting
AI disclosure: investigated, implemented and tested with OpenAI Codex, independently reviewed/tested by a GPT-6 Luna worker and root. An initial wider linear-fusion candidate was dropped after review identified an unsupported 3D eligibility risk; this PR contains only the verified Conv2d correction. No personal human review/test attestation is claimed.