add operator base #2725

jacquesqiao · 2017-07-04T03:42:26Z

No description provided.

… opbase

jacquesqiao · 2017-07-05T09:20:32Z

paddle/framework/operator.h

+  explicit Operator(const OpDesc& desc);
+  virtual ~Operator() {}
+
+  void InitializeAttrs();


maybe we can remove this interface and do initialize in the ctor

reyoung · 2017-07-06T07:56:51Z

paddle/framework/operator.cc

+namespace paddle {
+namespace framework {
+
+OperatorBase::OperatorBase(const OpDesc &desc): desc_(desc) {


It seems that OpBase doesn't take whole OpDesc as the arguments, but only inputs and outputs.

So maybe std::vector<string> input and std::vector<string> output is enough.

done, remove construct function

reyoung · 2017-07-06T07:58:58Z

paddle/framework/operator.cc

+}
+
+std::string OperatorBase::DebugString() {
+  return desc_.DebugString();


DebugString() is not in OpDesc.

It should be generated in run time.

Like Operator Cosine, from (var_input1, var_input2) to (var_output)

reyoung · 2017-07-06T07:59:13Z

paddle/framework/operator.cc

+  }
+}
+
+std::string OperatorBase::DebugString() {


Also, const is need.

reyoung · 2017-07-06T07:59:56Z

paddle/framework/operator.cc

+  return desc_.DebugString();
+}
+
+Variable* OperatorBase::input(Scope *scope, int index) {


It should not be a part of the interface.

Because the invoker of Operator is not need input method.

done, will put input() into OpRunContext

reyoung · 2017-07-06T08:00:19Z

paddle/framework/operator.h

+
+  /// initialize Attributes of this OP from proto message desc.attrs()
+  /// you should derive this function to init the attr you need in OP.
+  virtual void InitializeAttributes() = 0;


That interface is not needed, due to @Canpio 's design.

done, move to CreateOperator

reyoung · 2017-07-06T08:00:33Z

paddle/framework/operator.h

+  Variable* output(Scope* scope, int index);
+
+protected:
+ const OpDesc desc_;


Op do not store OpDesc

why not store OpDesc, it will not change and can use directly when to serialize.

done.... though i still want to keep it

reyoung · 2017-07-06T08:00:52Z

paddle/framework/operator.h

+  Variable* input(Scope* scope, int index);
+  Variable* output(Scope* scope, int index);
+
+protected:


inputs_ and outputs_ are all public.

reyoung · 2017-07-06T08:01:02Z

paddle/framework/CMakeLists.txt

@@ -12,3 +12,4 @@ cc_test(op_proto_test SRCS op_proto_test.cc DEPS op_proto protobuf)

 proto_library(op_desc SRCS op_desc.proto DEPS attr_type)
 cc_test(op_desc_test SRCS op_desc_test.cc DEPS op_desc protobuf)
+cc_library(operator SRCS operator.cc)


Need unittest.

… opbase

wangkuiyi · 2017-07-06T23:57:10Z

paddle/framework/operator.h

+ * device resource such as CUDA stream, cublas handle, etc. from
+ * OpRunContext. User should construct it before run the Operator.
+ */
+class OpRunContext {


Given that Caffe2 doesn't have OpRunContext, it seems that we are doing over-engineering here. How about put Scope* scope in the base class Context, which has sub-classes CPUContext and GPUContext?

wangkuiyi · 2017-07-06T23:58:16Z

paddle/framework/operator.h

+namespace framework {
+
+class DeviceContext {};
+class CpuContext : public DeviceContext {};


Cpu => CPU. It is an acronym.

wangkuiyi · 2017-07-06T23:58:23Z

paddle/framework/operator.h

+
+class DeviceContext {};
+class CpuContext : public DeviceContext {};
+class GpuContext : public DeviceContext {};


wangkuiyi · 2017-07-07T00:01:28Z

paddle/framework/operator.h

+  DeviceContext* device_context;
+};
+
+using Attribute =


I can see three ways of implementing Operator attributes:

std::map<string, boost:variant<...> >

std::map<string, shared_ptr<Variable> >

OperatorDesc, given that OperatorBase::OperatorBase takes a OperatorDesc and saves it in desc_.

Which is the simplest one?

It looks to me that the last one is the simplest because the former two introduce a new form, std::map, of OperatorDesc::attrs_.

wangkuiyi · 2017-07-07T00:11:05Z

paddle/framework/operator.h

+  /// InferShape infer the size of Variables used by this Operator with
+  /// information
+  /// inside scope
+  virtual void InferShape(const Scope* scope) const = 0;


InferShape should have default implementation?

virtual void InferShape(const Scope* scope) {}

Also, it might change properties of inputs and outputs, so shouldn't be suffixed with const?

wangkuiyi · 2017-07-07T00:23:33Z

paddle/framework/operator.h

+
+  std::string DebugString() const;
+
+ public:


Why should we expose all these fields? I don't seem at any chance the outside world need to access them.

Indeed, even from inside an Operator class, we don't need to access them. Instead, the following methods should use them:

template <typename T> const T& GetAttr(std::string name); Variable* Input(std::string name) const; Variable* output(std::string name) const;

caffe里面是把这几个函数放在了OpBase中，因为caffe的op在new的时候把workspace和context都创建好放在了op里边，所以caffe op的run不需要加什么参数。

我们目前scope和devicecontext都是运行时才能确定，通过组装成一个 OpRunContext 给 Run(OpRunContext* context) ，所以目前的想法是这几个函数都有OpRunContext提供：

这样的话，很自然的，我们就会有OpKernel这个概念了，Operator存储一些非运行时的配置，运行之前组装OpRunContext传给OpKernel的run。

class OpKernel { void Run(OpRunContext* context) { auto input0 = context.Input(0); auto cuda_stream = contex.cuda_stream(); } }

… opbase

Superjomn · 2017-07-07T08:43:08Z

paddle/framework/operator.h

+ * device resource such as CUDA stream, cublas handle, etc. from
+ * OpRunContext. User should construct it before run the Operator.
+ */
+class OpRunContext {


OpContext in mxnet, why not call it OpContext given that their functions are the same? OpRunContext too long and sounds strange.

Superjomn · 2017-07-07T09:02:12Z

paddle/framework/operator.h

+
+  std::string DebugString() const;
+
+ public:


protected if they need to be accessed easily by subclasses better ?

add getter function and set all this to private

Superjomn · 2017-07-07T09:24:54Z

paddle/framework/operator.h

+ * device resource such as CUDA stream, cublas handle, etc. from
+ * OpRunContext. User should construct it before run the Operator.
+ */
+class OpRunContext {


class OpRunContext -> struct better ?

Maybe OpContext will be more complex later, and will add more interface.

qingqing01 · 2017-07-07T14:41:42Z

paddle/framework/operator.cc

+  }
+  for(auto it = attrs.begin(); it != attrs.end(); ++it) {
+    attrs_[it->first] = it->second;
+  }


The declaration of attrs_:

AttributeMap attrs_;

The type is same with attrs, so why not use attrs_ = attrs？

qingqing01 · 2017-07-07T14:45:42Z

paddle/framework/operator.h

+
+  inline const AttributeMap attrs() const {
+    return attrs_;
+  }


template const T& GetAttr(std::string name);

如wangyi老师所说这样觉得更好些，返回Attribute、AttributeMap对于写Op的人来说也还需要查怎么获取值~

qingqing01 · 2017-07-07T14:50:36Z

paddle/framework/operator.h

+  OpDesc desc_;
+  std::vector<std::string> inputs_;
+  std::vector<std::string> outputs_;
+  AttributeMap attrs_;


Each operator should have a unique name. Is the name saved in attrs_ or desc_?

there is no uniq name now, maybe we can add one

… opbase

qingqing01 · 2017-07-10T05:57:32Z

paddle/framework/operator.h

+#include <boost/variant.hpp>
+#include <string>
+#include <unordered_map>
+#include <vector>


The above four lines can be removed. They have been included in paddle/framework/attr_checker.h.

reyoung · 2017-07-10T07:07:16Z

paddle/framework/operator.h

+  virtual void Run(OpContext* context) const = 0;
+
+ private:
+  OpDesc desc_;


Is OpDesc used for some functions?

to get type of op

inline const OpDesc desc() const { return desc_; }

reyoung · 2017-07-10T07:08:09Z

paddle/framework/operator.h

+    return boost::get<T>(attrs_.at(name));
+  }
+
+  inline const std::vector<std::string> inputs() const { return inputs_; }


Reference.

const std::vector<std::string>&.

Also, I suggest all private data member of OpBase should be public. Not to add noising Getter

I think the inputs_ are owned by an operator itself, better be a protected if subclasses want to edit, but with a const getter by visitors that just want to read, such as a Net?

Superjomn · 2017-07-10T09:06:25Z

paddle/framework/operator.cc

+
+void OperatorBase::InferShape(Scope* scope) const {}
+
+const std::string OperatorBase::DebugString() const {


why should this be a const?

the returned string is copied, and free to mutate?

const removed

Superjomn · 2017-07-10T09:11:56Z

paddle/framework/operator.h

+
+  inline const std::vector<std::string> inputs() const { return inputs_; }
+
+  inline const std::vector<std::string> outputs() const { return outputs_; }


do not need an inline here? a function defined within a class should be set inline by the compiler automatically.

Superjomn · 2017-07-10T09:12:27Z

paddle/framework/operator.h

+
+  inline const std::vector<std::string> outputs() const { return outputs_; }
+
+  const std::string DebugString() const;


const is useless here.

Superjomn · 2017-07-10T09:14:26Z

paddle/framework/operator.h

+  void InferShape(Scope* scope) const;
+
+  void Run(std::shared_ptr<Scope> scope, DeviceContext* dev_ctx) {
+    OpContext* op_ctx = new OpContext(this, scope, dev_ctx);


new here, no need to manage the memory?

… opbase

reyoung · 2017-07-11T07:33:13Z

paddle/framework/operator.cc

+namespace paddle {
+namespace framework {
+
+void OperatorBase::InferShape(std::shared_ptr<Scope> scope) const {}


reyoung · 2017-07-11T07:34:00Z

paddle/framework/operator.h

+  Variable* Output(int index) const;
+
+ public:
+  const OperatorBase* op;


op -> op_;

reyoung · 2017-07-11T07:34:10Z

paddle/framework/operator.h

+
+ public:
+  const OperatorBase* op;
+  std::shared_ptr<Scope> scope;


scope -> scope_

reyoung · 2017-07-11T07:34:21Z

paddle/framework/operator.h

+ public:
+  const OperatorBase* op;
+  std::shared_ptr<Scope> scope;
+  DeviceContext* device_context;


device_context -> device_context_;

reyoung · 2017-07-11T07:35:48Z

paddle/framework/operator.h

+  virtual ~OperatorBase() {}
+
+  template <typename T>
+  inline const T GetAttr(const std::string& name) const {


Useless function. boost::get<T>(attrs_.at(name)) is enough. Even this function is needed, it should belong to AttributeMap not OperatorBase

Also, this function should return const T& if needed.

I plan to store attrs as variable later, this interface is used for now

reyoung · 2017-07-11T07:36:28Z

paddle/framework/operator.h

+  virtual void InferShape(std::shared_ptr<Scope> scope) const;
+
+  /// Net will call this function to Run an op.
+  void Run(std::shared_ptr<Scope> scope, DeviceContext* dev_ctx) {


const std::shared_ptr&

reyoung · 2017-07-11T07:38:06Z

paddle/framework/operator.h

+
+  /// when implement an Op, your should implement this function.
+  /// this function should be moved to OpKernel later
+  virtual void Run(OpContext* context) const = 0;


OpContext --> OpRunContext

Also, this function is not in OperatorBase interface. The OperatorBase interface should only contains

virtual void Run(const std::shared_ptr<Scope>& scope, DeviceContext* dev_ctx) = 0;

yes, but now we have no kernel OP, and this interface is used by test now, so I keep it and will move it to op with kernel in the next PR

reyoung · 2017-07-11T07:38:26Z

paddle/framework/operator.h

+  virtual void Run(OpContext* context) const = 0;
+
+ public:
+  OpDesc desc_;


Is this field needed now?

yes, I think we need this field.

… opbase

reyoung

LGTM

… opbase

jacquesqiao added 7 commits July 4, 2017 11:41

add operator base

ff92a58

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

3f75b66

… opbase

import attr_type.proto

28e2313

add test

22c2bec

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

c6407c3

… opbase

do not return error when run

a591e78

remove Error of InitializeAttrs

8d8a448

jacquesqiao commented Jul 5, 2017

View reviewed changes

interface of operator

cb7c234

reyoung reviewed Jul 6, 2017

View reviewed changes

jacquesqiao added 3 commits July 6, 2017 20:31

refactor of operator

57f1f6a

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

c248ba0

… opbase

add comment, optimize code style

57d1fbd

wangkuiyi reviewed Jul 7, 2017

View reviewed changes

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

f7d825b

… opbase

Superjomn reviewed Jul 7, 2017

View reviewed changes

net interface of op

4f23ec3

qingqing01 reviewed Jul 7, 2017

View reviewed changes

refine operator interface

88f6621

jacquesqiao requested a review from JiayiFeng July 8, 2017 01:25

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

231e056

… opbase

qingqing01 reviewed Jul 10, 2017

View reviewed changes

rm name, use DeviceContext

2d1d021

jacquesqiao mentioned this pull request Jul 10, 2017

need op kernel for Operator #2790

Closed

jacquesqiao added 2 commits July 10, 2017 14:59

prepare for opkernel

1e35e83

change op in OpContext from ref to const pointer

4c68deb

reyoung reviewed Jul 10, 2017

View reviewed changes

Superjomn reviewed Jul 10, 2017

View reviewed changes

jacquesqiao added 5 commits July 10, 2017 17:25

change op member to public

d0fae91

remove New OpContext

40022f7

fix style problem

e34c579

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

b710d5b

… opbase

use fake DeviceContext

cd2338f

reyoung reviewed Jul 11, 2017

View reviewed changes

paddle/framework/operator.h Outdated

Variable* Output(int index) const;

public:

const OperatorBase* op;

Copy link

Collaborator

reyoung Jul 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

op -> op_;

reyoung reviewed Jul 11, 2017

View reviewed changes

jacquesqiao added 2 commits July 11, 2017 16:43

use const func call

dfbf13d

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

c9aa77c

… opbase

reyoung approved these changes Jul 11, 2017

View reviewed changes

jacquesqiao added 5 commits July 11, 2017 17:56

add operator with kernel

6a893c1

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

0fc8207

… opbase

merge new attr

9324915

optimize code

f0332fb

fix cpp style

c1d8cbb

jacquesqiao merged commit a2e5f65 into PaddlePaddle:develop Jul 11, 2017

reyoung added this to Done in PaddlePaddle Refactoring: Phase 1 Aug 2, 2017


		void OperatorBase::InferShape(Scope* scope) const {}

		const std::string OperatorBase::DebugString() const {


		inline const std::vector<std::string> inputs() const { return inputs_; }

		inline const std::vector<std::string> outputs() const { return outputs_; }


		inline const std::vector<std::string> outputs() const { return outputs_; }

		const std::string DebugString() const;

add operator base #2725

add operator base #2725

Conversation

jacquesqiao commented Jul 4, 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

wangkuiyi Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacquesqiao Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

Superjomn Jul 7, 2017 • edited Loading

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

qingqing01 Jul 7, 2017 • edited Loading

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

jacquesqiao Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Superjomn Jul 10, 2017 • edited Loading

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

jacquesqiao Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyoung left a comment

Choose a reason for hiding this comment

wangkuiyi Jul 7, 2017 •

edited

Loading

jacquesqiao Jul 7, 2017 •

edited

Loading

Superjomn Jul 7, 2017 •

edited

Loading

qingqing01 Jul 7, 2017 •

edited

Loading

jacquesqiao Jul 10, 2017 •

edited

Loading

Superjomn Jul 10, 2017 •

edited

Loading

jacquesqiao Jul 11, 2017 •

edited

Loading