Feature/error handling in paddle #1149

reyoung · 2017-01-14T08:23:19Z

错误处理应该是Paddle API比较重要的一方面，因为把Paddle作为一个库引入的话，调几个函数就log(FATAL)了肯定不合适。

这里的Status是不是可以暂定为Paddle的错误类型？Status基本上是一个std::string的指针。如果为空，那么便没有error，否则error的详细信息是这个字符串的内容。

这种实现下，如果没有错误的话，返回这个错误码是基本没有性能开销的(和返回一个整数开销应该一样)。如果返回错误的话，开销在指针引用上(主体可能是多线程的shared_ptr锁)，不会复制字符串很多次。

reyoung · 2017-01-16T06:53:58Z

@wangkuiyi @hedaoyuan 我们用这个Status对象做Paddle的错误值合适么？

hedaoyuan

另外，写个ISSUE描述一下这个工作吧。最主要的是，要让别人明白Status怎么用；以及要让别人知道，后续写哪些东西的时候要用Status作为返回值。

hedaoyuan · 2017-01-17T03:22:19Z

paddle/utils/Status.h

+ * @brief isOK
+ * @return true if OK.
+ */
+ inline bool isOK() const noexcept { return errMsg_ == nullptr; }


(!errMsg_), compares shared_ptr with a null pointer
http://en.cppreference.com/w/cpp/memory/shared_ptr/operator_cmp

其实，感觉maybe这两种写法在release模式下没有啥性能区别。。。我回头测试一下。。

XXX==nullptr感觉可读性反而好一点。

测试了一下，判断shared_ptr是否为空。使用隐式类型转换成bool型，或者直接和nullptr比较，在clang 8里，-O3下，二者生成的汇编完全一致。没有差别。

感觉还是用 xxx == nullptr 或者 xxx != nullptr 可读性好一点。

使用隐式类型转换成bool型 !errMsg_ 并不是隐式类型转换，是调用了operator bool()；errMsg_ == nullptr; 也是调用了!errMsg_。

7) !lhs 8) !rhs 9) (bool)lhs 10) (bool)rhs

恩，都是inline的调用啦，-O3都会没有的 :-)

hedaoyuan · 2017-01-17T03:34:47Z

paddle/utils/tests/test_Status.cpp

