Add proto support for overloaded functions #5011
Conversation
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
|
Just wanted to say I think this is a really good idea. Functions are currently way too limited to be really useful for a runtime/tooling IMO. Adding overloads can be a good first step towards actually expressing a wider part of the standard with them. If I understand correctly, they would be parametrised by input types and attribute values which seems like exactly the way to go. |
|
Thank you @gramalingam ! This will enable clean and unified function names for dynamo onnx exported specialized nn.Module functions, as well as potentially the currently inlined 'trace_only' functions in onnxscript torchlib. One thing to point out though this does appear to vary slightly with traditional pl function overload concept, whereas here it is possible to have two function overloads with exact same input and attribute types. I think following the pl convention would restrict the usefulness in above mentioned use cases, but I'd love to learn about concerns if any. |
|
Also: @yuslepukhin for comments/feedback if any |
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #5011 +/- ##
==========================================
- Coverage 56.65% 56.64% -0.01%
==========================================
Files 506 506
Lines 30194 30201 +7
Branches 4556 4558 +2
==========================================
+ Hits 17105 17108 +3
- Misses 12266 12268 +2
- Partials 823 825 +2 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
|
The new string would be consumed by a NodeProto to avoid any ambiguity or can it be left empty or is it just an extension of the function name? I assume this PR goes with #5903. |
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
Both FunctionProto and NodeProto have this new field. But it is an optional string field. It can be left empty. As I understand Protobuf, older versions (IR version 9 and below), when read in (with new proto), it will have an empty string value. When both FunctionProto and NodeProto have this empty, the behavior is same as before. However, if this is non-empty, then it is used to match the names from NodeProto to FunctionProto. |
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
…onnx into func-overload-proto
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
Good catch. I updated it, thanks. |
|
A question on the syntax (used in onnxtext format): I use "opname:overload" to indicate the overload. Please let me know if you have any suggestions: eg., if you think "opname::overload" would be better. A single colon seems sufficient and minimal. Double colon has the advantage of being aligned with C++ notation. |
|
@xadupre : one thing pending is updating the reference implementation. Do you know where it looks up a model-local function to execute a node? |
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
…onnx into func-overload-proto
Does |
|
Hmm maybe not since domain can be dot separated already. |
A minimal and simple extension to the proto definitions to allow multiple (overloaded) functions in its model-local function list with same (domain, name). Model-local functions in ONNX provide a default-implementation for certain ops used in the model (expressed in terms of other primitive ops). This extension will allow us to provide different default-implementations for different calls to the same op (which might differ in the types of input-arguments or in the values of attribute-parameters). * Update proto * Update helper.py constructors * Update parser * Update shape inference implementation * Update in-memory IR * Add test-case * Minimal extensions in inliner to support this --------- Signed-off-by: Ganesan Ramalingam <grama@microsoft.com> Signed-off-by: Abhishek-TyRnT <kulkarnia168@gmail.com>
|
Should op schemas be extended to support function overloads? @gramalingam |
A minimal and simple extension to the proto definitions to allow multiple (overloaded) functions in its model-local function list with same (domain, name). Model-local functions in ONNX provide a default-implementation for certain ops used in the model (expressed in terms of other primitive ops). This extension will allow us to provide different default-implementations for different calls to the same op (which might differ in the types of input-arguments or in the values of attribute-parameters). * Update proto * Update helper.py constructors * Update parser * Update shape inference implementation * Update in-memory IR * Add test-case * Minimal extensions in inliner to support this --------- Signed-off-by: Ganesan Ramalingam <grama@microsoft.com> Signed-off-by: Linsho Kaku <linsho@preferred.jp>
A minimal and simple extension to the proto definitions to allow multiple (overloaded) functions in its model-local function list with same (domain, name).
Model-local functions in ONNX provide a default-implementation for certain ops used in the model (expressed in terms of other primitive ops). This extension will allow us to provide different default-implementations for different calls to the same op (which might differ in the types of input-arguments or in the values of attribute-parameters).