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

ci: rust has to use wasm32-wasip1 instead of wasm32-wasi #167

Merged
merged 5 commits into from
Jan 13, 2025

Conversation

hydai
Copy link
Member

@hydai hydai commented Jan 9, 2025

No description provided.

@hydai hydai requested a review from dm4 January 9, 2025 16:22
Copy link
Member

juntao commented Jan 9, 2025

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


.github/workflows/build_openvino_mobilenet.yml

Potential issues

  1. The target specified in the "Install Rust-stable" step is hardcoded to wasm32-wasip1, which may not align with all possible build scenarios if future changes require a different target.
  2. The apt install -y libtbbmalloc2 command does not handle installation failures, which could lead to subsequent steps failing silently without proper error handling.
  3. The VERSION environment variable for WasmEdge installation is hardcoded to "0.14.1", potentially causing issues if a newer version of WasmEdge is needed or desired without updating the workflow file.

Summary of changes

  • bullet points:
  • Updated Rust version from 1.71 to 1.84.
  • Changed the target from wasm32-wasi to wasm32-wasip1.
  • Updated WasmEdge version from 0.13.4 to 0.14.1.

.github/workflows/build_openvino_road_seg_adas.yml

Potential issues

  1. Issue 1: The target in the Rust installation step is incorrectly set to wasm32-wasip1, which should be wasm32-wasi as per the original configuration before the patch.

  2. Issue 2: The build and run commands for the openvino-road-segmentation-adas example include a redundant cd command (cd openvino-road-seg-adas followed by cd ..), which could be simplified.

  3. Issue 3: The workflow uses hardcoded environment variables and paths in multiple places (e.g., -p /usr/local, ./model/road-segmentation-adas-0001.xml), reducing flexibility and maintainability.

Summary of changes

    • Target Change: Updated the Rust target from wasm32-wasi to wasm32-wasip1.
  • Rust Version Update: Upgraded the Rust version in the matrix from 1.71 to 1.84.
  • Dependency Update: Changed the installation of libtbb2 to libtbbmalloc2.

.github/workflows/build_pytorch_yolo.yml

