-
Notifications
You must be signed in to change notification settings - Fork 645
Fix ONNX node name sanitization and add ai.onnx.ml domain support #3371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Sanitizes node names containing invalid identifier characters like ':' and '/' by replacing them with underscores, preventing panic during ONNX import.
The check_opset_version function now supports both the standard ONNX ('') and ML ('ai.onnx.ml') domains, panicking for unsupported domains. This improves compatibility with models using ML operators.
The name sanitization is happening in burn-import instead of onnx-ir. This is simpler for now and updates existing sanitization in place related. Added a todo to come back. #3208 PR attempted to fix in onnx-ir but added up introducing new issues because some names relied on old unsanitized names thus breaking code. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3371 +/- ##
==========================================
- Coverage 35.19% 30.90% -4.29%
==========================================
Files 342 413 +71
Lines 53164 60529 +7365
==========================================
- Hits 18712 18709 -3
- Misses 34452 41820 +7368 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix looks good, just a minor comment for the implementation 🙂
What's the motivation behind extending support for ai.onnx.ml
domain? We would need a separate list for these ops, which I'm not sure is desirable.
Sorry I should have explained. TensorFlow adds this when exporting even though no ops are used. I have discovered this when testing an onnx file in #2878 |
Simplifies and improves the logic for sanitizing names to valid Rust identifiers. The new implementation replaces invalid characters with underscores and ensures the name starts with a valid character, removing special handling for numeric names.
Added a section listing ONNX ML domain operators that are currently not supported for import or Burn support, along with reference links for each operator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I should have explained. TensorFlow adds this when exporting even though no ops are used.
Ahh ok I see, LGTM then!
But I think we should just accept the domain without listing the ops.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Pull Request Template
Checklist
cargo run-checks
command has been executed.Related Issues/PRs
Changes
Fixed ONNX import issues:
This allows importing ONNX models generated by tf2onnx that contain special characters in node names.
Testing