-
Notifications
You must be signed in to change notification settings - Fork 5.7k
add GetPlaces operator #6732
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
Conversation
paddle/operators/get_places_op.cc
Outdated
: 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"); |
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.
use_gpu => use_cuda?
since we wanna support AMD in the future.
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. Change this attribute to a string. Now support "CPU" and "CUDA".
paddle/operators/get_places_op.cc
Outdated
places.resize(trainer_count); | ||
if (use_gpu) { | ||
for (int i = 0; i < trainer_count; i++) { | ||
places.emplace_back(platform::GPUPlace(i)); |
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.
maybe CUDAPlace
here?
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.
Also check if there are enough GPUs.
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.
We now have GPUPlace
in place.h. Will change it together in next PR.
paddle/operators/get_places_op.cc
Outdated
AddAttr<bool>("use_gpu", "(bool)use gpu").SetDefault(false); | ||
AddComment(R"DOC( | ||
GetPlaces Operator. | ||
|
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.
Doc "Returns a list of places based on flags. The list will be used for parallel execution."
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/operators/get_places_op.cc
Outdated
framework::OpAttrChecker *op_checker) | ||
: OpProtoAndCheckerMaker(proto, op_checker) { | ||
AddOutput("Out", "vector of Place"); | ||
AddAttr<int>("trainer_count", "(int)trainer count").SetDefault(1); |
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.
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
.
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.
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 |
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.
Maybe device
? utils
is too general
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/operators/get_places_op.cc
Outdated
#else | ||
PADDLE_THROW("'GPUPlace' is not supported in CPU only device."); | ||
#endif | ||
} else if (device_type == "CPU") { |
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.
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?
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'm confusing why to specify a ratio number, but I think the device_count
means the CPU cores when device_type="CPU"
.
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.
If device_count = 0, then it will use all devices of CPUs or GPUs.
Fix #6728