Skip to content
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

[Feat] Enable PluginManager::nn_preload #74

Merged
merged 7 commits into from
Sep 25, 2023

Conversation

apepkuss
Copy link
Collaborator

@apepkuss apepkuss commented Sep 25, 2023

In this PR, the PluginManager::nn_preload API in wasmedge-sdk is enabled. In addition, improve the original argument design.

The new design has been verified by the ggml-llama-via-wasinn example.

Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Copy link
Member

juntao commented Sep 25, 2023

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.


Overall Summary:

This pull request adds support for the nn_preload API in the PluginManager struct. The changes include enabling the nn_preload function, updating dependencies, refactoring code, and adding new structures and APIs.

There are a few potential issues and errors to address:

  1. The nn_preload function is not guarded by the cfg(feature = "wasi_nn") attribute, so it can be called even if the wasi_nn feature is not enabled. It should be wrapped in a conditional compilation attribute.
  2. The nn_preload function should be modified to return a Result type for error handling.
  3. The value returned by the WasmEdge_PluginInitWASINN function needs clarification with documentation or comments.
  4. The compatibility and impact of updating the rust-sys, wasmedge-sdk, and Cargo.toml versions should be thoroughly reviewed.
  5. The new structures and APIs added should be properly handled and reviewed for any potential issues, such as compatibility and accessibility.

These potential issues should be addressed before merging the pull request.

Details

Commit db23e7613fff03e4c7b8d61e135d7573830b0b79

Key Changes:

  • The patch enables the nn_preload function in the PluginManager struct of the wasmedge-sys crate.
  • The nn_preload function takes a vector of string references (&str) as its argument, converts them to CString objects, and then converts them to a vector of raw pointers (*const i8). It then passes the raw pointers to the WasmEdge_PluginInitWASINN function from the ffi module.
  • The nn_preload function is conditionally compiled using the wasi_nn feature.

Potential Problems:

  • None of the code in the nn_preload function is guarded by the cfg(feature = "wasi_nn") attribute. This means that the function can be called even if the wasi_nn feature is not enabled. The code should be wrapped in an cfg attribute to ensure that it is only compiled when the feature is enabled.
  • The nn_preload function does not return any value or provide any error handling mechanism. It should be modified to return a Result type to indicate success or failure.
  • It is unclear what value is returned by the WasmEdge_PluginInitWASINN function and how it should be handled. Some documentation or comments should be added to clarify this.
  • There is a commented-out version of the nn_preload function that was previously implemented. It should be removed from the codebase.

Commit 1edef87c77602c04e0e93a2bea7b917551aaf9f5

Key changes:

  • Bumped the version of rust-sys to 0.17.3 in the Cargo.toml file.

Potential problems:

  • No potential problems found in this patch.

Commit 5e4cb3bc2ace04c5032877e1b0f858c490259809

Key Changes:

  • The nn_preload method has been added to the PluginManager struct.
  • The method takes a vector of string references (&str) as the argument.
  • The method calls the nn_preload method of the PluginManager in the sys::plugin module.

Potential Problems:

  • There doesn't appear to be any potential problems with this change. The code looks correct and there are no obvious issues or concerns.

Overall, this pull request adds support for the nn_preload API in the Rust SDK's PluginManager struct. It seems to be a straightforward addition without any expected problems.

Commit 1a5dc480f158d6820a6e3e8cd6e91598e8674828

Key changes:

  • The version of the "wasmedge-sdk" in Cargo.toml file has been bumped from "0.12.2" to "0.12.3-dev".

Potential problems:

  • The new version "0.12.3-dev" may introduce compatibility issues with other dependencies or components of the project. It is important to ensure that these dependencies are also compatible and updated accordingly.
  • The changes in the SDK version may require changes in the codebase or other parts of the project that depend on it. It is crucial to verify that the code still compiles and functions as expected with the updated SDK version.

It would be helpful to review the commit history and the rest of the changes in the pull request to get a better understanding of the overall impact and reasoning behind this specific change.

Commit 22dd4cda86b7835a62048f1fa6923dfd59ac9f9e

Key Changes:

  • Added a new struct NNPreload to represent preloaded neural network models.
  • Added an enum NNBackend to represent different neural network backends.
  • Added an enum ExecutionTarget to represent where the graph should be executed.
  • Updated the nn_preload function in PluginManager to take a vector of NNPreload instead of a vector of strings.

Potential Problems:

  • The NNPreload struct and the associated enums and implementation appear to be specific to the wasi_nn feature. Ensure that the feature is properly handled in other parts of the codebase.
  • The to_string methods in NNPreload, NNBackend, and ExecutionTarget are currently private. This may limit their usability outside of the plugin module. Consider making them public if they need to be accessed from other parts of the codebase.

Commit 7e99c651a623565b906232b3b5455286c50302a2

Key Changes:

  • Refactored code in src/plugin.rs.
  • Updated the NNPreload struct to use std::path::PathBuf for the path field.
  • Implemented std::fmt::Display trait for the NNPreload, NNBackend, and ExecutionTarget enums.
  • Modified the PluginManager::nn_preload method to accept a vector of &str instead of a vector of String.

Potential Problems:

  • The change from Path to std::path::PathBuf for the NNPreload struct may introduce compatibility issues if the path field was previously used as a Path.
  • The PluginManager::nn_preload method now accepts a vector of &str instead of a vector of String. This change may require modifying how the method is called elsewhere in the codebase.
  • The refactored code may introduce new bugs or regressions that need to be thoroughly tested.

Commit 844f6231ae1d436a0ad13ad83b2fd45be563e8c4

Key changes:

  • Added a new API function NNPreload::new to the plugin.rs file.

Potential problems:

  • There don't seem to be any potential problems with this patch.

@apepkuss apepkuss requested a review from hydai September 25, 2023 05:37
@apepkuss
Copy link
Collaborator Author

@hydai Could you please help review this PR? Thanks a lot!

@apepkuss
Copy link
Collaborator Author

@hydai Thanks for the review!

@apepkuss apepkuss merged commit 373221d into WasmEdge:main Sep 25, 2023
34 checks passed
@apepkuss apepkuss deleted the feat/enbale-nn-preload branch September 25, 2023 11:13
@juntao juntao mentioned this pull request Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants