Closed apepkuss closed 1 year ago
Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.
Overall Summary:
The pull request titled "[Refactor] Update GraphEncoding
and ExecutionTarget
in wasmedge-sdk::plugin
" introduces several changes to the codebase. The key changes include adding new variants to the GraphEncoding
and ExecutionTarget
enums, updating the implementations of Display
and FromStr
for the GraphEncoding
enum, and adding a conditional import.
While most of the changes are straightforward, there are a few potential issues and errors that need to be addressed. These include a typo in the formatting of a variant in the NNBackend
enum, the need for additional changes to fully support the new variant NNBackend::TensorFlow
, outdated comments and code in the NNPreload
struct, and incomplete error handling in the implementations of FromStr
for both enums.
It is important to address these issues and errors before merging the pull request.
Key changes:
NNBackend::TensorFlow
in the NNBackend
enum.NNBackend::ONNX
in the NNBackend
enum.fmt
implementation for NNBackend
to include the new variants.Potential problems:
NNBackend
enum, the variant NNBackend::TensorFlowLite
is formatted as "TensorflowLite" instead of "TensorFlowLite". This might be a typo.NNBackend::TensorFlow
might require additional changes elsewhere in the codebase to fully support it.Overall, the changes seem to be straightforward and introduce new options for the NNBackend
. However, the typo in the formatting and the potential impact of the new variant should be addressed.
Key changes:
use wasmedge_types::error::WasmEdgeError
to the imports.NNBackend
enum to GraphEncoding
.OpenVINO
, ONNX
, TensorFlow
, Autodetect
, and GGML
.Display
for GraphEncoding
enum.FromStr
for GraphEncoding
enum.AUTO
to ExecutionTarget
.Potential problems:
NNBackend
in the code has not been updated to reflect the changes to the enum.FromStr
for GraphEncoding
enum should return an error if the input string does not match any of the enum variants.FromStr
for ExecutionTarget
enum should return an error if the input string does not match any of the enum variants.NNPreload
struct still uses the old NNBackend
enum instead of the new GraphEncoding
enum. This should be updated.Overall, the key changes involve renaming and updating the enums NNBackend
and ExecutionTarget
. Some comments and code in the NNPreload
struct need to be updated accordingly. Additionally, the implementations of FromStr
for both enums should handle unknown input strings correctly by returning an error.
Key changes:
wasmedge_types::error::WasmEdgeError
is conditionally used when the feature flag wasi_nn
is enabled.Potential problem:
In this PR, refactor
GraphEncoding
andExecutionTarget
types to keep their names consistent with the counterparts defined in wasi-nn crate (v0.6.0).In addition, support for the conversion from Strings to Enum variants.