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: try to enable merge queue #178

Merged
merged 1 commit into from
Feb 21, 2025
Merged

ci: try to enable merge queue #178

merged 1 commit into from
Feb 21, 2025

Conversation

hydai
Copy link
Member

@hydai hydai commented Feb 20, 2025

No description provided.

@hydai hydai requested a review from dm4 February 20, 2025 17:54
Copy link
Member

juntao commented Feb 20, 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. Incorrect Rust Toolchain Action: The uses: dtolnay/rust-toolchain@stable action is incorrect and likely outdated; it should use actions-rs/toolchain@v1 for setting up the Rust toolchain.

  2. Hardcoded CMake Version: The VERSION: "0.14.1" in the WasmEdge installation script is hardcoded, which may lead to version mismatches or failures if an update is needed; consider using a variable or latest stable tag.

  3. Missing Error Handling in Bash Scripts: The bash scripts (install_openvino.sh, download_mobilenet.sh) lack error handling (e.g., checking exit statuses), which can cause silent failures during the workflow execution.

Summary of changes

    • Added merge_group: to attempt enabling merge queue in CI.
  • No other significant changes in the patch.

.github/workflows/build_openvino_road_seg_adas.yml

Potential issues

  1. Incorrect working-directory in build step: The working-directory: openvino-road-segmentation-adas is redundant since the cd openvino-road-seg-adas command inside the run script already changes to that directory.
  2. Hardcoded WasmEdge version: The WasmEdge installation script uses a hardcoded version (0.14.1). This can lead to maintenance issues and should be parameterized or updated regularly.
  3. Potential path issue in build step: The cp target/wasm32-wasip1/release/openvino-road-seg-adas.wasm .. command assumes the script is run from within a subdirectory of openvino-road-segmentation-adas, which might not be guaranteed due to the working directory setup.

Summary of changes

    • Added merge_group: under the on: section to enable merge queue functionality.
  • No other significant changes were made.

.github/workflows/build_pytorch_yolo.yml

Potential issues

  1. Incorrect Checkout Repository: The step to "Checkout Wasi-NN examples" uses actions/checkout@v3, which seems incorrect for a repository related to Pytorch Yolo Detection, possibly leading to wrong source code being checked out.

  2. LibTorch Installation Path: The script copies LibTorch libraries directly into /lib/ without verifying if the directory exists or handling potential permission issues, which could lead to build failures on systems with restricted write permissions for root directories.

  3. Hardcoded WasmEdge Version: Specifying a hardcoded version 0.13.4 for WasmEdge installation may cause compatibility issues in future updates unless the repository is actively maintained to update this version accordingly.

Summary of changes

    • Enabled Merge Queue: Introduced merge_group under the workflow triggers to enable a merge queue.
  • No Additional Changes: The patch does not introduce any other significant changes to the existing configuration.

.github/workflows/chatTTS.yml

Potential issues

  1. The merge_group key is used incorrectly as a top-level key under on, which will cause a configuration error since merge_group should be within a job.
  2. The Verify output step uses test command syntax that may not work as expected in all shell environments; it's safer to use [ or [[ for string comparison, e.g., [ "$(file --brief output1.wav)" == 'RIFF (little-endian) data, WAVE audio, mono 24000 Hz' ].
  3. The installation of Rust target wasm32-wasip1 does not ensure that the Rust toolchain is installed; it should precede with a step to install Rust if not already present using rustup.

Summary of changes

  • bullet points:
  • Added merge_group: to the workflow configuration, enabling merge queue functionality.
  • No other significant changes were made in this patch.

.github/workflows/llama.yml

