second-state / WasmEdge-WASINN-examples

Apache License 2.0
217 stars 35 forks source link

[Example] ggml: add multimodel example with CI #111

Closed dm4 closed 2 months ago

juntao commented 2 months ago

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


Potential Issues and Errors:

Most Important Findings:

Overall, while the changes bring improvements, it is crucial to address the identified issues and ensure that modifications are intentional and do not compromise security or functionality. Additional details and clarity in commit messages would also benefit future reference and understanding.

Details

Commit 2a1357242b0ed58adf99bad1d50e97e96e446e7c

Key Changes:

  1. Added a new multimodel example with Continuous Integration (CI) to the wasmedge-ggml repository.
  2. Added new files Cargo.toml, README.md, and src/main.rs under the wasmedge-ggml/multimodel directory.
  3. Updated the .github/workflows/llama.yml workflow file to include a step for the new "Multiple Models Example".
  4. The README.md file explains the use of the Llama2 and Llava models for chaining model results in the WASI-NN environment.

Potential Problems:

  1. Security Risk: The patch includes the curl commands to download files directly from external sources (e.g., huggingface.co, github.io). Fetching content from external URLs could potentially introduce security risks if not validated.

  2. No Error Handling in main.rs: The main.rs file lacks extensive error handling for various operations like file reading, environment variable retrieval, or WASI-NN operations, which could lead to runtime issues.

  3. Hardcoded Paths: The paths to models and images are hardcoded in the script within llama.yml. This may cause issues in different environments.

  4. Incomplete Error Handling: The handling of errors in get_options_from_env() function is basic and could be improved for better user feedback.

  5. Magic Numbers: In src/main.rs, there are magic numbers like 4096 * 6 which may not be clear to maintainers. Consider using named constants for clarity.

  6. Consistency in Option Names: The options in set_metadata_to_context have different naming conventions (data, metadata) compared to other functions. It would be beneficial to maintain consistency.

Overall, the additions seem to enhance the repository with a new useful example, but the concerns above should be addressed for robustness and security.

Commit 4a8387a772893b1b5578f8994f9be116b18977c8

Key Changes:

  1. Updated README for multimodel example in the wasmedge-ggml/multimodel directory.
  2. Updated the command in the README to remove the default argument from the wasmedge-ggml-multimodel.wasm invocation.

Findings:

  1. Important: The command in the README was modified to remove the default argument in the wasmedge-ggml-multimodel.wasm invocation. This change could potentially impact the expected behavior of the example. It's important to ensure that the removal of default is intentional and doesn't cause any issues.
  2. Minor: The commit message could be more descriptive to provide better context about the changes being made. A more detailed description would be beneficial for future reference and understanding.
  3. It's advisable to have more information on why the change was made, especially the reasoning behind removing the default argument. This context would help reviewers and contributors better understand the intent behind the modification.

Overall, the changes seem straightforward, but it's essential to verify the impact of removing the default argument in the command for wasmedge-ggml-multimodel.wasm.

Commit 53bed1c6c075f4b742ac711bbdd3ad393117f73d

Key changes:

Potential problems:

  1. MacOS version change: Removing the macos-14 runner may impact the testing coverage for the project on that specific MacOS version. Ensure that this change was made intentionally and does not introduce any issues for MacOS-14 users.
dm4 commented 2 months ago

Let's use tinyllama + llava instead, to show that we can handle not only the chat model but also the image model.

Given that the performance of the tinyllama model is unsatisfactory, I will attempt to use the llava and llama2 models for this example.