[VirtualMachine] new method allowing to set one input tensor by its index or name #10293
Conversation
vvchernov
requested review from
vinx13,
tqchen,
liangfu,
areusch,
tmoreau89,
masahi,
kazum,
ZihengJiang and
junrushao
as code owners
vvchernov
force-pushed
the
vc/vm-set-input-patch
branch
from
53eb213
to
e47062d
Compare
vvchernov
force-pushed
the
vc/vm-set-input-patch
branch
from
e47062d
to
1b705f8
Compare
vvchernov
force-pushed
the
vc/vm-set-input-patch
branch
from
56b028f
to
e9eb686
Compare
vvchernov
force-pushed
the
vc/vm-set-input-patch
branch
from
cfecf52
to
289bd82
Compare
vvchernov
force-pushed
the
vc/vm-set-input-patch
branch
from
73e7e43
to
4e01e40
Compare
…mber to VMFunction constructor
…_one_input) to VM API, the joined methods were removed
vvchernov
force-pushed
the
vc/vm-set-input-patch
branch
4 times, most recently
from
9311b72
to
de3e0ed
Compare
vvchernov
force-pushed
the
vc/vm-set-input-patch
branch
from
de3e0ed
to
e668147
Compare
vvchernov
changed the title
vvchernov
changed the title
There was a problem hiding this comment.
This looks good overall, thank you @vvchernov ! I'd like to see some new unit tests in here: https://github.com/apache/tvm/blob/main/tests/python/relay/test_vm.py to at least demonstrate how this new method is intended to be used.
| @@ -221,6 +211,9 @@ PackedFunc VirtualMachine::GetFunction(const std::string& name, | |||
| } else if (name == "set_input") { | |||
| return PackedFunc( | |||
| [sptr_to_self, this](TVMArgs args, TVMRetValue* rv) { SetInput(args[0], args, 1); }); | |||
| } else if (name == "set_one_input") { | |||
There was a problem hiding this comment.
nit: suggest a unit test exercising this new method from vm.py (though I'm assuming you need to change that anyway, perhaps in a follow up PR?) Maybe you are just using the runtime module directly?
There was a problem hiding this comment.
Yes, I use runtime module directly on native side. It is a reason why I try to use set_input methods instead of direct using of invoke. The latter requires to pack all input tensors to TvmArgs in certain sequence on native side. For me it is more controlable and clear to input one by one tensor with its tag (index or name). It is simultaneously the answer to your below question. Nevertheless I agree that my patch was not full and I've extended python API of VirtualMachine by set_one_input method. I observed that there is no even set_input unit test. Unit tests are in progress.
There was a problem hiding this comment.
I've added pytests.
|
One more question (sorry to double dip!): I agree a uniform parameter passing convention for all executors would be a Fine Thing. What I don't understand is why we separate binding and invocation in the first place? Surely the uniform API would be invoke(func, *args)? |
vvchernov
force-pushed
the
vc/vm-set-input-patch
branch
from
bcd3847
to
445d707
Compare
vvchernov
force-pushed
the
vc/vm-set-input-patch
branch
from
445d707
to
ba6533d
Compare
vvchernov
requested review from
comaniac,
jroesch,
yzhliu,
merrymercy and
icemelon
as code owners
…e implemented and checked
vvchernov
force-pushed
the
vc/vm-set-input-patch
branch
from
4bd5d01
to
4441a6e
Compare
There was a problem hiding this comment.
Thanks for the unit tests.
I can see there's a potential performance advantage to being able to selectively bind function parameters, eg to pay the device copy / format change cost just once for weights or other args which do not vary frequently between calls.
However, I can't think of a reason it needs to be bound up with the VM, and could imagine a separate 'argument builder' interface to manage that. However that's technically a refactor so would need a design review. So perhaps leave a TODO in this PR to that effect (sorry if that means another 6 hours of CI!) so you can make progress. And if you'd be at all interested in taking on that design & refactor please do so!
|
Hello @mbs-octoml, thanks for your comment. I'm not sure that I fully caught your idea, I think it is related to design improvement of an executor, but for this the more qualified ingeneer is needed who knows functionality and design of GE, VM and possible future executor to evaluate profits and risk related to back compatability of the code. My aim was to solve my local task only and try to do it so that somebody also can use my tool. @tmoreau89 what do you think about it? |
|
It sounds like there are more deeply rooted issues with the VM design choice to do explicit parameter binding etc. This seems indeed like a bigger discussion that warrants its own RFC, tracking issue etc; not sure a TODO here would be the best way to track the need to redesign. Due to time difference and latency in getting the PR changed, then approved, we could merge this without the TODO change. WDYT @mbs-octoml ? |
|
I filed CORE-142, though obviously that's not visible to the wider community. @jroesch expressed concern with bringing GE APIs over to the VM since the one-param-at-a-time API is fragile. |
…ed for correct CI)
…ndex or name (apache#10293) * set_input_with_index was implemented for VM * clean code * add getInputIndexFromName. add function descriptions. lint fix * fix lint * transfer comparison of parameter names number and assigned devices number to VMFunction constructor * add GetVMFunctionWithName to Executable API * clean code * add SetInputWithName (set_input_with_name) to VM API * join SetInputWithIndex and SetInputWithName to SetOneInputTensor (set_one_input) to VM API, the joined methods were removed * fix lint * some fixes after review * add set_one_input method to python API of VirtualMachine * pytests for set_input and set_one_input methods of VirtualMachine were implemented and checked * CI restart * construct simple model for pytests by relay instead of onnx tools (need for correct CI) Co-authored-by: Valery Chernov <valery.chernov@deelvin.com>
Implementation of
set_one_inputmethod in VirtualMachine (VM) class. It provides more convenient way to work with multiple input models on native side. Now you can use index or name to set one input tensor to VM.Examples:
set_one_input("main", "data", tensor0)set_one_input("main", 1, tensor1)It was required due to GraphExecutor (GE) and VM have different signature for
set_inputmethod.set_inputfrom VM is not so convenient for work with multiple tensor input. Implementedset_one_inputmethod in VM is repeated signature fromset_inputin GE.I think it will be good if we have the same API for both GraphExecutor and VirtualMachine. But I do not see simple way for it.
My current solution seems solid but not final.
Possible improvements: I can insert functionality of
set_one_inputmethod to base methodset_input. But I do not think that it is good idea to mix them. Other possible extension is to additionally reloadset_inputwith args: func_name, index1(name1), tensor1, index2(name2), tensor2, …, indexN(nameN), tensorN, where N is number of all input tensors. But it is still not perfect solution for me.