Add OpProto implementation #2703
Conversation
reyoung
requested review from
Superjomn,
a user,
jacquesqiao,
dzhwinter,
JiayiFeng and
QiJune
reyoung
force-pushed
the
feature/op_proto
branch
2 times, most recently
from
fbfdfa1 to
053ab40
Compare
paddle/framework/CMakeLists.txt
Outdated
| @@ -5,3 +5,4 @@ nv_test(dim_test SRCS dim_test.cu DEPS ddim) | |||
| cc_test(variable_test SRCS variable_test.cc) | |||
| cc_test(scope_test SRCS scope_test.cc) | |||
| cc_test(enforce_test SRCS enforce_test.cc) | |||
| proto_library(op_proto SRCS op_proto.proto attr_type.proto) | |||
There was a problem hiding this comment.
since attr_type.proto is separated and also used by op_desc.proto, if it's better to write like below, I am not sure if proto_library can do this.
proto_library(attr_type SRCS attr_type.proto)
proto_library(op_proto SRCS op_proto.proto DEPS attr_type)
proto_library(op_desc SRCS op_desc.proto DEPS attr_type)There was a problem hiding this comment.
proto_library doesn't contain DEPS argument. Each protobuf file can compile to the independent library and doesn't depends on other files.
There was a problem hiding this comment.
I will give a unit test to demonstrate that.
There was a problem hiding this comment.
Done. @jacquesqiao I add a unit test also.
paddle/framework/op_proto.proto
Outdated
| required AttrType type = 1; | ||
|
|
||
| // Supported attribute name. e.g. `scale` for cosine op. | ||
| required string name = 2; |
There was a problem hiding this comment.
In my mind, name should always be the first item in a proto.
There was a problem hiding this comment.
You're right! Let me fix them
OpProto is a proto message that helps 3rd-party language bindings, e.g. `Python`, to generate operator creation methods. The operator creation method is the low-level API for 3rd-party language bindings. Op creation methods take the user's input in that language, and convert users inputs into `OpDesc` message, then passing that `OpDesc` message to Paddle's C++ core and create an operator. * A separated `attr_type.proto` is added, because that file wound be included by `op_desc.proto` in future.
| @@ -0,0 +1,28 @@ | |||
| /* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserve. | |||
There was a problem hiding this comment.
Are we going to put our xx.proto file into the dir that need to use it or just in paddle/proto?
There was a problem hiding this comment.
I am not sure.
But tensorflow make protobuf files into sub-directories. Since we are using google code style now, it seems that we should put our protobuf files in sub-directories.
Another advantage that make protobuf file in sub-directories is it make include line pretty like below.
#include <paddle/framework/op_proto.pb.h>
| required string name = 1; | ||
|
|
||
| // The comment for that input. It helps 3rd-party language generate doc-string. | ||
| required string comment = 2; |
There was a problem hiding this comment.
add required bool is_tensor = 3 ?
There was a problem hiding this comment.
That field will be added when it is needed for generating layer because maybe just a bool flag is not enough. But we do not know what is better.
Maybe an enum type is needed, like
enum VarType {
LAYER_INPUT = 0;
LAYER_OUTPUT = 1;
LAYER_PARAM = 2;
LAYER_AUX_INPUT = 3;
LAYER_AUX_OUTPUT = 4;
RNN_SCOPE = 5;
};
But I can not have a precise design about that field. It should be clearer when we start writing layer methods.
|
so fast as you guys! 👍 |
OpProto is a proto message help 3rd-party language bindings, e.g.
Pythonto generate operator creation methods. The operator creationmethod is the low-level API for 3rd-party language bindings. Op creation
methods take the user's input in that language, and convert users inputs
into
OpDescmessage, then passing thatOpDescmessage to Paddle'sC++ core and create an operator.
attr_type.protois added, because that file woundbe included by
op_desc.protoin future.