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

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

Merged
merged 3 commits into from
Apr 23, 2024
Merged

Conversation

dm4
Copy link
Member

@dm4 dm4 commented Apr 16, 2024

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

  • Merge PR in wasmedge-wasi-nn
  • Merge PR in WasmEdge runtime
  • Release the new wasmedge-wasi-nn crate
  • Update the WasmEdge WASINN examples to use the newer version of wasmedge-wasi-nn crate

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.

Copy link
Member

juntao commented Apr 16, 2024

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.

Signed-off-by: dm4 <dm4@secondstate.io>
@hydai hydai force-pushed the dm4/errno-model-not-found branch from 9395ff3 to ca92d07 Compare April 20, 2024 01:50
@dm4 dm4 force-pushed the dm4/errno-model-not-found branch from ca92d07 to 5837fba Compare April 22, 2024 07:10
dm4 added 2 commits April 23, 2024 16:22
Signed-off-by: dm4 <dm4@secondstate.io>
Signed-off-by: dm4 <dm4@secondstate.io>
@dm4 dm4 marked this pull request as ready for review April 23, 2024 08:32
@dm4 dm4 force-pushed the dm4/errno-model-not-found branch from 905a5af to 4a38db9 Compare April 23, 2024 09:35
@dm4 dm4 merged commit 7f67945 into master Apr 23, 2024
74 of 122 checks passed
@dm4 dm4 deleted the dm4/errno-model-not-found branch April 23, 2024 10:26
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.

None yet

3 participants