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.

Sign up for GitHub

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

Jump to bottom

Executor need reasonable default value #5681

Closed
wants to merge 4 commits into from
Closed

Executor need reasonable default value #5681

wants to merge 4 commits into from

Conversation

tonyyang-svail
Copy link

@tonyyang-svail tonyyang-svail commented Nov 15, 2017 •
edited
Loading

fix #5648

When using exe = Executor(place=None), the logic is the following:

compiled without GPU compiled with GPU
run without GPU CPU warning, then use CPU
run with GPU CPU GPU

helinwang
helinwang reviewed Nov 15, 2017
View reviewed changes
python/paddle/v2/fluid/executor.py
def __init__(self, places):
def __init__(self, places=None):
if places is None:
if core.is_compile_gpu():
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to detect at runtime whether a GPU is present. It's very likely a compiled GPU version runs on a CPU only machine.

helinwang
helinwang reviewed Nov 15, 2017
View reviewed changes
paddle/pybind/pybind.cc Outdated
@@ -78,6 +78,14 @@ bool IsCompileGPU() {
#endif
}

int GetCUDADeviceCount() {
#ifndef PADDLE_WITH_CUDA
return 0;
Copy link
Contributor

@helinwang helinwang Nov 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe return -1 here to indicate error. Because the function name is GetCUDADeviceCount, if PADDLE_WITH_CUDA is OFF, the function can not do what its name suggests.

helinwang
helinwang reviewed Nov 15, 2017
View reviewed changes
paddle/pybind/pybind.cc Outdated
@@ -497,6 +505,7 @@ All parameter, weight, gradient are variables in Paddle.
m.def("init_gflags", InitGflags);

m.def("is_compile_gpu", IsCompileGPU);
m.def("get_cuda_device_count", GetCUDADeviceCount());
Copy link
Contributor

Choose a reason for hiding this comment

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

Not familiar with Pybind, why it's a function invocation here? GetCUDADeviceCount(), rather than GetCUDADeviceCount?

Copy link
Author

Choose a reason for hiding this comment

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

It is a bug. Thanks for pointing it out.

@helinwang
Copy link
Contributor

Thanks for the logic table! Very helpful.

reyoung
reyoung reviewed Nov 16, 2017
View reviewed changes
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.

I do not think there should be a default value of Executor.

Users can use CPU only training even the Paddle is compiled with GPU and there is a GPU on their computer.

@tonyyang-svail
Copy link
Author

tonyyang-svail commented Nov 16, 2017 •
edited
Loading

@reyoung Thanks for your comments. The reason for this PR is more of python API thing.

We want to hide core.CPUPlace and core.GPUPace from the user. We can achieve this by either having a default value (like the current PR) or using the interface like Executor(is_gpu=False).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reyoung reyoung reyoung left review comments

@helinwang helinwang helinwang left review comments

@QiJune QiJune Awaiting requested review from QiJune

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Executor need reasonable default value
3 participants
@tonyyang-svail @helinwang @reyoung