Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Jul 11, 2025

This PR implements a NameFixPass in src/onnx_ir/passes/common/naming.py to ensure that all values and nodes in an ONNX IR model have unique names according to the specified policy:

  1. Graph inputs and outputs have precedence - Their names are preserved when possible, and only renamed if duplicates exist
  2. Scoped unique names: Names in subgraphs are unique within their naming scopes and their parent scopes.
  3. Subgraph inputs/outputs are processed
  4. Support custom naming generators with generate_node_name and generate_value_name params.

I create the enter_graph and exit_graph callbacks on the RecursiveIterator to allow users to handle getting into and out of graph scopes.

@justinchuby
Copy link
Member

You should maintain a ir.Value to assigned name dictionary to ensure that a value is only assigned a name once. Alternatively maintain a set of seen values.

Co-authored-by: justinchuby <11205048+justinchuby@users.noreply.github.com>
@Copilot Copilot AI changed the title [WIP] Create a name fix pass Create a name fix pass to ensure unique names for all values and nodes Jul 11, 2025
@Copilot Copilot AI requested a review from justinchuby July 11, 2025 22:14
Copilot finished work on behalf of justinchuby July 11, 2025 22:14
Copy link

codecov bot commented Jul 11, 2025

Codecov Report

❌ Patch coverage is 87.02290% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.40%. Comparing base (4a775d0) to head (8f213bf).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/onnx_ir/passes/common/naming.py 88.79% 6 Missing and 7 partials ⚠️
src/onnx_ir/traversal.py 71.42% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #124      +/-   ##
==========================================
+ Coverage   76.03%   76.40%   +0.36%     
==========================================
  Files          39       40       +1     
  Lines        4586     4717     +131     
  Branches      899      929      +30     
==========================================
+ Hits         3487     3604     +117     
- Misses        818      827       +9     
- Partials      281      286       +5     

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

…racking

Co-authored-by: justinchuby <11205048+justinchuby@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Jul 11, 2025

You should maintain a ir.Value to assigned name dictionary to ensure that a value is only assigned a name once. Alternatively maintain a set of seen values.

I've updated the implementation to use a dict[ir.Value, str] mapping instead of a set to track processed values. This ensures each value is assigned a name exactly once and makes the intent clearer. The dictionary explicitly maps each ir.Value to its final assigned name. Commit: b979a3d

Copilot finished work on behalf of justinchuby July 11, 2025 22:23
Copy link
Contributor

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

lintrunner found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
@justinchuby justinchuby linked an issue Jul 23, 2025 that may be closed by this pull request
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
@justinchuby justinchuby marked this pull request as ready for review July 28, 2025 23:46
@justinchuby justinchuby requested review from titaiwangms and a team as code owners July 28, 2025 23:46
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>
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>
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>
@justinchuby justinchuby requested a review from gramalingam August 8, 2025 14:08
@justinchuby

This comment was marked as resolved.

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>
@justinchuby justinchuby merged commit aab601f into main Aug 11, 2025
23 checks passed
@justinchuby justinchuby deleted the copilot/fix-123 branch August 11, 2025 16:46
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.

Create a name-fix pass
4 participants