+
+#include <gtest/gtest.h>
+
+TEST(Status, testAll) {


TEST 除了check Status本身以外，还可以去构造一些实际用途的Case。
比如，构造一些函数调用，错误后返回Status这样的例子，也能比较好的让别人知道Status的用法。

写到这个类的注释里面了。

写到类的注释里面了。

hedaoyuan · 2017-01-17T03:40:33Z

paddle/utils/Status.h

+ */
+ template <typename... ARGS>
+ inline void setByPrintf(const char* fmt, ARGS... args) noexcept {
+ constexpr size_t kBufferSize = 4096;


4096，需要这么大。。。

Change to 1024

wangkuiyi

提了一些建议。我担心这个工作有点儿over engineering了。用了很多C++的路子。但是貌似都是C就能做的。

wangkuiyi · 2017-01-18T00:10:55Z

paddle/utils/Status.h

+ * Although Status inherits the std::exception, but do not throw it except you
+ * know what you are doing.
+ */
+class Status final : public std::exception {


为什么要从 std::exception 派生呢？定义成更简单的形式，比如

class Error { private: const char *msg_; }

不行吗？

派生了也没啥坏处。。std::exception算是C++的通常惯例。。

const char* msg_;和这个事情是两件事，如果用const char* msg_; 那我们还得手写一个复制构造函数，而且Error的传递从之前的引用传递变成了值传递。
将msg_定义成shared_ptr，那么普通的赋值Error对象，就是引用传递了。这玩意其实是和golang里面的那个error语意一致的写法。

我了解需要 std::shared_ptrstd::string 的原因了。

回到派生——我建议从最简化的原则出发“如果没有特别的好处，我们就不要引入这个派生了“。

wangkuiyi · 2017-01-18T00:12:52Z

paddle/utils/Status.h

+namespace paddle {
+
+/**
+ * Status is Paddle error code. It only contain a std::string as error message.


如果只包含一个error message，那么叫Error就好了。比如Go的error type

有道理。

wangkuiyi · 2017-01-18T00:17:18Z

paddle/utils/Status.h

+ * @brief set a error message for status.
+ * @param msg
+ */
+ inline void set(const std::string& msg) noexcept {


建议用一个全局friend函数（比如叫做Errorf），而不是用Error的method来创建内容。因为一个error的语法应该是“一旦创建不能被修改”，所以method，尤其是名为set的method不能贴切地表现这个syntax。

Error Network.allocateParameters() { ... if (...) { return Errorf("Cannot allocate memory block of size %d", kBufferSize); } ... }

wangkuiyi · 2017-01-18T03:14:36Z

paddle/utils/Status.h

+ * create a error status by C style printf.
+ */
+ template <typename... ARGS>
+ inline static Status printf(const char* fmt, ARGS... args) noexcept {


不需要很多风格的error creation吧。

wangkuiyi · 2017-01-18T03:16:52Z

paddle/utils/Status.h

+ /**
+ * @brief what will return the error message. If status is OK, return nullptr.
+ */
+ const char* what() const noexcept override {


为什么需要 override 和 noexcept 关键词呢？

因为这玩意其实还是继承了std::exception，符合标准的C++ exception的定义。

虽然我们不用Exception，但是借用这个接口定义我们的error应该是可以的。

因为没有特别的好处，所以别派生了，读者也就不需要了解override和noexcept这两个关键字了。毕竟C++里的大部分发明都是利弊兼备的，读者也被code style帮助着尽量少了解这些利弊之争。

wangkuiyi · 2017-01-18T03:18:03Z

paddle/utils/Status.h

+ /**
+ * @brief what will return the error message. If status is OK, return nullptr.
+ */
+ const char* what() const noexcept override {


what ==> msg?

这样用的时候就是 error.msg():

auto err = Errorf("...); LOG(ERROR) << err.msg();

what函数名是C++对于exception的普遍约定。虽然Paddle不鼓励使用exception，但是按照C++的普遍约定命名也比较有道理。

同上，不用exception，也就不用what了。

wangkuiyi · 2017-01-18T03:18:35Z

paddle/utils/Status.h

+ inline bool isOK() const noexcept { return errMsg_ == nullptr; }
+
+private:
+ std::shared_ptr<std::string> errMsg_;


这里就是一个 const char * msg_ 就好了吧。

在上面回复了一下。

* Also make ErrorF as a global method.

reyoung · 2017-01-18T05:35:44Z

@wangkuiyi @hedaoyuan Partially follow comments. Done.

wangkuiyi · 2017-01-18T08:11:28Z

贴一个我想到的简单实现：

#include <stdarg.h> #include <string> // We try to make error looks like a native POD type, so we broke the // naming convention for C++ classes. // // The following example shows supported syntax of error: /* error nomem("Cannot allocate %d bytes", 100); // 1. ctor is like printf. if (nomem) { // 2. type conversion to bool. printf("An error: %s", nomem.msg()); // 3. retrieve message. } error e = nomem; // 4. feel free to copy. */ class error { public: explicit error(const char* format, ...); operator bool () const { return msg_.get() != NULL; } const char* msg() const; private: std::shared_ptr<std::string> msg_; }; error::error(const char* format, ...) { char buffer[256]; va_list ap; va_start(ap, format); vsnprintf(buffer, 256, format, ap); va_end(ap); msg_.reset(new std::string(buffer)); } const char* error::msg() const { if (msg_.get() == NULL) { return NULL; } return msg_.get()->c_str(); } int main() { error nomem("Cannot allocate %d bytes", 100); if (nomem) { printf("An error: %s\n", nomem.msg()); } error e = nomem; if (e) { printf("The same error: %s\n", e.msg()); } }

reyoung · 2017-01-18T13:06:09Z

@wangkuiyi Follow comments, except use Error as class name.

wangkuiyi · 2017-01-18T16:39:47Z

paddle/utils/Error.h

+ * use log(FATAL) or CHECK to make program exit before. When we clean all
+ * log(FATAL) and CHECK in Paddle, 'check' method will be removed.
+ */
+class Error final {


不需要final吧？有些C++程序员不知道final。而且这里也非必须final。

wangkuiyi · 2017-01-18T16:42:21Z

paddle/utils/Error.h

+ /**
+ * Default Status. OK
+ */
+ inline Error() {}


这里不需要inline。如果是为了优化编译性能，尽量让编译器去决定性能吧。

wangkuiyi · 2017-01-18T16:42:55Z

paddle/utils/Error.h

+ /**
+ * @brief Create an Error use printf syntax.
+ */
+ inline explicit Error(const char* fmt, ...) {


这里不需要inline。

wangkuiyi · 2017-01-18T16:44:37Z

paddle/utils/Error.h

+class Error final {
+public:
+ /**
+ * Default Status. OK


这个comment写得是不是太简洁了？下面这样？

@brief Construct an no-error value.

wangkuiyi · 2017-01-18T16:46:17Z

paddle/utils/Error.h

+ va_start(ap, fmt);
+ constexpr size_t kBufferSize = 1024;
+ this->errMsg_.reset(new std::string(kBufferSize, 0));
+ auto sz = vsnprintf(&(*errMsg_)[0], kBufferSize, fmt, ap);


这样写暴露了string的太多实现细节了——string里有一个连续的buffer。没有必要。我之前那样写更简短。

wangkuiyi · 2017-01-18T16:46:29Z

paddle/utils/Error.h

+ /**
+ * @brief what will return the error message. If no error, return nullptr.
+ */
+ inline const char* msg() const {


不需要inline

wangkuiyi · 2017-01-18T16:46:52Z

paddle/utils/Error.h

+ inline void check() const { CHECK(*this) << msg(); }
+
+private:
+ std::shared_ptr<std::string> errMsg_;


class已经叫error了，这里只需要叫msg_。

wangkuiyi · 2017-01-18T16:47:57Z

paddle/utils/Error.h

+ * @note It is a temp method used during cleaning Paddle code. It will be
+ * removed later.
+ */
+ inline void check() const { CHECK(*this) << msg(); }


error是新加入的代码，为什么要等着“remove later”？应该一开始就不要制造需要remove的问题吧？

因为Paddle内部充斥着LOG(Fatal)和CHECK。我们不太可能在一个PR里面，把这些东西全部替换掉。。只能逐层的一步一步来，先替换掉内层的CHECK和LOG(FATAL)，返回给外层。

但是，这样一步一步来的时候，在外层处理这个Error就需要还调用CHECK。这里这个函数就是方便外层去检查这个Error的。

最终，我们的Paddle会将这个Error，全部返回给最上层的调用(例如Main函数或者Trainer中)，这样我们就可以把这个check去掉，然后在一个地方写这个CHECK了。

理解了。好的。

wangkuiyi · 2017-01-18T16:49:16Z

paddle/utils/Error.h

+ */
+ inline const char* msg() const {
+ if (errMsg_) {
+ return errMsg_->data();


这里应该用c_str，不可以用 data： http://stackoverflow.com/questions/194634/string-c-str-vs-data

wangkuiyi · 2017-01-18T16:50:57Z

paddle/utils/Error.h

+ /**
+ * @brief operator bool, return True if there is no error.
+ */
+ inline operator bool() const { return !errMsg_; }


因为msg调用的c_str可能运行代价比较高（如果string内部使用的不是连续buffer），所以这里应该延承 error::error() 的 syntax，用 msg_.get() != NULL 作为判断标准。

这里实际上调用的是shared_ptr是不是空指针。

我明白了。

不过这是在我知道C++里可以重载operator ()，并且去文档里查阅，确定了shared_ptr重载了operator () 的情况下，才能理解这个用法。如果我不查阅文档，我甚至不确定 shared_ptr 重载的 operator () 是返回的 bool 类型还是 pointer 类型。

所以我建议这里还是用朴素的写法：msg_.get() != NULL。

我们的原则是希望源码的读者（维护着）具备最少的知识，就能理解我们的代码。

wangkuiyi · 2017-01-18T16:59:20Z

我之前贴的那个例子，没有办法构造“no error”。受到 @reyoung 的修改的启发，更新一下我的那个例子：

#include <stdarg.h> #include <string> // We try to make error looks like a native POD type, so we broke the // naming convention for C++ classes. // // The following example shows supported syntax of error: /* error noerr; // 1. ctor of no error. error nomem("Cannot allocate %d bytes", 100); // 2. ctor is like printf. if (nomem) { // 3. type conversion to bool. printf("An error: %s", nomem.msg()); // 4. retrieve message. } error e = nomem; // 5. feel free to copy. */ class error { public: error() {} error(const char* format, ...); operator bool () const { return msg_.get() != NULL; } const char* msg() const; private: std::shared_ptr<std::string> msg_; }; error::error(const char* format, ...) { char buffer[256]; va_list ap; va_start(ap, format); vsnprintf(buffer, 256, format, ap); va_end(ap); msg_.reset(new std::string(buffer)); } const char* error::msg() const { if (msg_.get() == NULL) { return NULL; } return msg_.get()->c_str(); }

以下代码适合改成unit test，放在 error_test.cc里：

int main() { error noerr; if (noerr) { printf("We should never see this!"); } error nomem("Error"); if (nomem) { printf("An error: %s\n", nomem.msg()); } }

* update install doc for release 1.5.2, test=develop * update Tables.md for 1.5.2, test=develop * update Tables.md to 1.5.1 for windows, test=develop

reyoung added 2 commits January 14, 2017 16:18

Add Status

0248031

Try using status to handle Paddle Error

6c20e08

reyoung requested review from hedaoyuan and wangkuiyi January 14, 2017 08:23

reyoung added 2 commits January 16, 2017 16:15

Add printf method to Status.

741637e

Fix compile error.

8aefc30

hedaoyuan reviewed Jan 17, 2017

View reviewed changes

Stash

3d01c60

wangkuiyi requested changes Jan 18, 2017

View reviewed changes

reyoung added 4 commits January 18, 2017 12:45

Add more comments, also add __must_check.

9bc1203

Add some comments to compiler.h

8605544

Rename Status => Error.

ec790e1

* Also make ErrorF as a global method.

Change unittest variable name

699d18f

Make Error interface cleaner

5a15c70

reyoung force-pushed the feature/ErrorHandlingInPaddle branch from 02830cf to 5a15c70 Compare January 18, 2017 13:02

wangkuiyi requested changes Jan 18, 2017

View reviewed changes

Follow comments

a6d4a31

reyoung force-pushed the feature/ErrorHandlingInPaddle branch from 4bb9eb7 to f3739dc Compare January 19, 2017 02:09

Fix typo

c88dec2

reyoung force-pushed the feature/ErrorHandlingInPaddle branch from f3739dc to c88dec2 Compare January 19, 2017 02:10

Make code more readable

843fb2e

wangkuiyi approved these changes Jan 19, 2017

View reviewed changes

reyoung merged commit 7f0ad62 into PaddlePaddle:develop Jan 19, 2017

reyoung deleted the feature/ErrorHandlingInPaddle branch February 24, 2017 06:41

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

fix MiniLMv2 model name in readme (PaddlePaddle#1149)

fd02d65

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

fix act typo (PaddlePaddle#1149)

567a97d

Feature/error handling in paddle #1149

Feature/error handling in paddle #1149

Uh oh!

Conversation

reyoung commented Jan 14, 2017

reyoung commented Jan 16, 2017

hedaoyuan 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

wangkuiyi 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

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 Jan 18, 2017

wangkuiyi commented Jan 18, 2017

reyoung commented Jan 18, 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

Choose a reason for hiding this comment

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

Uh oh!

Choose a reason for hiding this comment

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

Uh oh!

Labels

3 participants

wangkuiyi Jan 19, 2017 •

edited

Loading

wangkuiyi commented Jan 18, 2017 •

edited

Loading