Skip to content

fix: invoke NameFixPass after graph-modifying operations in rewriter, version-converter, and optimizer#2922

Open
gramalingam wants to merge 2 commits into
mainfrom
rama/rename-pass
Open

fix: invoke NameFixPass after graph-modifying operations in rewriter, version-converter, and optimizer#2922
gramalingam wants to merge 2 commits into
mainfrom
rama/rename-pass

Conversation

@gramalingam
Copy link
Copy Markdown
Collaborator

Problem

TapeBuilder creates values with explicit names that can clash with existing graph value names when nodes are inserted via replace_nodes_and_values. NameAuthority silently registers duplicate names without conflict checking, causing ValueError when the clashing value is an initializer.

Fix

Invoke NameFixPass conditionally (only when changes were actually made) at the end of the three graph-modifying callers:

  • RewriteRuleSet.apply_to_model -- runs when count > 0
  • _VersionConverter.visit_model -- runs when self._modified is True (set in replace_node)
  • FoldConstantsPass.call -- runs when self._modified is True

Each call site includes an explanatory comment and the _modified check is encapsulated within the class that owns it.

Tests

Added name-clash regression tests to all three test suites:

  • NameClashAfterRewriteTest in onnxscript/rewriter/pattern_test.py
  • NameClashAfterConversionTest in onnxscript/version_converter/_version_converter_test.py
  • NameClashAfterFoldTest in onnxscript/optimizer/_constant_folding_test.py

Each test triggers the operation, injects a duplicate value name to simulate what TapeBuilder can produce via NameAuthority, and asserts all value names are unique after the operation.

Known limitation

There is a pre-existing terminal NameFixPass in the optimizer pipeline (_optimizer.py), so optimize_ir may call NameFixPass up to 5 times on a 2-iteration run. This is acceptable overhead and is tracked as a follow-up to investigate deduplication.

gramalingam and others added 2 commits May 21, 2026 16:25
…alue names

TapeBuilder creates nodes with named values. When inserted into a graph via
replace_nodes_and_values, NameAuthority silently registers duplicate names
without conflict checking. This causes ValueError when the clashing value
is an initializer.

Invoke NameFixPass (the deduplication pass) conditionally -- only when
changes were actually made -- in the three callers:

- onnxscript/rewriter/_rewrite_rule.py: RewriteRuleSet.apply_to_model
  runs NameFixPass when count > 0
- onnxscript/version_converter/_version_converter.py: convert_version
  tracks _modified flag via replace_node and runs NameFixPass when set
- onnxscript/optimizer/_constant_folding.py: FoldConstantsPass.call
  runs NameFixPass when self._modified is True

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fix 1 (comments): add explanatory comments at all three NameFixPass call
sites explaining why TapeBuilder can produce clashing names.

Fix 2 (encapsulation): move NameFixPass call inside
_VersionConverter.visit_model so the private _modified flag is read
only within the class, matching the pattern in FoldConstantsPass.call.
Remove the now-redundant external check in convert_version.

Fix 3 (tests): add NameClashAfter*Test classes to all three test suites.
Each test:
- Parses/builds a model that triggers the operation (rewrite fires,
  version is converted, or constants are folded)
- Injects a duplicate value name on two surviving (non-pattern) values
  to simulate what TapeBuilder can produce via NameAuthority
- Asserts that all value names are unique after the operation, proving
  NameFixPass was invoked and resolved the duplicates

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented May 21, 2026

Codecov Report

❌ Patch coverage is 84.33735% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.67%. Comparing base (8fdb1e0) to head (53a187f).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
onnxscript/rewriter/pattern_test.py 82.14% 2 Missing and 3 partials ⚠️
...cript/version_converter/_version_converter_test.py 77.27% 2 Missing and 3 partials ⚠️
onnxscript/optimizer/_constant_folding_test.py 86.36% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2922      +/-   ##
==========================================
+ Coverage   72.64%   72.67%   +0.03%     
==========================================
  Files         259      259              
  Lines       31652    31735      +83     
  Branches     2980     3004      +24     
==========================================
+ Hits        22994    23064      +70     
- Misses       7649     7653       +4     
- Partials     1009     1018       +9     

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

Copy link
Copy Markdown
Contributor

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 prevents value-name collisions after graph-modifying operations by conditionally running onnx_ir.passes.common.NameFixPass at the end of three call sites (rewriter, version converter, constant folding). This addresses cases where TapeBuilder can introduce pre-named values that collide with existing graph value/initializer names during replace_nodes_and_values.

Changes:

  • Run NameFixPass after rewrites when at least one rewrite fired (count > 0).
  • Track _modified in the version converter and invoke NameFixPass at the end of conversion only when replacements occurred.
  • Invoke NameFixPass after constant folding only when the pass modified the model, and add regression tests covering all three pathways.

Reviewed changes

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

Show a summary per file
File Description
onnxscript/version_converter/_version_converter.py Tracks whether node replacements occurred and runs NameFixPass after conversion only when modified.
onnxscript/version_converter/_version_converter_test.py Adds a regression test that injects a duplicate value name and asserts conversion deduplicates names.
onnxscript/rewriter/_rewrite_rule.py Runs NameFixPass after applying rewrites when at least one rewrite fired.
onnxscript/rewriter/pattern_test.py Adds a regression test asserting apply_to_model results in unique value names after an injected clash.
onnxscript/optimizer/_constant_folding.py Runs NameFixPass after folding when the model was modified.
onnxscript/optimizer/_constant_folding_test.py Adds a regression test asserting fold_constants results in unique value names after an injected clash.

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

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

3 participants