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

add init_gflags interface #5193

Merged
merged 3 commits into from
Oct 30, 2017
Merged

add init_gflags interface #5193

merged 3 commits into from
Oct 30, 2017

Conversation

QiJune
Copy link
Member

@QiJune QiJune commented Oct 29, 2017

Fix #5194

@@ -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) {
Copy link
Collaborator

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.

Copy link
Member Author

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;

void InitGflags(std::vector<std::string> argv_vec) {
Copy link
Collaborator

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

Copy link
Member Author

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;
Copy link
Collaborator

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.

Copy link
Collaborator

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

Copy link
Member Author

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.

@@ -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) {
Copy link
Collaborator

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.

Copy link
Member Author

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'):
Copy link
Collaborator

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@QiJune QiJune mentioned this pull request Oct 29, 2017
Copy link
Collaborator

@reyoung reyoung left a comment

Choose a reason for hiding this comment

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

LGTM

@QiJune QiJune merged commit a186b53 into PaddlePaddle:develop Oct 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants