https://github.com/yomorun/yomo-wasmedge-tensorflow/pull/12

[juntao]

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

---

flow/app.go

The code is generally well-written, but there are a few potential issues and improvements to consider:

1. Error handling
   • In the Handler function, the error returned by initVM() is handled, but the error returned by bg.Execute("infer", img) is not. This should be handled appropriately, e.g., by returning an appropriate value (e.g., 0, nil) and logging the error.

res, _, err := bg.Execute("infer", img)
if err != nil {
    log.Printf("Failed to execute 'infer': %v", err)
    return 0, nil
}
fmt.Println("GO: Run bindgen -- infer:", string(res[0].([]byte)))

2. Multiple VM initializations

   • The Handler function is initializing a new VM each time it's called. This can result in high memory usage and low performance, especially if the handler is called frequently. A better approach would be to initialize the VM once (e.g., in the main function or as a package-level variable) and reuse it. Note that this may require ensuring proper thread-safety if the handler can be called concurrently.

3. Use of select {}

   • The select {} statement at the end of the main function is used to block the main goroutine indefinitely. While this is a valid way to prevent the program from exiting, using signals would be a more idiomatic and elegant approach. You can catch the termination signal (e.g., os.Interrupt) and gracefully shut down the application.

shutdown := make(chan os.Signal, 1)
signal.Notify(shutdown, os.Interrupt, syscall.SIGTERM)
<-shutdown

// Perform cleanup and shut down gracefully.

4. Redundant call to fmt.Sprintf
   • In the genSha1 function, the fmt.Sprintf call can be replaced by a simple type conversion. sha1.New() returns a hash.Hash, which means .Sum(nil) returns a slice of bytes that can be cast directly to a hex-encoded string.

h.Write(buf)
return hex.EncodeToString(h.Sum(nil))

5. Avoid global state (if possible)

   • Global state, such as the atomic counter variable, can make it harder to reason about the program and test individual components. Consider injecting the necessary state into your components when initializing them, or encapsulating state within constructs like a struct with associated methods. However, using atomic counters is quite reasonable for this specific use case.

6. Declare constants at the beginning of the file

   • The convention is to define constants at the beginning of the file after importing packages, before defining types and global variables.

7. Include documentation

   • Adding comments to explain the purpose of each function, and any non-obvious implementation details, improves code readability and maintainability. Follow Go's documentation commenting convention for exported functions and types.

These changes can help improve the code quality, but overall, the code is generally well-structured and easy to read.

The key change in the patch is related to loading the appropriate file for WasmEdge's VM. Instead of directly loading a hardcoded filename rust_mobilenet_food_lib.wasm, the patch introduces a loop that searches for the correct file with either a .so or .wasm extension.

Here's the breakdown of the change:

1. A new variable filename is introduced and set to the initial part of the filename "rust_mobilenet_food_lib".

2. A flag variable ok is set to false initially.

3. A loop iterates over a list of two possible suffixes: ".so" and ".wasm".

4. During the loop, the patch checks if the file exists with the current suffix:

   • If the file exists, the suffix is appended to the filename, the ok flag is set to true, and the loop is terminated using break.

5. After the loop, if the ok flag is still false, this means none of the expected files were found, and an error is returned with a message indicating the problem.

6. Finally, instead of loading the hardcoded file, the vm.LoadWasmFile method is called with the filename resulting from the loop. This allows the VM to load the correct file with either a .so or .wasm extension.

cc https://github.com/yomorun/yomo-wasmedge-tensorflow/pull/12

[juntao]

flows summarize

[juntao]

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

---

Commit cdafe5efd3e81d0e7cb9db930709396a346cdbd6

Summary of key changes:

1. Changed directory names from 'rust_mobilenet_foods/src' to 'rust_mobilenet_food/src' in README.md and README_CN.md.
2. Updated the code snippets in README.md and README_CN.md for compiling and copying the wasm file.
3. Added instructions for optionally compiling the wasm file to AOT mode in README.md and README_CN.md.
4. Updated flow/app.go to load either .so (AOT mode) or .wasm files by checking their existence in the file system.

Potential problems:

1. If neither '.so' nor '.wasm' files exist, an error is returned indicating the model file cannot be found. It might be helpful to add more information in the error message or README file covering how to build or obtain these files.
2. It is not clear if there may be any compatibility issues between the current code and the now optional AOT compilation mode. Further testing or examination might be needed to ensure compatibility.

cc https://github.com/yomorun/yomo-wasmedge-tensorflow/pull/12

[juntao]

flows summarize

[juntao]

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

---

Commit cdafe5efd3e81d0e7cb9db930709396a346cdbd6

This patch includes changes to the WebAssembly (WASM) compilation and loading process for a Rust-based machine learning application. The key changes are as follows:

1. Corrections in README.md and README_CN.md:

   • Fixed folder name from rust_mobilenet_foods/src to rust_mobilenet_food/src.
   • Added instructions to compile the Rust code and create the WASM file using cargo build.
   • Updated instructions to copy the compiled WASM file.
   • Added optional instructions to compile WASM file in Ahead-of-Time (AOT) mode using wasmedgec.

2. Changes in flow/app.go:

   • Updated the initVM() function to:
     • Look for both ".so" (shared object) and ".wasm" files.
     • Return an error if neither file is found.
     • Load the appropriate file into the WasmEdge virtual machine (VM).

Potential Problems:

1. Although the changes in app.go improve the way WASM files are found and loaded, they introduce coupling between the filename and the implementation. This could lead to issues if the filename changes in the future.

2. Handling both .wasm and .so files in the same build setup might create confusion for users who are not familiar with AOT compilation or shared objects. To avoid confusion, it may be beneficial to provide further information and clarification about the different options and their impact on performance in the README files.

cc https://github.com/yomorun/yomo-wasmedge-tensorflow/pull/12