Executor Design Doc #4649
Executor Design Doc #4649
Conversation
tonyyang-svail
requested review from
Superjomn,
reyoung,
wangkuiyi,
helinwang,
jacquesqiao,
dzhwinter,
JiayiFeng and
QiJune
There was a problem hiding this comment.
Can you add a section describing why we need executor, what does the executor do (and not do), what's the executor's input and output, and what does executor own.
I think we need to be clear on what does the executor do and not do in the design doc. For example, does the executor need to know about RNN? Feels like the RNN is just one kind of OP (an OP interface implementation detail), which the executor should not care about (and not depend on). If the executor is depending on OP's implementation detail, it will be a pain - every time the OP implementation changes, we need to change the executor, or at least verify that the executor is not broken.
doc/design/executor.md
Outdated
|
|
||
| // Instantiate all the vars in the global scope | ||
| for (auto& var : block.vars()) { | ||
| scope->NewVar(var.name()); |
There was a problem hiding this comment.
A pdesc may run multiple times, do we need to instantiate the vars every time?
There was a problem hiding this comment.
if the var.name() already exists, it won't be re-created.
There was a problem hiding this comment.
Not related to this PR, I think NewVar should return an error if no new var is created, otherwise it should not be call NewVar.
doc/design/executor.md
Outdated
|
|
||
| // Run the block | ||
| Scope& local_scope = scope->NewScope(); | ||
| for (size_t i = 0; i < should_run.size(); ++i) { |
There was a problem hiding this comment.
In my mind the executor need to run an OP as soon as its dependent OPs are all finished.
The code here is a viable but low performance solution. Do we need to put implementation details in the design doc?
There was a problem hiding this comment.
Do we need to put implementation details in the design doc? The current implementation is just one way of implementing it :)
doc/design/executor.md
Outdated
|
|
||
| // Run the block | ||
| Scope& local_scope = scope->NewScope(); | ||
| for (auto& op_desc : block.ops()) { |
There was a problem hiding this comment.
The code is a detailed implementation, if we want to show people the detailed code, maybe we can just put a link here. The implementation detail in a design doc gets stale easily.
There was a problem hiding this comment.
Good point. Changed to the link.
doc/design/executor.md
Outdated
|
|
||
| ## Implementation | ||
|
|
||
| `Executor` evaluates a `ProgramDesc`. Essentially, it instantiates Variables and Operators, then run all the operators |
There was a problem hiding this comment.
How will the operators run? Will them run as a sequence, or concurrently whenever a operator is ready (e.g., when all dependent operators are finished) to run.
There was a problem hiding this comment.
They will be run in sequence. Added "in sequence" to the doc.
… executor-design
Implementation: #4537