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] Implement new API PluginManager::nn_preload #69

Merged
merged 7 commits into from
Sep 21, 2023

Conversation

apepkuss
Copy link
Collaborator

In this PR, add new API nn_repload for PluginManager to support new C-API WasmEdge_PluginInitWASINN.

Copy link
Member

juntao commented Sep 21, 2023

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


Overall Summary:
Based on the individual summaries, there are several potential issues and errors that should be addressed before merging the pull request. These include:

  • Lack of parameter validation or checking for null values in the nn_preload API, which could lead to unexpected behavior or crashes.
  • Missing return type for the nn_preload API, which should be specified based on the expected behavior.
  • Lack of documentation or comments explaining the purpose and functionality of the new API.
  • Unclear usage and handling of the Vec<&str> input.
  • Unclear expected behavior if the wasi_nn feature flag is not enabled.
  • Potential missing context and documentation for changes made to PluginManager::nn_preload and the switch to a different function.

In addition, there are some less critical findings such as updates to crate versions and the Rust SDK version bump, which could require further clarification or justification.

Overall, it is recommended to address these potential issues and provide more comprehensive documentation and explanation for the changes before merging the pull request.

Details

Commit aa9b0ec1d06d0d998a620cf27a3094d9be4e734d

Key Changes:

  • Added a new API function nn_preload to the PluginManager struct.
  • The nn_preload function accepts a Vec<&str> parameter for preloading plugins.
  • The function converts the input strings to CString and then to *const i8 pointers.
  • The converted pointers are passed to the C function WasmEdge_ModuleInstanceInitWASINN for initialization.

Potential Problems:

  • None of the parameters, including preloads, are validated or checked for null values. This could lead to unexpected behavior or crashes if invalid or null values are passed.
  • The function is missing a return type. It should specify a return type based on the expected behavior (e.g., Result<(), Error>).
  • The function should have documentation, including details about its purpose, usage, and any preconditions or side effects.

Commit 126297b24659ce24812569bc7f4413d40fdf90fb

Key changes:

  • Added a new API PluginManager::nn_preload in the plugin.rs file.
  • The new API is conditional on the feature flag wasi_nn.
  • The new API takes a vector of string references (Vec<&str>) as input.

Potential problems:

  • It is unclear what the purpose of the nn_preload API is without further context.
  • There is no documentation or comments explaining the functionality of the new API.
  • It is not clear how the Vec<&str> input is expected to be used.
  • There could be potential issues with error handling or validation of the input vector.
  • It is unclear what the expected behavior is if the wasi_nn feature flag is not enabled.

Recommendations:

  • Request the author to provide a detailed explanation of the purpose and functionality of the nn_preload API.
  • Ask the author to add comments or documentation to describe the behavior and usage of the new API.
  • Insist on error handling and input validation for the Vec<&str> input, if applicable.
  • Consider requesting the author to handle the case when the wasi_nn feature flag is not enabled, or provide an explanation if it is intentionally left out.

Commit 54990d67e36e8b17cb68d3b44190063fc8e67ae6

Key changes:

  • The version of the rust-sys crate in the wasmedge-sys package is being updated from 0.17.1 to 0.17.2.

Potential problems:

  • There doesn't appear to be any potential problems with this patch. It simply updates the version of the rust-sys crate.

Commit 0b86bc9344d9a22db004c80d709142a431907cfe

Key changes:

  • The version of the wasmedge-sdk in the Cargo.toml file has been updated from 0.12.1 to 0.12.2.

Potential problems:

  • No potential problems were identified in this patch.

Commit 2030bbb5ea8130e02c0da2024c516adc3bb028ff

Key changes:

  • Updates the Rust version to 1.72 in the .github/workflows/ci-build.yml file.
  • Updates the Rust version to 1.72 in the .github/workflows/standalone.yml file.

Potential problems:

  • There doesn't seem to be any potential problems with these changes. It is a straightforward update of the Rust version in the GitHub workflows.

Commit 83906b12cbb3340c3c828f7da670dcdb8f963c7a

Key changes:

  • The method PluginManager::nn_preload has been updated to use the function WasmEdge_PluginInitWASINN instead of WasmEdge_ModuleInstanceInitWASINN.

Potential problems:

  • It is unclear why the change was made to use a different function. This change should be justified in the pull request description or in the code comments.
  • The pull request title states that a new API PluginManager::nn_preload is implemented, but the patch only shows a modification to an existing implementation. It's important to verify if the patch includes all the necessary changes to fully implement the new API.
  • The commit message and code comments are minimal. It would be beneficial to have more detailed explanations of the changes made.

Overall, it seems there may be some missing context and documentation in the pull request that should be addressed before merging.

Commit 723ec2e51eed2da0e6d6973c029fc071bcb33f61

Summary of key changes:

  • The version of the Rust SDK has been bumped from 0.12.2 to 0.12.2-dev.

Potential problems:

  • The new version, 0.12.2-dev, is a development version. It might not be suitable for production use.
  • There is no information provided about the reason for bumping the version or any changes included in this update. It would be helpful to include this information in the commit message or pull request description.
  • It is unclear if there are any API or compatibility changes in this new version. It would be important to verify if any adjustments are necessary for existing code that depends on this SDK.

@apepkuss apepkuss requested a review from L-jasmine September 21, 2023 06:29
@apepkuss
Copy link
Collaborator Author

@L-jasmine Could you please help review this PR? Thanks a lot!

@apepkuss
Copy link
Collaborator Author

@L-jasmine Thanks for the review!

@apepkuss apepkuss merged commit 9114794 into WasmEdge:main Sep 21, 2023
34 checks passed
@apepkuss apepkuss deleted the feat/support-new-capi branch September 21, 2023 08:59
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