Recordio cloud and local interface #2665

gongweibao · 2017-06-29T03:17:57Z

Fix #2637
同时，在返回值上增加errno以便区分是EOF还是error。

…into cloudandlocal

helinwang

Great PR! Some question about if we need to return error for next_record.

helinwang · 2017-06-29T19:06:49Z

python/paddle/v2/master/client.py

@@ -26,14 +26,27 @@ def set_dataset(self, paths):
            holder[idx] = c_ptr
        lib.paddle_set_dataset(self.c, holder, len(paths))

+    # return format: (record, errno)
+    # errno =  0: ok
+    #       = -1: EOF


Maybe we don't need to return EOF. The function name is next_record , from the user's perspective, it's an infinite stream of data. EOF does not make sense here.

Actually, do we really need to return error here? Current implementation is next_record will log error and try to recover from the error.
If we return error here, what can user do with it? (the retry logic is already implemented inside next_record).

如线下讨论：错误是需要暴露的，cloud端EOF不需要暴露。
我在这里建立了一个ISSUE：#2678
这个PR的py接口先不暴露错误，以便保持本地和cloud接口的统一。

helinwang · 2017-06-30T23:41:13Z

go/master/c/CMakeLists.txt

@@ -6,7 +6,7 @@ set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${PARENT_DIR}/cmake")

 project(cxx_go C Go)

-include(golang)
+#include(golang)


这个文件在最新的develop branch已经大改了，需要rebase或者pull一下。

helinwang · 2017-07-01T00:22:39Z

go/master/c/client.go

@@ -84,11 +84,22 @@ func paddle_set_dataset(client C.paddle_master_client, path **C.char, size C.int
 	return C.PADDLE_MASTER_OK
 }

+// return value:
+//     0:ok
+//    -2:error


Why not -1 error? -2 is a little strange.

helinwang · 2017-07-01T00:24:40Z

python/paddle/v2/master/client.py

    def next_record(self):
        p = ctypes.c_char_p()
        ret = ctypes.pointer(p)
        size = lib.paddle_next_record(self.c, ret)
+        if size < -1:


Perhaps size < 0 is better. Errors are typically < 0, not < -1.

helinwang · 2017-07-01T00:24:49Z

python/paddle/v2/master/client.py

@@ -26,14 +26,22 @@ def set_dataset(self, paths):
            holder[idx] = c_ptr
        lib.paddle_set_dataset(self.c, holder, len(paths))

+    # return format: (record, errno)
+    # errno =  0: ok
+    #       < -1: error


< 0: error.

helinwang · 2017-07-01T00:28:15Z

python/paddle/v2/reader/creator.py

-    :path: path of recordio file
-    :returns: data reader of recordio file
+    Creates a data reader that outputs record one one by one 
+        from given local recordio fils path.


Maybe change Creates a data reader that outputs record one one by one from given local recordio fils path. to Creates a data reader from given RecordIO file paths separated by ",", glob pattern is supported.

Great! Done.

helinwang · 2017-07-01T00:29:23Z

