use operator context and infer context #3024

jacquesqiao · 2017-07-23T16:56:28Z

now we use

virtual void InferShape(const std::vector<const Tensor*>& inputs,		
                          const std::vector<Tensor*>& outputs) const = 0;

for shape infer. but there are some problems:

some times it's more convinient to get tensor/variable with name that we defined in OpProto

auto wight = ctx.Input("wight");
auto bias = ctx.Input("bias");

for variant input and net op, the input index will change and we cannot make sure what the index means.

for complex op like rnn, some input's length is variant and the index will change with the length of variant Inputs. In this situation, we can only get what we need throgh name:

std::vector<Tensor*> x = ctx.Inputs("X");

… optimize-context

wangkuiyi · 2017-07-25T00:41:33Z

paddle/framework/operator.cc

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

+void OperatorBase::InferShape(const std::shared_ptr<Scope>& scope) const {
+  InferShapeImpl(InferShapeContext(this, scope));


Does scope here have to be of type shared_ptr? It seems simpler if we can use const Scope&.

It cannot be done, because inside InferShape/Run in some operator, e.g., RNN, the developer will create a new local Scope which uses std::shared_ptr<Scope> as an argument.

这里传递指针的引用确实比较confusing，能加一下注释吗？
指针的引用表示指针本身也会被改变，那改变之后，这个指针之前指向的对象怎么办呢？会有内存泄漏吗？

wangkuiyi · 2017-07-25T00:50:22Z

paddle/framework/net.h

@@ -57,9 +57,9 @@ class PlainNet : public Net {
   * Infer all the operators' input and output variables' shapes, will be called
   * before every mini-batch
   */
-  void InferShape(const std::shared_ptr<Scope>& scope) const override {
+  void InferShapeImpl(const InferShapeContext& ctx) const override {


In my mind, Impl is usually a suffix for a class name, which implements an interface. Do we really need to name a function Impl here?

That's right, maybe just called InferShape is cool.

wangkuiyi · 2017-07-25T02:42:26Z

paddle/framework/operator.h

    return scope_->GetVariable(op_.outputs_[index]);
  }

-  const Variable* Input(const std::string& name) const {
+  const Variable* InputVar(const std::string& name) const {


可不可以 std::vector<const Variable*> 整个作为 Variable::Get()的类型：

typedef std::vector<Tensor*> TensorArray; TensorArray tensors = var.Get<TensorArray>();

wangkuiyi · 2017-07-25T02:44:18Z

paddle/framework/operator.h

@@ -110,29 +98,32 @@ class OperatorBase {
  std::shared_ptr<std::unordered_map<std::string, int>> in_out_idxs_;
 };

-class KernelContext {
+class OperatorContext {


OperatorContext => ExecutionContext?

我们会有两个概念分别叫做 OperatorContext 和 KernelContext 的吗？如果其实没有，那么就叫 Context 或者 ExecutionContext 是不是更清楚？

ExecutionContext is better

We actually have two contexts, one for InferShape, other for Run.

See line 35 and 36

QiJune · 2017-07-25T02:58:43Z

paddle/framework/operator.h

-                const platform::DeviceContext& device_context)
-      : op_(*op), scope_(scope), device_context_(device_context) {}
+  OperatorContext(const OperatorBase* op, const std::shared_ptr<Scope>& scope)
+      : op_(*op), scope_(scope) {}


Have to check OperatorBase* op not null first.
And we use const OperatorBase& op_ as a member, why not const std::shared_ptr<OperatorBase> op_

we donot need to check because context will only be construct inside a op, so op will nevel be null. And so be need not to use std::shared_ptr

… optimize-context

Superjomn · 2017-07-25T07:40:41Z

paddle/framework/operator.h

-  const Variable* Input(int index) const {
+  int OutputSize() const { return static_cast<int>(op_.outputs_.size()); }
+
+  const Variable* InputVar(int index) const {


In OperatorBase , Input returns string, In Context, Input returns Tensor, and here is another InputVar

Can we uniform all the Input()s, and use a single API like template <typename T> input(std::string), and implements three types: string, Tensor, Variable.

This is much simplier to understand. @jacquesqiao

great suggestion, thanx!

JiayiFeng · 2017-07-25T08:53:36Z

paddle/framework/operator.h

@@ -84,7 +71,8 @@ class OperatorBase {

  /// InferShape infer the size of Variables used by this Operator with
  /// information inside scope
-  virtual void InferShape(const std::shared_ptr<Scope>& scope) const = 0;
+  virtual void InferShape(const std::shared_ptr<Scope>& scope) const final;


Here we use the reference of std::shared_ptr<Scope> as a parameter, and all InferShape share the same std::shared_ptr<Scope>.
Does it means that there will be only one std::shared_ptr<Scope> ? If so, why not use std::unique_ptr<Scope> instead?

reyoung

Please complete unittest

reyoung · 2017-07-27T04:22:27Z

paddle/framework/operator.cc

+  auto names = op_.Inputs(name);
+  std::vector<const Variable*> res;
+  std::transform(
+      names.begin(), names.end(), res.begin(),


std::back_inserter(res) ? because res is empty, transform cannot copy data to res container.

reyoung · 2017-07-27T04:24:03Z

paddle/framework/operator.cc

+std::vector<const Variable*> OperatorContext::MultiOutput(
+    const std::string& name) const {
+  auto names = op_.Outputs(name);
+  std::vector<const Variable*> res;


Same as above

reyoung · 2017-07-27T04:28:03Z

paddle/framework/operator.h

+  OperatorContext(const OperatorBase* op, const std::shared_ptr<Scope>& scope)
+      : op_(*op), scope_(scope) {}
+
+  int InputSize() const { return static_cast<int>(op_.inputs_.size()); }


??? why not size_t?

… optimize-context

reyoung · 2017-07-27T04:36:35Z

paddle/operators/add_op.cc

+  void InferShape(const framework::InferShapeContext &ctx) const override {
+    PADDLE_ENFORCE(ctx.InputSize() == 2, "Input size of AddOp must be two");
+    PADDLE_ENFORCE(ctx.OutputSize() == 1, "Output size of AddOp must be one");
+    PADDLE_ENFORCE(ctx.Input<framework::Variable>(0) != nullptr &&


The lines of code are not shrunk at all.

Maybe we could add a default template argument as framework::Variable?

template <typename T = framework::Variable> T Input(size_t idx);

I am not sure if that is a good design or not.

currently, the return type is a pointer, look like this

template <typename T = framework::Variable> T* Input(size_t idx);

but this does not support std:: string, that means we will add another 4 interfaces like:

std::string InputName(); std::vector<std::string> MultiInputNames(); std::string OutputName(); std::vector<std::string> MultiOutputNames();

Is there some way to embed std::string as a template type as follows ?

auto variable_name = Input<std::string>("x")

@jacquesqiao @reyoung

reyoung · 2017-07-27T04:37:35Z

paddle/framework/operator.cc

@@ -99,5 +99,48 @@ std::string OperatorBase::DebugString() const {
  return ss.str();
 }

+template <>
+const Variable* OperatorContext::Input<Variable>(int index) const {


Please use size_t instead of int.

size_t is the standard type for stl container.

reyoung · 2017-07-27T04:37:42Z

paddle/framework/operator.cc

+}
+
+template <>
+Variable* OperatorContext::Output<Variable>(int index) const {


int --> size_t

reyoung · 2017-07-27T04:39:02Z

paddle/framework/operator.h

@@ -110,29 +98,32 @@ class OperatorBase {
  std::shared_ptr<std::unordered_map<std::string, int>> in_out_idxs_;
 };

-class KernelContext {
+class OperatorContext {


We actually have two contexts, one for InferShape, other for Run.

reyoung · 2017-07-27T04:41:12Z

paddle/framework/operator.h

+  void InferShape(const std::shared_ptr<Scope>& scope) const {
+    InferShape(InferShapeContext(this, scope));
+  }
+  virtual void InferShape(const InferShapeContext& ctx) const = 0;


Make this interface protected

reyoung · 2017-07-27T04:49:04Z

paddle/framework/operator.h

+  int OutputSize() const { return static_cast<int>(op_.outputs_.size()); }
+
+  template <typename T>
+  const T* Input(int index) const {


Why return const T* not const T&

… optimize-context

Superjomn · 2017-07-31T23:25:44Z

paddle/framework/operator.h

  }

-  const Variable* Input(const std::string& name) const {
+  const Variable* InputVar(const std::string& name) const {


Still need InputVar although we have template <typename T> Input(name) ?

Isn't InputVar is Input<Variable> ?

Superjomn · 2017-07-31T23:27:29Z

paddle/framework/operator.h

        [this](const std::string& name) { return scope_->GetVariable(name); });
    return res;
  }

+  template <typename T>
+  const T* Input(size_t index) const {
+    return &(InputVar(index)->Get<T>());


add a template specification here to support Variable ?

template <> Variable* Input<Variable>(const std::string &name);

Superjomn · 2017-07-31T23:27:50Z

paddle/framework/operator.h

+  }
+
+  template <typename T>
+  T* Output(const std::string& name) const {


same as top

Superjomn · 2017-07-31T23:36:21Z

paddle/operators/sigmoid_op.cc

-    PADDLE_ENFORCE(outputs.size() == 1, "Sigmoid Op only have one output");
-    outputs[0]->Resize(inputs[0]->dims());
+  void InferShape(const InferShapeContext &ctx) const override {
+    PADDLE_ENFORCE(ctx.InputSize() == 1, "Sigmoid Op only have one input");


enforce the number of real inputs is the duty of OperatorBase, here 1 is just the number of inputs in definition of sigmoid.

The simgoid's op definition has only one input X

SigmoidOpMaker(OpProto *proto, OpAttrChecker *op_checker) : OpProtoAndCheckerMaker(proto, op_checker) { AddInput("X", "sigmoid input"); AddOutput("Y", "sigmoid output"); AddComment("Sigmoid function"); }

so we need OperatorBase's infershape to automatically enforce number of inputs and outputs, the real operator's implementation doesn't need to enforce itself.
@jacquesqiao @reyoung @QiJune

yes, the input size condition should be guaranteed by the Op creator.

Superjomn · 2017-07-31T23:45:58Z

paddle/operators/softmax_op.cc

-    PADDLE_ENFORCE(inputs.size() == 1, "Only one input is need for softmax");
-    PADDLE_ENFORCE(inputs[0]->dims().size() == 2,
+  void InferShape(const InferShapeContext &ctx) const override {
+    PADDLE_ENFORCE(ctx.InputSize() == 1, "Only one input is need for softmax");


same as top, when an operator InferShape, it means to check the weather the real inputs and outputs match the definition in its op_proto.

here the 1 is softmax_op.op_proto.inputs().size(), and OperatorBase can do this enforce automatically for SoftmaxOp.

reyoung

LGTM

jacquesqiao added 3 commits July 24, 2017 00:52

use operator context

299525d

optimize code

11eabf8

update net infershape

4280a60

jacquesqiao requested review from reyoung, a user, QiJune and Superjomn July 24, 2017 07:39

jacquesqiao added 3 commits July 24, 2017 19:46

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

dda4881

… optimize-context

update InferShape

fb1b3d1

disable override InferShape(scope) in OperatorBase

081c7ca

wangkuiyi reviewed Jul 25, 2017

View reviewed changes

QiJune reviewed Jul 25, 2017

View reviewed changes

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

5273c7e

… optimize-context

Superjomn reviewed Jul 25, 2017

View reviewed changes

JiayiFeng reviewed Jul 25, 2017

View reviewed changes

jacquesqiao added 4 commits July 25, 2017 17:17

change InferShapeImpl to InferShape

0d693fe

add template to OperatorContext Input/Output

bf3940b

merge Input InputVar, Output OutputVar

362ba2f

change Inputs to MultiInput

a4bfb61

reyoung reviewed Jul 27, 2017

View reviewed changes

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

217186e

… optimize-context

reyoung reviewed Jul 27, 2017

View reviewed changes

jacquesqiao added 8 commits July 27, 2017 13:31

fix conflict

1c91df3

fix MultiInput bugs and add unit test

2460af4

rename KernelContext to ExecutionContext

fb1980e

clean code

9ff3595

change InferShape to protected

9fafc46

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

e87d253

… optimize-context

fix template bug

9a2640b

refine code

b6764c9

jacquesqiao added 4 commits July 31, 2017 07:04

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

fab7737

… optimize-context

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

e4445d6

… optimize-context

use InputVar instead of Input<Variable>

eda5493

typo

5f0ed40

Superjomn reviewed Jul 31, 2017

View reviewed changes

optimize code

bd8872c

reyoung approved these changes Aug 1, 2017

View reviewed changes

jacquesqiao merged commit 61ebacb into PaddlePaddle:develop Aug 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use operator context and infer context #3024

use operator context and infer context #3024

jacquesqiao commented Jul 23, 2017 •

edited

Loading

wangkuiyi Jul 25, 2017

reyoung Jul 25, 2017

QiJune Jul 25, 2017

wangkuiyi Jul 25, 2017

reyoung Jul 25, 2017

wangkuiyi Jul 25, 2017

wangkuiyi Jul 25, 2017

jacquesqiao Jul 25, 2017

reyoung Jul 27, 2017

reyoung Jul 27, 2017

QiJune Jul 25, 2017

jacquesqiao Jul 25, 2017

Superjomn Jul 25, 2017

jacquesqiao Jul 25, 2017

JiayiFeng Jul 25, 2017

reyoung left a comment

reyoung Jul 27, 2017

reyoung Jul 27, 2017

reyoung Jul 27, 2017

reyoung Jul 27, 2017

Superjomn Jul 27, 2017

reyoung Jul 27, 2017

reyoung Jul 27, 2017

reyoung Jul 27, 2017

reyoung Jul 27, 2017

reyoung Jul 27, 2017

Superjomn Jul 31, 2017

Superjomn Jul 31, 2017

Superjomn Jul 31, 2017

Superjomn Jul 31, 2017

jacquesqiao Aug 1, 2017

Superjomn Jul 31, 2017

reyoung left a comment

use operator context and infer context #3024

use operator context and infer context #3024

Conversation

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

reyoung left a 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 left a comment

Choose a reason for hiding this comment

jacquesqiao commented Jul 23, 2017 •

edited

Loading