Potential issues

  1. The dtolnay/rust-toolchain@stable action specifies toolchain: ${{ matrix.rust }}, which is redundant since the action defaults to using the stable toolchain.
  2. The cp ./libtorch/lib/* /lib/ command in the Install LibTorch step may overwrite essential system libraries if any exist in /lib/.
  3. The cargo build --target=wasm32-wasip1 --release command uses a specific target that might not be compatible with all dependencies or configurations, potentially leading to build failures.

Summary of changes

    • Updated Rust version from 1.71 to 1.84.
  • Changed target from wasm32-wasi to wasm32-wasip1.
  • Adjusted cargo build commands and paths accordingly to use wasm32-wasip1.

.github/workflows/chatTTS.yml

Potential issues

  1. Issue with CMake Configuration: The cmake command is missing the -DWASMEDGE_PLUGIN_WASI_NN_CHAT_TTS=ON flag, which may be necessary to enable the ChatTTS plugin during the build of WasmEdge.

  2. Potential Incorrect Path for Wasm Edge Execution: In the Execute step, the path WasmEdge-WASINN-examples/wasmedge-chatTTS/target/wasm32-wasip1/release/wasmedge-chattts.wasm might be incorrect if the compiled file has a different name. Verify that wasmedge-chattts.wasm is the correct output.

  3. Hardcoded Output File Check: The Verify output step checks for a hardcoded WAV format (RIFF (little-endian) data, WAVE audio, mono 24000 Hz). If the output format changes or if the plugin produces different formats, this check will fail unexpectedly. Consider making this more flexible.

Summary of changes

    • Updated Rust target: Changed wasm32-wasi to wasm32-wasip1 for Rust installation.
  • Modified build command: Updated cargo build to use wasm32-wasip1 instead of wasm32-wasi.
  • Adjusted artifact path: Modified the path in the Execute step to match the new target wasm32-wasip1.

.github/workflows/llama.yml

Potential issues

  1. Incorrect Environment Variable Usage: The time command is used to measure execution time, but it does not set any environment variable for NGL. Ensure that the environment variable NGL is correctly set and accessible in all jobs where it is used.

  2. Potential Missing Model Files: Multiple jobs download model files from Hugging Face before building and running. If a download fails or the file URL changes, the job will fail. Consider adding error checking after each curl command to verify successful downloads.

  3. Hardcoded Paths for Models: The paths for the downloaded models are hardcoded (e.g., llama-2-7b-chat.Q5_K_M.gguf). If there is any change in the model naming or structure, this can lead to build failures. Use variables or parameters to manage these paths dynamically.

Summary of changes

  • Key Changes:
  • Updated Build Target: Changed wasm32-wasi to wasm32-wasip1 in all cargo build --target commands.
  • Path Adjustments: Updated the output paths for compiled Wasm modules from wasm32-wasi to wasm32-wasip1.
  • Rust Target Installation: Modified the Rust target installation command to add wasm32-wasip1 instead of wasm32-wasi.

.github/workflows/piper.yml

Potential issues

  1. Issue: The Install ONNX Runtime step uses a script path that may not exist in the WasmEdge repository, leading to potential build failures.

    • Explanation: The script utils/wasi-nn/install-onnxruntime.sh is referenced but its existence and location within the WasmEdge repository are not confirmed.
  2. Issue: The Execute step assumes a specific architecture (x86_64) for the espeak-ng-data, which might not be compatible with the target platform.

    • Explanation: Downloading piper_linux_x86_64.tar.gz and extracting espeak-ng-data could lead to compatibility issues if the build or execution environment is not x86_64.
  3. Issue: The Verify output step uses a hardcoded expected file type, which might not match the actual output format.

    • Explanation: Assuming the output file welcome.wav will always be in RIFF (little-endian) WAVE format with specific attributes may cause false negatives if the file format changes.

Summary of changes

    • Updated Rust target: Changed the Rust target from wasm32-wasi to wasm32-wasip1.
  • Modified build command: Updated the cargo build command to use the wasm32-wasip1 target.
  • Adjusted wasm file path: Modified the path in the Execute step to reflect the new wasm32-wasip1 output directory.

.github/workflows/pytorch.yml

Potential issues

  1. Hardcoded PyTorch Version: The PYTORCH_VERSION variable is hardcoded to "1.8.2", which may lead to dependency issues if newer versions of WasmEdge require a different version. Consider making this configurable.

  2. Redundant Export of LD_LIBRARY_PATH: The LD_LIBRARY_PATH is exported twice, once in the "Install WasmEdge + WASI-NN + PyTorch" step and again in the "Example" step. This redundancy can be removed to simplify the script.

  3. Potential Build Failure on Rust Version Mismatch: The patch specifies using wasm32-wasip1, which may not be compatible with all versions of Rust used by developers or CI environments. Ensure that the specified target is supported by the Rust version being used, or enforce a specific Rust version in the CI setup.

Summary of changes

    • Updated Rust target: Changed from wasm32-wasi to wasm32-wasip1.
  • Modified cargo build command: Updated the target in the cargo build command to wasm32-wasip1.
  • Adjusted wasmedge compile paths: Updated the file paths in the wasmedge compile commands from wasm32-wasi to wasm32-wasip1.

.github/workflows/tflite.yml

Potential issues

  1. Issue: The Install Rust target for wasm step uses the wrong target name (wasm32-wasip1) which is not a standard Rust toolchain target. It should use the correct target if intended, or the patch's intent should be verified.

  2. Issue: The Example step compiles and runs a WASM file with WasmEdge but does not handle potential compilation failures. Adding error checking after the cargo build command is crucial to ensure that subsequent steps do not proceed with an invalid artifact.

  3. Issue: The use of hardcoded paths (rust/target/wasm32-wasip1/release/...) in the Example step can lead to issues if the project structure changes or if different Rust profiles are used. Using variables or more flexible path specifications would improve maintainability.

Summary of changes

    • Changed Rust target from wasm32-wasi to wasm32-wasip1 in the installation step.
  • Updated Cargo build command to use wasm32-wasip1 instead of wasm32-wasi.
  • Modified WasmEdge compile command path to reflect the new target directory wasm32-wasip1.

scripts/install_openvino.sh

Potential issues

  1. Issue 1: The script assumes the system is Ubuntu 24, which may not be true for all environments where this script runs.

    Explanation: Hardcoding ubuntu24 in the sources list can cause issues if the script is executed on a different Linux distribution or version.

  2. Issue 2: The apt-key command is deprecated and should be replaced with a more secure method of adding keys.

    Explanation: Using apt-key add poses security risks, as it allows for the addition of untrusted keys to the system's keyring. It's recommended to use wget -O - https://apt.repos.intel.com/intel-gpg-keys/$KEY_FILE | gpg --dearmor | tee /usr/share/keyrings/intel-archive-keyring.gpg > /dev/null and modify the sources list to use this keyring.

  3. Issue 3: The script does not handle errors for apt-get install, which can lead to incomplete installations.

    Explanation: Failing to check if the installation was successful can leave the system in an unstable state, as it assumes that the installation always completes without issues.

Summary of changes

  • bullet points:
  • Updated OpenVINO installation from version 2023.0.0 to 2024.2.0.
  • Added cleanup of the GPG key file after adding it to apt-key.
  • Changed the Ubuntu release tag in the APT repository URL and list file name from ubuntu20 to ubuntu24.

@hydai hydai merged commit 593a2bf into master Jan 13, 2025
52 of 54 checks passed
@hydai hydai deleted the hydai/fix_wasm32-wasip1 branch January 13, 2025 04:33
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