-
Notifications
You must be signed in to change notification settings - Fork 6
Support initializers with onnxtext #60
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Justin Chu <[email protected]>
Signed-off-by: Justin Chu <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #60 +/- ##
==========================================
- Coverage 73.59% 73.39% -0.21%
==========================================
Files 37 37
Lines 4492 4507 +15
Branches 902 906 +4
==========================================
+ Hits 3306 3308 +2
- Misses 858 869 +11
- Partials 328 330 +2 ☔ View full report in Codecov by Sentry. |
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.
Pull Request Overview
Enhance ONNX IR serialization/deserialization to support textual format with optional initializer handling.
- Introduce
from_onnx_text
accepting an optional initializer mapping. - Add
to_onnx_text
with a flag to exclude initializers. - Import
_convenience
to build value mappings for initializer insertion.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
src/onnx_ir/serde.py | New methods from_onnx_text and to_onnx_text with initializer support |
docs/api/core.md | Registered to_onnx_text in the API reference |
Comments suppressed due to low confidence (1)
src/onnx_ir/serde.py:196
- The
Mapping
type is not imported; this will raise a NameError. Either prefix withtyping.Mapping
or addfrom typing import Mapping
at the top.
with_initializers: Mapping[str, _protocols.TensorProtocol] | None = None,
Signed-off-by: Justin Chu <[email protected]>
bae7922
to
f2e774d
Compare
Signed-off-by: Justin Chu <[email protected]>
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.
Can't we use text model to improve ir.Model print out? Maybe that way we don't need two IRs?
The print outs are created for better readability and are more free formed (can include any information we want to put there). The text representation needs to support round tripping and is rigidly defined. I think there is a benefit in defining the repr for an ir model instead of directly using the text format. We also save the need to serialize when printing this way |
Enhancements to Serialization and Deserialization:
to_onnx_text
method: Introduced a new method insrc/onnx_ir/serde.py
to convert an IR model to ONNX textual representation. The method includes an option to exclude initializers from the output.from_onnx_text
method: Enhanced the existing method to support adding initializers to the model during deserialization using a mapping of names to tensors. This addition improves flexibility and usability.