Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
May 28, 2024
Merged

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

merged 4 commits into from
May 28, 2024

Conversation

dm4
Copy link
Member

@dm4 dm4 commented May 23, 2024

No description provided.

Copy link
Member

juntao commented May 23, 2024

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:

  • Testing Enhancements: Significant improvements have been made related to testing phi-3 models and chatbot functionality.
  • CI Workflow Updates: Updates to CI workflow configurations and plugin versions for enhanced testing and compatibility.
  • Refactored Code: The addition of new functions and refactoring in the codebase points towards better maintainability and scalability.
  • Specific Job Configurations: Specific job configurations for different versions and runners show robust testing strategies.
  • Verification and Testing: Thorough verification and testing are crucial to ensure changes do not introduce regressions and unexpected issues.

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:

  • The patch renames phi-3-mini to phi-3 and makes various changes related to testing different phi-3 models.
  • It updates references in .github/workflows/llama.yml and related test files.
  • It deletes the wasmedge-ggml-phi-3-mini.wasm binary and creates a new wasmedge-ggml-phi-3.wasm binary.
  • It adds new test configurations for Phi 3 Mini 128k and Phi 3 Medium 4k.

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:

  • The CI workflow file llama.yml has been modified to update the plugin from wasi_nn-ggml to wasi_nn-ggml-b2963.
  • The macos-13 option for the runner has been removed, and only macos-m1 is retained as a runner option.

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.

dm4 added 2 commits May 23, 2024 17:04
Signed-off-by: dm4 <dm4@secondstate.io>
@dm4 dm4 changed the title [Example] ggml: rename phi-3-mini to phi-3, test more phi-3 models [Example] ggml: add CI for phi-3, yi-1.5-9b May 24, 2024
Signed-off-by: dm4 <dm4@secondstate.io>
@dm4 dm4 marked this pull request as ready for review May 24, 2024 08:33
@dm4 dm4 requested a review from hydai May 27, 2024 08:07
@dm4 dm4 merged commit 654ffc5 into master May 28, 2024
97 checks passed
@dm4 dm4 deleted the dm4/phi-3 branch May 28, 2024 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants