-
Notifications
You must be signed in to change notification settings - Fork 717
Integration of WASI-NN #1521
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
Integration of WASI-NN #1521
Conversation
TODO
|
039620a
to
b168c09
Compare
My team is planning to continuously support WASI-NN by adding support to GPU, etc.
|
@tonibofarull Sounds like it is not a short term work and may take some time to implement all the features, right? Normally we will develop such features under a development branch (like |
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 comments about the scripts, license headers and so on, the C/Python source code logic isn't reviewed yet, will read it soon.
1. Build the dockre image, | ||
|
||
``` | ||
docker build -t wasi-nn -f core/iwasm/libraries/wasi-nn/test/Dockerfile . |
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 sure whether the test is just a simple interface test (like unit test), or a sample? If it is a sample, please put it under <wamr_root>/samples
, for example, samples/wasi-nn.
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.
I'm testing multiple neural network models. Once it's ready for a release we can add a real sample!
|
||
if (buffer == NULL) { | ||
fputs("Memory error\n", stderr); | ||
exit(2); |
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.
how about changing to return with an error id?
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.
because here I'm doing the test of WASI-NN. As soon as there is an error I want to exit the program.
Thank you, I'll take a look! |
Let me address your comments before we discuss this 🙇 |
* feat: "load module done" * fix: "pr fixes" * fix: "pr fixes"
* feat: "set input" * feat: "formatted files" * fix: "PR changes" * fix: renaming files
* feat: "set input" * feat: "formatted files" * feat: ouptut and compute functions * PR fixes * fix: PR fixes
* feat: draft opening file * feat: ongoing progress tflite * feat: working test * fix: setting wasi_nn flag * Remove legacy cmake and clean build file * max sum and average models tested and working * multiple outputs begining * refractor saving model * pr fixes * pr fixes * pr fixes * pr fixes * Fix dockerfile * Fix unnecesary modified files * Working inferencer and tests passing * Assert in all tests * Generation of models in the build * Formatting python scripts Co-authored-by: tonibofarull <[email protected]>
I've fixed all the previous comments!
I've created the issue here.
As stated in the issue, the first milestone corresponds to support TensorFlow in CPU and FP32, which are the features implemented in this PR. Having the dev branch makes sense for having the future intermediate results! |
Great! We will read the code again, many thanks! |
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
@tonibofarull Many thanks for the fixing, I have merged the PR. [ 8%] Building C object CMakeFiles/libiwasm.dir/home/agent/WASM/wamr-internal-test/wamr-ba/core/iwasm/libraries/libc-wasi/sandboxed-system-primitives/src/random.c.o |
Yes, of course! Thank you very much 🥳 |
Initial integration of WASI-NN based on bytecodealliance#1225: - Implement the library core/iwasm/libraries/wasi-nn - Support TensorFlow, CPU, F32 at the first stage - Add cmake variable `-DWAMR_BUILD_WASI_NN` - Add test case based on Docker image and update document Refer to bytecodealliance#1573
@@ -425,6 +428,13 @@ wasm_native_init() | |||
goto fail; | |||
#endif /* WASM_ENABLE_LIB_RATS */ | |||
|
|||
#if WASM_ENABLE_WASI_NN != 0 | |||
n_native_symbols = get_wasi_nn_export_apis(&native_symbols); | |||
if (!wasm_native_register_natives("wasi_nn", native_symbols, |
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.
@tonibofarull where did this "wasi_nn" module name come from?
can you give me a reference to an exact version of the spec this was based on?
eg. commit id of wasi-nn repo
as far as i researched wasi-nn has always been using "wasi_ephemeral_nn".
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.
It was an arbitrary name as I couldn't get an API list based on witx/wit.
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.
ok. so this is not compatible with any versions of wasi-nn. right?
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.
Compatibility is not guaranteed.
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.
So, there will be no harm if we replace wasi-nn with wasi_ephemeral_nn in the repo?
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.
Compatibility is not guaranteed.
well, actually incompatibility is guaranteed because of the different module name, isn't it?
Initial integration of WASI-NN based on #1225.