Skip to content

Conversation

justinchuby
Copy link
Member

@justinchuby justinchuby commented Aug 15, 2025

Previously when an initializer in a sub-graph is an output to the subgraph, the pass would attempt to lift the initializer. Since the value still belongs to the subgraph, this is invalid.

(Q: Can an initializer be an output to a graph? For now I assume yes.)

This change inlcludes fixes:

  1. When an initializer is an output, we will not lift it to the main graph
  2. Fixed infer_attribute_type in the convenience module to correctly handle empty Graph. Previously it was treated as an empty sequence (and became INTS)
  3. Refined the logic for deduplicating names when lifting initializers.

BC Breaking

When an attribute is an empty list, users need to create an ir.Attr explicitly because the type would otherwise be ambiguous. Previously it was defaulted to be INTS, which is not a behavior by design.

Fixes microsoft/onnxscript#2493

Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Copilot

This comment was marked as outdated.

@justinchuby justinchuby changed the title Fix naming conflicts in constant manipulation Fix initializer handling constant manipulation pass Aug 15, 2025
Copy link

codecov bot commented Aug 15, 2025

Codecov Report

❌ Patch coverage is 60.71429% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.28%. Comparing base (f98ea79) to head (8bef6d6).
⚠️ Report is 6 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/onnx_ir/_convenience/__init__.py 64.70% 2 Missing and 4 partials ⚠️
src/onnx_ir/passes/common/constant_manipulation.py 54.54% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #148      +/-   ##
==========================================
- Coverage   76.41%   76.28%   -0.13%     
==========================================
  Files          40       40              
  Lines        4723     4731       +8     
  Branches      932      936       +4     
==========================================
  Hits         3609     3609              
- Misses        824      828       +4     
- Partials      290      294       +4     

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

@justinchuby justinchuby changed the title Fix initializer handling constant manipulation pass Fix initializer handling in LiftSubgraphInitializersToMainGraphPass pass Aug 15, 2025
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
@justinchuby justinchuby requested a review from Copilot August 15, 2025 18:47
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 fixes a bug in the LiftSubgraphInitializersToMainGraphPass where the pass would incorrectly attempt to lift initializers that are outputs of subgraphs, which is invalid since these values still belong to the subgraph.

  • Added a check to prevent lifting initializers that are subgraph outputs
  • Fixed the infer_attribute_type function to properly handle empty sequences instead of defaulting to INTS
  • Improved the name deduplication logic for lifted initializers to ensure unique naming

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/onnx_ir/passes/common/constant_manipulation.py Added output check and fixed name deduplication logic in the lift pass
src/onnx_ir/_convenience/__init__.py Fixed empty sequence handling in attribute type inference
src/onnx_ir/passes/common/constant_manipulation_test.py Added test case for the new output initializer handling

Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
@justinchuby justinchuby merged commit 24f2968 into main Aug 15, 2025
23 checks passed
@justinchuby justinchuby deleted the justinchu/fix-constant-manipulation-naming branch August 15, 2025 18:58
gramalingam pushed a commit to microsoft/onnxscript that referenced this pull request Aug 19, 2025
Due to changes onnx/ir-py#148, we cannot create
an empty list attribute without specifying type because it would be
ambiguous.

Fix #2496

Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimizer error on quantized model
2 participants