Add ParameterServerController for parameter server python api by jacquesqiao · Pull Request #1051 · PaddlePaddle/Paddle

jacquesqiao · 2017-01-03T02:52:12Z

1，code clean, ParameterServer2Main.cpp and TrainerMain.cpp have duplicated code.
2, the duplicated code use many gflags to control the init of parameter server, so add a wrapper for pserver, change gflags to proto config.
refs #1039

More about this PR (copied from IM chat with @jacquesqiao):

1,ParameterServer2Main.cpp and TrainerMain.cpp 这两个文件有一大段重复代码。使用很多gflags + if/else控制parameterserver的初始化逻辑，比价容易造成混乱
所以现在统一用proto 来配置，兼容之前的gflags方式，在后边的python api中，就可以通过python构造proto的方式来初始化parameter server，而不用通过 initPaddle("-flags=1", "flags2=2")的方式进行初始化了

… add-pserver-util

wangkuiyi · 2017-01-03T15:47:57Z

paddle/pserver/PServerUtil.h

@@ -0,0 +1,70 @@
+/* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserve.


我现在对util这样的命名特别紧张——我发现一般都是大家懒得想明白应该叫什么的时候就叫util了。

在这里，看上去是想叫 PServerController 什么的。

Cool，同意。

PServerController是个好名字。
感觉程序员起名字是一个非常痛苦的事情。一痛苦就会用一些比较常用的名字，比如Utils

wangkuiyi · 2017-01-03T15:48:47Z

paddle/pserver/PServerUtil.h

+  /**
+   * @brief start all pserver thread in this PServerUtil.
+   */
+  void start();


我们的naming convention指定的怎么样了？ @reyoung

在这里constructor是camel形式，但是methods都是小写。显然不一致呀。

这里因为构造函数和类名必须一致。类名必须是UpperCamelCase。比如 "SomeClass".而函数名是"lowerCamelCase"，比如"someMethod"。

这样做的好处是，我们可以通过判断出一个东西是不是类型了。

比如

class SomeClass { public: class Helper { }; static Helper helper(); };

SomeClass::Helper 是类型，而SomeClass::helper是函数。

wangkuiyi · 2017-01-03T15:49:41Z

proto/ParameterServerConfig.proto

@@ -0,0 +1,43 @@
+/* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserve.


上面有文件叫 PServerUtils.*，这里叫ParameterServer，显然不一致呀。

这个配置文件确实是用来配置parameter server的，目前的pserverutil封装了几个parameter server线程，根据config来创建这些线程。

我的意思是到底应该叫 pserver 还是 parameter server 呢？

已经按照命名规范修改为ParameterServerController

reyoung · 2017-01-04T02:31:08Z

paddle/pserver/PServerUtil.h

@@ -0,0 +1,70 @@
+/* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserve.


Cool，同意。

PServerController是个好名字。
感觉程序员起名字是一个非常痛苦的事情。一痛苦就会用一些比较常用的名字，比如Utils

reyoung · 2017-01-04T02:35:57Z

paddle/pserver/PServerUtil.h

+  /**
+   * @brief start all pserver thread in this PServerUtil.
+   */
+  void start();


这里因为构造函数和类名必须一致。类名必须是UpperCamelCase。比如 "SomeClass".而函数名是"lowerCamelCase"，比如"someMethod"。

这样做的好处是，我们可以通过判断出一个东西是不是类型了。

比如

class SomeClass { public: class Helper { }; static Helper helper(); };

SomeClass::Helper 是类型，而SomeClass::helper是函数。

reyoung · 2017-01-04T02:36:59Z

paddle/pserver/PServerUtil.h

+  void join();
+
+private:
+  std::vector<std::shared_ptr<ParameterServer2>> pservers_;


std::vector<std::shared_ptr<ParameterServer2>> =>std::vector<std::unique_ptr<ParameterServer2>>

reyoung · 2017-01-04T02:37:36Z

paddle/pserver/PServerUtil.h

