Feature/global context #6537
Feature/global context #6537
Conversation
paddle/framework/executor.cc
Outdated
| // delete device_context; | ||
| // } | ||
| // } | ||
| // } |
There was a problem hiding this comment.
We need Executor::Executor(const platform::DeviceContext& device) for recurrent_op.
https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/operators/recurrent_op.cc#L236
There was a problem hiding this comment.
I think Executor::Executor(const platform::DeviceContext& device) should be removed in whole project.
There was a problem hiding this comment.
Since Executor(places) no longer owns the DeviceContext, I believe you are right.
| class DeviceContextPool { | ||
| public: | ||
| static DeviceContextPool& Get() { | ||
| PADDLE_ENFORCE_NOT_NULL(pool, "Need to Create DeviceContextPool first!"); |
There was a problem hiding this comment.
Can create if nullptr here. So the Create function is not neded.
There was a problem hiding this comment.
Yes, a singleton ONLY always mix these two function together, but here we have a slightly problem.
Create will create the all the devices. But when we need get always only need create part of it.
For example, Init function create all the available GPU, CPU devices, when we want to create two or more executors, then Get() happens part may not have all the device information.
In previous implementation, we definitly have one and only one executor, but now it is different since executor does not contain resources, we may have two or more executors.
paddle/framework/executor.h
Outdated
| } | ||
| // TODO(dzhwinter) : assign first find device. Will enhanced later. | ||
| borrowed_contexts.emplace_back(range.first->second); | ||
| device_tasks_[place] += 1; |
There was a problem hiding this comment.
Is this a reference count? See no place to decrease it.
There was a problem hiding this comment.
Currently, it is useless. We will improve it after migrating to DevicePool for GPU.
There was a problem hiding this comment.
Then add some TODO comments here.
| ASSERT_EQ(InitDevices(ds1), true); | ||
|
|
||
| #ifdef PADDLE_WITH_CUDA | ||
| std::vector<std::string> ds2 = {"CPU", "GPU:0", "GPU:1"}; |
There was a problem hiding this comment.
Please document the format of place strings, or add a TODO
There was a problem hiding this comment.
It is documented in the init.cc function definition part.
paddle/framework/executor.cc
Outdated
| @@ -132,8 +112,8 @@ void Executor::Run(const ProgramDescBind& pdesc, Scope* scope, int block_id, | |||
| } | |||
| } | |||
|
|
|||
| Executor::Executor(const platform::DeviceContext& device) | |||
| : device_contexts_({&device}), own_(false) {} | |||
| // Executor::Executor(const platform::DeviceContext& device) | |||
There was a problem hiding this comment.
Why comment these lines? I think this ctor is still useful.
There was a problem hiding this comment.
This interface should be removed, because we only need the place indicator. I will rewrite how the while_op, recurrent_op using executor in next PR.
There was a problem hiding this comment.
I see.
Before rewriting ops that need to run executor, please take a look at #6664
paddle/framework/executor.h
Outdated
| explicit Executor(const std::vector<platform::Place>& places); | ||
| explicit Executor(const platform::DeviceContext& devices); | ||
| ~Executor(); | ||
| // ~Executor(); |
There was a problem hiding this comment.
Delete the commented code.
paddle/framework/init.cc
Outdated
| // device format | ||
| // CPU | ||
| // GPU:1 | ||
| // FPGA:2 |
There was a problem hiding this comment.
No FPGA in the following code, so maybe we can delete this comment firstly.
| ASSERT_EQ(InitDevices(ds1), true); | ||
|
|
||
| #ifdef PADDLE_WITH_CUDA | ||
| std::vector<std::string> ds2 = {"CPU", "GPU:0", "GPU:1"}; |
There was a problem hiding this comment.
Does the CPU means all CPU cores? do we need a approach to specify the number of CPU cores?
There was a problem hiding this comment.
It only means one CPUDeviceContext, our multi-CPU design is not complete at the current stage, so I think that specify the number of CPU cores is useless.
dzhwinter
dismissed
tonyyang-svail’s stale review
Executor(places) no longer owns the DeviceContext, I will fix DeviceContext creation inside op in next PR.
fix #6536