-
Notifications
You must be signed in to change notification settings - Fork 88
Add utility function for converting model protos to function proto #2655
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
Contribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”), and conveys certain license rights to Microsoft Corporation and its affiliates (“Microsoft”) for Your contributions to Microsoft open source projects. This Agreement is effective as of the latest signature date below.
@microsoft-github-policy-service agree company="Block" |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2655 +/- ##
==========================================
+ Coverage 70.46% 70.49% +0.03%
==========================================
Files 224 225 +1
Lines 26572 26595 +23
Branches 2637 2638 +1
==========================================
+ Hits 18723 18749 +26
+ Misses 6928 6924 -4
- Partials 921 922 +1 ☔ 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
This PR adds a utility function to convert ONNX model protos to function protos, addressing the first part of issue #2616. The key challenge is that function protos don't support initializers, so the implementation converts them to Constant nodes.
Key Changes:
- Implements
convert_model_proto_to_function_proto()that transforms model initializers into Constant nodes - Adds comprehensive test coverage for the conversion utility with multi-function model scenarios
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
onnxscript/utils/model_proto_to_function_proto.py |
Core implementation of model-to-function proto conversion with initializer handling |
tests/utils/model_proto_to_function_proto_test.py |
Test suite validating conversion functionality with nested function calls |
tests/utils/__init__.py |
Package initialization file for the test utils module |
4d69bd6 to
8c06ce4
Compare
604eb60 to
ea84ae4
Compare
| model = _initializers_to_constants(model) | ||
| model_ir = onnx_ir.serde.deserialize_model(model) | ||
| function_ir = onnx_ir.Function( | ||
| domain=domain, name=name, graph=model_ir.graph, attributes={} |
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.
Copy opset_import over?
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.
Also: doc_string, metadata_props?
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.
And value_info over from the graph ... we would also need to copy the value_info for graph inputs/outputs over
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.
Updated here: aadc881
Though not sure about how to set the value_info? or if metadata_props is done correctly. If I understand correctly does a setter need to exist in the upstream onnx_ir part?
ea84ae4 to
791cdbc
Compare
This addresses the first part of: #2616
It tries to convert arbitrary model protos to function protos. Since function protos don't have a concept of initializers, they are made to be constants. This approach should address a large number of model protos (including 'models' from skl2onnx)
Happy for any particular feedback.
Note that this does not address the second part of #2616 , running the script with the operator embedded will not work.