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 GetPlaces operator #6732

Merged
merged 14 commits into from
Jan 8, 2018
Merged

add GetPlaces operator #6732

merged 14 commits into from
Jan 8, 2018

Conversation

QiJune
Copy link
Member

@QiJune QiJune commented Dec 19, 2017

Fix #6728

@QiJune QiJune changed the title add Get places op add GetPlaces operator Dec 19, 2017
: OperatorBase(type, inputs, outputs, attrs) {}
void Run(const framework::Scope &scope,
const platform::DeviceContext &dev_ctx) const override {
auto use_gpu = Attr<bool>("use_gpu");

Choose a reason for hiding this comment

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

use_gpu => use_cuda?
since we wanna support AMD in the future.

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. Change this attribute to a string. Now support "CPU" and "CUDA".

places.resize(trainer_count);
if (use_gpu) {
for (int i = 0; i < trainer_count; i++) {
places.emplace_back(platform::GPUPlace(i));

Choose a reason for hiding this comment

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

maybe CUDAPlace here?

Choose a reason for hiding this comment

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

Also check if there are enough GPUs.

Copy link
Member Author

Choose a reason for hiding this comment

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

We now have GPUPlace in place.h. Will change it together in next PR.

AddAttr<bool>("use_gpu", "(bool)use gpu").SetDefault(false);
AddComment(R"DOC(
GetPlaces Operator.

Choose a reason for hiding this comment

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

Doc "Returns a list of places based on flags. The list will be used for parallel execution."

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

framework::OpAttrChecker *op_checker)
: OpProtoAndCheckerMaker(proto, op_checker) {
AddOutput("Out", "vector of Place");
AddAttr<int>("trainer_count", "(int)trainer count").SetDefault(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe trainer_count is not a suitable name since trainer/pserver is used in distributed training, and trainer_count is confusing with the count of trainers.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's actually trainer count in local machine. I have not think out a proper name yet. Is there any suggestion?

@@ -8,10 +8,13 @@
from tensor import *
import control_flow
from control_flow import *
import utils
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe device? utils is too general

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

#else
PADDLE_THROW("'GPUPlace' is not supported in CPU only device.");
#endif
} else if (device_type == "CPU") {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the relationship between device_count and CPU cores?Or do we need to limit the device_count to a ratio large than CPU cores?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confusing why to specify a ratio number, but I think the device_count means the CPU cores when device_type="CPU".

Copy link
Collaborator

Choose a reason for hiding this comment

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

If device_count = 0, then it will use all devices of CPUs or GPUs.

@reyoung reyoung merged commit 219fbd5 into PaddlePaddle:develop Jan 8, 2018
@tonyyang-svail tonyyang-svail mentioned this pull request Jan 9, 2018
3 tasks
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

5 participants