second-state / WasmEdge-WASINN-examples

Apache License 2.0
220 stars 36 forks source link

[Example] ggml: add CI for phi-3, yi-1.5-9b #143

Closed dm4 closed 2 weeks ago

juntao commented 3 weeks 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:

  1. File Deletion and Renaming: The deletion of files and renaming could lead to referencing issues if not handled carefully.
  2. Test Configurations Complexity: The introduction of new test configurations should be well-documented and validated for necessity.
  3. Environment Variables Compatibility: Changes in environment variables need thorough review to ensure compatibility with existing setups.
  4. Dependency Risks: Verify stability and security of URLs used for external resources in tests.
  5. Build Process Impact: Changes to build process and file references may impact the overall pipeline; ensure they do not break the process.
  6. Removed CI Runner Option: The removal of macos-13 without clear explanation should be validated.
  7. Plugin Update Compatibility: Verify compatibility of the updated plugin wasi_nn-ggml-b2963 with the existing setup.
  8. Binary Changes: Binary changes may complicate code review and understanding for the wasmedge-ggml-chatml.wasm file.
  9. Functionality Changes: Functional changes related to testing and workflow improvements should align with project requirements.
  10. Hardcoded Values: Use of hardcoded values for WasmEdge installation script should be parameterized for future flexibility.
  11. Descriptive Commit Messages: Enhance commit messages to clarify the rationale behind changes for better team understanding.

Important Findings:

Overall, the patch introduces important enhancements to testing, CI workflows, and functionality, but thorough review, validation, and testing are essential to prevent any unforeseen complications or regressions. Consider addressing potential issues and errors highlighted for a smoother integration process.

Details

Commit f287a5329e36e9ec4563d835fafb0b50fac3abf1

Key Changes:

Potential Problems:

  1. File Deletion: The deletion of wasmedge-ggml/test/phi-3-mini/wasmedge-ggml-phi-3-mini.wasm and renaming of related files could cause issues if referenced elsewhere in the project.

  2. Test Configurations: The addition of new test configurations introduces complexity. Ensure that they are necessary and well-documented.

  3. Environment Variables: Changes in environment variables like n_gpu_layers="$NGL" should be carefully reviewed to ensure compatibility with existing setups.

  4. Resource URLs: The URLs used for downloading external resources in the tests can be dependency risks. Check for dependencies and ensure they are stable and secure.

  5. Build Process: Changes to the build process and file references may affect the overall build pipeline. Verify that the changes do not break the build process.

Overall, the patch seems to introduce important changes related to testing phi-3 models, but it should be reviewed thoroughly to ensure it does not introduce unexpected issues.

Commit 9c7a1ec585e3b2b64a373e31391b15b644e1d720

Key changes:

Potential problems:

  1. It seems like the macos-13 option has been removed without a clear explanation. This change should be verified to ensure it does not introduce any unexpected issues with the CI workflow.
  2. The update to the plugin wasi_nn-ggml-b2963 should be validated to confirm compatibility with the existing setup and any dependencies. Testing with this new plugin version is recommended to prevent any integration issues.

Overall, the changes seem straightforward, but it's essential to review them thoroughly to prevent any unforeseen complications in the CI workflow.

Commit 9d2b0dce32be248b11400302bcd49467681a7a05

Key Changes:

  1. Updated the chatml example for better continuous integration testing.
  2. Added two new jobs in the CI workflow for "Yi 1.5 9B 16K" and "Yi 1.5 9B" with specific configurations and commands.
  3. Refactored the main.rs file in the chatml directory to include a new function get_options_from_env to set options based on environment variables.
  4. Changed the way options for the graph are set in the main.rs file using the get_options_from_env function.
  5. Added handling for a third argument in the main function to use as a prompt in non-interactive mode, mainly for the CI workflow.

Potential Problems:

  1. The patch includes binary changes to the wasmedge-ggml-chatml.wasm file, making it harder to review the specific code modifications.
  2. The removal of commented-out code related to metadata retrieval in the main.rs file may have been intentional, but it's worth confirming.
  3. The update seems to focus on testing and workflow improvements, so functionality changes should be verified to ensure they align with the project requirements.
  4. It's essential to check if the new configurations and commands in the CI jobs are correctly set up and do not introduce any issues.
  5. The additional changes made to the main.rs file need thorough testing to ensure they do not introduce any regressions in the chatbot functionality.

Commit 1f4584bc0eefc8605c8be838d54f63a2633ee1c6

Key Changes:

  1. Updated the GitHub Actions workflow file (.github/workflows/llama.yml) to use the latest plugin version with both WasmEdge 0.13 and 0.14.
  2. Modified the job configuration to specify WasmEdge versions 0.13.5 and 0.14.0.
  3. Updated the job configurations to include specific versions of WasmEdge for each runner (ubuntu-20.04 and macos-m1).

Potential Problems:

  1. It seems like there might be a mistake in the matrix definition for the 'plugin' field. It was changed to 'wasi_nn-ggml', but it was previously 'wasi_nn-ggml-b2963'. This change should be verified if the correct plugin is being used.
  2. The use of hardcoded values in the script to install WasmEdge can lead to potential issues if the version numbers change in the future. Consider parameterizing version numbers to ensure flexibility.
  3. The addition of the 'wasmedge' field in the matrix might have introduced a typo in the final job name concatenation. Verify that the correct values are being displayed in the job name.
  4. It would be beneficial to include more descriptive commit messages explaining the rationale behind these changes for better understanding by other team members.