second-state / WasmEdge-WASINN-examples

Apache License 2.0
217 stars 35 forks source link

[Example] ggml: update command-r example to use command-r-plus with tool use prompt #127

Closed dm4 closed 1 month ago

dm4 commented 1 month ago
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.


Potential Issues and Errors:

Key Findings:

Overall, the PR covers a broad spectrum of updates, but it requires careful consideration of naming conventions, external dependencies, resource management, error handling, and documentation to ensure the reliability and coherence of the changes. Testing and review should focus on addressing these potential issues and enhancing the clarity and robustness of the modifications.

Details

Commit 892dd094a0a765cb8eef8d30724b9def2babf2ec

Key Changes:

  1. Updated the README.md file to reflect using the c4ai-command-r-plus-GGUF model instead of the previous one.
  2. Changed the download links for the new model files.
  3. Added sections in the README for tool prompt and tool actions using JSON.
  4. Updated the main.rs file to include the new system tool prompt and system instruction prompt.
  5. Adjusted the formatting of prompts and user interactions in the main.rs file.

Potential Problems:

  1. The naming convention for the model (c4ai-command-r-plus-GGUF) and related files should be consistent throughout.
  2. It might be beneficial to ensure proper spelling and grammar in the README for clarity and professionalism.
  3. The length of the tool prompt and instruction prompt in main.rs could be a bit overwhelming for users, consider simplifying where possible.
  4. The extensive use of newlines and formatting in the patches could lead to unintended formatting issues, so it would be worth testing these changes thoroughly once integrated.
  5. Verify that the new prompts and actions align with the expected functionality and user experience goals of the project.

Commit 9d343bf21326e928fb52bf3c3989b6f706ff07c4

Key Changes:

  1. Added a new job in the llama.yml workflow file titled "Embedding Example (Llama-2)" which includes the steps for downloading a file, building with cargo, and running a wasm file with specific parameters.
  2. Renamed an existing job from "Embedding Example" to "Embedding Example (All-MiniLM)" to differentiate it from the new job.

Potential Problems:

  1. The new job "Embedding Example (Llama-2)" involves downloading a file from an external source using curl. This introduces a dependency on an external endpoint that may change or become unavailable, impacting the reproducibility of the workflow.
  2. The script in the new job is hardcoded with certain parameters like the file name ("llama-2-7b-chat.Q5_K_M.gguf") and the input string ("hello world"). These hardcoded values might need to be parameterized or made more configurable for better flexibility.
  3. The job seems to be building and running wasm files, which can be resource-intensive operations. Ensuring proper resource allocation and cleanup after these operations is important to avoid resource leaks or unexpected behavior in the CI environment.
  4. It would be helpful to include comments or documentation to explain the purpose and expected outcome of each job to aid future maintainers of the workflow.

Overall, the addition of a new job for embedding testing is a significant change and should be carefully reviewed to ensure the reliability and maintainability of the workflow.

Commit a9d8b71009f9006dcb664b7b05d4bc51fe4382af

Key changes in the Pull Request:

  1. Added a new test called "Llama2 7B (Streaming)" to llama.yml workflow, which utilizes the llama-stream code.
  2. Updated the main.rs file to include a new function get_options_from_env() to manage environment variables.
  3. Added code to process a third argument as the prompt and run in non-interactive mode.

Potential problems or suggestions:

  1. In the new test "Llama2 7B (Streaming)", the command cargo build --target wasm32-wasi --release may fail if the necessary Cargo dependencies are not properly resolved.
  2. The use of unwrap() in handling JSON serialization/deserialization in get_options_from_env() can lead to panics if the values are not valid JSON.
  3. The conditional logic in main.rs for processing a third argument could be further optimized to handle potential edge cases or invalid inputs gracefully.
  4. The new test and changes in main.rs lack comprehensive documentation/comments explaining the rationale behind the modifications.
  5. The binary patch in the wasm file seems out of place and warrants further investigation to ensure its integrity and relevance to the PR.