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

 #include "paddle/framework/op_registry.h"

 namespace paddle {
 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


    VariableBuilder& SetMultiple() {
-      var_->set_multiple(true);
-      on_multiple_();
+      var_->set_duplicable(true);
      return *this;
    }

    VariableBuilder& SetTemporary() {


This method name should be renamed to SetIntermediate

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

paddle/framework/op_registry.h

-      PADDLE_ENFORCE(bool(on_temporary_), "Cannot set temporary");
-      var_->set_temporary(true);
-      on_temporary_();
+      var_->set_intermediate(true);
      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

 };

 class OpRegistry {
  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

@@ -61,8 +59,13 @@ REGISTER_OP(my_test_op, paddle::framework::MyTestOp,
 TEST(OpRegistry, CreateOp) {
  paddle::framework::OpDesc op_desc;
  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


 public:
  std::string type_;
  // NOTE: in case of OpGrad, inputs_ contains:
  // I (Inputs)
  // 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

@@ -62,8 +62,13 @@ REGISTER_OP(test_operator, paddle::framework::OpWithoutKernelTest,
 TEST(OperatorBase, all) {
  paddle::framework::OpDesc op_desc;
  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

@@ -117,7 +129,7 @@ std::shared_ptr<OperatorBase> BackwardRecursive(
      insert_position.push_back(
          {dup_op.back(),
           OpRegistry::CreateOp(
-               "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

-      PADDLE_ENFORCE(bool(on_temporary_), "Cannot set temporary");
-      var_->set_temporary(true);
-      on_temporary_();
+      var_->set_intermediate(true);
      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

@@ -295,11 +217,13 @@ class OpRegistry {

  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 reyoung, jacquesqiao, gangliao and JiayiFeng 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


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

		@@ -295,11 +217,13 @@ class OpRegistry {

		static void GenerateTempVariableName(OperatorBase* op) {

Refactorize framework/*.proto #3322

Refactorize framework/*.proto #3322

Conversation

wangkuiyi commented Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

reyoung Aug 8, 2017 • edited Loading

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

wangkuiyi commented Aug 8, 2017 •

edited

Loading

reyoung Aug 8, 2017 •

edited

Loading