Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try to use Error refactor LOG(FATAL)/CHECK #1067

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions paddle/trainer/TrainerConfigHelper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ struct TrainerConfigHelperPrivate {
TrainerConfig conf;
};

TrainerConfigHelper::TrainerConfigHelper(const std::string &configFilePath)
TrainerConfigHelper::TrainerConfigHelper(
const std::string &configFilePath) throw(ErrorPtr &)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不过似乎构造函数里的Error,就只能用Exception返回了。。我再想想其他办法。

: m(new TrainerConfigHelperPrivate()) {
std::ostringstream configArgs;
configArgs << "trainer_id=" << FLAGS_trainer_id << ",local=" << FLAGS_local
Expand All @@ -50,11 +51,19 @@ TrainerConfigHelper::TrainerConfigHelper(const std::string &configFilePath)
}

VLOG(3) << "Parsing trainer config " << configFilePath;
std::string errStr;
std::string configProtoStr =
callPythonFunc(kConfigParserModuleName,
kConfigParserFuncName,
{configFilePath, configArgs.str()});
CHECK(m->conf.ParseFromString(configProtoStr));
{configFilePath, configArgs.str()},
&errStr);
if (!errStr.empty()) {
Error::throwError(errStr);
}

if (!m->conf.ParseFromString(configProtoStr)) {
Error::throwError("cannot parse configuration proto string");
}
}

TrainerConfigHelper::TrainerConfigHelper(const TrainerConfig &config)
Expand Down
9 changes: 4 additions & 5 deletions paddle/trainer/TrainerConfigHelper.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ limitations under the License. */

#pragma once

#include <paddle/utils/Error.h>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

