Skip to content

Conversation

Copy link
Member

juntao commented Dec 24, 2024

Hello, I am a code review agent on flows.network. Here are my reviews of changed source code files in this PR.


.github/workflows/llama.yml

Potential issues

  1. Hardcoded Model Paths and URLs: The script includes hardcoded URLs for downloading models, which can become stale or unavailable if the source changes. Consider using environment variables or a configuration file to manage these paths.

  2. Lack of Error Handling in Downloads: There is no error handling for the curl commands used to download model files. If a download fails, the script will continue executing without any indication that the required models are missing, leading to potential runtime errors.

  3. Environment Variable Dependency Without Validation: The scripts rely heavily on environment variables like $NGL, but there is no validation or default values set for these in the workflow file, which can lead to unexpected behavior if the environment is not properly configured.

Summary of changes

  • Key Changes:
  • Updated the wasmedge version in the job matrix from ["0.13.5", "0.14.0"] to ["0.14.1"].
  • Added a new job step for the Qwen2-VL example, including downloading necessary files and running a specific command with WasmEdge.
  • Introduced a new command to test the Qwen2-VL model with an image input (monalisa.jpg).

wasmedge-ggml/qwen2vl/Cargo.toml

Potential issues

  1. Hardcoded Dependency Versions: The versions of serde_json and wasmedge-wasi-nn are hardcoded, which can lead to potential version conflicts or missed security updates. Consider using a range specifier for better flexibility.

  2. Lack of Version Management Tools: There is no mention of dependency management tools like cargo-edit, which could help manage dependencies more efficiently. Adding a toolchain section or documentation on managing dependencies might be beneficial.

  3. Missing Documentation: The [package] metadata lacks descriptions or any form of documentation, making it difficult for other developers to understand the purpose and usage of the package. Consider adding a description field in the Cargo.toml.

Summary of changes

  • • Added a new Cargo.toml file for a package named wasmedge-ggml-qwen2vl.
    • Specified the edition as 2021.
    • Included dependencies on serde_json and wasmedge-wasi-nn.

wasmedge-ggml/qwen2vl/src/main.rs

Potential issues

  1. Issue with Optional Parameters Parsing: The get_options_from_env function uses serde_json::from_str(val.as_str()).unwrap() which can panic if the environment variable value is not valid JSON. This should be replaced with a fallible parsing method to handle invalid inputs gracefully.

  2. Redundant Metadata Output: In the non-interactive mode, the number of input tokens is printed twice from get_metadata_from_context before and after calling context.compute(). This redundancy should be removed to avoid confusion.

  3. Inconsistent Prompt Formatting for Qwen2VL: The prompt formatting logic in the interactive loop does not include the system prompt and image placeholder after the first user message, which might lead to incorrect prompt construction for subsequent messages. Ensure the prompt format remains consistent with the initial setup.

Summary of changes

  • Key Changes:
  • Addition of Qwen2-VL Example: New Rust code to implement an example application for the Qwen2-VL model using WASI-NN and ggML.
  • Environment Variable Parsing: Introduced get_options_from_env function to parse required and optional parameters from environment variables.
  • Interactive Chat Loop: Implemented a loop for interactive chat mode, handling user inputs and resetting context when necessary.

wasmedge-ggml/qwen2vl/wasmedge-ggml-qwen2vl.wasm

Potential issues

  1. Memory Corruption in A Array Access: The code accesses and manipulates the A array using various indices that are not validated, which can lead to out-of-bounds access and memory corruption.

  2. Uninitialized Variables: Several variables, such as j in loops or as function parameters, appear to be used without being initialized, leading to undefined behavior.

  3. Hardcoded Offsets and Sizes: The code contains hardcoded offsets and sizes for array accesses, which can lead to maintenance issues and bugs if the underlying data structures change.

Summary of changes

  • • Added a new example directory for Qwen2-VL, demonstrating integration or usage with ggml.
    • Included necessary files and configuration to run the Qwen2-VL model using ggml.
    • Updated documentation or README within the project to include information about the new Qwen2-VL example.
@dm4 dm4 force-pushed the dm4/qwen2vl branch 2 times, most recently from fc753ac to 92ea4d4 Compare December 25, 2024 07:30
@dm4 dm4 marked this pull request as draft December 25, 2024 08:03
@dm4 dm4 marked this pull request as ready for review January 2, 2025 08:36
@dm4 dm4 force-pushed the dm4/qwen2vl branch 2 times, most recently from 8f014ea to 7e97b6d Compare January 7, 2025 12:21
Signed-off-by: dm4 <sunrisedm4@gmail.com>
@hydai hydai merged commit bc5a061 into master Jan 9, 2025
47 checks passed
@hydai hydai deleted the dm4/qwen2vl branch January 9, 2025 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants