add TaskFail interface #2719

gongweibao · 2017-07-04T02:38:22Z

No description provided.

gongweibao · 2017-07-04T02:48:44Z

go/master/service.go

 }

 type taskQueues struct {
 	Todo    []taskEntry
 	Pending map[int]taskEntry // map from task ID to task entry
 	Done    []taskEntry
-	Failed  []Task
+	Failed  []taskEntry


把Task改成了taskEntry的原因是，我觉得Failed task应该保留进入错误队列时候的上下文状态。

helinwang · 2017-07-05T00:28:20Z

go/master/service.go

 	NumTimeout int
 	Task       Task
+	NumFailed  int


Maybe we can remove NumTimeout, we can treat timeout as failure. We can add NumTimeout back if we really need it. Let's keep the system simple.

helinwang · 2017-07-05T00:30:06Z

go/master/service.go

@@ -34,29 +34,30 @@ type Chunk struct {
 // Task is the basic unit of data instances assigned to trainers.
 type Task struct {
 	ID     int
+	Epoch  int


Task分发给Trainer的一个要做的事情，Trainer貌似不需要知道这是第几个Epoch。
我大概明白为什么需要Epoch，为的是唯一标示一个Task，不然多个机器可能会有同一个Task的不同Epoch，伟宝考虑的周到！
这样看来的话，是不是考虑每个Task分发出去的时候就给一个新的ID就好？

我添加了一个ISSUE：#2752

感觉用全局唯一的ID表示Task就可以了，最终可以统计这个task被执行过几次，哪些成功哪些失败了。Epoch用来对timeout计数可以放在taskEntry中就可以了。

任务结束时，就可以统计task的成功和失败的情况。

在ISSUE中讨论这个问题吧。已经回了。

helinwang · 2017-07-05T00:31:01Z

go/master/service.go

@@ -91,11 +92,11 @@ func partition(chunks []Chunk, chunksPerTask int) []taskEntry {
 }

 // NewService creates a new service.
-func NewService(store Store, chunksPerTask int, timeoutDur time.Duration, timeoutMax int) (*Service, error) {
+func NewService(store Store, chunksPerTask int, timeoutDur time.Duration, failortimeoutMax int) (*Service, error) {


failortimeoutMax改成failureMax吧（前面有个comment提到删掉NumTimeout）

helinwang · 2017-07-05T00:32:56Z

go/master/service.go

@@ -389,3 +396,23 @@ func (s *Service) TaskFinished(taskID int, dummy *int) error {
 	}
 	return err
 }
+
+// TaskFailed tell the service that a task is failed.
+func (s *Service) TaskFailed(taskID int, epoch int) error {


一个"net/rpc"的函数需要第二个参数的类型是指针: https://golang.org/pkg/net/rpc/

Thanks!
Done.

helinwang · 2017-07-05T00:36:34Z

go/master/service.go

+		return err
+	}
+
+	s.checkTaskStatus(t, epoch)


从逻辑上来讲，上报task失败之后，再调用checkTaskStatus（检查task状态）有点奇怪，都已经失败了，为啥还需要检查？

checkTaskStatus实际做的是如果任务超时，从pending删除加到TODO中。这里我理解如果是client调用TaskFailed应该不需要调用checkTaskStatus，而直接从pending删除，加到Failed队列中。

@gongweibao 解释了需要重试的原因。明白了，这里的确需要调用checkTaskStatus重试几次task后fail掉

I may not have internet connection in the following few days. Don't want to block this PR from being merged.

typhoonzero · 2017-07-05T05:12:18Z

go/master/service.go

+		return err
+	}
+
+	s.checkTaskStatus(t, epoch)


checkTaskStatus实际做的是如果任务超时，从pending删除加到TODO中。这里我理解如果是client调用TaskFailed应该不需要调用checkTaskStatus，而直接从pending删除，加到Failed队列中。

typhoonzero · 2017-07-05T05:39:22Z

go/master/service.go

+		return err
+	}
+
+	s.checkTaskStatus(t, epoch)


@gongweibao 解释了需要重试的原因。明白了，这里的确需要调用checkTaskStatus重试几次task后fail掉

typhoonzero · 2017-07-05T05:40:57Z

go/master/service.go

-
-		t.NumTimeout++
-		if t.NumTimeout > s.timeoutMax {
-			log.Warningf("Task %v timed out %d times, discard.", t.Task, t.NumTimeout)


这里可能也会被failed调用，所以不一定都是time out，可以用泛化点的描述。

… taskfail

Yancey1989 · 2017-07-07T02:57:06Z

go/master/service.go

@@ -348,9 +354,9 @@ func (s *Service) GetTask(dummy int, task *Task) error {
 	}

 	*task = t.Task


Delete unused *task

这行是返回值，是有用的。：）

Yancey1989

根据#2752 (comment) ，Master是不是要加一个定期检查Pending队列的Task是不是超出执行时间的机制呢，有可能由于网络原因，Trainer永远无法汇报自己的Task是不是失败，这样的话会导致Task永远在Pending队列里了。

gongweibao · 2017-07-10T06:30:05Z

@Yancey1989 现在已经实现这样的机制。

gongweibao · 2017-07-10T09:39:13Z

ISSUE #2752 已经回复了。

typhoonzero

LGTM++

helinwang · 2017-07-11T01:07:36Z

go/master/service.go

@@ -257,6 +258,34 @@ func (s *Service) SetDataset(globPaths []string, dummy *int) error {
 	return nil
 }

+func (s *Service) procFailedTask(t taskEntry, epoch int) {


请问proc是什么意思？

helinwang · 2017-07-11T01:25:36Z

go/master/service.go

@@ -34,27 +34,28 @@ type Chunk struct {
 // Task is the basic unit of data instances assigned to trainers.
 type Task struct {
 	ID     int
+	Epoch  int


Maybe change like this is more consistent with func (s *Service) TaskFailed(taskID TaskID, dummy *int) error:

type TaskMeta struct { ID int Epoch int } type Task struct { TaskMeta Meta Chunks []Chunk } func (s *Service) TaskFailed(meta TaskMeta, dummy *int) error { }

helinwang · 2017-07-11T01:28:55Z

go/master/service.go

@@ -348,9 +354,9 @@ func (s *Service) GetTask(dummy int, task *Task) error {
 	}

 	*task = t.Task
-	log.WithFields(s.logFields()).Infof("Task #%d dispatched.", task.ID)
+	log.WithFields(s.logFields()).Infof("Task #%v dispatched.", t)


A task contains []Chunks, maybe that is too much information to print.
Perhaps change to log.WithFields(s.logFields()).Infof("Task #%v dispatched.", t.Task.Meta)

helinwang · 2017-07-11T01:30:12Z

go/master/service.go

+	if !ok {
+		err := errors.New("pending task not found")
+		log.WithFields(s.logFields()).Warningln("TaskFailed:Pending task #%v not found.", taskID)
+		return err


Should we return error here? I think it's normal if that task is no longer pending (completed by other workers).

helinwang · 2017-07-11T01:37:16Z

go/master/client.go

@@ -112,6 +112,11 @@ func (c *Client) taskFinished(taskID int) error {
 	return c.conn.Call("Service.TaskFinished", taskID, nil)
 }

+// TaskFailed tell the master server as task is failed.
+func (c *Client) taskFailed(taskID TaskID) error {
+	return c.conn.Call("Service.TaskFinished", taskID, nil)


Should be "Service.TaskFailed" :)

汗。Done.

加了测试用例。

Sorry, I missed so many mistakes when reviewing.

@typhoonzero No worries, that's why we have multiple developers reviewing, we make mistake :)

汗死。我应该加unit test。盲目的自信是不可以有的。也不符合我们的做事方法。惭愧。

… taskfail

helinwang

LGTM++

add taskfail interface

e25c155

gongweibao requested review from Yancey1989, jacquesqiao and typhoonzero July 4, 2017 02:38

fix bugs

52cc601

gongweibao requested a review from helinwang July 4, 2017 02:42

gongweibao commented Jul 4, 2017

View reviewed changes

helinwang previously requested changes Jul 5, 2017

View reviewed changes

typhoonzero requested changes Jul 5, 2017

View reviewed changes

typhoonzero reviewed Jul 5, 2017

View reviewed changes

gongweibao mentioned this pull request Jul 6, 2017

Assign new id when we restart a task? #2752

Closed

gongweibao added 5 commits July 6, 2017 06:40

fix by helin and wuyi's comments

108b0fa

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

7663a40

… taskfail

add TaskID

a94d217

fix bugs

8f70885

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

578dd09

… taskfail

Yancey1989 reviewed Jul 7, 2017

View reviewed changes

typhoonzero approved these changes Jul 10, 2017

View reviewed changes

helinwang requested changes Jul 11, 2017

View reviewed changes

helinwang reviewed Jul 11, 2017

View reviewed changes

gongweibao added 4 commits July 11, 2017 10:25

fix by helin's comments

a40a7a5

fix by helin's comments

b64c7a6

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

d05d19b

… taskfail

fix bug

dd8685f

helinwang approved these changes Jul 11, 2017

View reviewed changes

gongweibao merged commit ef67d08 into PaddlePaddle:develop Jul 11, 2017

gongweibao deleted the taskfail branch July 11, 2017 08:23

		@@ -348,9 +354,9 @@ func (s Service) GetTask(dummy int, task Task) error {
		}

		*task = t.Task

add TaskFail interface #2719

add TaskFail interface #2719

Conversation

gongweibao 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

Choose a reason for hiding this comment

Yancey1989 left a comment

Choose a reason for hiding this comment

gongweibao commented Jul 10, 2017

gongweibao commented Jul 10, 2017

typhoonzero 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

gongweibao Jul 11, 2017 • edited

Choose a reason for hiding this comment

helinwang left a comment

Choose a reason for hiding this comment

gongweibao Jul 11, 2017 •

edited