Skip to content

Conversation

tensor-tang
Copy link
Contributor

@tensor-tang tensor-tang commented Nov 21, 2017

fix #5801

hi @jacquesqiao I do not have mac to test, so could u help?

I have already verified it on Linux, and it works.

wangkuiyi
wangkuiyi previously approved these changes Nov 21, 2017
# do not support on other platform yet
return False

def get_logical_processors():
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function returns the number of logical processors, other than a list of logical processors, so it should be named either get_num_logical_processor or num_logical_processor. Otherwise, we'd lose the most important concept "number".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, thanks, I will use num_logical_processor instead.

'''Get the logical processors number'''
import platform
if platform.system() == "Linux":
processors = os.popen(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly, processors => num_proc or num_processors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.

return int(processors.read())
else:
# do not support on other platform yet
return 1
Copy link
Collaborator

@wangkuiyi wangkuiyi Nov 21, 2017

Choose a reason for hiding this comment

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

I think this function definition (26 lines, from L82 to L108) can be significantly shortened (into 4 lines) while keeping the readability:

def num_logical_processor():
    cmds = { "Linux" : "grep \"processor\" /proc/cpuinfo|sort -u|wc -l",
             "Darwin" : "sysctl hw.logicalcpu" }
    return int(os.popen(cmds.get(platform.system(), "expr 1")).read())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, Thx.

for key in args_dict.keys():
args.append('--%s=%s' % (key, str(args_dict[key])))

auto_set_cpu_env(kwargs.get('trainer_count', 1))
Copy link
Collaborator

Choose a reason for hiding this comment

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

All programs, including auto_set_cpu_env, were written to do something automatically, so it seems that an accurate name could be set_omp_mkl_env_vars.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, thx.

Copy link
Contributor Author

@tensor-tang tensor-tang left a comment

Choose a reason for hiding this comment

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

Thanks, I will update it later.

# do not support on other platform yet
return False

def get_logical_processors():
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, thanks, I will use num_logical_processor instead.

'''Get the logical processors number'''
import platform
if platform.system() == "Linux":
processors = os.popen(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.

return int(processors.read())
else:
# do not support on other platform yet
return 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, Thx.

for key in args_dict.keys():
args.append('--%s=%s' % (key, str(args_dict[key])))

auto_set_cpu_env(kwargs.get('trainer_count', 1))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, thx.

@QiJune
Copy link
Member

QiJune commented Nov 22, 2017

Have tested on Mac, v2 Python API is fine.

Copy link
Member

@QiJune QiJune 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 e28157d into PaddlePaddle:develop Nov 22, 2017
@tensor-tang tensor-tang deleted the fix5801 branch November 22, 2017 05:27
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.

v2 init have some code that can not run on mac

3 participants