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 FromStr trait for NNPreload in wasmedge-sdk #81

Merged
merged 2 commits into from
Oct 26, 2023

Conversation

apepkuss
Copy link
Collaborator

In this PR, implement the FromStr trait for the NNPreload type. The changes in this PR fixed #79.

Copy link
Member

juntao commented Oct 25, 2023

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


Summary:

The patch adds the implementation of the FromStr trait for the NNPreload struct in the wasmedge-sdk crate, along with the necessary unit test and supporting functions. It also includes a code change to check the format of the input string and return an error if it does not meet the expected format.

The potential issues found in the patch include:

  1. The unit test only covers valid preload strings, so it would be beneficial to add tests for invalid preload strings to ensure proper error handling.
  2. The from_str function assumes a preload string will always have four parts separated by :, and it will panic if a different format is passed. It would be better to handle invalid formats gracefully and return an error instead.

The most important finding is that the patch adds input validation to the from_str function for NNPreload, ensuring that the expected format is followed and returning an error if it is not.

Overall, the changes appear to be reasonable, but it is recommended to address the potential issues mentioned above to improve the code's robustness and error handling.

Details

Commit 74a49e126278beb235238b9dd83d931b9d76b9a7

Key changes in the patch:

  • Added implementation of the FromStr trait for the NNPreload struct in the wasmedge-sdk crate.
  • Added a unit test for the FromStr implementation.
  • Added the from_str function to parse a preload string into the NNPreload struct.
  • Added the nn_preload function to initialize the wasi_nn plug-in with preloads.

Potential problems:

  • The unit test for parsing a preload string only covers valid preload strings. It would be beneficial to also have tests for invalid preload strings to ensure proper error handling.
  • The from_str function assumes a preload string will always contain four parts separated by :. If a preload string with a different format is passed, the function will panic. It would be better to handle invalid format gracefully and return an error.

Commit 444e8d98d40ea92fb1ef09e3da1539715ef61ad2

Key changes:

  • Added code to check the format of the input string preload.
  • If the input string does not have 4 parts separated by ":" or if the parsing of backend or target fails, an error is returned.
  • Added a new test case to check the error message for an invalid preload string.

Potential problems:

  • None found.

Overall, the changes appear to be reasonable and add input validation to the from_str function for NNPreload.

@apepkuss apepkuss requested a review from L-jasmine October 25, 2023 12:28
@apepkuss
Copy link
Collaborator Author

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

@apepkuss apepkuss force-pushed the feat-impl-fromstr-for-nnpreload branch from 8ad6457 to 444e8d9 Compare October 26, 2023 06:49
@apepkuss apepkuss merged commit 3146e89 into WasmEdge:main Oct 26, 2023
@apepkuss apepkuss deleted the feat-impl-fromstr-for-nnpreload branch October 26, 2023 07:54
@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.

Leaky abstraction about preload function
3 participants