move net_design to framework #2553

Superjomn · 2017-06-22T01:59:49Z

I fixed all the comments in the previous PR

wanghaoshuang · 2017-06-22T02:19:55Z

paddle/framework/net_design.md

+// The minimum a network should be implemented.
+class NetworkBase {
+public:
+  BaseNetwork(const NetDef &def, Scope *scope);


#2547 (comment)
If Network won`t hold scope, why we feed a scope to constructor?

wanghaoshuang · 2017-06-22T02:22:43Z

paddle/framework/net_design.md

+A simple implemention is as followed:
+
+```c++
+class Network final : public BaseNetwork {


I think you fogot modifying 'BaseNetwork'.

wangkuiyi · 2017-06-22T03:07:33Z

Please consider the following use case while we go on improving the design:

Workspace w1(GPUPlace(0));
Workspace w2(CPUPlace());

w1.CreateVariables(net_desc);
w1.InitVariables(net_desc);
w1.GetVar("weights")->FixValue();

w2.CreateVariables(net_desc);
w2.InitVariables(net_desc);
w2.GetVar("hello")->SetValue(...);

auto net = CreateNet(net_desc);

net.Run(&w1);

net.Run(&w2);

reyoung · 2017-06-23T02:57:10Z

paddle/framework/net_design.md

+
+  // run all the operators and return success(true) or not, all the
+  // variables are located in `scope`.
+  virtual bool Run(Scope* scope) = 0;


There is a discussion in here. Should our Operator be stateless? If our Operator is stateless, then NetBase::Run() should be const.

wanghaoshuang · 2017-06-23T03:02:42Z

paddle/framework/net_design.md

+A method of factory pattern can be defined like
+
+```c++
+std::unique<NetworkBase* CreateNet(const NetDef& def) {


lost a '>'?

wanghaoshuang · 2017-06-23T03:17:55Z

paddle/framework/net_design.md

+
+ protected:
+  // Create operators accordding to `def`.
+  bool CreateNet(const NetDef &def);


What`t the difference ScratchNet::CreateNet and CreateNet in line#43. Or shall we rename ScratchNet::CreateNet to a better name, such as: CreateOps ?

I am not sure ScratchNet.CreateNet just will create operators, maybe some other things should do like some network-level initializations.

Maybe we should rename it to InitNet ? @wanghaoshuang

reyoung · 2017-06-23T05:28:04Z

paddle/framework/net_design.md

+class NetworkBase {
+ public:
+  // `def` is a proto message that describe the structure of a network.
+  NetworkBase(const NetDef& def);


Maybe addOperator method is very useful, especially when we generating backward ops and generating optimization ops.

Also, the constructor of NetworkBase should invoke addOperator as well.

class NetworkBase { public: Error AddBackwardOps(const std::string& lossVariableName, float lossGrad = 1.0f) { for (eachOp reversely) { for (gradOp in GetGradientOps(eachOp)) { addOperator(gradOp) } } } Error AddOptimizerOps(const Map<string, string> paramToGradMap) { for (paramName, gradName in paramToGradMap) { addOperator(SGD(input={gradName, paramName}, output={paramName})); } } };

Is it better to unify all the AddXXOps to AddOp(const OpDef &def) in base class ?

Or move the above interfaces into ScratchNet? @reyoung

reyoung · 2017-06-23T05:31:06Z

paddle/framework/net_design.md

+class NetworkBase {
+ public:
+  // `def` is a proto message that describe the structure of a network.
+  NetworkBase(const NetDef& def);


Also, If NetworkBase is a base class, the constructor may not include NetDef protobuf. Because it is not used in base class.

reyoung · 2017-06-23T05:31:37Z

paddle/framework/net_design.md

+
+ protected:
+  // keys of the input variables feed into the network.
+  std::vector<string> inputs_;


Why a network needs inputs_ & outputs_ field?

reyoung · 2017-06-23T05:34:41Z

paddle/framework/net_design.md

+ public:
+  // `def` is a proto message that describe the structure of a network.
+  NetworkBase(const NetDef& def);
+


I think addOperator should return a Index. Maybe code like this,

class NetworkBase { public: using OpIndex = size_t; OpIndex addOperator(const OpDesc& op); };

So that Run method can take a begin and end argument like:

bool Run(Scope* scope, OpIndex begin = -1UL, OpIndex end = -1UL);

It will be very helpful if a network can run partially.

@reyoung Network maybe run from multiple start-operators to multiple end-operators?

Why don't we use the OpName instead of the OpIndex?

Agree with @wanghaoshuang. It seems that operators within [begin_index, end_index] cannot define arbitrary sub-DAG.

reyoung · 2017-06-23T05:35:08Z

paddle/framework/net_design.md

+
+  // run all the operators and return success(true) or not, all the
+  // variables are located in `scope`.
+  virtual bool Run(Scope* scope) = 0;


Maybe Run can return paddle::Error.

reyoung · 2017-06-23T05:39:19Z

paddle/framework/net_design.md

+A very basic implemention is as followed, all it does is simply to run every operators in sequence.
+
+```c++
+class ScratchNet final : public NetworkBase {


Sorry, I don't understand why the basic implementation is called Scratch

Build a network from scratch?

Or name it "BasicNet"? @reyoung

Or SimpleNet as same as Caffe2?

reyoung · 2017-06-23T05:39:33Z

paddle/framework/net_design.md

+
+## A Simple Network Implemention
+
+A very basic implemention is as followed, all it does is simply to run every operators in sequence.


implemention -> implementation

as followed --> as follows.

", all" --> "All"

reyoung · 2017-06-23T05:42:21Z

paddle/framework/net_design.md

+  // Add a operator which is identified as `type` and has attributes described
+  // in `attrs`, the `inputs` are the keys of readonly input variables,
+  // `outputs` are keys of mutable output variables.
+  bool AddOp(const std::string &type, const std::vector<string> &inputs,


Maybe addOp should be an interface of NetworkBase. Because there may be some instance will manipulate NetworkBase in CPP, such as adding gradient ops, adding optimization ops, etc.

If we want user passing a protobuf message contains backward ops, we need to write AddGradientOps in Python. It will let Paddle coupled with Python language, which we did not want to.

reyoung · 2017-06-23T05:43:21Z

paddle/framework/net_design.md

+
+ private:
+  // the operators owned by `Network`.
+  std::vector<std::unique_ptr<Operator>> ops_;


Maybe just std::vector<Operator> ops_ is enough? I am not sure. But it seems in interface, we are not using ops_ as a pointer?

reyoung · 2017-06-23T05:44:26Z

paddle/framework/net_design.md

+
+```c++
+// create an empty scope located on CPU device.
+Scope scope(CPUPlace());


I think scope do not take Place as an argument. A scope is just an association from name to variable. Each tensor in variables could have different Place.

sample code
As described in the sample code, Scope has a place, maybe more discussion here? @wangkuiyi @reyoung

reyoung · 2017-06-23T05:49:36Z

paddle/framework/net_design.md

+Benefit from the decoupling of `ScratchNet.Run` and `Scope`, `ScratchNet` is compatible with future RNN design, 
+for example we can implement a simple recurrent neural network as followed
+
+```c++


Well, this part should be rewritten.

Scope contains a parent argument. The local scope can access the parent scope variables. It should not copy variables between scopes.

Scope parent; // in RNN NetworkBase& stepNet; std::vector<Scope> timestepScopes; for (size_t i=0; i < seqLen; ++i) { timestepScopes.push_back(Scope(&parent)); stepNet.Run(&timestepScopes); }

I will give another commit soon to fix this.

jacquesqiao · 2017-06-23T05:51:04Z

paddle/framework/net_design.md

+
+Network is designed as the container of operators, to make it more extendable,
+we decompling it from the related variable resources. 
+A `scope` is provided to `Run` so that the network structure can be reused 


I think

Run(Scope* scope) takes a scope as the parameter and so that it can run in different scopes

is better for understanding.

jacquesqiao · 2017-06-23T05:55:07Z

paddle/framework/net_design.md

+
+ protected:
+  // Create operators accordding to `def`.
+  bool CreateNet(const NetDef &def);


Maybe InitNet() is better? Because I think CreatNet will return a created Net and InitNet will fulfill and init itself.

jacquesqiao · 2017-06-23T06:23:40Z

paddle/framework/net_design.md

@@ -0,0 +1,159 @@
+# Network Design
+
+`Network` is the container and controller of a set of operators in a network, 


Network is the container and controller of a set of operators in a network.
=>
Network is the container and controller of a set of operators.

jacquesqiao · 2017-06-23T06:24:12Z

paddle/framework/net_design.md

+# Network Design
+
+`Network` is the container and controller of a set of operators in a network, 
+users can build a real network from a `NetDef` in protobuf message 


or use AddOp to modify this Net

xinghai-sun · 2017-06-23T06:51:53Z

paddle/framework/net_design.md

+ public:
+  // `def` is a proto message that describe the structure of a network.
+  NetworkBase(const NetDef& def);
+


Agree with @wanghaoshuang. It seems that operators within [begin_index, end_index] cannot define arbitrary sub-DAG.

xinghai-sun · 2017-06-23T06:52:47Z

paddle/framework/net_design.md

+  virtual bool Run(Scope* scope) const = 0;
+
+ protected:
+  // keys of the input variables feed into the network.


feed --> fed

xinghai-sun · 2017-06-23T06:56:04Z

paddle/framework/net_design.md

+
+All network implementations should build networks from  a protobuf message which 
+describes the structure of a real network; `Run` method should be implemented by 
+all implementations to offer a universal method to forward or backward compute a network.


should be implemented by all implementations --> should be implemented by all its inheriting class

xinghai-sun · 2017-06-23T07:30:23Z

paddle/framework/net_design.md

+}
+```
+
+Network is designed as the container of operators, to make it more extendable,


--> Network is designed as a container for operators, with related variable resources decoupled from it to make it more extendable.

xinghai-sun · 2017-06-23T07:32:17Z

paddle/framework/net_design.md

+A `scope` is provided to `Run` so that the network structure can be reused 
+in different scopes.
+
+Finally, `NetworkBase` can be used as followed


as followed --> as follows:

xinghai-sun · 2017-06-23T07:35:10Z

paddle/framework/net_design.md

+
+## A Simple Network Implemention
+
+A very basic implemention is as followed, all it does is simply to run every operators in sequence.


as followed --> as follows.

", all" --> "All"

reyoung · 2017-06-23T09:08:33Z

@wangkuiyi

我觉得NetworkBase中，包含AddOperator(const proto::OperatorDesc& );的接口是很必要的。

因为:

Paddle让用户配置的只有神经网络的前向部分。而Backward和SGD的一些Op(每一个前向Op都对应一个或者多个Backward Op，每一个参数都对应着一个SGD Op)都是由Paddle框架添加的。
如何添加Backward和SGD Op的逻辑应该写在C++端。因为这段逻辑如果写在Python端，如果我们需要其他语言的language binding，我们还要再其他语言重写一遍。
1.添加Backward和SGD Ops的函数，应该直接操作NetworkBase类型。没必要通过protobuf中转一遍。因为这些函数都是C++函数了。
所以，在AddBackwardOps函数中，需要调用NetworkBase::AddOperator(const proto::OperatorDesc&)接口。

故NetworkBase::AddOperator接口是必要的，也得是public的。

Superjomn · 2017-06-23T10:09:29Z

paddle/framework/net_design.md

+Scope scope(CPUPlace());
+
+// create and init variables described in `net_desc`.
+scope.CreateVariables(net_desc);


NetworkBase has a member function called InferShape(Scope*) to infer shapes and create variables in scope, is its function duplicate with scope.CreateVariables?

Because the variables' information is located in OpDef and there is no field called vars in NetDef that holds all the information of variables required by the network, is scope.CreateVariables necessary?
@wangkuiyi @reyoung @jacquesqiao

Variable information is not stored in NetDef, and in OpDef, there will only be input/output names for Variable. So CreateVariables be performed only by NetDef.

Maybe there should be a high-level concept called NetworkBuilder or Model. A NetworkBuilder takes a NetworkBase and a Scope as its inputs. In NetworkBuilder, the user can add layers, add backward ops, and get all parameters. During AddLayer functions, the variable is created by Network Builder.

NetworkBase network; Scope scope; Variable* image = scope.CreateVariable("Image"); Variable* label = scope.CreateVariable("Label"); NetworkBuilder builder(&network, &scope); Variable* fc_out = builder.FCLayer(input=image, size=100, activation="Sigmoid"); Variable* prediction = builder.FCLayer(input=fc_out, size=10, activation="Sigmoid"); Variable* loss = builder.CrossEntropy(input=prediction, label=label); Variable* avg_loss = builder.Mean(loss); auto allParams = builder.Parameters(); builder.BackwardFrom(avg_loss) builder.AddOptimization(1e-4, "adam"); // train one mini-batch network.run(&scope);

Can we merge NetworkBuiler into NetworkBase?
NetworkBase is the base class of all "network" implementations, with a different implementation, NetBuilder may be different.
For example, SimpleNet 's network structure is different with DAGNet 's structure,
so a different NetBuilder.ApplyGradient may be needed. In other words, NetBuilder is coupled to specific implementation of NetworkBase.

If we merge NetBuilder's functions into NetworkBase 's specific implementations, then it is natural to have different "BackwardFrom" implementations.
@reyoung

Maybe the logic of ApplyGradient is same for each implementation of NetworkBase. The different between PlainNet and other implementation is how to implement AddOperator.

I create another PR with NetBuilder
NetBuilder is treated as a user-friendly syntax API.
NetBase is the detailed implementation of Network-related APIs.

Users can create BaseNet using NetBuilder, but the detail of BaseNet is not visible to them.
With this two-level design, we will focus our use-related API on NetBuilder and implementation details only on NetBase.

Both of them are not coupled with each other.

@reyoung

* add senet * add annotations according to review

move net_design to framework

5886959

Superjomn requested review from a user, dzhwinter, xinghai-sun and wanghaoshuang June 22, 2017 02:00

Superjomn added 2 commits June 22, 2017 10:10

add nullptr to Network.Run

a00900a

add usage

4fb581b

wanghaoshuang reviewed Jun 22, 2017

View reviewed changes

remove scope* in construction

bb33f7a

Superjomn added this to Doing in PaddlePaddle Refactoring: Phase 1 Jun 22, 2017

Superjomn added 11 commits June 22, 2017 11:14

remove AddOp from user's interface

a7dbfe0

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

8cf2d60

net new design

2933da0

add more details to net design

7d5b1b2

disable CreateOp from user

b6450da

move w1 to scope

a46506d

add rnn compatibility

3c7bf55

fix english grammer

ebe143d

rrn -> rnn

4e5a359

change CreateNet result to unique_ptr

9be4c74

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

7102b80

reyoung reviewed Jun 23, 2017

View reviewed changes

wanghaoshuang reviewed Jun 23, 2017

View reviewed changes

Superjomn added 2 commits June 23, 2017 13:40

make Run() stateless

2953215

complete a unique_ptr's >

58fdcd0

Superjomn requested a review from wangkuiyi June 23, 2017 05:44

reyoung reviewed Jun 23, 2017

View reviewed changes

jacquesqiao reviewed Jun 23, 2017

View reviewed changes

pr details fix

417b279

xinghai-sun reviewed Jun 23, 2017

View reviewed changes

add InferShape

b68c8bf

Superjomn commented Jun 23, 2017

View reviewed changes

Superjomn added 3 commits June 26, 2017 08:58

rename "ScratchNet" -> "PlainNet"

62823b1

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

deff7cc

detail config

740f3a6

Superjomn closed this Jun 29, 2017

Superjomn moved this from Doing to Done in PaddlePaddle Refactoring: Phase 1 Aug 2, 2017

heavengate pushed a commit to heavengate/Paddle that referenced this pull request Aug 16, 2021

add senet (PaddlePaddle#2553)

f239d89

* add senet * add annotations according to review


		## A Simple Network Implemention

		A very basic implemention is as followed, all it does is simply to run every operators in sequence.

		@@ -0,0 +1,159 @@
		# Network Design

		`Network` is the container and controller of a set of operators in a network,

move net_design to framework #2553

move net_design to framework #2553

Conversation

Superjomn commented Jun 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangkuiyi commented Jun 22, 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

wanghaoshuang Jun 23, 2017 • edited

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

Superjomn Jun 23, 2017 • edited

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 Jun 23, 2017

Superjomn Jun 23, 2017 • edited

Choose a reason for hiding this comment

reyoung Jun 24, 2017 • edited

Choose a reason for hiding this comment

Superjomn Jun 26, 2017 • edited

Choose a reason for hiding this comment

reyoung Jun 26, 2017 • edited

Choose a reason for hiding this comment

Superjomn Jun 26, 2017 • edited

Choose a reason for hiding this comment

Superjomn commented Jun 22, 2017 •

edited

wanghaoshuang Jun 23, 2017 •

edited

Superjomn Jun 23, 2017 •

edited

Superjomn Jun 23, 2017 •

edited

reyoung Jun 24, 2017 •

edited

Superjomn Jun 26, 2017 •

edited

reyoung Jun 26, 2017 •

edited

Superjomn Jun 26, 2017 •

edited