Refactorize framework/*.proto #3322

wangkuiyi · 2017-08-08T01:03:45Z

Merge attribute.proto, op_desc.proto, op_proto.proto into framework.proto.
Move message VarProto into OpProto::Var.
Add message OpDesc::Var to change the way to representing "multiple" (which should be "duplicable") attributes.
Remove op_desc_test.cc and op_proto_test.cc, which doesn't include effective test code.

jacquesqiao · 2017-08-08T01:50:36Z

paddle/framework/framework.proto

+
+ message Var {
+ required string name; // e.g. "X"
+ optional int dup = 2 [ default = 0 ]; // e.g., "1"


add comment here ？

// indices the duplica optional int dup = 2 [ default = 0 ];

message Var { required string op_proto_name; repeated string name; };

jacquesqiao · 2017-08-08T02:09:06Z

paddle/framework/framework.proto

+ required string comment = 2;
+ // OpDesc::Var::dup indices the duplica.
+ optional bool duplicable = 3 [ default = false ];
+ optional bool intermediate = 4 [ default = false ];


is intermediate used to replace temporary?

jacquesqiao · 2017-08-08T02:11:53Z

The source code should be changed a lot after this PR and we also need a design_doc to describe the design of OpDesc and OpProto.

* Although backward_test/rnn_test is not pass, just comment them.

Step 1: Make code compile well.

… gtest.'

…orize_framework_proto

Update grad_op_builder after refactoring framework proto.

…ork_proto Modify rnn op unit test after refactoring framework proto.

…ramework_proto

Feature/refactorize framework proto

…orize_framework_proto

Catch-up with develop branch

reyoung · 2017-08-14T03:03:46Z

paddle/framework/grad_op_builder.cc

 namespace framework {

-typedef std::vector<int> Ints;
+class OpRegistry;


Do not use forward declare in C++;
https://google.github.io/styleguide/cppguide.html#Forward_Declarations

reyoung · 2017-08-14T03:04:58Z

paddle/framework/grad_op_builder.cc

- std::vector<int>* dst_format = AttrFormat(grad_attrs, dst_type);
-
- const OpProto& proto = OpRegistry::protos().at(src_op->type_);
+static void TransOpArg(const OperatorBase* src_op, OperatorBase* dst_op,


Since src_op is not nullable, it should be const OperatorBase&.

But it is not related to this PR. Please issue another PR when this PR has been merged.

https://google.github.io/styleguide/cppguide.html#Reference_Arguments

reyoung · 2017-08-14T03:06:48Z

paddle/framework/op_registry.h

 return *this;
 }

 VariableBuilder& SetTemporary() {


This method name should be renamed to SetIntermediate

reyoung · 2017-08-14T03:08:01Z

paddle/framework/op_registry.h

 return *this;
 }

 VariableBuilder& IgnoreGradient() {


Rename to NoGradient.

However, I do not think NoGradient is a straight name. Maybe we should change it to another name in following PR @Canpio @qingqing01

I also think the meaning of IgnoreGradient is not clear. But NoNeedInGradientOp is too long.

reyoung · 2017-08-14T03:09:08Z

paddle/framework/op_registry.h

 using OpCreator = std::function<OperatorBase*()>;
- using VarIndexMap = std::unordered_map<std::string, int>;
- using VarNameList = std::vector<std::string>;
+ using VarNameMap = std::map<std::string, std::vector<std::string>>;


Put this typedef to OperatorBase is a better idea. Because it is used by OperatorBase::inputs_/outputs_

reyoung · 2017-08-14T03:12:33Z

paddle/framework/op_registry.h

- inputs.reserve((size_t)op_desc.inputs_size());
- std::copy(op_desc.inputs().begin(), op_desc.inputs().end(),
- std::back_inserter(inputs));
+ VarNameMap inputs;


Copy and Paste code here. This logic should be extracted as a common function.

reyoung · 2017-08-14T03:15:52Z

paddle/framework/op_registry_test.cc

 op_desc.set_type("cos_sim");
- op_desc.add_inputs("aa");
- op_desc.add_outputs("bb");
+ auto input = op_desc.add_inputs();


Same Code Again & Again.

Maybe we can extract them into a function.

reyoung · 2017-08-14T03:16:24Z

paddle/framework/operator.cc

- "Input Out Of Range");
+const std::string& OperatorBase::Input(const std::string& name) const {
+ auto it = inputs_.find(name);
+ PADDLE_ENFORCE(it != inputs_.end(), "Op %s does not have input %s", type_,


PADDLE_ENFORCE_NE

It cannot use PADDLE_ENFORCE_NE because operator << of it is not defined.

reyoung · 2017-08-14T03:16:56Z

paddle/framework/operator.cc

-  inputs_.begin() + input_format.at(offset),
-  inputs_.begin() + input_format.at(offset + 1)};
+const std::vector<std::string>& OperatorBase::Inputs(
+ const std::string& name) const {


PADDLE_ENFORCE when not found name. Give a reasonable error.

reyoung · 2017-08-14T03:17:07Z

paddle/framework/operator.cc

- auto it = in_out_idxs_->find(name);
- PADDLE_ENFORCE(it != in_out_idxs_->end(), "no key [%s] in in_out_idxs_",
+ auto it = outputs_.find(name);
+ PADDLE_ENFORCE(it != outputs_.end(), "Op %s does not have output %s", type_,


PADDLE_ENFORCE_NE

It cannot use PADDLE_ENFORCE_NE because operator << of it is not defined.

reyoung · 2017-08-14T03:17:25Z

paddle/framework/operator.cc

- outputs_.begin() + output_format.at(offset),
- outputs_.begin() + output_format.at(offset + 1)};
+const std::vector<std::string>& OperatorBase::Outputs(
+ const std::string& name) const {


PADDLE_ENFORCE_NOT_FOUND

reyoung · 2017-08-14T03:18:32Z

paddle/framework/operator.h

- std::vector<std::string> Outputs(const std::string& name) const;
+ const std::vector<std::string>& Outputs(const std::string& name) const;
+
+ virtual std::vector<std::string> OutputVars(bool has_intermediate) const {


Not in the header.

reyoung · 2017-08-14T03:19:39Z

paddle/framework/operator.h

+ }

- const std::string Type() const { return type_; }
- const std::vector<std::string> Inputs() const { return inputs_; }


Bad design in develop. Return a const T is not useful in C++. It should return T or const T&

reyoung · 2017-08-14T03:23:01Z

paddle/framework/operator.h

 // O (Outputs)
 // OG (Output Gradients)
- std::vector<std::string> inputs_;
+ std::map<std::string, std::vector<std::string>> inputs_;


Use VarNameMap before

reyoung · 2017-08-14T03:23:22Z

paddle/framework/operator.h

-
- const Variable* InputVar(const size_t index) const {
- return scope_.FindVar(op_.inputs_.at(index));
+ size_t InputSize(const std::string& name) const {


PADDLE_ENFORCE here

reyoung · 2017-08-14T03:24:19Z

paddle/framework/operator.h


- Variable* OutputVar(const size_t index) const {
- return scope_.FindVar(op_.outputs_.at(index));
+ size_t OutputSize(const std::string& name) const {


PADDLE_ENFORCE

reyoung · 2017-08-14T03:26:25Z

paddle/framework/operator_test.cc

 op_desc.set_type("test_operator");
- *op_desc.mutable_inputs()->Add() = "IN1";
- *op_desc.mutable_outputs()->Add() = "OUT1";
+ auto* ipt = op_desc.mutable_inputs()->Add();


Same code again & again

reyoung · 2017-08-14T03:29:12Z

paddle/operators/recurrent_op_test.cc

- rnn_op_->Run(scope_, ctx);
-}
+using namespace paddle::framework;
+// using framework::make_ddim;


Either Remove or uncomment this lines.

reyoung · 2017-08-14T03:31:36Z

python/paddle/v2/framework/tests/test_add_two_op.py

- expected = '''Op(add_two_grad), inputs:(X, Y, Out, Out@GRAD), outputs:(X@GRAD, Y@GRAD).'''
- self.assertEqual(expected, str(backward_op))
-
+#class TestAddGradOp(unittest.TestCase):


This unit test should be uncomment

qingqing01 · 2017-08-14T03:35:01Z

paddle/framework/backward.cc

- "add", {dup_outputs}, {name},
+ "add", {{"X", {dup_outputs}}}, {{"Out", {name}}},
 {{"input_format",
 std::vector<int>{0, static_cast<int>(dup_outputs.size())}}})});


The input_format is out of use now. It can be removed.

qingqing01 · 2017-08-14T03:41:09Z

paddle/framework/backward_test.cc

- if (b_name != kEmptyVarName) {
- AddOp(OpRegistry::CreateOp("rowwise_add", {Output("mul_result"), b_name},
- {Output("add_result")}, {}));
+ if (input_b.size() != 0) {


if (InputSize("b") != 0) {

qingqing01 · 2017-08-14T03:43:13Z

paddle/framework/backward_test.cc

- {Output("add_result")}, {}));
+ if (input_b.size() != 0) {
+ AddOp(OpRegistry::CreateOp(
+ "rowwise_add", {{"X", {Output("mul_result")}}, {"b", {input_b[0]}}},


Input("b") and remove line 87.

qingqing01 · 2017-08-14T03:46:06Z

paddle/framework/op_registry.h

 return *this;
 }

 VariableBuilder& IgnoreGradient() {


I also think the meaning of IgnoreGradient is not clear. But NoNeedInGradientOp is too long.

qingqing01 · 2017-08-14T03:50:51Z

paddle/framework/op_registry.h

 return op_checkers_;
 }

 static void GenerateTempVariableName(OperatorBase* op) {


Should we replace temp by intermediate? since we use intermediate in proto.

Polish Our code by YuYang's review

reyoung · 2017-08-14T06:00:40Z

@qingqing01 I think we should create several issues that this PR discovered but has not been fixed.

…kuiyi/Paddle into refactorize_framework_proto

reyoung

Merge it to develop soon, not block others implementation.

Update CMakeLists

e0e9a81

wangkuiyi requested review from JiayiFeng, gangliao, jacquesqiao and reyoung August 8, 2017 01:04

wangkuiyi added 2 commits August 7, 2017 18:15

Update operator/CMakeLists.txt

662aeed

update framework.proto

72e3ba5

jacquesqiao reviewed Aug 8, 2017

View reviewed changes

reyoung and others added 20 commits August 8, 2017 14:55

Try make pass

7e83011

Make Compile Pass

dba618c

* Although backward_test/rnn_test is not pass, just comment them.

Merge pull request #3 from reyoung/feature/refactorize_framework_proto

d97a2b4

Step 1: Make code compile well.

Resovle conflicts manually

9544068

Add a temporary test case otherwise there would be linking error with…

4a78885

… gtest.'

Merge branch 'develop' of github.com:baidu/Paddle into feature/refact…

7702ab1

…orize_framework_proto

Merge branch 'develop' of github.com:baidu/Paddle into feature/refact…

3d4ba22

…orize_framework_proto

Rename op_proto_name/var_names -> parameter/arguments

b368c6c

Modify rnn op unit test after refactoring framework proto.

5a59111

Add OutputVars method to get all outputs or outputs without intermediate

78af6e6

Merge branch 'develop' of github.com:baidu/Paddle into feature/refact…

030f430

…orize_framework_proto

Update grad_op_builder after refactoring framework proto.

665e1a3

Merge pull request #7 from qingqing01/grad_op_builder

5f6e5ed

Update grad_op_builder after refactoring framework proto.

Merge pull request #5 from qingqing01/rnn_test_for_refactorize_framew…

36709d0

…ork_proto Modify rnn op unit test after refactoring framework proto.

Merge branch 'refactorize_framework_proto' into feature/refactorize_f…

7202f42

…ramework_proto

Merge pull request #6 from reyoung/feature/refactorize_framework_proto

c7e8c1a

Feature/refactorize framework proto

Tiny fix

71acaff

Merge branch 'develop' of github.com:baidu/Paddle into feature/refact…

7fab7dd

…orize_framework_proto

Merge pull request #8 from reyoung/feature/refactorize_framework_proto

5ac3641

Catch-up with develop branch

Fix merge error

0f84bb3

reyoung reviewed Aug 14, 2017

View reviewed changes

qingqing01 reviewed Aug 14, 2017

View reviewed changes

reyoung added 4 commits August 14, 2017 12:54

Polish Our code by YuYang's review

4a604c2

Simplify unit test code

ef29b52

Follow comments from WangYi

f09cb65

Merge pull request #14 from reyoung/feature/refactorize_framework_proto

1ed5f02

Polish Our code by YuYang's review

reyoung added 3 commits August 14, 2017 14:22

Fix CI Test

63b2e45

Merge branch 'refactorize_framework_proto' of https://github.com/wang…

8371f67

…kuiyi/Paddle into refactorize_framework_proto

Fix CI

64a4dfe

reyoung approved these changes Aug 14, 2017

View reviewed changes

reyoung merged commit 81f5f86 into PaddlePaddle:develop Aug 14, 2017

Refactorize framework/*.proto #3322

Refactorize framework/*.proto #3322

Uh oh!

Conversation

wangkuiyi commented Aug 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

reyoung Aug 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacquesqiao commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyoung commented Aug 14, 2017

reyoung left a comment

Choose a reason for hiding this comment

Labels

4 participants

wangkuiyi commented Aug 8, 2017 •

edited

Loading

reyoung Aug 8, 2017 •

edited

Loading