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

Bump llama cpp b1656 #3095

Merged
merged 2 commits into from Dec 20, 2023
Merged

Bump llama cpp b1656 #3095

merged 2 commits into from Dec 20, 2023

Conversation

hydai
Copy link
Member

@hydai hydai commented Dec 18, 2023

No description provided.

@hydai hydai requested a review from dm4 December 18, 2023 04:34
Copy link
Member

juntao commented Dec 18, 2023

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.


The pull request titled "Bump llama cpp b1656" comprises majorly of two sets of changes. The first set pertains to the update to the llama library which has been modified in the plugins/wasi_nn/CMakeLists.txt file from b1616 to b1656. The chief intent behind this was to potentially leverage new features or rectify existing bugs. However, the outlined changes might trigger potential issues such as incompatibility due to breaking changes in the updated versions, errors introduced due to unmodified application of ggml.patch, potential implications on the runtime, or the build process which necessitates thorough testing and difficulties in debugging due to incomplete git history due to GIT_SHALLOW being set to TRUE.

The second set of changes pertain to device-specific configurations for an AI model parsing function in the WASI-NN plugin with a focus on enhanced compatibility with macOS devices. The code has been modified to enforce Metal (Apple's hardware-accelerated graphics API) on macOS by setting the number of GPU layers to 1 and moving the hack workaround into the non-Apple devices section. However, the change could incur drawbacks such as the lack of checking for the actual availability of Metal API on macOS, inconsistencies due to behavior changes based on the platform, or suboptimal settings of GPU layer on macOS for certain use cases.

The overall modifications seem straightforward, however, in light of the potential implications, it is recommended to meticulously review and test the changes and offer configuration options for end-users for enhanced source code maintainability.

Details

Commit 47ec62392e44137b4f4e63d06414be0a04f78878

The key change in this patch is an update to the version of the llama library used by the WASI-NN plugin's ggml backend. Specifically, the GIT_TAG in the plugins/wasi_nn/CMakeLists.txt file has been updated from b1616 to b1656. This essentially means that the build system will fetch a newer version of the llama library when setting up the project dependencies. This is presumably done to take advantage of new features or bug fixes in the updated version.

However, there are a few potential problems:

  1. Potential Incompatibility: The update from b1616 to b1656 could introduce incompatibilities if there were breaking changes in the updated versions.

  2. Testing: This change might have impacts in the runtime or build process, it is advisable to perform thorough testing to ensure that the new version doesn't introduce any bugs or unintended side effects.

  3. Patch File: The patch file ggml.patch is applied without any changes. If there are any changes in the newer version of llama that affect the parts of the code that ggml.patch is modifying, this could potentially introduce errors or unexpected behavior.

  4. GIT_SHALLOW is set to TRUE, which means that git history is not complete, it only gets the latest snapshot of the specified version. This makes debugging harder if something goes wrong in the future because you don't have the full git history.

Overall, this change appears to be straightforward but potential implications make it necessary to carefully review and test the change before merging it into the main codebase.

Commit 77805792903bc9ccf43e1c3f29e0a22e39f78b07

The patch for the file ggml.cpp appears to be implementing device-specific configurations for an AI model parsing function in the WASI-NN plugin. The author is trying to adjust the behavior of the code depending on whether it is run on macOS or not.

Key Changes:

  1. The developer has added a device check for Apple devices where the number of GPU layers (GraphRef.NGPULayers) is set to 1, signifying that Metal (Apple's hardware-accelerated graphics API) is forced enabled on macOS.

  2. The code documentation mentioning the hack workaround was moved into the non-Apple section.

  3. Expanded the commentary about the workaround, making clear that it is for non-macOS devices. In addition to the original statement about the limitation of the WASI-NN proposal, the comment now warns other developers not to use this approach if the model parameters are updated in the Config stage to avoid reloading the model.

Potential Problems:

  1. Not checking for the actual availability of Metal API on macOS. Just because the OS is macOS, it doesn't mean Metal API is available. Old Mac machines may not support it.

  2. There's a potential case where this could cause behavior changes based on the platform which could lead to inconsistencies if not properly managed.

  3. The forced set-up for 1 GPU layer on macOS might not be the most optimal setting for all use cases.

Finally, as a reviewer, I would suggest that a better practice would be to offer configuration options for the end-user, rather than hardcoding the settings in the source code, providing the user with better control over their system settings. This would require a more substantial code change but would generally be a more maintainable solution.

@github-actions github-actions bot added the c-Plugin An issue related to WasmEdge Plugin label Dec 18, 2023
dm4
dm4 previously approved these changes Dec 18, 2023
@dm4 dm4 self-requested a review December 18, 2023 05:54
plugins/wasi_nn/ggml.cpp Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f576ba7) 80.84% compared to head (7780579) 80.84%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3095      +/-   ##
==========================================
- Coverage   80.84%   80.84%   -0.01%     
==========================================
  Files         159      159              
  Lines       23035    23035              
  Branches     4734     4734              
==========================================
- Hits        18623    18622       -1     
  Misses       3131     3131              
- Partials     1281     1282       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: hydai <z54981220@gmail.com>
Signed-off-by: hydai <z54981220@gmail.com>
@hydai hydai merged commit cc038b4 into master Dec 20, 2023
49 of 50 checks passed
@hydai hydai deleted the hydai/bump_llama_cpp_b1656 branch December 20, 2023 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-Plugin An issue related to WasmEdge Plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants