-
Notifications
You must be signed in to change notification settings - Fork 716
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
[WASI-NN] neural_speed: add backend structure #3303
base: master
Are you sure you want to change the base?
Conversation
Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR. |
f916aeb
to
af7d519
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3303 +/- ##
=======================================
Coverage 79.83% 79.84%
=======================================
Files 253 253
Lines 34948 34948
Branches 6138 6122 -16
=======================================
+ Hits 27902 27905 +3
+ Misses 5625 5623 -2
+ Partials 1421 1420 -1 ☔ View full report in Codecov by Sentry. |
a60159f
to
984d13a
Compare
ef72299
to
065fc95
Compare
plugins/wasi_nn/CMakeLists.txt
Outdated
@@ -149,6 +149,64 @@ if(BACKEND STREQUAL "ggml") | |||
endif() | |||
endif() | |||
|
|||
if(BACKEND STREQUAL "neuralspeed") | |||
find_package(simdjson QUIET) |
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.
We changed the way to find the simdjson
package, please use the new way like this pr: #3426
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 have changed. Please check, thank you.
plugins/wasi_nn/neuralspeed.cpp
Outdated
#endif | ||
void printImformation(Graph &GraphRef, Context &CxtRef) { | ||
spdlog::info( | ||
"[WASI-NN][Info] Neural speed backend: Number of input tokens: {}"sv, |
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.
You don't need to add [Info]
here, because the spdlog::info
will do this for 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.
I have changed. Please check, thank you.
plugins/wasi_nn/neuralspeed.cpp
Outdated
"[WASI-NN][Debug] Neural speed: Model path not found in nn-preload, " | ||
"write model into a tmpfile."sv); | ||
} | ||
// TODO: pass the model directly to ggml |
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 would like to know the details of this TODO item.
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 refers to ggml backend. Although current implementation is enough. Should I remove it?
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.
We can remove it.
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 have changed. Please check, thank you.
plugins/wasi_nn/neuralspeed.cpp
Outdated
auto &CxtRef = Env.NNContext[ContextId].get<Context>(); | ||
auto &GraphRef = Env.NNGraph[CxtRef.GraphId].get<Graph>(); | ||
if (!Py_IsInitialized()) { | ||
spdlog::info( |
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.
We should use spdlog::error
here.
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 have changed. Please check, thank you.
plugins/wasi_nn/neuralspeed.cpp
Outdated
Expect<WASINN::ErrNo> compute(WasiNNEnvironment &Env, | ||
uint32_t ContextId) noexcept { | ||
if (!Py_IsInitialized()) { | ||
spdlog::info( |
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.
We should use spdlog::error
here. This is a fatal 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.
I have changed. Please check, thank you.
plugins/wasi_nn/neuralspeed.cpp
Outdated
"[WASI-NN] neural speed backend: Input transfer tensor failed."sv); | ||
return WASINN::ErrNo::InvalidArgument; | ||
} | ||
// PyObject *GenerateArgs = PyTuple_Pack(1, LongTensor); |
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.
Why does this line comment out?
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 have changed. Please check, thank you.
plugins/wasi_nn/neuralspeed.cpp
Outdated
} | ||
} | ||
Py_DECREF(Result); | ||
// Py_DECREF(GenerateArgs); |
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.
Ditto.
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 have changed. Please check, thank you.
test/plugins/wasi_nn/CMakeLists.txt
Outdated
elseif(BACKEND STREQUAL "neuralspeed") | ||
message( STATUS "Download ML artifacts to ${CMAKE_CURRENT_BINARY_DIR}/wasinn_neural_speed_fixtures") | ||
download( | ||
https://huggingface.co/TheBloke/Llama-2-7B-Chat-GGUF/resolve/main/llama-2-7b-chat.Q4_0.gguf |
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.
Do we really need to download this huge file if we would like to do the test?
Is it possible to have a small one instead?
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 have changed. Please check, thank you.
test/plugins/wasi_nn/wasi_nn.cpp
Outdated
uint32_t BuilderPtr = UINT32_C(0); | ||
uint32_t LoadEntryPtr = UINT32_C(0); | ||
uint32_t SetInputEntryPtr = UINT32_C(0); | ||
uint32_t OutBoundPtr = UINT32_C(61000 * 65536); |
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.
Should we use UINT32_C(61000) * UINT32_C(65536)
?
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 have changed. Please check, thank you.
Signed-off-by: grorge <[email protected]>
Signed-off-by: grorge <[email protected]>
Signed-off-by: grorge <[email protected]>
Signed-off-by: grorge <[email protected]>
Signed-off-by: grorge <[email protected]>
Signed-off-by: grorge <[email protected]>
Signed-off-by: grorge <[email protected]>
Signed-off-by: grorge <[email protected]>
Signed-off-by: grorge <[email protected]>
Signed-off-by: grorge <[email protected]>
Signed-off-by: grorge <[email protected]>
Signed-off-by: grorge <[email protected]>
Signed-off-by: grorge <[email protected]>
Signed-off-by: grorge <[email protected]>
Signed-off-by: grorge <[email protected]>
Signed-off-by: grorge <[email protected]>
Signed-off-by: grorge <[email protected]>
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.
And I think we should also add the workflow to build and upload the assets.
plugins/wasi_nn/neuralspeed.cpp
Outdated
return ErrNo::InvalidEncoding; | ||
} | ||
if (Doc.at_key("model_type").error() == simdjson::SUCCESS) { | ||
std::string_view model_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.
ModelType
.
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 have changed it.
Signed-off-by: grorge <[email protected]>
No description provided.