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

[WASI-NN] ggml: add ErrNo::ModelNotFound #3338

Merged
merged 4 commits into from
Apr 18, 2024
Merged

Conversation

dm4
Copy link
Collaborator

@dm4 dm4 commented Apr 16, 2024

To add ModelNotFound error to wasmedge-wasi-nn and use it in our WasmEdge WASINN examples, we need:

  • Merge PR in wasmedge-wasi-nn
  • Merge PR in WasmEdge runtime
  • Release the new wasmedge-wasi-nn crate
  • Update the WasmEdge WASINN examples to use the newer version of wasmedge-wasi-nn crate

Copy link
Member

juntao commented Apr 16, 2024

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


This pull request, titled "[WASI-NN] ggml: add ErrNo::ModelNotFound," predominantly includes two broad changes. One caters to handling a scenario where a model file specified in the code cannot be located, and the other is intended to address a macOS-related setup in the continuous integration (CI) build, particularly in connection with ffmpeg@6 library.

Several potential issues and concerns identified across these changes include:

  • Availability and compatibility of the library in all build environments, which if not present, may pose portability problems.
  • The logging level specified for the error message in the case of model file unavailability, if conspicuous, could flood logs when dealing with numerous missing models.
  • The elimination of the most recent graph from the environment could lead to unforeseen behaviours, mainly if clients are unprepared to handle such situations.
  • If the compiled code is incompatible with exception handling, for example, if exceptions are disabled within certain systems, this could lead to problems. This issue arises from the dependency on exceptions for error handling.
  • The absence of additional unit tests to verify new functionality or error conditions is a critical point of concern.
  • In relation to changes in build operations, this pull request modifies the PKG_CONFIG_PATH specific to macOS and the ffmpeg@6 library. This could lead to issues if multiple versions of ffmpeg are needed, or if these are incorrectly installed or missing.
  • Moreover, the hard-coding of the specific pkgconfig path may promote portability problems in the long run.
  • Lack of tests as part of this pull request could lead to potential risks, especially since the modifications to the build script involve paths essential for macOS builds.

In conclusion, although the two changes appear to cater to essential aspects of the software - error handling mechanism and macOS specific build configurations, they introduce several potential risks that need careful consideration and proper handling.

Details

Commit 6b9b6c6e4b0fc78fce9a0a9bfba6052f34630d8f

Summary
In this commit, the developer has added a new error code, ErrNo::ModelNotFound, which appears to be returned when a specific model file cannot be located. This error is checked in the load function in the ggml.cpp file. If the model file specified in ModelFilePath does not exist, the function logs an error message and returns the new error code.

Key changes:

  1. An include directive for the <filesystem> library has been added to ggml.cpp, which enables filesystem operations in the code.
  2. A check has been added in ggml.cpp to see if the model file at ModelFilePath exists. If it doesn't, an error is logged, the last model is removed from the environment, and ErrNo::ModelNotFound is returned.
  3. ErrNo::ModelNotFound has been added to the ErrNo enum in types.h.

Potential Problems:

  1. The <filesystem> library may not be available in all build environments, potentially causing portability issues.
  2. Removing the last graph from the environment (Env.NNGraph.pop_back()) could result in unexpected behavior if clients aren't prepared to handle this case.
  3. The logging level for the error is set at spdlog::error, which could flood the log when handling a large number of missing models. It may be beneficial to allow configurable logging levels.
  4. This patch relies on exceptions for error handling. If the compiled code isn't compatible with exception handling (i.e., exceptions disabled in certain embedded systems), this could cause issues.
  5. Lastly, there aren't additional tests added to verify this change. It's important to have unit tests to cover new functionality or error conditions – in this case, what happens when the model file isn't found.

Commit 2ebce1be4d65972aa00b471308730218fed4e201

The key change given in the patch is that the environment's PKG_CONFIG_PATH being setup for the ffmpeg@6 library. This is done within the Github Action Script or workflow file build-extensions.yml. The PKG_CONFIG_PATH is set by prepending the pkgconfig path for ffmpeg@6 retrieved using the brew command. The path being set is specific to a macOS setup as the Homebrew packet manager used to get the ffmpeg's pkgconfig path is common in macOS systems.

Key takeaways from this patch:

  1. Change impacts CI build, particularly for macOS environment.
  2. ffmpeg@6's pkgconfig path is being added to PKG_CONFIG_PATH.
  3. This change was made by dm4@secondstate.io.

Lines 355 - 359 in the build-extensions.yml represent a script block within a Github Action environment. This change should not impact other system builds, provided that other system-specific paths are correctly defined in the neighboring environments.

Possible issues:

  1. This patch concerns itself with a specific version of ffmpeg, namely ffmpeg@6. If a different version of ffmpeg is expected or necessary, this could lead to some problems.
  2. If for some reason, ffmpeg is not installed or incorrectly installed using Homebrew, or an error occurred while changing the ffmpeg version, the brew command could fail, braking the CI build.
  3. No tests have been added as part of this pull request to verify that the changes work as expected. This may not be ideal, particularly because the change is in a CI build script. Having tests or some form of verification can help mitigate future potential issues.
  4. The package manager's (brew's) specific path to ffmpeg@6 has been hard-coded. This might cause portability issues, if the path changes in future versions of the package manager.

@github-actions github-actions bot added c-Plugin An issue related to WasmEdge Plugin c-WASI-NN labels Apr 16, 2024
@dm4 dm4 marked this pull request as ready for review April 16, 2024 06:45
@dm4 dm4 requested a review from ibmibmibm as a code owner April 16, 2024 06:45
dannypsnl
dannypsnl previously approved these changes Apr 16, 2024
plugins/wasi_nn/ggml.cpp Outdated Show resolved Hide resolved
Signed-off-by: dm4 <dm4@secondstate.io>
hydai
hydai previously approved these changes Apr 17, 2024
@hydai hydai added the v-0.14.0 label Apr 17, 2024
Copy link

codecov bot commented Apr 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.94%. Comparing base (649f50f) to head (2ebce1b).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3338   +/-   ##
=======================================
  Coverage   79.93%   79.94%           
=======================================
  Files         251      251           
  Lines       34368    34368           
  Branches     5988     5963   -25     
=======================================
+ Hits        27472    27474    +2     
+ Misses       5515     5509    -6     
- Partials     1381     1385    +4     

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

Signed-off-by: dm4 <dm4@secondstate.io>
@dm4 dm4 force-pushed the dm4/errno-model-not-found branch from f3cc61a to 2ebce1b Compare April 18, 2024 09:00
@dm4 dm4 merged commit 64f6148 into master Apr 18, 2024
73 checks passed
@dm4 dm4 deleted the dm4/errno-model-not-found branch April 18, 2024 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-CI c-Plugin An issue related to WasmEdge Plugin c-WASI-NN v-0.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants