Fix initializer-safe bulk renaming and NameFixPass initializer iteration#369
Fix initializer-safe bulk renaming and NameFixPass initializer iteration#369
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes initializer-safe renaming in the ONNX IR by introducing a bulk rename helper that can handle swaps/permutations of initializer-backed values, and by making NameFixPass robust to initializer dict-key mutation during renames.
Changes:
- Add
onnx_ir.convenience.rename_values()(implemented in_convenience) to perform bulk renames safely for initializer-backed values via temporary names. - Update
NameFixPassto iterate over a snapshot of initializer values to avoidRuntimeErrorwhen initializer dict keys change during the pass. - Add regression tests covering initializer swaps and initializer-name collisions during
NameFixPass.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/onnx_ir/passes/common/naming_test.py | Adds regression test ensuring initializer collisions don’t mutate the initializer dict during iteration. |
| src/onnx_ir/passes/common/naming.py | Snapshots initializers.values() to avoid dict mutation during iteration. |
| src/onnx_ir/convenience.py | Re-exports rename_values as part of the public convenience API. |
| src/onnx_ir/_convenience/_convenience_test.py | Adds test verifying initializer-backed value swaps work with bulk renaming. |
| src/onnx_ir/_convenience/init.py | Implements rename_values bulk helper with initializer-safe temporary naming. |
You can also share your feedback on Copilot code review. Take the survey.
justinchuby
left a comment
There was a problem hiding this comment.
To rename initializers, it is possible to simply pop the initializers, rename, and put them back. Renaming other values is quite straightforward (just set the name field). I think the logic can be simplified.
|
I accidentally opened #370; it is closed now. The review fixes are pushed here. |
Applied in I simplified This also removed the temporary-name permutation logic and the |
|
Could you fix DCO? Thanks |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #369 +/- ##
==========================================
- Coverage 80.04% 80.02% -0.02%
==========================================
Files 52 52
Lines 6394 6444 +50
Branches 1294 1314 +20
==========================================
+ Hits 5118 5157 +39
- Misses 912 917 +5
- Partials 364 370 +6 ☔ View full report in Codecov by Sentry. |
Signed-off-by: enpasos <matthias.unverzagt@enpasos.com>
Signed-off-by: enpasos <matthias.unverzagt@enpasos.com>
Agree. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: enpasos <matthias.unverzagt@enpasos.com>
Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com> Signed-off-by: enpasos <matthias.unverzagt@enpasos.com>
Signed-off-by: enpasos <matthias.unverzagt@enpasos.com>
Done. |
Signed-off-by: enpasos <matthias.unverzagt@enpasos.com>
Summary
Fixes #353.
This PR addresses two related rename failures:
NameFixPasscan raiseRuntimeError: dictionary keys changed during iterationwhen a rename changes initializer dict keys while the pass is iterating them.Changes
onnx_ir.convenience.rename_values()for bulk value renamingNameFixPassiterate over a snapshot of initializer values instead of the live dict viewNameFixPassWhy
A single
value.name = ...rename is not enough for safe initializer swaps, because the initializer name guard correctly rejects taking another live initializer name. The new bulk helper provides the missing multi-rename operation, while theNameFixPasschange makes renaming safe when initializer dict keys are updated during the pass.Validation
pytest -q src/onnx_ir/_convenience/_convenience_test.py -k rename_valuespytest -q src/onnx_ir/passes/common/naming_test.py -k initializer_collisionReported from downstream integration work in jax2onnx.