-
Notifications
You must be signed in to change notification settings - Fork 711
Add onnxruntime as wasi-nn backend #4485
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add onnxruntime as wasi-nn backend #4485
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the relationship with #4304?
1, Adapt to latest wasi-nn arch and support WAMR_BUILD_WASI_EPHEMERAL_NN |
aa88085
to
1fb25ad
Compare
1e60909
to
29e4dd5
Compare
1, type converter btw wasi-nn and onnx runtime returns bool instead of type 2, out_buffer_size does not hold the expected size. 3, onnx runtime does not need calculate input_tenser size.
0dcdcab
to
cd3cb6c
Compare
using this model, i had to use non-zero index for get_output. thus i had to fix nn-cli bug. |
I'm using this one: https://github.com/onnx/models/blob/main/validated/vision/object_detection_segmentation/ssd-mobilenetv1/model/ssd_mobilenet_v1_10.onnx |
thank you. however, this model looks same in the regard. (have 4 outputs) |
maybe you somehow interpreted only the first (idx=0) output, which contains bounding boxes? |
output shape is [1, 100, 4]: in which, one bounding box for example: it looks good for my test picure (http://images.cocodataset.org/val2017/000000088462.jpg) |
ok. i understood. actually, the model has 4 outputs as documented
as wasi-nn doesn't have get-output-by-name, you need to use integer indexes. you were only looking at detection_boxes. |
OK, Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@lum1n0us could you help review the code? |
|
||
/* Helper functions */ | ||
static void | ||
check_status_and_log(const OnnxRuntimeContext *ctx, OrtStatus *status) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not used?
} | ||
|
||
static bool | ||
convert_ort_type_to_wasi_nn_type(ONNXTensorElementDataType ort_type, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not used?
err = convert_ort_error_to_wasi_nn_error(ctx, status); | ||
NN_ERR_PRINTF("Failed to create ONNX Runtime environment: %s", | ||
error_message); | ||
ctx->ort_api->ReleaseStatus(status); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems convert_ort_error_to_wasi_nn_error()
will ReleaseStatus(status)
. therefore, L194 might not be necessary
wasi_nn_error err = convert_ort_error_to_wasi_nn_error(ctx, status); | ||
NN_ERR_PRINTF("Failed to create ONNX Runtime session: %s", | ||
error_message); | ||
ctx->ort_api->ReleaseStatus(status); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems convert_ort_error_to_wasi_nn_error() will ReleaseStatus(status). therefore, L352 might not be necessary
NN_ERR_PRINTF("Maximum number of graphs reached"); | ||
return runtime_error; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just my doubt, add a protector about name
? like name
is empty or NULL
__attribute__((visibility("default"))) wasi_nn_error | ||
load(void *onnx_ctx, graph_builder_array *builder, graph_encoding encoding, | ||
execution_target target, graph *g) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a protector about onnx_ctx
like others?
if (!onnx_ctx) {
return runtime_error;
}
__attribute__((visibility("default"))) wasi_nn_error | ||
load_by_name(void *onnx_ctx, const char *name, uint32_t filename_len, graph *g) | ||
{ | ||
OnnxRuntimeContext *ctx = (OnnxRuntimeContext *)onnx_ctx; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a protector about onnx_ctx
like others?
if (!onnx_ctx) {
return runtime_error;
}
ctx->ort_api->ReleaseValue(output.second); | ||
} | ||
ctx->ort_api->ReleaseMemoryInfo(ctx->exec_ctxs[i].memory_info); | ||
ctx->exec_ctxs[i].is_initialized = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
input_names
and output_names
?
Steps to verify:
Generate output.bin, with shape [1, 100, 4] and f32 type, which contents match the sample's output