按照 https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes,这里应该写 `#include "paddle/utils/Error.h" 。只有system header files 才用尖括号。

#include <paddle/utils/Logging.h>
#include <paddle/utils/Util.h>
#include <memory>
Expand All @@ -32,19 +33,17 @@ class DataConfig;
*
* The all operation to TrainerConfig object should use this object. It remove
* many copy & paste code in trainer.
*
* @TODO(yuyang18): Make cmake check compiler support keyword 'final' or not.
* Define a macro to unify 'final' keyword
*/
class TrainerConfigHelper /*final*/ {
class TrainerConfigHelper final {
public:
DISABLE_COPY(TrainerConfigHelper);

/**
* @brief Ctor, Create a TrainerConfig from config file
* @param configFilePath Config file path.
*/
explicit TrainerConfigHelper(const std::string& configFilePath);
explicit TrainerConfigHelper(const std::string& configFilePath) throw(
ErrorPtr&);
explicit TrainerConfigHelper(const TrainerConfig& config);

/**
Expand Down
18 changes: 18 additions & 0 deletions paddle/utils/Error.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserved.
Licensed under the Apache License, Version 2.0 (the "License");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

缺少空行。下面有些地方也缺少了空行。

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 "Error.h"

namespace paddle {
void paddle::Error::throwError(const std::string &what) throw(ErrorPtr &) {
throw std::unique_ptr<std::exception>(new Error(what));
}
} // namespace paddle
28 changes: 28 additions & 0 deletions paddle/utils/Error.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserved.
Licensed under the Apache License, Version 2.0 (the "License");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

缺少空行。

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
#include <memory>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#pragma 和 #include 之间缺少空行

#include <string>
namespace paddle {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#include 和 namespace 之间缺少空行

typedef std::unique_ptr<std::exception> ErrorPtr;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

缺少空行

class Error : public std::exception {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

缺少空行

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

按照C++ style guide的要求,我们不应该使用exceptions,为什么要定义这个exception type?

public:
explicit inline Error(const std::string& what) noexcept : what_(what) {}
virtual const char* what() const noexcept { return this->what_.c_str(); }

static void throwError(const std::string& what) throw(ErrorPtr&);

private:
std::string what_;
};

} // namespace paddle
41 changes: 31 additions & 10 deletions paddle/utils/PythonUtil.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -116,26 +116,39 @@ static void printPyErrorStack(std::ostream& os,
}
PyObjectPtr callPythonFuncRetPyObj(const std::string& moduleName,
const std::string& funcName,
const std::vector<std::string>& args) {
const std::vector<std::string>& args,
std::string* errorString) {
PyGuard guard;
PyObjectPtr pyModule = py::import(moduleName);
PyObjectPtr pyModule = py::import(moduleName, errorString);
if (pyModule == nullptr) {
return nullptr;
}

PyObjectPtr pyFunc(PyObject_GetAttrString(pyModule.get(), funcName.c_str()));
CHECK_PY(pyFunc) << "GetAttrString failed.";
CHECK_PY_WITH_ERRORSTR(
pyFunc, "GetAttrString failed.", errorString, return nullptr);
PyObjectPtr pyArgs(PyTuple_New(args.size()));
for (size_t i = 0; i < args.size(); ++i) {
PyObjectPtr pyArg(PyString_FromString(args[i].c_str()));
CHECK_PY(pyArg) << "Import pyArg failed.";
CHECK_PY_WITH_ERRORSTR(
pyArg, "Import pyArg failed.", errorString, return nullptr);
PyTuple_SetItem(pyArgs.get(), i, pyArg.release()); // Maybe a problem
}
PyObjectPtr ret(PyObject_CallObject(pyFunc.get(), pyArgs.get()));
CHECK_PY(ret) << "Call Object failed.";
CHECK_PY_WITH_ERRORSTR(
ret, "Call Object failed", errorString, return nullptr);
return ret;
}

std::string callPythonFunc(const std::string& moduleName,
const std::string& funcName,
const std::vector<std::string>& args) {
PyObjectPtr obj = callPythonFuncRetPyObj(moduleName, funcName, args);
const std::vector<std::string>& args,
std::string* errorString) {
PyObjectPtr obj =
callPythonFuncRetPyObj(moduleName, funcName, args, errorString);
if (obj == nullptr) {
return std::string("");
}
return std::string(PyString_AsString(obj.get()), PyString_Size(obj.get()));
}

Expand Down Expand Up @@ -181,10 +194,18 @@ std::string getPyCallStack() {
return os.str();
}

PyObjectPtr import(const std::string& moduleName) {
PyObjectPtr import(const std::string& moduleName, std::string* errorString) {
auto module = PyImport_ImportModule(moduleName.c_str());
CHECK_PY(module) << "Import " << moduleName << "Error";
return PyObjectPtr(module);
if (errorString == nullptr) {
CHECK_PY(module) << "Import " << moduleName << "Error";
return PyObjectPtr(module);
} else {
if (module == nullptr) {
*errorString = getPyCallStack();
return nullptr;
}
return PyObjectPtr(module);
}
}

} // namespace py
Expand Down
19 changes: 16 additions & 3 deletions paddle/utils/PythonUtil.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ namespace paddle {

std::string callPythonFunc(const std::string& moduleName,
const std::string& funcName,
const std::vector<std::string>& args);
const std::vector<std::string>& args,
std::string* errorString = nullptr);

#ifndef PADDLE_NO_PYTHON

Expand Down Expand Up @@ -76,17 +77,29 @@ typedef std::unique_ptr<PyObject, PyObjectDeleter> PyObjectPtr;

PyObjectPtr callPythonFuncRetPyObj(const std::string& moduleName,
const std::string& funcName,
const std::vector<std::string>& args);
const std::vector<std::string>& args,
std::string* errorString = nullptr);

PyObjectPtr createPythonClass(const std::string& moduleName,
const std::string& className,
const std::vector<std::string>& args,
const std::map<std::string, std::string>& kwargs);

#define CHECK_PY(x) CHECK((x) != nullptr) << ::paddle::py::getPyCallStack()
#define CHECK_PY_WITH_ERRORSTR(x, extraMsg, errStr, ...) \
do { \
if ((errStr) == nullptr) { \
CHECK_PY(x) << (extraMsg); \
} else if ((x) == nullptr) { \
*(errStr) = (extraMsg); \
*(errStr) += ::paddle::py::getPyCallStack(); \
__VA_ARGS__; \
} \
} while (0)

namespace py {
PyObjectPtr import(const std::string& moduleName);
PyObjectPtr import(const std::string& moduleName,
std::string* errorString = nullptr);

/**
* Cast a PyLong or PyInt to int type T.
Expand Down
13 changes: 13 additions & 0 deletions paddle/utils/Util.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ limitations under the License. */
#include <gflags/gflags.h>

#include "CustomStackTrace.h"
#include "Error.h"
#include "Logging.h"
#include "StringUtil.h"
#include "Thread.h"
Expand Down Expand Up @@ -143,7 +144,19 @@ void runInitFunctions() {
});
}

static void logAllUnhandledExceptions() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果按照style guide不用exceptions的话,貌似也就不需要log exceptions了.

try {
throw;
} catch (ErrorPtr& err) {
std::cerr << err->what() << "\n";
} catch (...) {
std::cerr << "Unsupported error occured";
}
exit(1);
}

void initMain(int argc, char** argv) {
std::set_terminate(logAllUnhandledExceptions);
initializeLogging(argc, argv);
installLayerStackTracer();
std::string line;
Expand Down