second-state / WasmEdge-WASINN-examples

Apache License 2.0
217 stars 35 forks source link

[CI] llama: add more CI jobs #102

Closed dm4 closed 2 months ago

dm4 commented 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.


Commit be7b374b4c52e9d229afa50277643e8418ef5429

Key Changes:

  1. Three new jobs have been added to the CI workflow for llama: Gemma 2B, Llava v1.5 7B, and llama2 7b. These jobs involve building and running different components of the project with specific configurations and models.
  2. The Gemfile job involves building Gemma 2B with specific models for question answering.
  3. The Llava v1.5 7B job involves building Llava version 1.5 with specific models and image loading.
  4. The llama2 7b job involves building the llama component with specific configurations related to the assistant's behavior and answers.

Potential Problems:

  1. The patch includes additions to the CI workflow without clearly explaining the necessity or purpose of these new jobs. It's important to have clear documentation for the new CI jobs to understand their significance.
  2. The changes seem to introduce specific hardcoded text prompts and models for different components. This could be a potential problem if these hardcoded values are not suitable for all scenarios or need to be customized based on different use cases.
  3. The introduction of new jobs means an increase in complexity and potential failure points in the CI workflow. It's crucial to ensure that these new jobs are well-tested and won't negatively impact the existing CI pipelines.
  4. The use of $ in some command strings may lead to unwanted interpretations or errors if not handled correctly, especially in multiline strings.
  5. The patch modifies files outside of the .github directory (wasmedge-ggml/llava/src/main.rs). Any changes to source code outside of CI configurations should be thoroughly reviewed to ensure they align with the purpose of the PR and do not introduce unintended side effects.

Overall, the PR introduces new CI jobs to the workflow, but care should be taken to ensure clarity, flexibility, and stability in the changes made.