[Rewriter] Implement zero bias removal for Conv operations and related rules #2555

whyvineet · 2025-09-09T19:40:51Z

…d rules

whyvineet · 2025-09-09T19:44:52Z

@whyvineet please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
@microsoft-github-policy-service agree [company="{your company}"]
Options:

(default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
(when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"
Contributor License Agreement

@microsoft-github-policy-service agree

whyvineet · 2025-09-09T19:58:31Z

Hey @justinchuby, can you review this?

justinchuby · 2025-09-09T21:45:01Z

@AyoubMDL would you be able to help review/critique? Thanks!

onnxscript/rewriter/rules/common/_remove_zero_bias.py

onnxscript/rewriter/rules/common/_remove_zero_bias_test.py

…ve-optional-bias

…nd QLinearConv operations with additional tests

whyvineet · 2025-09-10T17:59:05Z

@AyoubMDL I made the changes you suggested...

justinchuby · 2025-09-10T20:14:26Z

@whyvineet please allow reviewers to resolve comments so they get a chance to validate the changes. Thanks.

onnxscript/rewriter/rules/common/_remove_zero_bias.py

onnxscript/rewriter/rules/common/_remove_zero_bias_test.py

onnxscript/rewriter/rules/common/_remove_zero_bias.py

…tructure

…ve-optional-bias

whyvineet · 2025-09-10T21:59:52Z

@AyoubMDL I've made the changes you suggested, except for the last one... After getting further clarity, I'll start working on that as well...

onnxscript/rewriter/rules/common/_remove_zero_bias.py

onnxscript/rewriter/__init__.py

onnxscript/rewriter/rules/common/_remove_zero_bias.py

onnxscript/rewriter/rules/common/_remove_zero_bias_test.py

codecov · 2025-09-11T16:45:07Z

Codecov Report

❌ Patch coverage is 93.93939% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.94%. Comparing base (f529292) to head (ce64fb7).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...pt/rewriter/rules/common/_remove_zero_bias_test.py	92.94%	2 Missing and 4 partials ⚠️
...xscript/rewriter/rules/common/_remove_zero_bias.py	95.45%	1 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (f529292) and HEAD (ce64fb7). Click for more details.

HEAD has 21 uploads less than BASE

Flag BASE (f529292) HEAD (ce64fb7)

42 21

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2555      +/-   ##
==========================================
- Coverage   70.33%   63.94%   -6.39%     
==========================================
  Files         222      224       +2     
  Lines       26656    26786     +130     
  Branches     2666     2679      +13     
==========================================
- Hits        18748    17128    -1620     
- Misses       6992     8811    +1819     
+ Partials      916      847      -69

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…maintainability

…ance clarity in Conv, ConvTranspose, QLinearConv, and Gemm operations

whyvineet · 2025-09-11T17:57:49Z

@AyoubMDL, I have completed the modifications recommended by you...

AyoubMDL

Thanks ! some final remarks.

onnxscript/rewriter/rules/common/_remove_zero_bias.py

AyoubMDL · 2025-09-12T14:24:52Z

onnxscript/rewriter/rules/common/_remove_zero_bias.py

+        original_inputs = list(node.inputs)
+        inputs_without_bias = original_inputs[:-1]


With my suggestion on Gemm, you can remove this. And pass x, w as arguments to the rewrite

instead of hardcoding x and w parameters, the base class can dynamically handle any number of inputs by filtering out just the bias parameter ('b') and keeping everything else in the new changes made by me.

onnxscript/rewriter/rules/common/_remove_zero_bias.py

AyoubMDL · 2025-09-12T14:31:42Z

onnxscript/rewriter/rules/common/_remove_zero_bias_test.py

Maybe you can pass some attributes when testing to check that every info is correctly transferred (e.g. stride, transA...)

…ias removal logic

…e attributes in Conv, ConvTranspose, Gemm, and QLinearConv operations

whyvineet · 2025-09-14T11:13:51Z

@AyoubMDL, is there anything else I should update or improve?

justinchuby · 2025-09-14T15:48:24Z

onnxscript/rewriter/rules/common/_remove_zero_bias.py

-        inputs_without_bias = original_inputs[:-1]
+        # Filter out the bias parameter and keep all other inputs
+        inputs = []
+        for param_name, param_value in _.items():


I understand now... There was just a misunderstanding on my part!

…ng efficiency in _RemoveZeroBiasBase class

AyoubMDL

The implementation looks good to me. I'll let @justinchuby share additional comments

onnxscript/rewriter/rules/common/_remove_zero_bias.py

…oBiasBase class

whyvineet · 2025-09-15T17:32:41Z

The implementation looks good to me. I'll let @justinchuby share additional comments

Thanks for the guidance @AyoubMDL! And I don't know how I missed domain=node.domain from the last review... I removed it now!

whyvineet · 2025-09-16T17:30:26Z

@justinchuby, can you explain why these checks are failing? I reviewed the logs but didn't gain much clarity...

justinchuby · 2025-09-16T17:36:34Z

onnxscript/rewriter/rules/common/_remove_zero_bias_test.py

+)
+
+
+def _apply_rule_and_check_optimization(


This test is a little too complicated to maintain. Please check other tests in the directory and simplify.

justinchuby · 2025-09-16T17:37:45Z

The error is saying the Sequence[int] attributes are supposed to be tuples but the expected values are lists. You can check where you provide the expected outputs.

justinchuby · 2025-09-16T17:38:13Z

This line

onnxscript/rewriter/rules/common/_remove_zero_bias_test.py:76: in _apply_rule_and_check_optimization
assert actual_value == expected_value, (

…t/onnxscript into remove-optional-bias

… and improved clarity

justinchuby · 2025-09-21T17:52:22Z

onnxscript/rewriter/onnxruntime/bfloat16_utils/bfloat16_converter_test.py

-        self.v1.dtype = ir.DataType.BFLOAT16
-        self.v2 = ir.val(name="v2", shape=ir.Shape([2, 3, 4]))
-        self.v2.dtype = ir.DataType.BFLOAT16
+        self.v0 = ir.Value(


Please revert. ir.val is a convenience function.

It was failing test cases... Okh I'll do so!

justinchuby · 2025-09-21T17:56:24Z

Please use judgement before applying AI edits, and please engage the reviewers if you have clarification questions. It is preferred if you understand what the edits should be before spending time doing them. Have a discussion and lay out your thought process before taking actions. Pure AI edits that don't help with addressing the issues raised in reviews and is a waste of everyone's time.

…sistency and improved clarity" This reverts commit 86de85f.

…tuple differences

whyvineet · 2025-09-21T18:18:12Z

Please use judgement before applying AI edits, and please engage the reviewers if you have clarification questions. It is preferred if you understand what the edits should be before spending time doing them. Have a discussion and lay out your thought process before taking actions. Pure AI edits that don't help with addressing the issues raised in reviews and is a waste of everyone's time.

I understand your point and will make sure to carefully consider the purpose of edits and engage reviewers before applying changes in future commits and PRs.

[Rewriter] Implement zero bias removal for Conv operations and relate…

6cfd6fd

…d rules

github-project-automation bot added this to ONNX Script Review Board Sep 9, 2025

github-project-automation bot moved this to Todo in ONNX Script Review Board Sep 9, 2025

Merge branch 'microsoft:main' into remove-optional-bias

48037a8

AyoubMDL reviewed Sep 10, 2025

View reviewed changes

whyvineet added 2 commits September 10, 2025 22:48

Merge branch 'main' of github-personal:whyvineet/onnxscript into remo…

fd028ee

…ve-optional-bias

[Rewriter] Enhance zero bias removal for Conv, ConvTranspose, Gemm, a…

3742e7b

…nd QLinearConv operations with additional tests

AyoubMDL reviewed Sep 10, 2025

View reviewed changes

whyvineet added 2 commits September 11, 2025 03:24

Refactor zero bias removal tests to use helper function and improve s…

8bfa65f

…tructure

Merge branch 'main' of github-personal:whyvineet/onnxscript into remo…

6fc3ca7

…ve-optional-bias

github-advanced-security bot found potential problems Sep 11, 2025

View reviewed changes

onnxscript/rewriter/__init__.py Fixed Show fixed Hide fixed

onnxscript/rewriter/rules/common/_remove_zero_bias.py Fixed Show fixed Hide fixed

onnxscript/rewriter/rules/common/_remove_zero_bias_test.py Fixed Show fixed Hide fixed

whyvineet added 3 commits September 11, 2025 23:25

Refactor test cases for zero bias removal to improve readability and …

b93a56c

…maintainability

Remove duplicate import of _fuse_batchnorm in rewriter module

e322625

Refactor zero bias removal logic to streamline input handling and enh…

121360e

…ance clarity in Conv, ConvTranspose, QLinearConv, and Gemm operations

AyoubMDL reviewed Sep 12, 2025

View reviewed changes

whyvineet added 2 commits September 14, 2025 16:17

Refactor Gemm operation pattern and check method to align with zero b…

ee7dafa

…ias removal logic

Enhance zero bias removal logic to filter bias parameters and preserv…

8ef6c41

…e attributes in Conv, ConvTranspose, Gemm, and QLinearConv operations

justinchuby reviewed Sep 14, 2025

View reviewed changes

Refactor bias removal logic to directly use operation inputs, improvi…

2b9dda4

…ng efficiency in _RemoveZeroBiasBase class

AyoubMDL reviewed Sep 15, 2025

View reviewed changes

onnxscript/rewriter/rules/common/_remove_zero_bias.py Outdated Show resolved Hide resolved

Remove redundant domain attribute from operation inputs in _RemoveZer…

153b4e7

…oBiasBase class

justinchuby reviewed Sep 16, 2025

View reviewed changes

Merge branch 'main' into remove-optional-bias

ce64fb7

whyvineet added 2 commits September 21, 2025 14:40

Merge HEAD, branch 'remove-optional-bias' of github-personal:whyvinee…

a94f8b9

…t/onnxscript into remove-optional-bias

Refactor IR value creation in tests to use ir.Value for consistency…

86de85f

… and improved clarity

justinchuby reviewed Sep 21, 2025

View reviewed changes

whyvineet added 2 commits September 21, 2025 23:43

Revert "Refactor IR value creation in tests to use ir.Value for con…

d62eafb

…sistency and improved clarity" This reverts commit 86de85f.

Enhance attribute comparison in optimization tests to handle list vs …

d4f73dd

…tuple differences

		original_inputs = list(node.inputs)
		inputs_without_bias = original_inputs[:-1]

		)


		def _apply_rule_and_check_optimization(

[Rewriter] Implement zero bias removal for Conv operations and related rules #2555

Are you sure you want to change the base?

[Rewriter] Implement zero bias removal for Conv operations and related rules #2555

Uh oh!

Conversation

whyvineet commented Sep 9, 2025

Uh oh!

whyvineet commented Sep 9, 2025

Uh oh!

whyvineet commented Sep 9, 2025

Uh oh!

justinchuby commented Sep 9, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

whyvineet commented Sep 10, 2025

Uh oh!

justinchuby commented Sep 10, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

whyvineet commented Sep 10, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

whyvineet commented Sep 11, 2025

Uh oh!

AyoubMDL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

whyvineet commented Sep 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AyoubMDL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

whyvineet commented Sep 15, 2025

Uh oh!

whyvineet commented Sep 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinchuby commented Sep 16, 2025

Uh oh!

justinchuby commented Sep 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinchuby commented Sep 21, 2025

Uh oh!

codecov bot commented Sep 11, 2025 •

edited

Loading