new parameterupdater use paddle pserver cclient of go #2413

jacquesqiao · 2017-06-07T15:45:06Z

refs:
issue: #2274
old pr: #2330

测试方法：
1，编译。
2，编译完成之后运行 go run ./go/cmd/pserver/pserver.go -port 3000 会在本地启动一个parameter server，监听3000端口。
3，python paddle/go/pserver/cclient/test/test_train.py

增加了一个简单的梯度下降optimizer，已经可以正常训练和收敛了

dzhwinter · 2017-06-07T16:08:43Z

paddle/trainer/NewRemoteParameterUpdater.h

+   * @param new_paras
+   * @param type
+   */
+  void initNewParameter(paddle_parameter**& new_paras, ParameterType type) {


这个引用去掉是不是比较好？这里只是 *type，指针拷贝没有代价。这样写风格有点怪~

initNewParameter seems only called inside this class, should it be "private"?

dzhwinter · 2017-06-07T16:13:53Z

go/pserver/cclient/test/main.c

@@ -41,30 +43,31 @@ int main() {

  unsigned char content[] = {0x00, 0x11, 0x22};
  paddle_gradient grads[2] = {
-      {"param_a", PADDLE_ELEMENT_TYPE_INT32, content, 3},
-      {"param_b", PADDLE_ELEMENT_TYPE_FLOAT32, content, 3}};
+      {"param_a", PADDLE_ELEMENT_TYPE_FLOAT32, content, 3},


yes! I remember there is a bug here when I test the pserver program

dzhwinter · 2017-06-07T16:41:50Z

paddle/trainer/NewRemoteParameterUpdater.h

+    new_paras =
+        (paddle_parameter**)malloc(sizeof(paddle_parameter*) * parameterSize());
+    for (int i = 0; i < parameterSize(); ++i) {
+      new_paras[i] = (paddle_parameter*)malloc(sizeof(paddle_parameter));


没有找到对应的free。
另外，

//cclient.go static inline void paddle_release_param(paddle_parameter* param)

释放parameter内存不应该由cclient负责吧？ @helinwang ，既然由remoteUpdater申请内存，释放空间最好还是在同一个scope内，否则容易出错。
我觉得这里用智能指针更好一些？

对的，parameter内存不由cclient拥有。

NewRemoteParameterUpdater destruct的时候需要释放对应成员变量的内存。

目前parameterUpdater只分配了几个指针数组，而没有分配具体的content buffer，所以只负责free自己分配的几个指针成员

dzhwinter · 2017-06-07T16:49:34Z

paddle/trainer/NewRemoteParameterUpdater.h

+      new_paras[i]->element_type = PADDLE_ELEMENT_TYPE_FLOAT32;
+      new_paras[i]->name = (char*)para->getName().c_str();
+      new_paras[i]->content =
+          (unsigned char*)(para->getBuf(type).get()->getData());


just a suggestion, it's up to you, do we need to unify the variable abbrevtion？ helin used to named parameter with param, this part named with para. How do you think? Both is fine to me.

dzhwinter · 2017-06-07T16:51:44Z

paddle/trainer/NewRemoteParameterUpdater.h

+  NewRemoteParameterUpdater(const OptimizationConfig& config,
+                            const std::string pserverSpec);
+  ~NewRemoteParameterUpdater() {
+    if (newGradients_) {


I can't understand this condition, and we need to free the newGradient after release client, aren't we?

helinwang · 2017-06-07T18:19:54Z

@jacquesqiao In future it's better to use the same PR, so reviewer can track the comment / reply. You can just force push to the PR branch if you rebased / did a merge. Please ask me if you are not sure how to do this :)

helinwang · 2017-06-07T18:46:52Z

paddle/trainer/NewRemoteParameterUpdater.cpp

+
+namespace paddle {
+NewRemoteParameterUpdater::NewRemoteParameterUpdater(
+    const OptimizationConfig &config, const std::string pserverSpec)


Just to confirm, does OptimizationConfig have all information about the optimizer used on each parameter? (parameters can have their own learning rate, L2 / L1 norm)

In fact, one OptimizationConfig is not enough, it needs more kinds optimizers when training a complicated network. e.g. some part use adagrad optimizer, some part use Ftrl optimizer https://arxiv.org/abs/1606.07792, which is I mentioned in last week. we can polish this part after we combining these modules together.

done, send the old parameter config to go cclient for now.

helinwang · 2017-06-07T19:02:36Z

paddle/trainer/NewRemoteParameterUpdater.cpp

+
+  // create parameter server client.
+  parameterClient_ =
+      paddle_new_pserver_client((char *)pserverSpec_.c_str(), FLAGS_trainer_id);


I think here should be paddle_new_pserver_client(..., FLAGS_trainer_id == 0);

helinwang · 2017-06-07T19:04:55Z

paddle/trainer/NewRemoteParameterUpdater.cpp

+    LOG(INFO) << "paddle_begin_init_params done";
+  } else {
+    paddle_get_params(
+        parameterClient_, names_, newParameters_, (int)parameters_.size());


It's more consistent / clear if use parameterSize() rather than parameters_.size().

helinwang · 2017-06-07T19:08:57Z

paddle/trainer/NewRemoteParameterUpdater.h

+  /// internal parameter client object for exchanging data with pserver
+  client parameterClient_ = -1;
+  /// the parameters for new pserver client
+  paddle_parameter** newParameters_;


These three pointers need to be freed in the destructor.

helinwang · 2017-06-07T20:26:10Z

paddle/trainer/NewRemoteParameterUpdater.h

+
+    for (int i = 0; i < parameterSize(); ++i) {
+      ParameterPtr para = parameters_[i];
+      new_paras[i]->content_len = 10;


I think having 10 here is not correct.

done, 笔误

helinwang · 2017-06-07T20:28:18Z

paddle/trainer/NewRemoteParameterUpdater.h

+    new_paras =
+        (paddle_parameter**)malloc(sizeof(paddle_parameter*) * parameterSize());
+    for (int i = 0; i < parameterSize(); ++i) {
+      new_paras[i] = (paddle_parameter*)malloc(sizeof(paddle_parameter));


对的，parameter内存不由cclient拥有。

helinwang · 2017-06-07T20:30:30Z

paddle/trainer/NewRemoteParameterUpdater.h

+    new_paras =
+        (paddle_parameter**)malloc(sizeof(paddle_parameter*) * parameterSize());
+    for (int i = 0; i < parameterSize(); ++i) {
+      new_paras[i] = (paddle_parameter*)malloc(sizeof(paddle_parameter));


NewRemoteParameterUpdater destruct的时候需要释放对应成员变量的内存。

typhoonzero · 2017-06-08T02:16:39Z

paddle/trainer/NewRemoteParameterUpdater.h

+  /**
+   * initialize the internal parameter client and itself.
+   */
+  virtual void init(const std::vector<ParameterPtr>& parameters);


If we are using google c++ style naming, should change the styles. Or it will be of differnet style to #2386

Or are we going to respect code style currently used in RemoteParameterUpdater.h?

we will change the style to google c++ style, it is the most popular style in c++ opensource world, why not?

emm... Do we need to unify the Go style and c++ style in naming function? It seems that Go style function naming convention is better. How do you think?

We need to following Google C++ style for C++ files.
Btw, for C file we have a different style used here: https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/capi/main.h

The function interface like init, startBatchof NewRemoteParameterUpdater is inherited from parent class ParameterUpdater that use lowercase as the first letter. So this function name has to maintain the style now, but I also agree that we use Google C++ style for C++ in the future.

typhoonzero · 2017-06-08T02:18:59Z

paddle/trainer/NewRemoteParameterUpdater.h

+   * @param new_paras
+   * @param type
+   */
+  void initNewParameter(paddle_parameter**& new_paras, ParameterType type) {


initNewParameter seems only called inside this class, should it be "private"?

typhoonzero · 2017-06-08T03:00:32Z

paddle/trainer/NewRemoteParameterUpdater.h

+    for (int i = 0; i < parameterSize(); ++i) {
+      ParameterPtr para = parameters_[i];
+      new_paras[i]->content_len = 10;
+      new_paras[i]->element_type = PADDLE_ELEMENT_TYPE_FLOAT32;


Need a comment that type is defined in "Vector.h" with typedef VectorT<real> Vector;, so parameter element type is always real

…er properly

helinwang · 2017-06-12T18:57:36Z

go/pserver/cclient/test/main.c

    fail();
  }

  paddle_parameter* params[2] = {NULL, NULL};
  char* names[] = {"param_a", "param_b"};
-  if (!paddle_get_params(c, names, params, 2)) {


Sorry about this mistake from me!

helinwang · 2017-06-12T19:01:43Z

go/pserver/cclient/cclient.go

 	var gs []pserver.Gradient
 	for i := 0; i < int(total); i++ {
-		grad := (*C.paddle_gradient)(unsafe.Pointer((uintptr(unsafe.Pointer(grads)) + uintptr(i)*unsafe.Sizeof(*grads))))
+		grad := *(**C.paddle_gradient)(unsafe.Pointer((uintptr(unsafe.Pointer(grads)) + uintptr(i)*unsafe.Sizeof(*grads))))


Really sorry about this mistake from me. Must take you very long to debug!

helinwang · 2017-06-12T19:43:32Z

go/pserver/cclient/cclient.go

@@ -204,12 +204,14 @@ func paddle_get_params(client C.client, names **C.char, dst **C.paddle_parameter
 		}

 		p := ps[i]
-		param := *(**C.paddle_parameter)(unsafe.Pointer((uintptr(unsafe.Pointer(dst)) + uintptr(i)*unsafe.Sizeof(*dst))))
+		paramPtr := (**C.paddle_parameter)(unsafe.Pointer((uintptr(unsafe.Pointer(dst)) + uintptr(i)*unsafe.Sizeof(*dst))))


Good catch! I got used to Go treating -> as . (inferred according to type automatically on whether dereference or not). Now I realized cgo should be treated more like C than Go :)

helinwang · 2017-06-12T19:45:53Z

go/pserver/cclient/test/main.c

    fail();
  }

+  printf("test success!\n");


Let's follow the Unix way, do not print anything (with return code 0) when everything went well.
If we print success on every test, the output would be very messy.

helinwang · 2017-06-12T19:47:26Z

go/pserver/cclient/test/test_cclient.c

+    fail();
+  }
+
+  printf("test success!\n");


Let's follow the Unix way, do not print anything (with return code 0) when everything went well.
If we print success on every test, the output would be very messy.

done, removed

helinwang · 2017-06-12T19:48:06Z

go/pserver/optimizer.c

@@ -32,7 +32,14 @@ int update_SGD(void* optimizer,
               const void* gradient,
               int num_bytes) {
  SGD_optimizer* o = (SGD_optimizer*)optimizer;
-  // TODO
+  // TODO(a simple SGD implement)


A simple SGD implementation is already done, maybe remove this comment?

done, removed

helinwang · 2017-06-12T19:48:28Z

go/pserver/service.go

@@ -29,6 +29,10 @@ type Parameter struct {
 	Content     []byte
 }

+func (p *Parameter) toString() {


Is this function necessary (not used anywhere).

done，removed

helinwang · 2017-06-12T19:50:46Z

paddle/trainer/NewRemoteParameterUpdater.h

+  /**
+   * initialize the internal parameter client and itself.
+   */
+  virtual void init(const std::vector<ParameterPtr>& parameters);


We need to following Google C++ style for C++ files.
Btw, for C file we have a different style used here: https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/capi/main.h

helinwang · 2017-06-12T19:51:41Z

paddle/trainer/NewRemoteParameterUpdater.h

+
+    for (int i = 0; i < parameterSize(); ++i) {
+      ParameterPtr param = parameters_[i];
+      new_params[i]->content_len = 10;


Maybe remove this line? (line 92 does the same thing correctly)

helinwang · 2017-06-12T19:54:23Z

paddle/trainer/NewRemoteParameterUpdater.cpp

+void NewRemoteParameterUpdater::init(
+    const std::vector<ParameterPtr> &parameters) {
+  ParameterUpdater::init(parameters);
+  LOG(INFO) << "NewRemoteParameterUpdater init in";


What does "in" in "NewRemoteParameterUpdater init in" mean?

helinwang

LGTM++!

typhoonzero · 2017-06-14T01:36:07Z

Need to resolve conflicts before merging!

… newparameterupdater

new parameterupdater use paddle pserver cclient of go

99dc606

jacquesqiao requested review from helinwang, gangliao, dzhwinter and typhoonzero June 7, 2017 15:45

jacquesqiao mentioned this pull request Jun 7, 2017

Design for remote parameter updater using cclient #2330

Closed

dzhwinter reviewed Jun 7, 2017

View reviewed changes

helinwang requested changes Jun 7, 2017

View reviewed changes

typhoonzero reviewed Jun 8, 2017

View reviewed changes

jacquesqiao added 8 commits June 8, 2017 22:38

refine code

6f1c91d

fix the problem of paddle_send_grad

28476f5

fix the problem in cclient when malloc paddle_parameter

966bf9a

add simple updater, this version can train a model

c44f5dd

add test file mnist_test.py, free resource of newRemoteParameterUpdat…

39d0b3d

…er properly

clang format

4f366be

change trainer_id

da3e84a

change go version

dc458a0

helinwang reviewed Jun 12, 2017

View reviewed changes

add paramConfig for each parameter

37594ea

helinwang approved these changes Jun 13, 2017

View reviewed changes

jacquesqiao added 3 commits June 14, 2017 10:07

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

8941a38

… newparameterupdater

update code with new cclient

ebba2b1

clang format check

c093a24

jacquesqiao merged commit 91f82ab into PaddlePaddle:develop Jun 14, 2017

new parameterupdater use paddle pserver cclient of go #2413

new parameterupdater use paddle pserver cclient of go #2413

Conversation

jacquesqiao commented Jun 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

helinwang Jun 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

helinwang commented Jun 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

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

helinwang Jun 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

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

helinwang left a comment

Choose a reason for hiding this comment

typhoonzero commented Jun 14, 2017 • edited Loading

jacquesqiao commented Jun 7, 2017 •

edited

Loading

helinwang Jun 7, 2017 •

edited

Loading

helinwang commented Jun 7, 2017 •

edited

Loading

helinwang Jun 7, 2017 •

edited

Loading

typhoonzero commented Jun 14, 2017 •

edited

Loading