[WIP][Runtime] Pipeline Executor Add Set and Get Input/Output interfaces. #9494
Conversation
1. Add "param" connection into pipeline config to support such case like set params into module 1 by a param name "param0" 2. Add using input name to locate backend runtime index and input index implemention. 3. Add interface like run/stop/set_input/get_output etc. 4. Add a implemention of serialized pipeline backend runtime execution for the purpose to test all the said interface.
huajsj
requested review from
ZihengJiang,
areusch,
comaniac,
icemelon,
jroesch,
junrushao,
kazum,
liangfu,
masahi,
merrymercy,
tmoreau89,
tqchen,
vinx13,
yzhliu and
zhiics
as code owners
comaniac
left a comment
comaniac
left a comment
There was a problem hiding this comment.
Please add the RFC and tracking issue information to the PR description. Also please update the tracking issue.
| v = self._get_input(key) | ||
| if v is None: | ||
| raise RuntimeError("Could not find '%s' in pipeline's inputs" % key) | ||
| v.copyfrom(value) |
There was a problem hiding this comment.
I just realized that you need to do this because all buffers were created when initializing the pipeline executor. In this way we will double the memory usage of global inputs. Can we avoid allocating redundant buffers to reduce memory consumption, similar to graph executor and VM?
There was a problem hiding this comment.
sure, the said logic seems like already follow the same logic as what graph_executor did and no redundant buffer creation, could you help to give more detail information about which part cause the problem?
| // If the source device and target device is not same, we use a local DLTensor | ||
| // as a medium to do the cross device copy work. |
There was a problem hiding this comment.
| // If the source device and target device is not same, we use a local DLTensor | |
| // as a medium to do the cross device copy work. | |
| // If the source device and target device are not the same, we use a temporary DLTensor on CPU | |
| // as the bridge. |
After thinking twice, I feel this may still introduce unnecessary overhead. Isn't it possible for non-CPU device APIs to support from? Should we first try TVMArrayCopyFromTo(from, to, nullptr), and then go through CPU if failed?
There was a problem hiding this comment.
Current existing TVMArrayCopyFromTo logic supported from device, once two device both are non-cpu, the said copy function will calling from DeviceAPI function to copy data from from to to, in such case reading data from from device is not problem, but write data to to device may cause crash issue. the reason is the from device may have no capability to access the data of DLTensor of to, for example when to device is VTA, the memory address in DLTensor actually is the address of data structure VTABuffer pointer instead of data memory address, a try of TVMArrayCopyFromTo to directly copy data into the to DLTensor will cause a crash by memory access violation.
if we revise the logic to let TVMArrayCopyFromTo to support to device, then read data from from device would become a new problem, hence here we use a bridge NDArray as a solution.
|
thanks @comaniac , all review comments addressed, please take a look. |
|
I can take a look this week end / next week. |
|
thanks @masahi. |
masahi
left a comment
masahi
left a comment
There was a problem hiding this comment.
I started reviewing, but on the first file I saw many basic grammar errors. I have to say you didn't learn from previous PRs. I won't review 1K code like this unless you make some effort to make reviewing easier.
| self._stop() | ||
|
|
||
| def set_input(self, key, value): | ||
| """Set inputs to the module via "value". |
| v.copyfrom(value) | ||
|
|
||
| def set_params(self, params_name, params_data): | ||
| """Set params to the module via param name and params data. |
| self._set_param(params_name, key, val) | ||
|
|
||
| def get_input(self, key): | ||
| """Get the input via a input name. |
| Returns | ||
| ------- | ||
| data : NDArray | ||
| Then input data. |
| The params name | ||
|
|
||
| params_data : dict of str to NDArray | ||
| A list of params data and params key name. |
There was a problem hiding this comment.
what is the difference between params key and params_name
| A list of params data and params key name. | ||
| """ | ||
| for key, val in params_data.items(): | ||
| self._set_param(params_name, key, val) |
There was a problem hiding this comment.
Related to the above comment, this is a weird API.
|
Thanks @masahi for the review, besides of fixing grammar issue, do you think that splitting this PR into couple small PR can make it be more review friendly? I will go to split this PR if you think that can help for the review. |
Yes! That sounds great, thanks. |
huajsj
changed the title
|
thanks @masahi @comaniac for the review, and sorry for inconvenience caused by the big PR code size, I start to split this PR into 4 small PR to make it more review friendly, following is the plan of patch splitting, associated tracking issue also get updated. the current PR will get keep for a while for review comments tracking purpose.
|
3 participants
Add "param" connection into pipeline config to support such case
like set params into module 1 by a param name "param0"
Add using input name to locate backend runtime index and input index
implemention.
Add interface like run/stop/set_input/get_output etc.
Add a implemention of serialized pipeline backend runtime execution
for the purpose to test all the said interface.
Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers by @ them in the pull request thread.