Implement the type of data in channel #8285
Conversation
chengduoZH
requested review from
abhinavarora,
helinwang,
kavyasrinet and
wangkuiyi
chengduoZH
force-pushed
the
feature/add_channel_type
branch
from
6e8144e to
4ee294d
Compare
kavyasrinet
left a comment
kavyasrinet
left a comment
There was a problem hiding this comment.
Thanks so much for making this PR! I just have a few questions I have posted.
paddle/framework/framework.proto
Outdated
| SELECTED_ROWS = 10; | ||
| LOD_TENSOR_ARRAY = 11; | ||
| } | ||
| required DataType base_type = 1; |
There was a problem hiding this comment.
From what I understand, base_type would be the channel type (type of elements in channel) right ?
There was a problem hiding this comment.
no, the type of element in the channel should be repeated MetaDataDesc.
For example, the type of element is <int32, int64, float32>.
paddle/framework/framework.proto
Outdated
| optional bool data_bool = 2; | ||
| optional int32 data_int32 = 3; | ||
| optional int64 data_int64 = 4; | ||
| optional float data_float = 5; |
There was a problem hiding this comment.
where would these fields be used ? As attributes for MetaDataDesc ?
There was a problem hiding this comment.
Yes, these fields are attributes of MetaDataDesc and are optional, so one MetaDataDesc only describes one base_type.
paddle/framework/framework.proto
Outdated
| optional LoDTensorArrayDesc tensor_array = 8; | ||
| } | ||
|
|
||
| message ChanEleDesc { repeated MetaDataDesc meta_data = 1; } |
There was a problem hiding this comment.
I assume this step would append multiple elements in the tuple together ? and the next step would append tuples of these ?
There was a problem hiding this comment.
We can regard the ChanEleDesc as a tuple, it can describe <int32,int32,int64,float32>.
paddle/framework/framework.proto
Outdated
|
|
||
| message MetaDataDesc { | ||
| enum DataType { | ||
| BOOL = 1; |
There was a problem hiding this comment.
Thank you. I see based on the discussion we had on Hi yesterday, you have included the basic data types as well.
paddle/framework/framework.proto
Outdated
| } | ||
| required DataType base_type = 1; | ||
| optional bool data_bool = 2; | ||
| optional int32 data_int32 = 3; |
There was a problem hiding this comment.
I am not sure what will we save in these optionals for fundamental types? Could you please explain with an example. For example, if we make a channel which can take tuple of 2 integers, how will this look?
There was a problem hiding this comment.
We can define a visitor, and the visitor can get the data of MetaData according to the data type.
paddle/framework/framework.proto
Outdated
| message ChanEleDesc { repeated MetaDataDesc meta_data = 1; } | ||
|
|
||
| message ChanDesc { | ||
| repeated ChanEleDesc channel_type = 1; |
There was a problem hiding this comment.
Could you also explain why ChanEleDesc is repeated here? From what I understand, the types of each of the dimensions in the n-tuple is captured by the statement repeated MetaDataDesc meta_data. Are we trying to make a tuple of tuples here? Sorry about the confusion.
There was a problem hiding this comment.
I know what you mean, let me think again.
paddle/framework/framework.proto
Outdated
| TENSOR = 8; | ||
| LOD_TENSOR = 9; | ||
| SELECTED_ROWS = 10; | ||
| LOD_TENSOR_ARRAY = 11; |
There was a problem hiding this comment.
I am not sure how we will be able to support channel of type channel. Go allows this. Do you think this code would suffice for that? Also, I am not sure if we need it right now.
There was a problem hiding this comment.
This is a good question, I think that we should also support channel of type channel, but this code maybe should be changed.
paddle/framework/framework.proto
Outdated
| message ChanEleDesc { repeated VarDesc meta_data = 1; } | ||
|
|
||
| message ChanDesc { | ||
| repeated ChanEleDesc channel_type = 1; |
There was a problem hiding this comment.
I think ChanEleDesc maybe not needed, each element inside one channel instance should be of the same type. So the ChanDesc should look like:
message ChanDesc {
required VarType ele_type = 1;
required int32 cap = 2 [ default = 0 ];
required string name = 3;
}Then VarType should have a new type called VAR_LIST representing a list of variables, so that element like tensor, label is able to put inside the channel.
The contents of the channel are manipulated at runtime.
There was a problem hiding this comment.
@typhoonzero thanks for your review! And this is a good suggestion.
There was a problem hiding this comment.
I think all the work to do in this PR is to add the following definition of ChannelDesc:
message ChannelDesc {
VarDesc elem_type = 1;
int cap = 2;
}Nothing more than that.
paddle/framework/framework.proto
Outdated
| message ChanDesc { | ||
| repeated ChanEleDesc channel_type = 1; | ||
| required int32 cap = 2 [ default = 0 ]; | ||
| required string name = 3; |
There was a problem hiding this comment.
I don't understand why would we need name here. I can see that we do need cap.
A reference design is the Go's channel definition: https://github.com/golang/go/blob/4fc9565ffce91c4299903f7c17a275f0786734a1/src/runtime/chan.go#L17-L29
We are particularly interested in elemtype.
…feature/add_channel_type
|
|
|
This PR is not mature enough, and it is turned off first. |
6 participants
fix #8284