From 02480316016bd04c10676d8cb859c473f07a819f Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Thu, 12 Jan 2017 17:18:39 +0800 Subject: [PATCH 01/22] Add Status --- paddle/utils/Status.h | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 paddle/utils/Status.h diff --git a/paddle/utils/Status.h b/paddle/utils/Status.h new file mode 100644 index 0000000000..398ae182ab --- /dev/null +++ b/paddle/utils/Status.h @@ -0,0 +1,39 @@ +/* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserve. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. */ +#include +#include + +namespace paddle { + +class Status final : public std::exception { +public: + Status() noexcept {} + + Status(const std::string& msg) : errMsg_(new std::string(msg)) {} + + virtual const char* what() const noexcept override { + if (errMsg_) { + return errMsg_->data(); + } else { + return nullptr; + } + } + + inline bool isOK() const noexcept { return errMsg_ == nullptr; } + +private: + std::unique_ptr errMsg_; +}; + +} // namespace paddle From 6c20e08b042e1351a4ea8c97a74129d211b1d636 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Thu, 12 Jan 2017 17:55:36 +0800 Subject: [PATCH 02/22] Try using status to handle Paddle Error --- .../activations/ActivationFunction.cpp | 126 ++++++++++++++---- .../gserver/activations/ActivationFunction.h | 5 +- paddle/gserver/layers/Layer.cpp | 7 +- paddle/utils/Status.h | 36 ++++- paddle/utils/tests/CMakeLists.txt | 1 + paddle/utils/tests/test_Status.cpp | 29 ++++ 6 files changed, 169 insertions(+), 35 deletions(-) create mode 100644 paddle/utils/tests/test_Status.cpp diff --git a/paddle/gserver/activations/ActivationFunction.cpp b/paddle/gserver/activations/ActivationFunction.cpp index f8c4bcac2f..8a938cf7e9 100644 --- a/paddle/gserver/activations/ActivationFunction.cpp +++ b/paddle/gserver/activations/ActivationFunction.cpp @@ -69,8 +69,14 @@ static ClassRegistrar gActivationRegistrar; class IdentityActivation : public ActivationFunction { public: static const std::string name; - void forward(Argument& act) { (void)act; } - void backward(Argument& act) { (void)act; } + Status forward(Argument& act) { + (void)act; + return Status(); + } + Status backward(Argument& act) { + (void)act; + return Status(); + } const std::string& getName() const { return name; } }; const std::string IdentityActivation::name = ""; @@ -86,8 +92,14 @@ static InitFunction __reg_activation__identity([] { * \f] */ BEGIN_DEFINE_ACTIVATION(sigmoid) -void forward(Argument& act) { act.value->sigmoid(*act.value); } -void backward(Argument& act) { act.grad->sigmoidDerivative(*act.value); } +Status forward(Argument& act) { + act.value->sigmoid(*act.value); + return Status(); +} +Status backward(Argument& act) { + act.grad->sigmoidDerivative(*act.value); + return Status(); +} END_DEFINE_ACTIVATION(sigmoid) /** @@ -103,9 +115,12 @@ MatrixPtr sftMaxDot_; MatrixPtr one_; public: -void forward(Argument& act) { act.value->softmax(*act.value); } +Status forward(Argument& act) { + act.value->softmax(*act.value); + return Status(); +} -void backward(Argument& act) { +Status backward(Argument& act) { MatrixPtr outputV = act.value; MatrixPtr outputG = act.grad; @@ -137,6 +152,7 @@ void backward(Argument& act) { act.grad->softmaxDerivative(*act.value, *sftMaxSum_); } + return Status(); } END_DEFINE_ACTIVATION(softmax) @@ -151,8 +167,11 @@ ACTIVATION_CLASS_NAME(softmax) softmax_; Argument argument_; public: -void forward(Argument& act) { - CHECK_EQ(act.value->getWidth(), 1UL); +Status forward(Argument& act) { + if (act.value->getWidth() != 1UL) { + return Status( + "Input width for each timestep of sequence softmax should be 1"); + } if (!argument_.value) { argument_.value = Matrix::create(nullptr, @@ -169,10 +188,14 @@ void forward(Argument& act) { auto starts = act.sequenceStartPositions->getVector(useGpu(act.deviceId)); act.value->sequenceSoftmax(*act.value, *starts); + return Status(); } -void backward(Argument& act) { - CHECK_EQ(act.grad->getWidth(), 1UL); +Status backward(Argument& act) { + if (act.value->getWidth() != 1UL) { + return Status( + "Input width for each timestep of sequence softmax should be 1"); + } size_t numSequences = act.getNumSequences(); const int* starts = act.sequenceStartPositions->getData(false); @@ -186,6 +209,7 @@ void backward(Argument& act) { softmax_.backward(argument_); } + return Status(); } END_DEFINE_ACTIVATION(sequence_softmax) @@ -200,9 +224,15 @@ END_DEFINE_ACTIVATION(sequence_softmax) * 0 otherwise. */ BEGIN_DEFINE_ACTIVATION(relu) -void forward(Argument& act) { act.value->relu(*act.value); } +Status forward(Argument& act) { + act.value->relu(*act.value); + return Status(); +} -void backward(Argument& act) { act.grad->reluDerivative(*act.value); } +Status backward(Argument& act) { + act.grad->reluDerivative(*act.value); + return Status(); +} END_DEFINE_ACTIVATION(relu) /** @@ -219,9 +249,15 @@ END_DEFINE_ACTIVATION(relu) * TODO(yuyang18): Remove magic number 24 or make it configuable. */ BEGIN_DEFINE_ACTIVATION(brelu) -void forward(Argument& act) { act.value->brelu(*act.value); } +Status forward(Argument& act) { + act.value->brelu(*act.value); + return Status(); +} -void backward(Argument& act) { act.grad->breluDerivative(*act.value); } +Status backward(Argument& act) { + act.grad->breluDerivative(*act.value); + return Status(); +} END_DEFINE_ACTIVATION(brelu) /** @@ -231,9 +267,15 @@ END_DEFINE_ACTIVATION(brelu) * \f] */ BEGIN_DEFINE_ACTIVATION(tanh) -void forward(Argument& act) { act.value->tanh(*act.value); } +Status forward(Argument& act) { + act.value->tanh(*act.value); + return Status(); +} -void backward(Argument& act) { act.grad->tanhDerivative(*act.value); } +Status backward(Argument& act) { + act.grad->tanhDerivative(*act.value); + return Status(); +} END_DEFINE_ACTIVATION(tanh) /** @@ -248,10 +290,14 @@ real a, b; public: ACTIVATION_CLASS_NAME(stanh)() : a(1.7159), b(2. / 3.) {} -void forward(Argument& act) { act.value->scaledTanh(*act.value, a, b); } +Status forward(Argument& act) { + act.value->scaledTanh(*act.value, a, b); + return Status(); +} -void backward(Argument& act) { +Status backward(Argument& act) { act.grad->scaledTanhDerivative(*act.value, a, b); + return Status(); } END_DEFINE_ACTIVATION(stanh) @@ -262,9 +308,15 @@ END_DEFINE_ACTIVATION(stanh) * \f] */ BEGIN_DEFINE_ACTIVATION(softrelu) -void forward(Argument& act) { act.value->softrelu(*act.value); } +Status forward(Argument& act) { + act.value->softrelu(*act.value); + return Status(); +} -void backward(Argument& act) { act.grad->softreluDerivative(*act.value); } +Status backward(Argument& act) { + act.grad->softreluDerivative(*act.value); + return Status(); +} END_DEFINE_ACTIVATION(softrelu) /** @@ -280,7 +332,7 @@ END_DEFINE_ACTIVATION(softrelu) * 0 if z=0 */ BEGIN_DEFINE_ACTIVATION(abs) -void forward(Argument& act) { +Status forward(Argument& act) { SetDevice device(act.deviceId); Matrix::resizeOrCreate(act.in, act.value->getHeight(), @@ -290,9 +342,13 @@ void forward(Argument& act) { act.in->copyFrom(*act.value); act.value->abs2(*act.value); + return Status(); } -void backward(Argument& act) { act.grad->absDerivative(*act.in); } +Status backward(Argument& act) { + act.grad->absDerivative(*act.in); + return Status(); +} END_DEFINE_ACTIVATION(abs) /** @@ -302,7 +358,7 @@ END_DEFINE_ACTIVATION(abs) * \f] */ BEGIN_DEFINE_ACTIVATION(square) -void forward(Argument& act) { +Status forward(Argument& act) { SetDevice device(act.deviceId); Matrix::resizeOrCreate(act.in, act.value->getHeight(), @@ -312,9 +368,13 @@ void forward(Argument& act) { act.in->copyFrom(*act.value); act.value->square2(*act.value); + return Status(); } -void backward(Argument& act) { act.grad->squareDerivative(*act.in); } +Status backward(Argument& act) { + act.grad->squareDerivative(*act.in); + return Status(); +} END_DEFINE_ACTIVATION(square) /** @@ -324,9 +384,15 @@ END_DEFINE_ACTIVATION(square) * \f] */ BEGIN_DEFINE_ACTIVATION(exponential) -void forward(Argument& act) { act.value->exp2(*act.value); } +Status forward(Argument& act) { + act.value->exp2(*act.value); + return Status(); +} -void backward(Argument& act) { act.grad->expDerivative(*act.value); } +Status backward(Argument& act) { + act.grad->expDerivative(*act.value); + return Status(); +} END_DEFINE_ACTIVATION(exponential) /** @@ -336,7 +402,7 @@ END_DEFINE_ACTIVATION(exponential) * \f] */ BEGIN_DEFINE_ACTIVATION(log) -void forward(Argument& act) { +Status forward(Argument& act) { SetDevice device(act.deviceId); Matrix::resizeOrCreate(act.in, act.value->getHeight(), @@ -346,9 +412,13 @@ void forward(Argument& act) { act.in->copyFrom(*act.value); act.value->log2(*act.value); + return Status(); } -void backward(Argument& act) { act.grad->dotDiv(*act.grad, *act.in); } +Status backward(Argument& act) { + act.grad->dotDiv(*act.grad, *act.in); + return Status(); +} END_DEFINE_ACTIVATION(log) ActivationFunction* ActivationFunction::create(const std::string& type) { diff --git a/paddle/gserver/activations/ActivationFunction.h b/paddle/gserver/activations/ActivationFunction.h index 601e3b6c0c..ad395ac28d 100644 --- a/paddle/gserver/activations/ActivationFunction.h +++ b/paddle/gserver/activations/ActivationFunction.h @@ -15,6 +15,7 @@ limitations under the License. */ #pragma once #include #include +#include "paddle/utils/Status.h" namespace paddle { @@ -48,7 +49,7 @@ public: * * Usually, act is Layer::output_ */ - virtual void forward(Argument& act) = 0; + virtual Status forward(Argument& act) = 0; /** * @brief Backward propagaion @@ -57,7 +58,7 @@ public: * - Before calling backward(), act.grad = dE / dy, where E is the error/cost * - After backward() returns, act.grad = dE / dx = (dE/dy) * (dy/dx) */ - virtual void backward(Argument& act) = 0; + virtual Status backward(Argument& act) = 0; virtual const std::string& getName() const = 0; }; diff --git a/paddle/gserver/layers/Layer.cpp b/paddle/gserver/layers/Layer.cpp index c47943f81c..06c936c3ae 100644 --- a/paddle/gserver/layers/Layer.cpp +++ b/paddle/gserver/layers/Layer.cpp @@ -16,6 +16,7 @@ limitations under the License. */ #include "paddle/math/SparseMatrix.h" #include "paddle/utils/Logging.h" +#include "paddle/utils/Status.h" #include "AddtoLayer.h" #include "CRFLayer.h" @@ -334,7 +335,8 @@ void Layer::showOutputStats() { void Layer::forwardActivation() { /* activation */ - activation_->forward(output_); + auto status = activation_->forward(output_); + CHECK(status.isOK()) << status.what(); /* dropout */ if (config_.drop_rate() > 0) { @@ -372,7 +374,8 @@ void Layer::backwardActivation() { oGrad->dotMul(*oGrad, *dropOutMask_); } - activation_->backward(output_); + auto status = activation_->backward(output_); + CHECK(status.isOK()) << status.what(); } void Layer::forwardDropOut() { diff --git a/paddle/utils/Status.h b/paddle/utils/Status.h index 398ae182ab..3456d7b686 100644 --- a/paddle/utils/Status.h +++ b/paddle/utils/Status.h @@ -11,18 +11,44 @@ distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. */ + +#pragma once + #include #include namespace paddle { +/** + * Status is Paddle error code. It only contain a std::string as error message. + * Although Status inherits the std::exception, but do not throw it except you + * know what you are doing. + */ class Status final : public std::exception { public: + /** + * Default Status. OK + */ Status() noexcept {} - Status(const std::string& msg) : errMsg_(new std::string(msg)) {} + /** + * @brief Create Status with error message + * @param msg + */ + explicit Status(const std::string& msg) : errMsg_(new std::string(msg)) {} + + /** + * @brief set a error message for status. + * @param msg + */ + inline void set(const std::string& msg) noexcept { + errMsg_.reset(new std::string(msg)); + } - virtual const char* what() const noexcept override { + /** + * @brief what will return the error message. If status is OK, return nullptr. + */ + const char* what() const noexcept override { if (errMsg_) { return errMsg_->data(); } else { @@ -30,10 +56,14 @@ public: } } + /** + * @brief isOK + * @return true if OK. + */ inline bool isOK() const noexcept { return errMsg_ == nullptr; } private: - std::unique_ptr errMsg_; + std::shared_ptr errMsg_; }; } // namespace paddle diff --git a/paddle/utils/tests/CMakeLists.txt b/paddle/utils/tests/CMakeLists.txt index 26fafbd1ab..a1cc32668d 100644 --- a/paddle/utils/tests/CMakeLists.txt +++ b/paddle/utils/tests/CMakeLists.txt @@ -4,6 +4,7 @@ add_simple_unittest(test_CustomStackTrace) add_simple_unittest(test_ThreadBarrier) add_simple_unittest(test_SpinLock) add_simple_unittest(test_SIMDFlags) +add_simple_unittest(test_Status) add_executable( test_CustomStackTracePrint diff --git a/paddle/utils/tests/test_Status.cpp b/paddle/utils/tests/test_Status.cpp new file mode 100644 index 0000000000..e2c2ae537d --- /dev/null +++ b/paddle/utils/tests/test_Status.cpp @@ -0,0 +1,29 @@ +/* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserve. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. */ + +#include "paddle/utils/Status.h" + +#include + +TEST(Status, testAll) { + paddle::Status status; + ASSERT_TRUE(status.isOK()); + status.set("I'm the error"); + ASSERT_FALSE(status.isOK()); + ASSERT_STREQ("I'm the error", status.what()); + + paddle::Status status2("error2"); + ASSERT_FALSE(status2.isOK()); + ASSERT_STREQ("error2", status2.what()); +} From 741637eba41f66b51bd1764900e75cc7d5bd9ce6 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Mon, 16 Jan 2017 16:14:29 +0800 Subject: [PATCH 03/22] Add printf method to Status. --- paddle/utils/Status.h | 23 +++++++++++++++++++++++ paddle/utils/tests/test_Status.cpp | 5 +++++ 2 files changed, 28 insertions(+) diff --git a/paddle/utils/Status.h b/paddle/utils/Status.h index 3456d7b686..db1edfb7c7 100644 --- a/paddle/utils/Status.h +++ b/paddle/utils/Status.h @@ -14,6 +14,7 @@ limitations under the License. */ #pragma once +#include #include #include @@ -45,6 +46,28 @@ public: errMsg_.reset(new std::string(msg)); } + /** + * @brief set a error message for status. Use C style printf + * @param fmt + */ + template + inline void setByPrintf(const char* fmt, ARGS... args) noexcept { + constexpr size_t bufferSize = 4096; + char buffer[bufferSize]; + snprintf(buffer, bufferSize, fmt, args...); + errMsg_.reset(new std::string(buffer)); + } + + /** + * create a error status by C style printf. + */ + template + inline static Status printf(const char* fmt, ARGS... args) noexcept { + Status s; + s.setByPrintf(fmt, args...); + return s; + } + /** * @brief what will return the error message. If status is OK, return nullptr. */ diff --git a/paddle/utils/tests/test_Status.cpp b/paddle/utils/tests/test_Status.cpp index e2c2ae537d..04cef09579 100644 --- a/paddle/utils/tests/test_Status.cpp +++ b/paddle/utils/tests/test_Status.cpp @@ -26,4 +26,9 @@ TEST(Status, testAll) { paddle::Status status2("error2"); ASSERT_FALSE(status2.isOK()); ASSERT_STREQ("error2", status2.what()); + + int i = 3; + auto status3 = paddle::Status::printf("error%d", i); + ASSERT_FALSE(status3.isOK()); + ASSERT_STREQ("error3", status3.what()); } From 8aefc30499e09729b5755fe8edfd32ba72a9baed Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Tue, 17 Jan 2017 10:59:27 +0800 Subject: [PATCH 04/22] Fix compile error. --- paddle/utils/Status.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/paddle/utils/Status.h b/paddle/utils/Status.h index db1edfb7c7..52f312378e 100644 --- a/paddle/utils/Status.h +++ b/paddle/utils/Status.h @@ -52,9 +52,9 @@ public: */ template inline void setByPrintf(const char* fmt, ARGS... args) noexcept { - constexpr size_t bufferSize = 4096; - char buffer[bufferSize]; - snprintf(buffer, bufferSize, fmt, args...); + constexpr size_t kBufferSize = 4096; + char buffer[kBufferSize]; + snprintf(buffer, kBufferSize, fmt, args...); errMsg_.reset(new std::string(buffer)); } From 3d01c60e25c1b5874e6854ac565646d6ad9432d7 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Tue, 17 Jan 2017 15:44:09 +0800 Subject: [PATCH 05/22] Stash --- paddle/utils/Compiler.h | 0 paddle/utils/Status.h | 9 ++++++++- 2 files changed, 8 insertions(+), 1 deletion(-) create mode 100644 paddle/utils/Compiler.h diff --git a/paddle/utils/Compiler.h b/paddle/utils/Compiler.h new file mode 100644 index 0000000000..e69de29bb2 diff --git a/paddle/utils/Status.h b/paddle/utils/Status.h index 52f312378e..cb66e4b225 100644 --- a/paddle/utils/Status.h +++ b/paddle/utils/Status.h @@ -24,6 +24,13 @@ namespace paddle { * Status is Paddle error code. It only contain a std::string as error message. * Although Status inherits the std::exception, but do not throw it except you * know what you are doing. + * + * + * There are two styles to return status in Paddle. + * + * 1. Return Status + * + * */ class Status final : public std::exception { public: @@ -52,7 +59,7 @@ public: */ template inline void setByPrintf(const char* fmt, ARGS... args) noexcept { - constexpr size_t kBufferSize = 4096; + constexpr size_t kBufferSize = 1024; // 1KB buffer char buffer[kBufferSize]; snprintf(buffer, kBufferSize, fmt, args...); errMsg_.reset(new std::string(buffer)); From 9bc12034002d0c0ca5f30bd1f11b30188978e327 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Wed, 18 Jan 2017 12:45:00 +0800 Subject: [PATCH 06/22] Add more comments, also add __must_check. --- .../activations/ActivationFunction.cpp | 55 +++++++++--------- .../gserver/activations/ActivationFunction.h | 4 +- paddle/gserver/layers/Layer.cpp | 4 +- paddle/gserver/layers/MDLstmLayer.cpp | 25 +++++---- paddle/gserver/layers/NCELayer.cpp | 6 +- paddle/gserver/layers/RecurrentLayer.cpp | 21 +++---- .../layers/SelectiveFullyConnectedLayer.cpp | 3 +- paddle/gserver/tests/test_WarpCTCLayer.cpp | 4 +- paddle/utils/Compiler.h | 25 +++++++++ paddle/utils/Status.h | 56 +++++++++++++++++++ 10 files changed, 147 insertions(+), 56 deletions(-) diff --git a/paddle/gserver/activations/ActivationFunction.cpp b/paddle/gserver/activations/ActivationFunction.cpp index 8a938cf7e9..666c2e01c8 100644 --- a/paddle/gserver/activations/ActivationFunction.cpp +++ b/paddle/gserver/activations/ActivationFunction.cpp @@ -69,11 +69,11 @@ static ClassRegistrar gActivationRegistrar; class IdentityActivation : public ActivationFunction { public: static const std::string name; - Status forward(Argument& act) { + Status __must_check forward(Argument& act) { (void)act; return Status(); } - Status backward(Argument& act) { + Status __must_check backward(Argument& act) { (void)act; return Status(); } @@ -92,11 +92,11 @@ static InitFunction __reg_activation__identity([] { * \f] */ BEGIN_DEFINE_ACTIVATION(sigmoid) -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { act.value->sigmoid(*act.value); return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { act.grad->sigmoidDerivative(*act.value); return Status(); } @@ -115,12 +115,12 @@ MatrixPtr sftMaxDot_; MatrixPtr one_; public: -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { act.value->softmax(*act.value); return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { MatrixPtr outputV = act.value; MatrixPtr outputG = act.grad; @@ -167,7 +167,7 @@ ACTIVATION_CLASS_NAME(softmax) softmax_; Argument argument_; public: -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { if (act.value->getWidth() != 1UL) { return Status( "Input width for each timestep of sequence softmax should be 1"); @@ -191,7 +191,7 @@ Status forward(Argument& act) { return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { if (act.value->getWidth() != 1UL) { return Status( "Input width for each timestep of sequence softmax should be 1"); @@ -207,7 +207,8 @@ Status backward(Argument& act) { argument_.value->setData(act.value->getData() + offset, 1UL, size); argument_.grad->setData(act.grad->getData() + offset, 1UL, size); - softmax_.backward(argument_); + Status status = softmax_.backward(argument_); + if (!status.isOK()) return status; } return Status(); } @@ -224,12 +225,12 @@ END_DEFINE_ACTIVATION(sequence_softmax) * 0 otherwise. */ BEGIN_DEFINE_ACTIVATION(relu) -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { act.value->relu(*act.value); return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { act.grad->reluDerivative(*act.value); return Status(); } @@ -249,12 +250,12 @@ END_DEFINE_ACTIVATION(relu) * TODO(yuyang18): Remove magic number 24 or make it configuable. */ BEGIN_DEFINE_ACTIVATION(brelu) -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { act.value->brelu(*act.value); return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { act.grad->breluDerivative(*act.value); return Status(); } @@ -267,12 +268,12 @@ END_DEFINE_ACTIVATION(brelu) * \f] */ BEGIN_DEFINE_ACTIVATION(tanh) -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { act.value->tanh(*act.value); return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { act.grad->tanhDerivative(*act.value); return Status(); } @@ -290,12 +291,12 @@ real a, b; public: ACTIVATION_CLASS_NAME(stanh)() : a(1.7159), b(2. / 3.) {} -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { act.value->scaledTanh(*act.value, a, b); return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { act.grad->scaledTanhDerivative(*act.value, a, b); return Status(); } @@ -308,12 +309,12 @@ END_DEFINE_ACTIVATION(stanh) * \f] */ BEGIN_DEFINE_ACTIVATION(softrelu) -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { act.value->softrelu(*act.value); return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { act.grad->softreluDerivative(*act.value); return Status(); } @@ -332,7 +333,7 @@ END_DEFINE_ACTIVATION(softrelu) * 0 if z=0 */ BEGIN_DEFINE_ACTIVATION(abs) -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { SetDevice device(act.deviceId); Matrix::resizeOrCreate(act.in, act.value->getHeight(), @@ -345,7 +346,7 @@ Status forward(Argument& act) { return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { act.grad->absDerivative(*act.in); return Status(); } @@ -358,7 +359,7 @@ END_DEFINE_ACTIVATION(abs) * \f] */ BEGIN_DEFINE_ACTIVATION(square) -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { SetDevice device(act.deviceId); Matrix::resizeOrCreate(act.in, act.value->getHeight(), @@ -371,7 +372,7 @@ Status forward(Argument& act) { return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { act.grad->squareDerivative(*act.in); return Status(); } @@ -384,12 +385,12 @@ END_DEFINE_ACTIVATION(square) * \f] */ BEGIN_DEFINE_ACTIVATION(exponential) -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { act.value->exp2(*act.value); return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { act.grad->expDerivative(*act.value); return Status(); } @@ -402,7 +403,7 @@ END_DEFINE_ACTIVATION(exponential) * \f] */ BEGIN_DEFINE_ACTIVATION(log) -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { SetDevice device(act.deviceId); Matrix::resizeOrCreate(act.in, act.value->getHeight(), @@ -415,7 +416,7 @@ Status forward(Argument& act) { return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { act.grad->dotDiv(*act.grad, *act.in); return Status(); } diff --git a/paddle/gserver/activations/ActivationFunction.h b/paddle/gserver/activations/ActivationFunction.h index ad395ac28d..737df2219d 100644 --- a/paddle/gserver/activations/ActivationFunction.h +++ b/paddle/gserver/activations/ActivationFunction.h @@ -49,7 +49,7 @@ public: * * Usually, act is Layer::output_ */ - virtual Status forward(Argument& act) = 0; + virtual Status __must_check forward(Argument& act) = 0; /** * @brief Backward propagaion @@ -58,7 +58,7 @@ public: * - Before calling backward(), act.grad = dE / dy, where E is the error/cost * - After backward() returns, act.grad = dE / dx = (dE/dy) * (dy/dx) */ - virtual Status backward(Argument& act) = 0; + virtual Status __must_check backward(Argument& act) = 0; virtual const std::string& getName() const = 0; }; diff --git a/paddle/gserver/layers/Layer.cpp b/paddle/gserver/layers/Layer.cpp index 06c936c3ae..f96070fe6e 100644 --- a/paddle/gserver/layers/Layer.cpp +++ b/paddle/gserver/layers/Layer.cpp @@ -336,7 +336,7 @@ void Layer::showOutputStats() { void Layer::forwardActivation() { /* activation */ auto status = activation_->forward(output_); - CHECK(status.isOK()) << status.what(); + status.check(); /* dropout */ if (config_.drop_rate() > 0) { @@ -375,7 +375,7 @@ void Layer::backwardActivation() { } auto status = activation_->backward(output_); - CHECK(status.isOK()) << status.what(); + status.check(); } void Layer::forwardDropOut() { diff --git a/paddle/gserver/layers/MDLstmLayer.cpp b/paddle/gserver/layers/MDLstmLayer.cpp index fb41af5631..88d934d782 100644 --- a/paddle/gserver/layers/MDLstmLayer.cpp +++ b/paddle/gserver/layers/MDLstmLayer.cpp @@ -506,9 +506,12 @@ void MDLstmLayer::forwardGate2OutputSequence(int start, *frameState_[start + preOffsetV[i]].value, *checkFgOneDim, 1.0, 1.0); } } - activationGate_->forward(frameInputGate_[idxCurr]); - activationGate_->forward(frameForgetGate_[idxCurr]); - activation_->forward(frameInputNode_[idxCurr]); + auto status = activationGate_->forward(frameInputGate_[idxCurr]); + status.check(); + status = activationGate_->forward(frameForgetGate_[idxCurr]); + status.check(); + status = activation_->forward(frameInputNode_[idxCurr]); + status.check(); frameState_[idxCurr].value->zeroMem(); for (int i = 0; i < numDims_; i++) { @@ -530,10 +533,12 @@ void MDLstmLayer::forwardGate2OutputSequence(int start, frameOutputGate_[idxCurr].value->addDotMul( *frameState_[idxCurr].value, *checkOg_, 1.0, 1.0); - activationGate_->forward(frameOutputGate_[idxCurr]); + status = activationGate_->forward(frameOutputGate_[idxCurr]); + status.check(); framePreOutput_[idxCurr].value->copyFrom(*(frameState_[idxCurr].value)); - activationState_->forward(framePreOutput_[idxCurr]); + status = activationState_->forward(framePreOutput_[idxCurr]); + status.check(); frameOutput_[idxCurr].value->dotMul(*framePreOutput_[idxCurr].value, *frameOutputGate_[idxCurr].value); @@ -640,12 +645,12 @@ void MDLstmLayer::backwardGate2OutputSequence(int start, framePreOutput_[idxCurr].grad->dotMul(*frameOutput_[idxCurr].grad, *frameOutputGate_[idxCurr].value); - activationState_->backward(framePreOutput_[idxCurr]); + activationState_->backward(framePreOutput_[idxCurr]).check(); frameState_[idxCurr].grad->copyFrom(*(framePreOutput_[idxCurr].grad)); frameOutputGate_[idxCurr].grad->dotMul(*frameOutput_[idxCurr].grad, *framePreOutput_[idxCurr].value); - activationGate_->backward(frameOutputGate_[idxCurr]); + activationGate_->backward(frameOutputGate_[idxCurr]).check(); frameState_[idxCurr].grad->addDotMul( *frameOutputGate_[idxCurr].grad, *checkOg_, 1.0, 1.0); @@ -702,9 +707,9 @@ void MDLstmLayer::backwardGate2OutputSequence(int start, } } - activationGate_->backward(frameInputGate_[idxCurr]); - activationGate_->backward(frameForgetGate_[idxCurr]); - activation_->backward(frameInputNode_[idxCurr]); + activationGate_->backward(frameInputGate_[idxCurr]).check(); + activationGate_->backward(frameForgetGate_[idxCurr]).check(); + activation_->backward(frameInputNode_[idxCurr]).check(); if (bias_->getWGrad()) { for (int i = 0; i < numDims_; i++) { diff --git a/paddle/gserver/layers/NCELayer.cpp b/paddle/gserver/layers/NCELayer.cpp index 5ab765247f..3542e739df 100644 --- a/paddle/gserver/layers/NCELayer.cpp +++ b/paddle/gserver/layers/NCELayer.cpp @@ -193,7 +193,8 @@ public: forwardOneInput(l); } - activation_->forward(sampleOut_); + auto status = activation_->forward(sampleOut_); + status.check(); forwardCost(); } @@ -207,7 +208,8 @@ public: backwardCost(); - activation_->backward(sampleOut_); + auto status = activation_->backward(sampleOut_); + status.check(); if (biases_->getWGrad()) { backwardBias(callback); diff --git a/paddle/gserver/layers/RecurrentLayer.cpp b/paddle/gserver/layers/RecurrentLayer.cpp index 55e0fdfb90..b843fa1265 100644 --- a/paddle/gserver/layers/RecurrentLayer.cpp +++ b/paddle/gserver/layers/RecurrentLayer.cpp @@ -217,21 +217,22 @@ void RecurrentLayer::forwardOneSequence(int start, int length) { if (prevOutput_) { frameOutput_[start].value->mul(*prevOutput_, *weight_->getW(), 1, 1); } - activation_->forward(frameOutput_[start]); + activation_->forward(frameOutput_[start]).check(); + for (int i = 1; i < length; ++i) { frameOutput_[start + i].value->mul( *frameOutput_[start + i - 1].value, *weight_->getW(), 1, 1); - activation_->forward(frameOutput_[start + i]); + activation_->forward(frameOutput_[start + i]).check(); } if (prevOutput_) { prevOutput_->assign(*frameOutput_[start + length - 1].value); } } else { - activation_->forward(frameOutput_[start + length - 1]); + activation_->forward(frameOutput_[start + length - 1]).check(); for (int i = length - 2; i >= 0; --i) { frameOutput_[start + i].value->mul( *frameOutput_[start + i + 1].value, *weight_->getW(), 1, 1); - activation_->forward(frameOutput_[start + i]); + activation_->forward(frameOutput_[start + i]).check(); } } } @@ -280,11 +281,11 @@ void RecurrentLayer::backwardOneSequence(int start, int length) { MatrixPtr weightT = weight_->getW()->getTranspose(); if (!reversed_) { for (int i = length - 1; i > 0; --i) { - activation_->backward(frameOutput_[start + i]); + activation_->backward(frameOutput_[start + i]).check(); frameOutput_[start + i - 1].grad->mul( *frameOutput_[start + i].grad, *weightT, 1, 1); } - activation_->backward(frameOutput_[start]); + activation_->backward(frameOutput_[start]).check(); if (weight_->getWGrad()) { weight_->getWGrad()->mul( *output_.value->subMatrix(start, length - 1)->getTranspose(), @@ -294,11 +295,11 @@ void RecurrentLayer::backwardOneSequence(int start, int length) { } } else { for (int i = 0; i < length - 1; ++i) { - activation_->backward(frameOutput_[start + i]); + activation_->backward(frameOutput_[start + i]).check(); frameOutput_[start + i + 1].grad->mul( *frameOutput_[start + i].grad, *weightT, 1, 1); } - activation_->backward(frameOutput_[start + length - 1]); + activation_->backward(frameOutput_[start + length - 1]).check(); if (weight_->getWGrad()) { weight_->getWGrad()->mul( *output_.value->subMatrix(start + 1, length - 1)->getTranspose(), @@ -333,7 +334,7 @@ void RecurrentLayer::forwardBatch(int batchSize, } Argument arg; arg.value = batch2; - activation_->forward(arg); + activation_->forward(arg).check(); } } batchValue_->copyBackSeq(*output_.value); @@ -363,7 +364,7 @@ void RecurrentLayer::backwardBatch(int batchSize, Argument arg; arg.value = batch1; arg.grad = batch2; - activation_->backward(arg); + activation_->backward(arg).check(); if (n != 0) { batch1 = batchGrad_->getBatchValue(n - 1, batch2->getHeight()); diff --git a/paddle/gserver/layers/SelectiveFullyConnectedLayer.cpp b/paddle/gserver/layers/SelectiveFullyConnectedLayer.cpp index 5eacff6b71..d9a91de8a6 100644 --- a/paddle/gserver/layers/SelectiveFullyConnectedLayer.cpp +++ b/paddle/gserver/layers/SelectiveFullyConnectedLayer.cpp @@ -192,7 +192,8 @@ void SelectiveFullyConnectedLayer::forward(PassType passType) { nnz, /*trans=*/false, /*useGpu=*/useGpu_); - activation_->forward(arg); + //! TODO(yuyang18): Why we cannot invoke forwardActivation here? + activation_->forward(arg).check(); } else /* train and test in train, not generating */ { // during training, this layer output value is *Matrix*, which is input of // eg. multi-class-cross-entropy diff --git a/paddle/gserver/tests/test_WarpCTCLayer.cpp b/paddle/gserver/tests/test_WarpCTCLayer.cpp index 23ae95852e..55427e2f12 100644 --- a/paddle/gserver/tests/test_WarpCTCLayer.cpp +++ b/paddle/gserver/tests/test_WarpCTCLayer.cpp @@ -148,11 +148,11 @@ LayerPtr createCTCLayer(string name, ActivationFunction* softmaxActivation = ActivationFunction::create("softmax"); - softmaxActivation->forward(dataLayer->getOutput()); + softmaxActivation->forward(dataLayer->getOutput()).check(); layer->forward(PASS_GC); layer->backward(); - softmaxActivation->backward(dataLayer->getOutput()); + softmaxActivation->backward(dataLayer->getOutput()).check(); return layer; } diff --git a/paddle/utils/Compiler.h b/paddle/utils/Compiler.h index e69de29bb2..22812e8398 100644 --- a/paddle/utils/Compiler.h +++ b/paddle/utils/Compiler.h @@ -0,0 +1,25 @@ +/* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserved. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. */ + +#pragma once + +#ifdef __GNUC__ +#define GCC_VERSION \ + (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__) +#else +#define GCC_VERSION +#endif + +#if GCC_VERSION >= 30400 +#define __must_check __attribute__((warn_unused_result)) +#else +#define __must_check +#endif diff --git a/paddle/utils/Status.h b/paddle/utils/Status.h index cb66e4b225..26329f8d19 100644 --- a/paddle/utils/Status.h +++ b/paddle/utils/Status.h @@ -14,9 +14,11 @@ limitations under the License. */ #pragma once +#include #include #include #include +#include "Compiler.h" namespace paddle { @@ -29,8 +31,55 @@ namespace paddle { * There are two styles to return status in Paddle. * * 1. Return Status + * When method return a status, the return must use `__must_check` attribute. + * Example as below. + * @code{cpp} + * Status __must_check foo(); * + * Status __must_check bar() { + * // do something. + * Status s = foo(); // invoke other method return status. + * if (!s.isOK()) return s; + * // do something else. + * return Status(); + * } + * @endcode{cpp} * + * 2. Return by parameter. + * It is another way to return a status, by using a pointer parameter. + * Example as below. + * + * @code{cpp} + * Status bar(); + * + * int foo(Status* status) { + * // Do something. + * Status s = bar(); + * if (!s.isOK()) { + * *status = s; + * return 0; + * } + * // Do something else. + * if (someInternalErrorHappend) { + * status->setByPrintf("Some dimension is too large, %d", dimension); + * return 0; + * } + * // End of method. + * return someValue; + * } + * + * Status foobar() { + * Status s; + * // do something. + * foo(&s); + * if (!s.isOK()) return s; + * } + * @endcode{cpp} + * + * + * Currently there is a helper method 'check' in status, because Paddle always + * 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 Status final : public std::exception { public: @@ -92,6 +141,13 @@ public: */ inline bool isOK() const noexcept { return errMsg_ == nullptr; } + /** + * @brief check this status by glog. + * @note It is a temp method used during cleaning Paddle code. It will be + * removed later. + */ + inline void check() const { CHECK(isOK()) << what(); } + private: std::shared_ptr errMsg_; }; From 8605544c0b0cbc5ad43d86a71402f3f4075b48e3 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Wed, 18 Jan 2017 12:49:00 +0800 Subject: [PATCH 07/22] Add some comments to compiler.h --- paddle/utils/Compiler.h | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/paddle/utils/Compiler.h b/paddle/utils/Compiler.h index 22812e8398..cebca5a2a3 100644 --- a/paddle/utils/Compiler.h +++ b/paddle/utils/Compiler.h @@ -10,7 +10,10 @@ See the License for the specific language governing permissions and limitations under the License. */ #pragma once - +/** + * This header defines some useful attribute by each compiler. It is the + * abstract layer of compilers. + */ #ifdef __GNUC__ #define GCC_VERSION \ (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__) @@ -18,6 +21,11 @@ limitations under the License. */ #define GCC_VERSION #endif +/** + * __must_check macro. It make the function's return value must be used, + * otherwise it will raise a compile warning. And also Paddle treat all compile + * warnings as errors. + */ #if GCC_VERSION >= 30400 #define __must_check __attribute__((warn_unused_result)) #else From ec790e1050b52b09a182eed95fc030c7879e5012 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Wed, 18 Jan 2017 13:24:02 +0800 Subject: [PATCH 08/22] Rename Status => Error. * Also make ErrorF as a global method. --- .../activations/ActivationFunction.cpp | 110 +++++++++--------- .../gserver/activations/ActivationFunction.h | 6 +- paddle/gserver/layers/Layer.cpp | 2 +- paddle/utils/{Status.h => Error.h} | 92 ++++++++------- paddle/utils/tests/CMakeLists.txt | 2 +- .../tests/{test_Status.cpp => test_Error.cpp} | 14 +-- 6 files changed, 114 insertions(+), 112 deletions(-) rename paddle/utils/{Status.h => Error.h} (70%) rename paddle/utils/tests/{test_Status.cpp => test_Error.cpp} (76%) diff --git a/paddle/gserver/activations/ActivationFunction.cpp b/paddle/gserver/activations/ActivationFunction.cpp index 666c2e01c8..f1f96fc67d 100644 --- a/paddle/gserver/activations/ActivationFunction.cpp +++ b/paddle/gserver/activations/ActivationFunction.cpp @@ -69,13 +69,13 @@ static ClassRegistrar gActivationRegistrar; class IdentityActivation : public ActivationFunction { public: static const std::string name; - Status __must_check forward(Argument& act) { + Error __must_check forward(Argument& act) { (void)act; - return Status(); + return Error(); } - Status __must_check backward(Argument& act) { + Error __must_check backward(Argument& act) { (void)act; - return Status(); + return Error(); } const std::string& getName() const { return name; } }; @@ -92,13 +92,13 @@ static InitFunction __reg_activation__identity([] { * \f] */ BEGIN_DEFINE_ACTIVATION(sigmoid) -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { act.value->sigmoid(*act.value); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { act.grad->sigmoidDerivative(*act.value); - return Status(); + return Error(); } END_DEFINE_ACTIVATION(sigmoid) @@ -115,12 +115,12 @@ MatrixPtr sftMaxDot_; MatrixPtr one_; public: -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { act.value->softmax(*act.value); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { MatrixPtr outputV = act.value; MatrixPtr outputG = act.grad; @@ -152,7 +152,7 @@ Status __must_check backward(Argument& act) { act.grad->softmaxDerivative(*act.value, *sftMaxSum_); } - return Status(); + return Error(); } END_DEFINE_ACTIVATION(softmax) @@ -167,9 +167,9 @@ ACTIVATION_CLASS_NAME(softmax) softmax_; Argument argument_; public: -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { if (act.value->getWidth() != 1UL) { - return Status( + return ErrorF( "Input width for each timestep of sequence softmax should be 1"); } @@ -188,12 +188,12 @@ Status __must_check forward(Argument& act) { auto starts = act.sequenceStartPositions->getVector(useGpu(act.deviceId)); act.value->sequenceSoftmax(*act.value, *starts); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { if (act.value->getWidth() != 1UL) { - return Status( + return ErrorF( "Input width for each timestep of sequence softmax should be 1"); } @@ -207,10 +207,10 @@ Status __must_check backward(Argument& act) { argument_.value->setData(act.value->getData() + offset, 1UL, size); argument_.grad->setData(act.grad->getData() + offset, 1UL, size); - Status status = softmax_.backward(argument_); + Error status = softmax_.backward(argument_); if (!status.isOK()) return status; } - return Status(); + return Error(); } END_DEFINE_ACTIVATION(sequence_softmax) @@ -225,14 +225,14 @@ END_DEFINE_ACTIVATION(sequence_softmax) * 0 otherwise. */ BEGIN_DEFINE_ACTIVATION(relu) -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { act.value->relu(*act.value); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { act.grad->reluDerivative(*act.value); - return Status(); + return Error(); } END_DEFINE_ACTIVATION(relu) @@ -250,14 +250,14 @@ END_DEFINE_ACTIVATION(relu) * TODO(yuyang18): Remove magic number 24 or make it configuable. */ BEGIN_DEFINE_ACTIVATION(brelu) -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { act.value->brelu(*act.value); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { act.grad->breluDerivative(*act.value); - return Status(); + return Error(); } END_DEFINE_ACTIVATION(brelu) @@ -268,14 +268,14 @@ END_DEFINE_ACTIVATION(brelu) * \f] */ BEGIN_DEFINE_ACTIVATION(tanh) -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { act.value->tanh(*act.value); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { act.grad->tanhDerivative(*act.value); - return Status(); + return Error(); } END_DEFINE_ACTIVATION(tanh) @@ -291,14 +291,14 @@ real a, b; public: ACTIVATION_CLASS_NAME(stanh)() : a(1.7159), b(2. / 3.) {} -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { act.value->scaledTanh(*act.value, a, b); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { act.grad->scaledTanhDerivative(*act.value, a, b); - return Status(); + return Error(); } END_DEFINE_ACTIVATION(stanh) @@ -309,14 +309,14 @@ END_DEFINE_ACTIVATION(stanh) * \f] */ BEGIN_DEFINE_ACTIVATION(softrelu) -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { act.value->softrelu(*act.value); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { act.grad->softreluDerivative(*act.value); - return Status(); + return Error(); } END_DEFINE_ACTIVATION(softrelu) @@ -333,7 +333,7 @@ END_DEFINE_ACTIVATION(softrelu) * 0 if z=0 */ BEGIN_DEFINE_ACTIVATION(abs) -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { SetDevice device(act.deviceId); Matrix::resizeOrCreate(act.in, act.value->getHeight(), @@ -343,12 +343,12 @@ Status __must_check forward(Argument& act) { act.in->copyFrom(*act.value); act.value->abs2(*act.value); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { act.grad->absDerivative(*act.in); - return Status(); + return Error(); } END_DEFINE_ACTIVATION(abs) @@ -359,7 +359,7 @@ END_DEFINE_ACTIVATION(abs) * \f] */ BEGIN_DEFINE_ACTIVATION(square) -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { SetDevice device(act.deviceId); Matrix::resizeOrCreate(act.in, act.value->getHeight(), @@ -369,12 +369,12 @@ Status __must_check forward(Argument& act) { act.in->copyFrom(*act.value); act.value->square2(*act.value); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { act.grad->squareDerivative(*act.in); - return Status(); + return Error(); } END_DEFINE_ACTIVATION(square) @@ -385,14 +385,14 @@ END_DEFINE_ACTIVATION(square) * \f] */ BEGIN_DEFINE_ACTIVATION(exponential) -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { act.value->exp2(*act.value); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { act.grad->expDerivative(*act.value); - return Status(); + return Error(); } END_DEFINE_ACTIVATION(exponential) @@ -403,7 +403,7 @@ END_DEFINE_ACTIVATION(exponential) * \f] */ BEGIN_DEFINE_ACTIVATION(log) -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { SetDevice device(act.deviceId); Matrix::resizeOrCreate(act.in, act.value->getHeight(), @@ -413,12 +413,12 @@ Status __must_check forward(Argument& act) { act.in->copyFrom(*act.value); act.value->log2(*act.value); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { act.grad->dotDiv(*act.grad, *act.in); - return Status(); + return Error(); } END_DEFINE_ACTIVATION(log) diff --git a/paddle/gserver/activations/ActivationFunction.h b/paddle/gserver/activations/ActivationFunction.h index 737df2219d..f208224e30 100644 --- a/paddle/gserver/activations/ActivationFunction.h +++ b/paddle/gserver/activations/ActivationFunction.h @@ -15,7 +15,7 @@ limitations under the License. */ #pragma once #include #include -#include "paddle/utils/Status.h" +#include "paddle/utils/Error.h" namespace paddle { @@ -49,7 +49,7 @@ public: * * Usually, act is Layer::output_ */ - virtual Status __must_check forward(Argument& act) = 0; + virtual Error __must_check forward(Argument& act) = 0; /** * @brief Backward propagaion @@ -58,7 +58,7 @@ public: * - Before calling backward(), act.grad = dE / dy, where E is the error/cost * - After backward() returns, act.grad = dE / dx = (dE/dy) * (dy/dx) */ - virtual Status __must_check backward(Argument& act) = 0; + virtual Error __must_check backward(Argument& act) = 0; virtual const std::string& getName() const = 0; }; diff --git a/paddle/gserver/layers/Layer.cpp b/paddle/gserver/layers/Layer.cpp index f96070fe6e..f76d41ad3e 100644 --- a/paddle/gserver/layers/Layer.cpp +++ b/paddle/gserver/layers/Layer.cpp @@ -15,8 +15,8 @@ limitations under the License. */ #include "paddle/utils/Util.h" #include "paddle/math/SparseMatrix.h" +#include "paddle/utils/Error.h" #include "paddle/utils/Logging.h" -#include "paddle/utils/Status.h" #include "AddtoLayer.h" #include "CRFLayer.h" diff --git a/paddle/utils/Status.h b/paddle/utils/Error.h similarity index 70% rename from paddle/utils/Status.h rename to paddle/utils/Error.h index 26329f8d19..f1597f93d2 100644 --- a/paddle/utils/Status.h +++ b/paddle/utils/Error.h @@ -34,9 +34,9 @@ namespace paddle { * When method return a status, the return must use `__must_check` attribute. * Example as below. * @code{cpp} - * Status __must_check foo(); + * Error __must_check foo(); * - * Status __must_check bar() { + * Error __must_check bar() { * // do something. * Status s = foo(); // invoke other method return status. * if (!s.isOK()) return s; @@ -50,9 +50,9 @@ namespace paddle { * Example as below. * * @code{cpp} - * Status bar(); + * Error bar(); * - * int foo(Status* status) { + * int foo(Error* status) { * // Do something. * Status s = bar(); * if (!s.isOK()) { @@ -61,15 +61,15 @@ namespace paddle { * } * // Do something else. * if (someInternalErrorHappend) { - * status->setByPrintf("Some dimension is too large, %d", dimension); + * *status = ErrorF("Some dimension is too large, %d", dimension); * return 0; * } * // End of method. * return someValue; * } * - * Status foobar() { - * Status s; + * Error foobar() { + * Error s; * // do something. * foo(&s); * if (!s.isOK()) return s; @@ -81,48 +81,12 @@ namespace paddle { * 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 Status final : public std::exception { +class Error final : public std::exception { public: /** * Default Status. OK */ - Status() noexcept {} - - /** - * @brief Create Status with error message - * @param msg - */ - explicit Status(const std::string& msg) : errMsg_(new std::string(msg)) {} - - /** - * @brief set a error message for status. - * @param msg - */ - inline void set(const std::string& msg) noexcept { - errMsg_.reset(new std::string(msg)); - } - - /** - * @brief set a error message for status. Use C style printf - * @param fmt - */ - template - inline void setByPrintf(const char* fmt, ARGS... args) noexcept { - constexpr size_t kBufferSize = 1024; // 1KB buffer - char buffer[kBufferSize]; - snprintf(buffer, kBufferSize, fmt, args...); - errMsg_.reset(new std::string(buffer)); - } - - /** - * create a error status by C style printf. - */ - template - inline static Status printf(const char* fmt, ARGS... args) noexcept { - Status s; - s.setByPrintf(fmt, args...); - return s; - } + Error() noexcept {} /** * @brief what will return the error message. If status is OK, return nullptr. @@ -148,8 +112,46 @@ public: */ inline void check() const { CHECK(isOK()) << what(); } + /** + * friend method to create Error. + */ + template + friend Error __must_check ErrorF(const char* fmt, ARGS... args); + private: std::shared_ptr errMsg_; }; +/** + * ErrorF will create an Error by printf syntax. + * + * Specialize this method because clang will give a warning when use printf(fmt) + * without arguments. + */ +template <> +inline Error __must_check ErrorF(const char* msg) { + Error e; + e.errMsg_.reset(new std::string(msg)); + return e; +} + +/** + * ErrorF will create an Error by printf syntax. + * + * Examples: + * @code{cpp} + * auto err = ErrorF("SomeError"); + * auto err2 = ErrorF("SomeErrorWithParameter %f %d", real_val, int_val); + * @endcode{cpp} + */ +template +inline Error __must_check ErrorF(const char* fmt, ARGS... args) { + constexpr size_t kBufferSize = 1024; + char buffer[kBufferSize]; + snprintf(buffer, kBufferSize, fmt, args...); + Error e; + e.errMsg_.reset(new std::string(buffer)); + return e; +} + } // namespace paddle diff --git a/paddle/utils/tests/CMakeLists.txt b/paddle/utils/tests/CMakeLists.txt index a1cc32668d..aa923b3553 100644 --- a/paddle/utils/tests/CMakeLists.txt +++ b/paddle/utils/tests/CMakeLists.txt @@ -4,7 +4,7 @@ add_simple_unittest(test_CustomStackTrace) add_simple_unittest(test_ThreadBarrier) add_simple_unittest(test_SpinLock) add_simple_unittest(test_SIMDFlags) -add_simple_unittest(test_Status) +add_simple_unittest(test_Error) add_executable( test_CustomStackTracePrint diff --git a/paddle/utils/tests/test_Status.cpp b/paddle/utils/tests/test_Error.cpp similarity index 76% rename from paddle/utils/tests/test_Status.cpp rename to paddle/utils/tests/test_Error.cpp index 04cef09579..96115f7053 100644 --- a/paddle/utils/tests/test_Status.cpp +++ b/paddle/utils/tests/test_Error.cpp @@ -12,23 +12,23 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. */ -#include "paddle/utils/Status.h" +#include "paddle/utils/Error.h" #include TEST(Status, testAll) { - paddle::Status status; + paddle::Error status; ASSERT_TRUE(status.isOK()); - status.set("I'm the error"); + status = paddle::ErrorF("I'm the error"); ASSERT_FALSE(status.isOK()); ASSERT_STREQ("I'm the error", status.what()); - paddle::Status status2("error2"); - ASSERT_FALSE(status2.isOK()); - ASSERT_STREQ("error2", status2.what()); + status = paddle::ErrorF("error2"); + ASSERT_FALSE(status.isOK()); + ASSERT_STREQ("error2", status.what()); int i = 3; - auto status3 = paddle::Status::printf("error%d", i); + auto status3 = paddle::ErrorF("error%d", i); ASSERT_FALSE(status3.isOK()); ASSERT_STREQ("error3", status3.what()); } From 699d18f11701aae1efe72c8bf6edc50723445050 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Wed, 18 Jan 2017 13:34:20 +0800 Subject: [PATCH 09/22] Change unittest variable name --- paddle/utils/tests/test_Error.cpp | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/paddle/utils/tests/test_Error.cpp b/paddle/utils/tests/test_Error.cpp index 96115f7053..e8643de9d2 100644 --- a/paddle/utils/tests/test_Error.cpp +++ b/paddle/utils/tests/test_Error.cpp @@ -16,19 +16,19 @@ limitations under the License. */ #include -TEST(Status, testAll) { - paddle::Error status; - ASSERT_TRUE(status.isOK()); - status = paddle::ErrorF("I'm the error"); - ASSERT_FALSE(status.isOK()); - ASSERT_STREQ("I'm the error", status.what()); +TEST(Error, testAll) { + paddle::Error error; + ASSERT_TRUE(error.isOK()); + error = paddle::ErrorF("I'm the error"); + ASSERT_FALSE(error.isOK()); + ASSERT_STREQ("I'm the error", error.what()); - status = paddle::ErrorF("error2"); - ASSERT_FALSE(status.isOK()); - ASSERT_STREQ("error2", status.what()); + error = paddle::ErrorF("error2"); + ASSERT_FALSE(error.isOK()); + ASSERT_STREQ("error2", error.what()); int i = 3; - auto status3 = paddle::ErrorF("error%d", i); - ASSERT_FALSE(status3.isOK()); - ASSERT_STREQ("error3", status3.what()); + auto error3 = paddle::ErrorF("error%d", i); + ASSERT_FALSE(error3.isOK()); + ASSERT_STREQ("error3", error3.what()); } From 5a15c70e167a83e6b06686e3152ca9b30ed7800e Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Wed, 18 Jan 2017 20:27:52 +0800 Subject: [PATCH 10/22] Make Error interface cleaner --- .../activations/ActivationFunction.cpp | 6 +- paddle/utils/Error.h | 92 +++++++------------ paddle/utils/tests/test_Error.cpp | 20 ++-- 3 files changed, 45 insertions(+), 73 deletions(-) diff --git a/paddle/gserver/activations/ActivationFunction.cpp b/paddle/gserver/activations/ActivationFunction.cpp index f1f96fc67d..c541b72e10 100644 --- a/paddle/gserver/activations/ActivationFunction.cpp +++ b/paddle/gserver/activations/ActivationFunction.cpp @@ -169,7 +169,7 @@ Argument argument_; public: Error __must_check forward(Argument& act) { if (act.value->getWidth() != 1UL) { - return ErrorF( + return Error( "Input width for each timestep of sequence softmax should be 1"); } @@ -193,7 +193,7 @@ Error __must_check forward(Argument& act) { Error __must_check backward(Argument& act) { if (act.value->getWidth() != 1UL) { - return ErrorF( + return Error( "Input width for each timestep of sequence softmax should be 1"); } @@ -208,7 +208,7 @@ Error __must_check backward(Argument& act) { argument_.grad->setData(act.grad->getData() + offset, 1UL, size); Error status = softmax_.backward(argument_); - if (!status.isOK()) return status; + if (!status) return status; } return Error(); } diff --git a/paddle/utils/Error.h b/paddle/utils/Error.h index f1597f93d2..a8de56b980 100644 --- a/paddle/utils/Error.h +++ b/paddle/utils/Error.h @@ -23,14 +23,12 @@ limitations under the License. */ namespace paddle { /** - * Status is Paddle error code. It only contain a std::string as error message. - * Although Status inherits the std::exception, but do not throw it except you - * know what you are doing. + * Error is Paddle error code. It only contain a std::string as error message. * * - * There are two styles to return status in Paddle. + * There are two styles to return error in Paddle. * - * 1. Return Status + * 1. Return Error * When method return a status, the return must use `__must_check` attribute. * Example as below. * @code{cpp} @@ -39,29 +37,29 @@ namespace paddle { * Error __must_check bar() { * // do something. * Status s = foo(); // invoke other method return status. - * if (!s.isOK()) return s; + * if (!s) return s; * // do something else. * return Status(); * } * @endcode{cpp} * * 2. Return by parameter. - * It is another way to return a status, by using a pointer parameter. + * It is another way to return an error, by using a pointer parameter. * Example as below. * * @code{cpp} * Error bar(); * - * int foo(Error* status) { + * int foo(Error* error) { * // Do something. - * Status s = bar(); - * if (!s.isOK()) { - * *status = s; + * Error s = bar(); + * if (!s) { + * *error = s; * return 0; * } * // Do something else. * if (someInternalErrorHappend) { - * *status = ErrorF("Some dimension is too large, %d", dimension); + * *error = Error("Some dimension is too large, %d", dimension); * return 0; * } * // End of method. @@ -72,7 +70,7 @@ namespace paddle { * Error s; * // do something. * foo(&s); - * if (!s.isOK()) return s; + * if (!s) return s; * } * @endcode{cpp} * @@ -81,17 +79,31 @@ namespace paddle { * 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 : public std::exception { +class Error final { public: /** * Default Status. OK */ - Error() noexcept {} + inline Error() {} /** - * @brief what will return the error message. If status is OK, return nullptr. + * @brief Create an Error use printf syntax. */ - const char* what() const noexcept override { + inline explicit Error(const char* fmt, ...) { + va_list ap; + 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); + this->errMsg_->resize(sz); + this->errMsg_->shrink_to_fit(); + va_end(ap); + } + + /** + * @brief what will return the error message. If no error, return nullptr. + */ + inline const char* msg() const { if (errMsg_) { return errMsg_->data(); } else { @@ -100,58 +112,18 @@ public: } /** - * @brief isOK - * @return true if OK. + * @brief operator bool, return True if there is no error. */ - inline bool isOK() const noexcept { return errMsg_ == nullptr; } - + inline operator bool() const { return !errMsg_; } /** * @brief check this status by glog. * @note It is a temp method used during cleaning Paddle code. It will be * removed later. */ - inline void check() const { CHECK(isOK()) << what(); } - - /** - * friend method to create Error. - */ - template - friend Error __must_check ErrorF(const char* fmt, ARGS... args); + inline void check() const { CHECK(*this) << msg(); } private: std::shared_ptr errMsg_; }; -/** - * ErrorF will create an Error by printf syntax. - * - * Specialize this method because clang will give a warning when use printf(fmt) - * without arguments. - */ -template <> -inline Error __must_check ErrorF(const char* msg) { - Error e; - e.errMsg_.reset(new std::string(msg)); - return e; -} - -/** - * ErrorF will create an Error by printf syntax. - * - * Examples: - * @code{cpp} - * auto err = ErrorF("SomeError"); - * auto err2 = ErrorF("SomeErrorWithParameter %f %d", real_val, int_val); - * @endcode{cpp} - */ -template -inline Error __must_check ErrorF(const char* fmt, ARGS... args) { - constexpr size_t kBufferSize = 1024; - char buffer[kBufferSize]; - snprintf(buffer, kBufferSize, fmt, args...); - Error e; - e.errMsg_.reset(new std::string(buffer)); - return e; -} - } // namespace paddle diff --git a/paddle/utils/tests/test_Error.cpp b/paddle/utils/tests/test_Error.cpp index e8643de9d2..85156466e2 100644 --- a/paddle/utils/tests/test_Error.cpp +++ b/paddle/utils/tests/test_Error.cpp @@ -18,17 +18,17 @@ limitations under the License. */ TEST(Error, testAll) { paddle::Error error; - ASSERT_TRUE(error.isOK()); - error = paddle::ErrorF("I'm the error"); - ASSERT_FALSE(error.isOK()); - ASSERT_STREQ("I'm the error", error.what()); + ASSERT_TRUE(error); + error = paddle::Error("I'm the error"); + ASSERT_FALSE(error); + ASSERT_STREQ("I'm the error", error.msg()); - error = paddle::ErrorF("error2"); - ASSERT_FALSE(error.isOK()); - ASSERT_STREQ("error2", error.what()); + error = paddle::Error("error2"); + ASSERT_FALSE(error); + ASSERT_STREQ("error2", error.msg()); int i = 3; - auto error3 = paddle::ErrorF("error%d", i); - ASSERT_FALSE(error3.isOK()); - ASSERT_STREQ("error3", error3.what()); + auto error3 = paddle::Error("error%d", i); + ASSERT_FALSE(error3); + ASSERT_STREQ("error3", error3.msg()); } From a6d4a31deae38f78e24ecaf198e9250927416041 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Thu, 19 Jan 2017 10:01:47 +0800 Subject: [PATCH 11/22] Follow comments --- paddle/utils/Error.h | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/paddle/utils/Error.h b/paddle/utils/Error.h index a8de56b980..ff11541bbd 100644 --- a/paddle/utils/Error.h +++ b/paddle/utils/Error.h @@ -79,33 +79,32 @@ namespace paddle { * 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 { +class Error { public: /** - * Default Status. OK + * Construct an no-error value. */ - inline Error() {} + Error() {} /** * @brief Create an Error use printf syntax. */ - inline explicit Error(const char* fmt, ...) { + explicit Error(const char* fmt, ...) { va_list ap; 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); - this->errMsg_->resize(sz); - this->errMsg_->shrink_to_fit(); + char buffer[kBufferSize]; + vsnprintf(buffer, kBufferSize, fmt, ap); + this->msg_.reset(new std::string(buffer)); va_end(ap); } /** * @brief what will return the error message. If no error, return nullptr. */ - inline const char* msg() const { - if (errMsg_) { - return errMsg_->data(); + const char* msg() const { + if (msg_) { + return msg_->c_str(); } else { return nullptr; } @@ -114,16 +113,16 @@ public: /** * @brief operator bool, return True if there is no error. */ - inline operator bool() const { return !errMsg_; } + operator bool() const { return !msg_; } /** * @brief check this status by glog. * @note It is a temp method used during cleaning Paddle code. It will be * removed later. */ - inline void check() const { CHECK(*this) << msg(); } + void check() const { CHECK(*this) << msg(); } private: - std::shared_ptr errMsg_; + std::shared_ptr msg_; }; } // namespace paddle From c88dec209f367cb3ac1bd3fe6964e63f7274d975 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Thu, 19 Jan 2017 10:08:33 +0800 Subject: [PATCH 12/22] Fix typo --- paddle/utils/Error.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/paddle/utils/Error.h b/paddle/utils/Error.h index ff11541bbd..6fe7b6ea88 100644 --- a/paddle/utils/Error.h +++ b/paddle/utils/Error.h @@ -82,7 +82,7 @@ namespace paddle { class Error { public: /** - * Construct an no-error value. + * Construct a no-error value. */ Error() {} @@ -100,7 +100,7 @@ public: } /** - * @brief what will return the error message. If no error, return nullptr. + * @brief msg will return the error message. If no error, return nullptr. */ const char* msg() const { if (msg_) { @@ -114,6 +114,7 @@ public: * @brief operator bool, return True if there is no error. */ operator bool() const { return !msg_; } + /** * @brief check this status by glog. * @note It is a temp method used during cleaning Paddle code. It will be From 42ea1376e25f7ed02ea55e11cb6114983c0d1e4c Mon Sep 17 00:00:00 2001 From: dangqingqing Date: Thu, 19 Jan 2017 10:11:06 +0800 Subject: [PATCH 13/22] bug fix in GatedRecurrentLayer which only occurs in predicting or job=test mode. --- paddle/gserver/layers/GatedRecurrentLayer.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/paddle/gserver/layers/GatedRecurrentLayer.cpp b/paddle/gserver/layers/GatedRecurrentLayer.cpp index 930d9a0561..d3aeea9218 100644 --- a/paddle/gserver/layers/GatedRecurrentLayer.cpp +++ b/paddle/gserver/layers/GatedRecurrentLayer.cpp @@ -314,13 +314,13 @@ void GatedRecurrentLayer::forwardBatch(int batchSize, batchValue_->resizeOrCreate(*output_.value); batchValue_->copy(*inputValue, *gate_.value, /* seq2batch */ true); - if (bias_ && bias_->getWGrad()) { + if (bias_) { gate_.value->addBias(*(bias_->getW()), 1); } { int numBatch = batchValue_->getNumBatch(); - int batchSize = 0; + int curBatchSize = 0; AsyncGpuBlock asyncGpuBlock; for (int n = 0; n < numBatch; n++) { MatrixPtr outputValueTmp = batchValue_->getBatchValue(n); @@ -330,16 +330,17 @@ void GatedRecurrentLayer::forwardBatch(int batchSize, gruValue.resetOutputValue = (batchValue_->getBatchValue(*resetOutput_.value, n))->getData(); - batchSize = outputValueTmp->getHeight(); + curBatchSize = outputValueTmp->getHeight(); gruValue.prevOutValue = - (n == 0 ? nullptr - : (batchValue_->getBatchValue(n - 1, batchSize))->getData()); + (n == 0 + ? nullptr + : (batchValue_->getBatchValue(n - 1, curBatchSize))->getData()); { if (useGpu_) { - GruCompute::forward<1>(gruValue, getSize(), batchSize); + GruCompute::forward<1>(gruValue, getSize(), curBatchSize); } else { - GruCompute::forward<0>(gruValue, getSize(), batchSize); + GruCompute::forward<0>(gruValue, getSize(), curBatchSize); } } } From 843fb2ea32d4f0b2d1f3667545487c8084229819 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Thu, 19 Jan 2017 15:12:42 +0800 Subject: [PATCH 14/22] Make code more readable --- paddle/utils/Error.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/paddle/utils/Error.h b/paddle/utils/Error.h index 6fe7b6ea88..2b4fbef4e0 100644 --- a/paddle/utils/Error.h +++ b/paddle/utils/Error.h @@ -15,6 +15,7 @@ limitations under the License. */ #pragma once #include +#include #include #include #include @@ -113,7 +114,7 @@ public: /** * @brief operator bool, return True if there is no error. */ - operator bool() const { return !msg_; } + operator bool() const { return msg_ == nullptr; } /** * @brief check this status by glog. From 7ff8c8eec3f44497816ae4fe66516f3ca3ba7cb7 Mon Sep 17 00:00:00 2001 From: xuwei06 Date: Thu, 19 Jan 2017 01:33:28 -0800 Subject: [PATCH 15/22] Compile glog with WITH_GFLAGS=ON Also initialize glog after gflags in Util.cpp initMain() Change-Id: I09ff062b462aa76d9f7b5f97e883b92939e6dcb2 --- cmake/external/glog.cmake | 9 +++++---- paddle/utils/Util.cpp | 4 ++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/cmake/external/glog.cmake b/cmake/external/glog.cmake index 71e20c8527..e1eade601e 100644 --- a/cmake/external/glog.cmake +++ b/cmake/external/glog.cmake @@ -1,11 +1,11 @@ # Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserve. -# +# # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. # You may obtain a copy of the License at -# +# # http://www.apache.org/licenses/LICENSE-2.0 -# +# # Unless required by applicable law or agreed to in writing, software # distributed under the License is distributed on an "AS IS" BASIS, # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -29,12 +29,13 @@ INCLUDE_DIRECTORIES(${GLOG_INCLUDE_DIR}) ExternalProject_Add( glog ${EXTERNAL_PROJECT_LOG_ARGS} + DEPENDS gflags GIT_REPOSITORY "https://github.com/google/glog.git" PREFIX ${GLOG_SOURCES_DIR} UPDATE_COMMAND "" CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=${GLOG_INSTALL_DIR} CMAKE_ARGS -DCMAKE_POSITION_INDEPENDENT_CODE=ON - CMAKE_ARGS -DWITH_GFLAGS=OFF + CMAKE_ARGS -DWITH_GFLAGS=ON CMAKE_ARGS -DBUILD_TESTING=OFF ) diff --git a/paddle/utils/Util.cpp b/paddle/utils/Util.cpp index 411a64aa8d..220aac1ff1 100644 --- a/paddle/utils/Util.cpp +++ b/paddle/utils/Util.cpp @@ -144,20 +144,20 @@ void runInitFunctions() { } void initMain(int argc, char** argv) { - initializeLogging(argc, argv); installLayerStackTracer(); std::string line; for (int i = 0; i < argc; ++i) { line += argv[i]; line += ' '; } - LOG(INFO) << "commandline: " << line; #ifndef GFLAGS_GFLAGS_H_ namespace gflags = google; #endif gflags::ParseCommandLineFlags(&argc, &argv, true); + initializeLogging(argc, argv); + LOG(INFO) << "commandline: " << line; CHECK_EQ(argc, 1) << "Unknown commandline argument: " << argv[1]; installProfilerSwitch(); From 63118767bf0ede148a7404604ea966d6f0bf35c7 Mon Sep 17 00:00:00 2001 From: liaogang Date: Thu, 19 Jan 2017 17:36:00 +0800 Subject: [PATCH 16/22] Add detect redhat and unknown in CMake --- cmake/system.cmake | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/cmake/system.cmake b/cmake/system.cmake index ab124a89dc..485cf0eea1 100644 --- a/cmake/system.cmake +++ b/cmake/system.cmake @@ -30,6 +30,10 @@ ELSE(WIN32) SET(HOST_SYSTEM "debian") ELSEIF(LINUX_ISSUE MATCHES "Ubuntu") SET(HOST_SYSTEM "ubuntu") + ELSEIF(LINUX_ISSUE MATCHES "Red Hat") + SET(HOST_SYSTEM "redhat") + ELSEIF(LINUX_ISSUE MATCHES "Fedora") + SET(HOST_SYSTEM "fedora") ENDIF() ENDIF(EXISTS "/etc/issue") @@ -40,6 +44,10 @@ ELSE(WIN32) ENDIF() ENDIF(EXISTS "/etc/redhat-release") + IF(NOT HOST_SYSTEM) + SET(HOST_SYSTEM "unknown") + ENDIF() + ENDIF(APPLE) ENDIF(WIN32) From febf9a0b070a7c5036f2b32f12167952f1597727 Mon Sep 17 00:00:00 2001 From: liaogang Date: Fri, 20 Jan 2017 00:11:08 +0800 Subject: [PATCH 17/22] Add comments and CMAKE_SYSTEM_NAME --- cmake/system.cmake | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/cmake/system.cmake b/cmake/system.cmake index 485cf0eea1..3e472da7e0 100644 --- a/cmake/system.cmake +++ b/cmake/system.cmake @@ -12,6 +12,14 @@ # See the License for the specific language governing permissions and # limitations under the License. +# Detects the OS and sets appropriate variables. +# CMAKE_SYSTEM_NAME only give us a coarse-grained name, +# but the name like centos is necessary in some scenes +# to distinguish system for customization. +# +# for instance, protobuf libs path is /lib64 +# on CentOS, but /lib on other systems. + IF(WIN32) SET(HOST_SYSTEM "win32") ELSE(WIN32) @@ -45,7 +53,7 @@ ELSE(WIN32) ENDIF(EXISTS "/etc/redhat-release") IF(NOT HOST_SYSTEM) - SET(HOST_SYSTEM "unknown") + SET(HOST_SYSTEM ${CMAKE_SYSTEM_NAME}) ENDIF() ENDIF(APPLE) From 95dec805aab0ddaef551b367f0c2f4e42a393819 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Fri, 20 Jan 2017 18:48:42 +0800 Subject: [PATCH 18/22] Make external/glog use local gflags. --- cmake/external/glog.cmake | 1 + 1 file changed, 1 insertion(+) diff --git a/cmake/external/glog.cmake b/cmake/external/glog.cmake index e1eade601e..ab105611c8 100644 --- a/cmake/external/glog.cmake +++ b/cmake/external/glog.cmake @@ -36,6 +36,7 @@ ExternalProject_Add( CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=${GLOG_INSTALL_DIR} CMAKE_ARGS -DCMAKE_POSITION_INDEPENDENT_CODE=ON CMAKE_ARGS -DWITH_GFLAGS=ON + CMAKE_ARGS -Dgflags_DIR=${GFLAGS_INSTALL_DIR}/lib/cmake/gflags CMAKE_ARGS -DBUILD_TESTING=OFF ) From a8583f0ccb230fb0564103a28a034a87156bac78 Mon Sep 17 00:00:00 2001 From: reyoung Date: Fri, 20 Jan 2017 11:39:52 +0000 Subject: [PATCH 19/22] Correct Handle Python Proto2 --- cmake/external/python.cmake | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cmake/external/python.cmake b/cmake/external/python.cmake index 209e679f2c..6372a9a768 100644 --- a/cmake/external/python.cmake +++ b/cmake/external/python.cmake @@ -26,10 +26,10 @@ IF(PYTHONLIBS_FOUND AND PYTHONINTERP_FOUND) find_python_module(wheel REQUIRED) find_python_module(google.protobuf REQUIRED) FIND_PACKAGE(NumPy REQUIRED) - IF(${PY_GOOGLE.PROTOBUF_VERSION} VERSION_LESS "3.0.0") + IF(${PY_GOOGLE.PROTOBUF_VERSION} AND ${PY_GOOGLE.PROTOBUF_VERSION} VERSION_LESS "3.0.0") MESSAGE(FATAL_ERROR "Found Python Protobuf ${PY_GOOGLE.PROTOBUF_VERSION} < 3.0.0, " - "please use pip to upgrade protobuf.") - ENDIF(${PY_GOOGLE.PROTOBUF_VERSION} VERSION_LESS "3.0.0") + "please use pip to upgrade protobuf. pip install -U protobuf") + ENDIF() ELSE(PYTHONLIBS_FOUND AND PYTHONINTERP_FOUND) MESSAGE(FATAL_ERROR "Please install python 2.7 before building PaddlePaddle.") ##################################### PYTHON ######################################## From 51fa6baebec5c29f4ff0f1c2e72f6feab02bbe5e Mon Sep 17 00:00:00 2001 From: FoREacH Date: Sat, 21 Jan 2017 00:24:38 +0200 Subject: [PATCH 20/22] Fix issue #1186 --- cmake/external/warpctc.cmake | 1 + 1 file changed, 1 insertion(+) diff --git a/cmake/external/warpctc.cmake b/cmake/external/warpctc.cmake index f5e4b3e1eb..f23a3969e9 100644 --- a/cmake/external/warpctc.cmake +++ b/cmake/external/warpctc.cmake @@ -54,6 +54,7 @@ ExternalProject_Add( CMAKE_ARGS -DWITH_GPU=${WITH_GPU} CMAKE_ARGS -DWITH_OMP=${USE_OMP} CMAKE_ARGS -DWITH_TORCH=OFF + CMAKE_ARGS -DCMAKE_DISABLE_FIND_PACKAGE_TORCH=TRUE CMAKE_ARGS -DBUILD_SHARED=ON ) From 7709b697ba85a99c9f784e5f2f8267f7a52c61fc Mon Sep 17 00:00:00 2001 From: F0REacH Date: Sat, 21 Jan 2017 00:52:39 +0200 Subject: [PATCH 21/22] Update warpctc.cmake Fix case-sensitiveness --- cmake/external/warpctc.cmake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmake/external/warpctc.cmake b/cmake/external/warpctc.cmake index f23a3969e9..172c318b35 100644 --- a/cmake/external/warpctc.cmake +++ b/cmake/external/warpctc.cmake @@ -54,7 +54,7 @@ ExternalProject_Add( CMAKE_ARGS -DWITH_GPU=${WITH_GPU} CMAKE_ARGS -DWITH_OMP=${USE_OMP} CMAKE_ARGS -DWITH_TORCH=OFF - CMAKE_ARGS -DCMAKE_DISABLE_FIND_PACKAGE_TORCH=TRUE + CMAKE_ARGS -DCMAKE_DISABLE_FIND_PACKAGE_Torch=TRUE CMAKE_ARGS -DBUILD_SHARED=ON ) From ed5624023549374259c0fc9c4849d5ae46347b1b Mon Sep 17 00:00:00 2001 From: FoREacH Date: Sat, 21 Jan 2017 05:46:07 +0200 Subject: [PATCH 22/22] Set protobuf CMAKE_INSTALL_LIBDIR to fixed value lib --- cmake/external/protobuf.cmake | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/cmake/external/protobuf.cmake b/cmake/external/protobuf.cmake index 613614c0e3..84f459033f 100644 --- a/cmake/external/protobuf.cmake +++ b/cmake/external/protobuf.cmake @@ -29,17 +29,12 @@ IF(WIN32) "${PROTOBUF_INSTALL_DIR}/lib/libprotoc.lib" CACHE FILEPATH "protoc library." FORCE) SET(PROTOBUF_PROTOC_EXECUTABLE "${PROTOBUF_INSTALL_DIR}/bin/protoc.exe" CACHE FILEPATH "protobuf executable." FORCE) ELSE(WIN32) - IF(${HOST_SYSTEM} STREQUAL "centos") - SET(LIB "lib64") - ELSE() - SET(LIB "lib") - ENDIF() SET(PROTOBUF_LITE_LIBRARY - "${PROTOBUF_INSTALL_DIR}/${LIB}/libprotobuf-lite.a" CACHE FILEPATH "protobuf lite library." FORCE) + "${PROTOBUF_INSTALL_DIR}/lib/libprotobuf-lite.a" CACHE FILEPATH "protobuf lite library." FORCE) SET(PROTOBUF_LIBRARY - "${PROTOBUF_INSTALL_DIR}/${LIB}/libprotobuf.a" CACHE FILEPATH "protobuf library." FORCE) + "${PROTOBUF_INSTALL_DIR}/lib/libprotobuf.a" CACHE FILEPATH "protobuf library." FORCE) SET(PROTOBUF_PROTOC_LIBRARY - "${PROTOBUF_INSTALL_DIR}/${LIB}/libprotoc.a" CACHE FILEPATH "protoc library." FORCE) + "${PROTOBUF_INSTALL_DIR}/lib/libprotoc.a" CACHE FILEPATH "protoc library." FORCE) SET(PROTOBUF_PROTOC_EXECUTABLE "${PROTOBUF_INSTALL_DIR}/bin/protoc" CACHE FILEPATH "protobuf executable." FORCE) ENDIF(WIN32) @@ -58,6 +53,7 @@ ExternalProject_Add( -DCMAKE_POSITION_INDEPENDENT_CODE=ON -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=${PROTOBUF_INSTALL_DIR} + -DCMAKE_INSTALL_LIBDIR=lib ) LIST(APPEND external_project_dependencies protobuf)