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] simplify llama examples #92

Merged
merged 4 commits into from
Feb 5, 2024
Merged

[Example] simplify llama examples #92

merged 4 commits into from
Feb 5, 2024

Conversation

dm4
Copy link
Member

@dm4 dm4 commented Feb 2, 2024

Simplify llama examples, split it into:

  • llama
  • llama-interactive
  • chat
  • embedding (newly added)

All of these are under wasmedge-ggml directory.

Copy link
Member

juntao commented Feb 2, 2024

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


Summary:

Overall, there are several potential issues and errors in this GitHub Pull Request. The patch introduces a significant number of changes, which can make it difficult to understand the overall impact and may lead to confusion for users. Additionally, some changes, such as the deletion of the wasmedge-ggml-llama-interactive directory and removal of installation instructions for Ubuntu and General Linux, lack explanations or alternatives. This may cause confusion and difficulties for users. Furthermore, there is a lack of documentation, tests, and explanations related to the changes made.

The most important findings in this review include the need for clearer explanations and alternatives for the changes made, proper documentation and tests to ensure the changes are well-documented and tested, and providing context and explanations for the new build steps added. It is crucial to address these issues to improve the quality and understandability of the code changes.

Details

Commit 8d97175955b7489884049417f90bcc55dcf73e78

Key changes:

  • The patch simplifies the organization of the repository by reorganizing the directory structure and removing unnecessary files.
  • The wasmedge-ggml-llama-interactive directory and its contents have been deleted.
  • The wasmedge-ggml directory has been created, containing the chatml and llama-stream subdirectories.
  • The chatml and llama-stream directories contain new source code files and Cargo.toml files.
  • New WASM files have been added for the chatml and llama-stream examples.

Potential problems:

  • The patch introduces a significant number of changes, which can make it difficult to review and understand the overall impact.
  • It's unclear why the wasmedge-ggml-llama-interactive directory was deleted and its contents moved to the chatml and llama-stream directories. This could potentially cause confusion if there were external references or dependencies to the old directory structure.
  • It's unclear what the purpose of the wasmedge-ggml directory is and why it was introduced.
  • The patch adds new source code files, but there is no description or explanation of what these files do or how they contribute to the overall functionality of the project.
  • The patch adds new WASM files for the chatml and llama-stream examples, but there is no explanation of how these files were generated or what they are used for.

Commit 1daefb910a1b4537644db33daf30a950e16eeb9d

Key changes:

  • Updated installation instructions for Linux with CUDA enabled and Linux without CUDA enabled.
  • Removed installation instructions for Ubuntu.
  • Removed installation instructions for General Linux.

Potential problems:

  • The patch removes installation instructions for Ubuntu and General Linux without providing an alternative. This may cause confusion for users of these platforms.
  • The patch removes some code examples without providing a reason or explanation. It would be useful to include a commit message explaining why these examples were removed.
  • The patch mentions building the application from source, but does not provide instructions or guidance on how to do so. This could be problematic for users who want to modify or customize the application.
  • The patch does not include any tests or documentation updates related to the changes made. It would be helpful to include these to ensure the changes are well-documented and tested.

Commit 5b0dfeced22074296e412d9f8d0ed2361d3638d2

Key changes:

  • The wasmedge-ggml-llama-embedding directory has been moved into the wasmedge-ggml/embedding directory.
  • The wasmedge-ggml-llama-embedding/README.md file has been deleted.

Potential problems:

  • The patch seems to be removing the wasmedge-ggml-llama-embedding example without providing a replacement or alternative.
  • It is unclear why the embedding example is being removed and where the related code has been relocated. More information is needed to understand the reason behind this change.

Overall, the key changes in this patch involve rearranging the project structure by moving the wasmedge-ggml-llama-embedding directory. However, without more context, it is difficult to determine if this change is appropriate or if it introduces any issues.

Commit 4676d2c6681fc215b7e0da3e8eb80ea0ed03c33d

Key changes in the patch:

  • Added build steps for llama-stream, chatml, and embedding in the macOS job.

Potential problems:

  • The patch adds build steps for three additional components without providing any context or explanation in the pull request. It would be helpful to include some information about why these builds are necessary and what purpose they serve.
  • It is unclear if the build steps for the new components (llama-stream, chatml, and embedding) have been tested and verified to work correctly.
  • The patch does not include any documentation or comments explaining the purpose or usage of the new build steps.

Overall, it would be beneficial to provide more context and documentation for the changes introduced by this patch. This would help reviewers and other contributors understand the rationale behind the changes and ensure that they have been implemented correctly.

wasmedge-ggml/README.md Outdated Show resolved Hide resolved
@dm4 dm4 marked this pull request as ready for review February 5, 2024 03:47
@dm4 dm4 force-pushed the dm4/simplify branch 2 times, most recently from 87583b7 to b453795 Compare February 5, 2024 08:02
@dm4 dm4 changed the title [Example]: simplify llama examples [Example] simplify llama examples Feb 5, 2024
@hydai hydai merged commit 6c4d3f2 into master Feb 5, 2024
3 checks passed
@hydai hydai deleted the dm4/simplify branch February 5, 2024 09:47
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