add a set of apis to configure wasi-nn via InstantiationArgs2 #4764
Conversation
zhanheng1
force-pushed
the
InstantiationArgs2-wasinn
branch
from
77794ae to
c73e4aa
Compare
zhanheng1
requested review from
TianlongLiang,
loganek,
lum1n0us,
no1wudi and
yamt
as code owners
| __attribute__((visibility("default"))) wasi_nn_error | ||
| load_by_name(void *tflite_ctx, const char *name, uint32_t namelen, graph *g); | ||
| load_by_name(void *tflite_ctx, const char *name, uint32_t namelen, | ||
| graph_encoding encoding, execution_target target, graph *g); |
There was a problem hiding this comment.
why do you want to add encoding argument?
There was a problem hiding this comment.
I noticed that both 'encoding' and 'target' are configurable options, so I incorporated them into the code. Do you think this was inappropriate?
| } WASMModuleInstMemConsumption; | ||
|
|
||
| #if WASM_ENABLE_WASI_NN != 0 || WASM_ENABLE_WASI_EPHEMERAL_NN != 0 | ||
| typedef struct WASINNGlobalContext { |
There was a problem hiding this comment.
in what sense is this "global"?
There was a problem hiding this comment.
InstantiationArgs2 will be freed soon after wasm_runtime_instantiate_ex2()(in posix/main.c for example), and user cannot use InstantiationArgs2 in wasm app later.
So I add this new struct to handle these command line options, so load_by_name() can get these information(like target, encoding) through WASINNGlobalContext.
WASINNGlobalContext will be registered into WASMModuleInstanceExtraCommon->context by wasm_native_set_context() after init.
| wasm_runtime_get_wasi_nn_global_ctx_ngraphs(wasi_nn_global_ctx); | ||
| // Assume filename got from user wasm app : max; sum; average; ... | ||
| // Assume file path got from user cmd opt: /your/path1/max.tflite; | ||
| // /your/path2/sum.tflite; ...... |
There was a problem hiding this comment.
i don't fully understand what this super complex logic is doing.
i'd say it's simpler and more flexible to let user give the name explicitly when adding a model to a registry.
There was a problem hiding this comment.
The changes have been made as your suggestions.
product-mini/platforms/posix/main.c
Outdated
| #if WASM_ENABLE_WASI_NN != 0 || WASM_ENABLE_WASI_EPHEMERAL_NN != 0 | ||
| wasi_nn_graph_registry_create(&nn_registry); | ||
| wasi_nn_graph_registry_set_args(nn_registry, encoding, target, n_models, | ||
| model_paths); |
There was a problem hiding this comment.
it's better to allow multiple --wasi-nn-graph=.
eg. if you have two --wasi-nn-graph=, the registry would contain two models.
--wasi-nn-graph=name=modelA,encoding=...,target=....
--wasi-nn-graph=name=modelB,encoding=...,target=....
conceptually a registry would be a map indexed by a string.
{
"modelA" -> Model(encodingA, targetA, filelistA, ...),
"modelB" -> Model(encodingB, targetB, filelistB, ...),
}
when a wasm module calls load-by-name("modelA"), it will get Model(encodingA, targetA, filelistA, ...).
There was a problem hiding this comment.
Thank you, the feature now supports multiple --wasi-nn-graphs arguments, and it works exactly as you described in this comment.
product-mini/platforms/posix/main.c
Outdated
| char *tokens[12] = { 0 }; | ||
|
|
||
| // encoding:tensorflowlite|openvino|llama target:cpu|gpu|tpu | ||
| // --wasi-nn-graph=encoding:target:model_file_path1:model_file_path2:model_file_path3:...... |
There was a problem hiding this comment.
complex argument parsing logic like this can be in libc_wasi.c so that it's shared among platforms.
There was a problem hiding this comment.
They are now in libc_wasi.c
3 participants
add a set of apis to configure wasi-nn via InstantiationArgs2: