-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
add init_gflags interface #5193
Conversation
paddle/pybind/pybind.cc
Outdated
@@ -40,6 +42,23 @@ static size_t UniqueIntegerGenerator() { | |||
return generator.fetch_add(1); | |||
} | |||
|
|||
std::once_flag gflags_init_flag; | |||
|
|||
void InitGflags(std::vector<std::string> argv_vec) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter type should be a reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
paddle/pybind/pybind.cc
Outdated
@@ -40,6 +42,23 @@ static size_t UniqueIntegerGenerator() { | |||
return generator.fetch_add(1); | |||
} | |||
|
|||
std::once_flag gflags_init_flag; | |||
|
|||
void InitGflags(std::vector<std::string> argv_vec) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
argv_vec => argv. The v
in argv has been "vectors".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -40,6 +42,23 @@ static size_t UniqueIntegerGenerator() { | |||
return generator.fetch_add(1); | |||
} | |||
|
|||
std::once_flag gflags_init_flag; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember that we have a convention from Google Style Guide -- global variables must be of POD type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@QiJune for the alternative, you can use pthread API for this global variable
http://pubs.opengroup.org/onlinepubs/7908799/xsh/pthread_once.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@reyoung pthread_once
is used as follows:
pthread_once(pthread_once_t *once_control, void (*init_routine)(void));
We have to pass a vector of string. So it's not suitable.
@wangkuiyi Yes, but once_flag has no deconstructor. It behaves like a POD. And I have not found another better way to write this code yet.
paddle/pybind/pybind.cc
Outdated
@@ -40,6 +42,23 @@ static size_t UniqueIntegerGenerator() { | |||
return generator.fetch_add(1); | |||
} | |||
|
|||
std::once_flag gflags_init_flag; | |||
|
|||
void InitGflags(std::vector<std::string> argv_vec) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can add InitGFlags into a new .cc files so could we prevent from making this pybind.cc infinitely long. @reyoung had an idea how to separate pybind.cc into multiple files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we actually have some initialization work to do before executing the neural network, such as glog/gflags settings, and device count/information detection. I will add a TODO in this code and move it to init.cc
later.
__all__ = ['proto'] | ||
argv = ['paddle'] | ||
if os.getenv('FLAGS_fraction_of_gpu_memory_to_use'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parsing from environment variables is not needed since gflags has already done that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fix #5194