search

second-state / WasmEdge-WASINN-examples

Apache License 2.0
217 stars 35 forks source link

[Example] ggml: add model not found test #130

Closed dm4 closed 3 weeks ago

dm4 commented 1 month ago

To add ModelNotFound error to wasmedge-wasi-nn and use it in our WasmEdge WASINN examples, we need:

In this PR, I also update the wasmedge-wasi-nn to the latest version 0.7.1 and add the unload test for testing unload and reload the graph multiple times.

juntao commented 1 month ago

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

Based on the individual summaries provided for each code patch in the GitHub Pull Request titled "[Example] ggml: add model not found test," several potential issues and important findings have been identified:

Potential Issues and Errors:

  1. Missing Explanation: Lack of detailed explanation about the purpose of the added tests, updates, and changes within the PR and commit messages, which could lead to confusion for reviewers and future developers.
  2. Error Handling and Clarity: Inadequate error handling and panic messages in certain code sections, potentially hindering debugging and troubleshooting efforts.
  3. File Permissions: Potential unnecessary file permissions set to execute a script, which should be confirmed and adjusted if not required.
  4. Dependency Compatibility and Testing: The update to the wasmedge-wasi-nn dependency version across multiple components may not have been thoroughly tested for compatibility and functionality validation.

Important Findings:

  1. Introduction of new tests, dependencies, and functionalities in the wasmedge-ggml project, requiring comprehensive testing to ensure stability and correctness.
  2. Potential issues with version control and diffs due to the use of binaries in the code changes.
  3. External file dependencies and hardcoded values in the script that may lead to inconsistencies or network-related failures during test execution.

In conclusion, while the changes in the PR appear to enhance the wasmedge-ggml project, addressing the potential issues related to missing explanations, error handling improvements, file permissions, and thorough testing for dependency updates is crucial before merging the code modifications to maintain the project's integrity and reliability.

Details

Commit 5837fba9b5dd53e063742d13139cd8e30698a5d6

Key Changes:

  1. Added a test for model not found in the model-not-found directory.
  2. Created new files: Cargo.toml, README.md, src/main.rs, and wasmedge-ggml-model-not-found.wasm.
  3. Updated the .github/workflows/llama.yml to include the new test execution.

Potential Problems:

  1. Missing Explanation: The purpose of the wasmedge-ggml-model-not-found test is not well-explained in the PR or the commit message. It would be helpful to provide more context on why this test was added.

  2. Error Handling: In the src/main.rs, the error handling might not be comprehensive. The panic messages could be improved for better clarity and debugging in case of failures.

  3. File Permissions: Confirm if setting the file mode to 100755 for the wasmedge-ggml/test/model-not-found/wasmedge-ggml-model-not-found.wasm is necessary as it can execute as a script. Consider changing it to 644 if execution permission is not required.

Overall, the changes look good, but the potential problems should be addressed before merging the pull request.

Commit eef9036d59f536209829eda590773f3fbfd29efb

Key Changes:

  1. Updated the dependency wasmedge-wasi-nn version from 0.7.0 to 0.7.1 in multiple Cargo.toml files across different directories.
  2. The patch is focused on updating the wasmedge-wasi-nn dependency in different components of the wasmedge-ggml project.

Potential Problems:

  1. Dependency Compatibility: Ensure that the new version 0.7.1 of wasmedge-wasi-nn is compatible with all the components within the wasmedge-ggml project. This update may introduce new issues if not thoroughly tested.
  2. Testing: While the update is localized to a specific dependency version change, it is crucial to run tests after this update to verify that all functionalities behave as expected.
  3. Consistency: Check if the version change to 0.7.1 is intended across all components or if there are any discrepancies in the intended versions.

Overall, the patch appears to be a routine update to a dependency version, but verification through testing is recommended to ensure the stability and correctness of the wasmedge-ggml project after the update.

Commit 4a38db92521e58bd63424447127c97f36da234a2

Key Changes:

  1. Added a new test for unloading and reloading the graph multiple times in the ggml library.
  2. Created new files for the test, including Cargo.toml, README.md, main.rs, and the test wasm binary.
  3. Updated the GitHub workflow to include the new unload test.
  4. The main.rs file includes functions for setting options, retrieving data from the context, and unloading the graph after inference.

Potential Problems:

  1. The patch introduces a significant amount of new code, which may not have been thoroughly tested.
  2. The use of binaries in the patch (wasmedge-ggml/test/unload/wasmedge-ggml-unload.wasm) could lead to potential issues with version control and diffs.
  3. The script in the GitHub workflow for the unload test downloads a file from an external URL (https://huggingface.co/second-state/Llama-2-7B-Chat-GGUF/resolve/main/llama-2-7b-chat.Q5_K_M.gguf), which could cause dependencies or network-related problems.
  4. The test script includes a long string that seems to be a prompt for inference. It might be better to have this as a configurable parameter rather than hardcoded in the script.