Potential issues

  1. Hardcoded Model URLs: The URLs for downloading models (e.g., https://huggingface.co/QuantFactory/Meta-Llama-3-8B-Instruct-GGUF/resolve/main/Meta-Llama-3-8B-Instruct.Q5_K_M.gguf) are hardcoded in multiple places, which can lead to maintenance challenges and potential failures if the URLs change. Issue: Hardcoded URLs.

  2. Lack of Error Handling: There is no error handling for the curl commands used to download models. If a model fails to download (e.g., due to network issues or incorrect URLs), the subsequent build and run steps will likely fail, but this scenario is not addressed in the script. Issue: No error handling for curl.

  3. Environment Variable Handling: The NGL environment variable is set using echo "NGL=${{ matrix.ngl || 0 }}" >> $GITHUB_ENV, but matrix.ngl is never defined in the provided workflow. This could lead to incorrect configurations or runtime errors if the default value of 0 is not suitable for all cases. Issue: Undefined matrix.ngl.

Summary of changes

-• Added merge_group: to the workflow configuration.
• This enables the merge queue for pull requests that trigger this workflow.
• The most significant change is the introduction of merge_group, which will allow for better management and merging of pull requests.

.github/workflows/piper.yml

Potential issues

  1. Hardcoded Release Version: The espeak-ng-data download URL is hardcoded to a specific release version (2023.11.14-2). This can lead to build failures if the release changes or becomes unavailable. Use a more dynamic approach, such as downloading from the latest stable release.

  2. Potential Path Issues: The WASMEDGE_PLUGIN_PATH environment variable in the "Execute" step assumes that the plugin is located at WasmEdge/build/plugins/wasi_nn. If the build process changes or if there are multiple builds, this path might not be correct, leading to runtime errors. Ensure the plugin path is dynamically set based on the build output.

  3. File Verification Command: The "Verify output" step uses a shell comparison that can fail due to differences in file metadata or additional whitespace. Use a more robust method for verifying file types, such as file with options that minimize variability, or use a tool specifically designed for checking audio files.

Summary of changes

    • Added merge_group: Introduced a new configuration under the workflow to enable merge queue functionality.
  • No changes to job definitions or triggers: The existing jobs and their triggers remain unchanged, ensuring stability while integrating the merge queue.
  • Minimal impact on current workflow logic: This addition aims to enhance the CI process flow without altering core functionalities.

.github/workflows/pytorch.yml

Potential issues

  1. Incorrect Rust Toolchain Action: The dtolnay/rust-toolchain@stable action is outdated and should be replaced with the official actions-rs/toolchain@v1 for proper Rust toolchain installation.

  2. Hardcoded PyTorch Version: The PyTorch version 1.8.2 is hardcoded in the script, which can lead to maintenance issues. Consider using an environment variable or input parameter to make it configurable.

  3. Redundant Library Path Export: The LD_LIBRARY_PATH is exported twice unnecessarily, once before and once inside the Example job step. This redundancy should be removed to clean up the code.

Summary of changes

-• Added merge_group: under the on section, indicating an attempt to enable merge queue functionality.
• No changes in job definitions or workflow structure were made; only a configuration change related to merging was introduced.

.github/workflows/tflite.yml

Potential issues

  1. Hardcoded TFVERSION: The TFVERSION is hardcoded to "2.12.0", which can lead to version mismatches or obsolescence. Consider using a variable or input parameter for flexibility.

  2. Environment Variable Handling: The ACCEPT_EULA=Y environment variable is set inline within the apt-get update and upgrade commands, which can be moved to the env section under the step for better readability and management.

  3. Error Handling Missing: There is no error handling or checks after critical steps like package installations and file downloads, which could lead to unnoticed failures in subsequent steps.

Summary of changes

    • Added merge_group to the workflow configuration, likely to enable GitHub's merge queue.
  • No other significant changes were made in this patch.

@hydai hydai enabled auto-merge February 20, 2025 17:56
@hydai hydai added this pull request to the merge queue Feb 21, 2025
Merged via the queue into master with commit 52e6365 Feb 21, 2025
43 of 45 checks passed
@hydai hydai deleted the hydai/enable_merge_queue branch February 21, 2025 03:48
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.

2 participants