Skip to content

Conversation

leshabirukov
Copy link
Contributor

@leshabirukov leshabirukov commented Apr 17, 2025

Fix #2211

This pull request enhances the functionality of the RemoveUnusedNodesPass class and its associated methods by introducing an option to remove unused initialized inputs. It also updates the corresponding tests to validate this new behavior. The changes improve the flexibility of the unused node removal process and ensure the model input signature remains consistent unless explicitly modified.

Enhancements to RemoveUnusedNodesPass:

  • Added a new remove_initialized_inputs attribute to the RemoveUnusedNodesPass class, allowing the removal of unused initialized inputs when enabled. This change modifies the model input signature if unused inputs are removed (onnxscript/ir/passes/common/unused_removal.py).

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Let's affect only inputs that are initializers
@justinchuby
Copy link
Collaborator

Looks good to me, thanks! Maybe create an option in the initializer so users can control?

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
[::-1]->reversed

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
trailing spaces remove
@leshabirukov
Copy link
Contributor Author

Looks good to me, thanks! Maybe create an option in the initializer so users can control?

May be. There are three alternatives:
do not remove inputs
remove all unused inputs
remove unused inputs-initializers

How about parameter remove_unused_inputs with default=False
True -> remove unused inputs-initializers

Copy link

codecov bot commented Apr 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.62%. Comparing base (feb20f1) to head (d12b6b2).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2212      +/-   ##
==========================================
+ Coverage   73.60%   73.62%   +0.02%     
==========================================
  Files         227      227              
  Lines       29052    29080      +28     
  Branches     3327     3330       +3     
==========================================
+ Hits        21383    21411      +28     
  Misses       6544     6544              
  Partials     1125     1125              

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

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>
@justinchuby
Copy link
Collaborator

I think option three (remove unused inputs-initializers) is a good balance between correctness and user expectation. It sounds like a good idea to default it to True.

@justinchuby
Copy link
Collaborator

Maybe call the option remove_initialized_inputs?

@justinchuby justinchuby changed the title Update unused_removal.py [pass] Remove unused initialized inputs Apr 17, 2025
@justinchuby justinchuby changed the title [pass] Remove unused initialized inputs [pass] Remove unused initialized inputs in DCE Apr 17, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
RemoveUnusedNodesPass + parameter remove_initialized_inputs

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
remove_unused_noodles + parameter remove_initialized_inputs

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
tests for issue microsoft#2211
@leshabirukov
Copy link
Contributor Author

@microsoft-github-policy-service agree

@justinchuby
Copy link
Collaborator

https://github.com/onnx/onnx/blob/baae427e70c27d740f5bea859111e3b63d33512b/onnx/onnx.proto#L72C1-L74C25 mentions initializers does not have to follow inputs. I need to think about the default behavior a little more

@justinchuby
Copy link
Collaborator

onnx/onnx@baae427/onnx/onnx.proto#L72C1-L74C25 mentions initializers does not have to follow inputs. I need to think about the default behavior a little more

After thinking about this, I suggest to keep the same logic, but default the option to False.

Copy link
Contributor Author

@leshabirukov leshabirukov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How I can approve it? It grayed


ok

leshabirukov and others added 2 commits April 21, 2025 12:26

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
For API compatibility!

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>
leshabirukov and others added 2 commits April 22, 2025 18:37

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>
@justinchuby
Copy link
Collaborator

Final round of suggestions. Thanks!

leshabirukov and others added 2 commits April 22, 2025 18:38

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>
Copy link
Contributor Author

@leshabirukov leshabirukov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aaand...

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
parametrization

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Copy link
Collaborator

@justinchuby justinchuby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for making the contribution!

@justinchuby justinchuby requested a review from Copilot April 22, 2025 17:25
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances the unused node removal functionality by introducing a configurable option to remove unused initialized inputs. Key changes include:

  • Updating the optimizer’s remove_unused_nodes function to accept a new remove_initialized_inputs parameter.
  • Modifying tests in unused_removal_test.py to verify that unused initializer inputs are removed when requested.
  • Updating the RemoveUnusedNodesPass to conditionally remove graph inputs matching unused initializers.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
onnxscript/optimizer/init.py Modified call signature to pass remove_initialized_inputs parameter
onnxscript/ir/passes/common/unused_removal_test.py Added tests to verify removal of unused initialized inputs
onnxscript/ir/passes/common/unused_removal.py Updated pass implementation to remove corresponding graph inputs
Comments suppressed due to low confidence (1)

onnxscript/ir/passes/common/unused_removal.py:113

  • [nitpick] Consider renaming the variable 'input' to avoid shadowing the built-in function 'input'.
for i, input in reversed(list(enumerate(graph_inputs))):

@justinchuby
Copy link
Collaborator

I think there is a format change required you can fix by running lintrunner f

@leshabirukov
Copy link
Contributor Author

Errr...
What about this one?
=================================== FAILURES ===================================
_ TestOnnxBackEnd.test_export2python_produces_correct_onnx_script_model_0397_test_ai_onnx_ml_tree_ensemble_set_membership _
[gw0] linux -- Python 3.11.12 /home/runner/work/onnxscript/onnxscript/.nox/test_ort_nightly/bin/python
onnxscript/converter.py:460: in _eval_constant_expr
return eval(cpl, self.globals, locals) # pylint: disable=eval-used
E NameError: name 'nan' is not defined

The above exception was the direct cause of the following exception:
.nox/test_ort_nightly/lib/python3.11/site-packages/parameterized/parameterized.py:620: in standalone_func
return func(*(a + p.args), **p.kwargs, **kw)

@justinchuby
Copy link
Collaborator

Those checks are fine. They are unrelated.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@justinchuby justinchuby enabled auto-merge (squash) April 22, 2025 19:37
@justinchuby justinchuby merged commit 6d33d22 into microsoft:main Apr 23, 2025
25 of 29 checks passed
@leshabirukov leshabirukov deleted the patch-1 branch April 23, 2025 11:58
@leshabirukov
Copy link
Contributor Author

Hurray!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

remove_unused_nodes: removing inputs
2 participants