python/paddle/v2/reader/creator.py

    """

    import recordio as rec

    def reader():
-        f = rec.reader(path)
+        for i, path in enumerate(paths):


Don't need enum here, rec.reader support "," separated paths: https://github.com/PaddlePaddle/recordio/blob/master/c/crecordio.go#L79

这个地方我觉得有些疑惑，我们让用户输入的是一个以','分割的字符串？还是一个字符串的array？还是两者的混合？
https://github.com/PaddlePaddle/Paddle/blob/develop/python/paddle/v2/master/client.py#L24

不好意思，不是很一致。这里跟set_dataset不同哈～set_dataset接受的是一个list，这里是","分割的字符串。具体接受什么参数要看一下Go那边是怎么实现的。

helinwang · 2017-07-01T00:30:50Z

python/paddle/v2/reader/creator.py

+
+    return reader
+
+def recordio(paths, addr="", buf_size=100):


Maybe put this function name into __all__ variable inside this file.

已经放了。

helinwang · 2017-07-01T00:32:44Z

python/paddle/v2/reader/creator.py

+
+    return reader
+
+def recordio(paths, addr="", buf_size=100):


I think addr need to be an environment variable, since user would never know the address of the etcd cluster. (so no way to provide this argument).

Great!Done.

helinwang · 2017-07-01T00:36:28Z

python/paddle/v2/reader/creator.py

+                yield r
+            f.close()
+
+    return reader


Since the recordio function takes the buf_size argument, we need to honor that argument in local reader as well. So perhaps return buffered(reader, buf_size), please see: https://github.com/PaddlePaddle/Paddle/blob/develop/python/paddle/v2/reader/decorator.py#L162

Thanks! Done.

helinwang

One comment, otherwise LGTM.

helinwang · 2017-07-03T14:55:46Z

python/paddle/v2/reader/creator.py

+        while True:
+            r, err = client.next_record()
+            if r is None:
+                break


I think we need to break only if error happens. None could be an empty record (which we should not break).

Thanks.Done.

… cloudandlocal

gongweibao added 3 commits June 28, 2017 12:01

modify cmake

3919b75

first add

fc3d031

Merge branch 'cloudandlocal' of https://github.com/gongweibao/Paddle …

183a5d4

…into cloudandlocal

gongweibao requested review from Yancey1989, helinwang and typhoonzero June 29, 2017 03:17

gongweibao added 3 commits June 29, 2017 04:28

fix bugs

4874810

fix bugs

0fa4092

fix bugs

b79784e

helinwang requested changes Jun 29, 2017

View reviewed changes

gongweibao added 2 commits June 30, 2017 03:11

rm cloud EOF

b3c5808

rm cloud EOF

97bbd17

helinwang reviewed Jul 1, 2017

View reviewed changes

gongweibao added 2 commits July 3, 2017 02:08

merge with upstream develop

af5ac2c

fix by helin's comments

26e661b

helinwang approved these changes Jul 3, 2017

View reviewed changes

gongweibao added 9 commits July 4, 2017 01:22

fix by helin's comments

e12d726

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

421d9f1

… cloudandlocal

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

24dc0d1

… cloudandlocal

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

20c6119

… cloudandlocal

add print messages

f2a82b1

modify to add paddle_master name

660475b

add -V

b396055

rm -v

4daa247

add cmake

126e64f

gongweibao merged commit 8e8f360 into PaddlePaddle:develop Jul 7, 2017

gongweibao deleted the cloudandlocal branch January 17, 2021 07:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recordio cloud and local interface #2665

Recordio cloud and local interface #2665

gongweibao commented Jun 29, 2017 •

edited

helinwang left a comment

helinwang Jun 29, 2017 •

edited

gongweibao Jun 30, 2017

helinwang Jun 30, 2017

gongweibao Jul 4, 2017

helinwang Jul 1, 2017

gongweibao Jul 3, 2017

helinwang Jul 1, 2017

gongweibao Jul 3, 2017

helinwang Jul 1, 2017

gongweibao Jul 3, 2017

helinwang Jul 1, 2017

gongweibao Jul 3, 2017

helinwang Jul 1, 2017

gongweibao Jul 3, 2017 •

edited

helinwang Jul 3, 2017 •

edited

helinwang Jul 1, 2017

gongweibao Jul 3, 2017

helinwang Jul 1, 2017

gongweibao Jul 3, 2017

helinwang Jul 1, 2017

gongweibao Jul 3, 2017

helinwang left a comment

helinwang Jul 3, 2017

gongweibao Jul 4, 2017

Recordio cloud and local interface #2665

Recordio cloud and local interface #2665

Conversation

gongweibao commented Jun 29, 2017 • edited

helinwang left a comment

Choose a reason for hiding this comment

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

gongweibao Jul 3, 2017 • edited

Choose a reason for hiding this comment

helinwang Jul 3, 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

helinwang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gongweibao commented Jun 29, 2017 •

edited

helinwang Jun 29, 2017 •

edited

gongweibao Jul 3, 2017 •

edited

helinwang Jul 3, 2017 •

edited