move map and sequence types to onnx domain, #2244
Conversation
… merging onnx-ml and onnx types.
|
Merging ONNX and ONNX-ML IR was agreed in infra SIG meeting on 7/1/2019 as https://github.com/onnx/sigs/blob/master/infra/meetings/001-20190701.md. Especially for types, the difference between DNN and traditional ML can/will be shown by different op domains. there's no need having two different IR formats for the differentiation. |
|
@bddppq IR version is bumped at most once for one ONNX release (following opset version bump policy). @gramalingam has already bumped the IR version to 6 when adding sparse tensor related support, so that, this PR does not need to bump IR version any more, make sense? |
| message Sequence { | ||
| // The type and optional shape of each element of the sequence. | ||
| // This field MUST be present for this version of the IR. | ||
| optional TypeProto elem_type = 1; |
There was a problem hiding this comment.
I am wondering if we still need this comment here? If this can be optional indeed, we can use Sequence to export heterogeneous tensors of different datatypes also by keeping elem_type unspecified.
There was a problem hiding this comment.
Supporting heterogeneous tensors can probably be done by adding another type Tuple. Sequence stills sounds like a homogeneous type like IEnumerable<ElementType> in C#. This Sequence is for containing non-tensor elements with the same type like the predicted probabilities of all classes, [{"ClassA", 0.5, "ClassB", 0.25, "ClassC", 0.25}, {"ClassA", 1, "ClassB", 0, "ClassC", 0}].
There was a problem hiding this comment.
I believe Spandan is talking about an unbounded list/sequence, but where the elements don't have the same type. I think there are some details concerning such a type worth sorting out first. E.g., do we just want a list of tensors of different types, or do we want truly heterogeneous lists (a list where some elements are tensors, other elements are lists of tensors, other elements are maps, etc.)? I guess it should be the later to be truly general-purpose. More importantly, we would need support for some form of dynamic-cast from untyped values to make use of such a type. I think it would be helpful to have a more comprehensive proposal (e.g., take a simple example with heterogeneous tensors and see what's required to support it).
There was a problem hiding this comment.
In short, I think we can do that generalization separately, so that this PR can be kept simple.
There was a problem hiding this comment.
Yes, I think hetergeneous lists is a use case that we need to target. But I agree that it can designed separately to keep this PR simple.
There was a problem hiding this comment.
If we need heterogeneous types in one single variable, I'd personally consider a new type Tuple instead of Sequence. Sequence is more like a collection type where elements are not stored in consecutive memory blocks.
|
|
||
| SparseTensor sparse_tensor_type = 8; | ||
|
|
||
| // #if ONNX-ML |
There was a problem hiding this comment.
no, this can't be removed.
onnx/onnx.in.proto
Outdated
| optional TensorShapeProto shape = 2; | ||
| } | ||
|
|
||
| // #if ONNX-ML |
There was a problem hiding this comment.
no, it's not. see line 520 before.
onnx/onnx.proto
Outdated
|
|
||
| // The type of a map. | ||
| Map map_type = 5; | ||
|
|
There was a problem hiding this comment.
| // The type of sparse tensor. This type is different than tensor. |
|
|
||
| oneof value { | ||
| // The type of a tensor. | ||
| Tensor tensor_type = 1; | ||
|
|
||
| // NOTE: DNN-only implementations of ONNX MAY elect to not support non-tensor values | ||
| // as input and output to graphs and nodes. These types are needed to naturally | ||
| // support classical ML operators. DNN operators SHOULD restrict their input |
There was a problem hiding this comment.
| // support classical ML operators. DNN operators SHOULD restrict their input | |
| // support classical ML operators. DNN operators SHOULD restrict their input |
Co-Authored-By: Wei-Sheng Chin <wschin@outlook.com>
There was a problem hiding this comment.
Documentation also needs to be updated: https://github.com/onnx/onnx/blob/master/docs/Overview.md, https://github.com/onnx/onnx/blob/master/docs/IR.md, etc
| message SparseTensor { | ||
| // This field MUST NOT have the value of UNDEFINED | ||
| // repeated T | ||
| message Sequence { |
There was a problem hiding this comment.
There is a discussion to consider renaming this from Sequence to List or something like that
|
@prasanthpul given this PR is not merging ONNX-ML with ONNX, I'm not updating the docs to remove ONNX-ML concept. |
Move map and sequence types to onnx domain, this is the first step of merging onnx-ml and onnx types.
this is the first step of merging onnx-ml and onnx types.