+
+namespace paddle {
+
+class PServerUtil {


class PServerUtil => class PServerUtil final

reyoung · 2017-01-04T02:42:45Z

paddle/pserver/PServerController.cpp

+}
+
+PServerController* PServerController::createByGflags() {
+  auto& pServerConfig = *paddle::PServerController::initConfigByGflags();


这里有内存泄露

这里直接用栈变量不可以么？也就是

ParameterServerConfig config; config.set_nics(FLAGS_nics); ...

这样。同时，initConfigByGflags 只被 createByGFlags 调用，没必要extract成一个private的static member function了吧。

reyoung · 2017-01-04T02:45:20Z

paddle/pserver/PServerController.cpp

+PServerController::~PServerController() { this->join(); }
+
+ParameterServerConfig* PServerController::initConfigByGflags() {
+  ParameterServerConfig* config = new ParameterServerConfig();


尽量不要用C++的new关键词。

最省事的方法是:

auto config = std::make_shared<ParameterServerConfig>();

或者是

auto config = std::make_unique<ParameterServerConfig>(); // since c++ 14

虽然目前没有make_unique，不过回头我加上吧。。

对了，勉强在Cpp里面和java new语意一致的东西是 std::make_shared<类型名>(参数)。只是std::make_unique会快一点。

赞，多谢，已经修改了，不过make_unique打算如何引入?

直接加上这个函数也可以。。http://stackoverflow.com/questions/17902405/how-to-implement-make-unique-function-in-c11

判断一下C++版本， if __cplusplus != 14，那么就加上make_unique。

reyoung

LGTM. Cool.

… add-pserver-util

wangkuiyi · 2017-01-11T02:15:21Z

paddle/pserver/ParameterServerController.h

+
+namespace paddle {
+
+class ParameterServerController final {


新增的class应该有class comment

wangkuiyi · 2017-01-11T02:18:33Z

proto/ParameterServerConfig.proto

+  required int32 trainer_id = 1;
+}
+
+message ParameterServerConfig {


这里应该有个注释说明这个proto message的用意。

wangkuiyi · 2017-01-11T02:18:51Z

proto/ParameterServerConfig.proto

+
+package paddle;
+
+message ParameterClientConfig {


这里应该有个注释说明这个proto message的用意。

done，写的比较简单

wangkuiyi · 2017-01-11T02:19:35Z

paddle/pserver/ParameterServerController.cpp

+}
+
+void ParameterServerController::start() {
+  LOG(INFO) << "pserver sizes : " << pservers_.size();


"pserver sizes" ==> "number of pserver instances"?

wangkuiyi · 2017-01-11T02:20:04Z

paddle/pserver/ParameterServerController.cpp

+  LOG(INFO) << "pserver sizes : " << pservers_.size();
+  int i = 0;
+  for (const auto& pserver : pservers_) {
+    LOG(INFO) << "pserver started : " << i;


LOG(INFO) << "Staring pserver " << i;

wangkuiyi · 2017-01-11T02:20:55Z

paddle/pserver/ParameterServerController.cpp

+
+ParameterServerController::~ParameterServerController() { this->join(); }
+
+ParameterServerController* ParameterServerController::createByGflags() {


createFromGflags

done，这个地方之前确实纠结过

wangkuiyi · 2017-01-11T02:22:35Z

paddle/pserver/ParameterServerController.cpp

+  }
+}
+
+void ParameterServerController::join() {


join 对应的是 fork, start 对应的是 wait。这里的join 应该改名为 wait 才对。

done，好主意

backyes · 2017-01-11T03:51:43Z

@jacquesqiao

trainer和pserver除了日常的通信，还有一个同步控制（核心目的是为了实现async-sgd、sync-sgd、pass起始、batch 起始等核心逻辑）的逻辑，从这个ParameterServerController实现来看，主要是起到对启动pserver进程的控制，没有同步的控制逻辑。

问题：

如果这个api 将来支持动态增减pserver的功能，那么这个api应该要支持对同步逻辑的感知。因为pserver动态增减以后，同步逻辑会有变化（比如同步节点数变化了），trainer是否应该同步这个ParameterServerController来获取pserver同步节点数的信息？如果是，那么这个api应该是否要考虑对pserver同步逻辑的感知。

backyes · 2017-01-11T03:54:23Z

@jacquesqiao

现在版本中有一个耦合在trainer中的类似『ParameterServerController』的功能，参见：

https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/trainer/RemoteParameterUpdater.cpp#L623

它主要强调对SGD算法（sync-sgd， async-sgd）层面的逻辑控制。

jacquesqiao · 2017-01-11T06:05:26Z

@backyes

trainer和pserver除了日常的通信，还有一个同步控制（核心目的是为了实现async-sgd、sync-sgd、pass起始、batch 起始等核心逻辑）的逻辑，从这个ParameterServerController实现来看，主要是起到对启动pserver进程的控制，没有同步的控制逻辑。

对的，目前ParameterServerController主要是对parameter server那些thread的封装，干了一些初始化和简单的控制功能，而且只能控制一个ps进程。

如果这个api 将来支持动态增减pserver的功能，那么这个api应该要支持对同步逻辑的感知。因为pserver动态增减以后，同步逻辑会有变化（比如同步节点数变化了），trainer是否应该同步这个ParameterServerController来获取pserver同步节点数的信息？如果是，那么这个api应该是否要考虑对pserver同步逻辑的感知。

后面可以考虑，不过trainer的这个controller貌似是训练这边的一个逻辑，直接和ParameterServer2通信，后面的具体实现，可以再考虑。

* test=develop,adding a description of the CMAKE command

…#1051) * fix cmake by use abs path * add boost dependency Co-authored-by: Zeyu Chen <chenzeyu01@baidu.com>

jacquesqiao added 2 commits January 3, 2017 10:43

add pserver util and parameter server config

f3c61cb

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

f9a65b0

… add-pserver-util

jacquesqiao mentioned this pull request Jan 3, 2017

[in progress] Expose API for parameter server #1039

Closed

jacquesqiao added 3 commits January 3, 2017 14:39

add comment and refine code

95f20b9

use unique_ptr

cfbb4c4

change FLAGS to proto config in PServerUtil

7783982

wangkuiyi requested changes Jan 3, 2017

View reviewed changes

rename PServerUtil to PServerController

93e74f8

reyoung requested changes Jan 4, 2017

View reviewed changes

rm initConfigByGflags of PServerController, use stack value instead

3f6c2b3

reyoung approved these changes Jan 4, 2017

View reviewed changes

jacquesqiao self-assigned this Jan 4, 2017

jacquesqiao mentioned this pull request Jan 4, 2017

ParameterServer python api #1066

Closed

jacquesqiao added developing and removed developing labels Jan 4, 2017

rename PServerController to ParameterServerController

5aaaef4

jacquesqiao changed the title ~~Add pserver util for pserver python api~~ Add ParameterServerController for parameter server python api Jan 6, 2017

jacquesqiao added 2 commits January 6, 2017 16:27

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

b1eeb2e

… add-pserver-util

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

d32c7a6

… add-pserver-util

wangkuiyi requested changes Jan 11, 2017

View reviewed changes

code refine, add comment and some naming problem

aa9f516

wangkuiyi approved these changes Jan 11, 2017

View reviewed changes

jacquesqiao merged commit f8a529c into PaddlePaddle:develop Jan 11, 2017

jacquesqiao mentioned this pull request Jan 20, 2017

Remove all global variables in Paddle #852

Closed

22 tasks

zhhsplendid pushed a commit to zhhsplendid/Paddle that referenced this pull request Sep 25, 2019

adding a description of the CMAKE command (PaddlePaddle#1051)

9f18368

* test=develop,adding a description of the CMAKE command

wangxicoding pushed a commit to wangxicoding/Paddle that referenced this pull request Dec 9, 2021

[FasterTransformer] Fix compiling error by use abs path (PaddlePaddle…

a3db9c9

…#1051) * fix cmake by use abs path * add boost dependency Co-authored-by: Zeyu Chen <chenzeyu01@baidu.com>

lizexu123 pushed a commit to lizexu123/Paddle that referenced this pull request Feb 23, 2024

Fix unitest of pruning (PaddlePaddle#1051)

2ef31f9

		@@ -0,0 +1,70 @@
		/* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserve.

		@@ -0,0 +1,43 @@
		/* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserve.


		ParameterServerController::~ParameterServerController() { this->join(); }

		ParameterServerController* ParameterServerController::createByGflags() {


		package paddle;

		message ParameterClientConfig {

Conversation

jacquesqiao commented Jan 3, 2017 • edited by wangkuiyi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reyoung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

backyes commented Jan 11, 2017

jacquesqiao commented Jan 3, 2017 •

edited by wangkuiyi

Loading

jacquesqiao commented Jan 11, 2017 •

edited

Loading