Skip to content

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

justinchuby
Copy link
Member

Enhancements to Serialization and Deserialization:

  • Added to_onnx_text method: Introduced a new method in src/onnx_ir/serde.py to convert an IR model to ONNX textual representation. The method includes an option to exclude initializers from the output.
  • Extended 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.

Signed-off-by: Justin Chu <[email protected]>
Signed-off-by: Justin Chu <[email protected]>
@justinchuby justinchuby requested review from titaiwangms and a team as code owners June 3, 2025 16:27
@justinchuby justinchuby requested a review from Copilot June 3, 2025 16:27
Copy link

codecov bot commented Jun 3, 2025

Codecov Report

Attention: Patch coverage is 33.33333% with 12 lines in your changes missing coverage. Please review.

Project coverage is 73.39%. Comparing base (60f8692) to head (d9dee73).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/onnx_ir/serde.py 33.33% 11 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@Copilot 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

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 with typing.Mapping or add from typing import Mapping at the top.
with_initializers: Mapping[str, _protocols.TensorProtocol] | None = None,

Signed-off-by: Justin Chu <[email protected]>
@justinchuby justinchuby force-pushed the justinchu/to-onnx-text branch from bae7922 to f2e774d Compare June 3, 2025 16:56
@justinchuby justinchuby marked this pull request as draft June 3, 2025 16:57
@justinchuby justinchuby marked this pull request as ready for review June 3, 2025 20:51
@justinchuby justinchuby added this to the 0.1.2 milestone Jun 3, 2025
Signed-off-by: Justin Chu <[email protected]>
Copy link
Collaborator

@titaiwangms titaiwangms left a 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?

@justinchuby
Copy link
Member Author

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

@justinchuby justinchuby added the do not merge Do not merge yet label Jun 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants