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] Enable daily run for each workflow #32

Merged
merged 10 commits into from
Sep 20, 2023
Merged

Conversation

hydai
Copy link
Member

@hydai hydai commented Sep 20, 2023

No description provided.

Copy link
Member

juntao commented Sep 20, 2023

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


Overall Summary:

The pull request titled "[CI] Enable PyTorch workflow" introduces several changes related to enabling PyTorch workflows and making adjustments to existing workflows. The patch includes adding new files, modifying existing scripts, and updating dependencies.

Potential issues and errors found in the pull request include:

  • Frequent schedule for PyTorch workflow, which may need adjustment depending on project needs.
  • Security risks and potential incompatibilities due to the use of 'sudo' without arguments, downloading and unzipping files without verifying integrity, and using relative paths.
  • Assumptions made about specific versions and environments, which may not be compatible with all setups.
  • Lack of explanation or justification for certain changes, such as directory structure modifications and selection of specific versions.
  • Missing error handling and error validation for different commands and scripts.

The most important findings are:

  • Security risks and potential incompatibilities due to improper handling of dependencies and downloaded files.
  • Assumptions made about specific versions and environments, which may cause issues and limit compatibility.
  • Lack of explanation or justification for certain changes, which makes it difficult to understand the reasoning behind them.
  • Missing error handling and validation, which may lead to problems during execution.

Further discussion, clarification, and testing are required to address the potential issues and ensure the changes are properly implemented.

Details

Commit 07e84cc9834d4480386528e4f42aac76d0b58dce

Key Changes:

  1. Added a new file .github/workflows/pytorch.yml to enable the PyTorch workflow.
  2. Added a new job in the PyTorch workflow to build and test PyTorch examples.
  3. Added paths for the PyTorch workflow in the push and pull_request events to trigger the workflow only when changes related to PyTorch are made.

Potential Problems:

  1. The schedule for the PyTorch workflow is set to run every day at midnight (0 0 * * *). This might be too frequent depending on the project's needs. Consider adjusting the schedule to run less frequently, such as once a week.
  2. The patch installs apt-get packages and rust targets using sudo without specifying any arguments. This can lead to potential security risks and might not work in some CI environments. It's recommended to use a system-level package manager or containerization to handle dependencies instead.
  3. The patch downloads PyTorch from a specific URL and unzips it without verifying the integrity of the downloaded file. This can lead to potential security risks and might not work if the file is unavailable or corrupted.
  4. The patch uses relative paths when running the PyTorch examples. This might cause issues if the working directory changes in the future. It's recommended to use absolute paths or ensure that the working directory is correctly set before running the examples.
  5. The patch assumes the usage of Ubuntu 20.04 or above and specific PyTorch versions. This might not be compatible with all environments. Consider adding version checks or more flexible installation options to support a wider range of setups.

Commit 02274a123311ee5305bacf0e5ef70254ddebcec4

Key Changes:

  • In the pytorch.yml workflow file, the directory change to the correct rust folder.
  • The cargo build command is executed in the rust folder.
  • The wasmedge compile commands are modified to use the correct output file path.
  • There is a cd command to move back to the previous directory after executing wasmedge compile.

Potential Problems:

  • The patch does not include any changes to the usage or functionality of the code. It only fixes the directory path and file output path.
  • It is not clear why the cd rust command is necessary. There should be a comment or explanation for this change.
  • It is important to ensure that the correct path is used for the wasmedge compile commands.

Commit 509833f16bef27b79d485c04daecb110af1689d8

Key changes:

  • The GitHub action 'checkout@v2' has been updated to 'checkout@v3' in the '.github/workflows/llama.yml', '.github/workflows/pytorch.yml', and '.github/workflows/tflite.yml' files.

Potential problems:

  • No potential problems were identified in this patch.

Commit 30f802e5cc4bf0f783a16dea32d70f6fd9fb2917

Key changes:

  • The patch changes the working directory back to the root of the repository.
  • The patch updates the commands to compile and run Rust code.

Potential problems:

  • The patch changes the directory structure assumption for the Rust code. It assumes that the code is now located at pytorch-mobilenet-image/rust instead of just rust. This change may cause issues if the assumption is incorrect or if the directory structure is not properly updated.
  • The patch removes the compilation of two wasm files (wasmedge-wasinn-example-mobilenet-image-aot.wasm and wasmedge-wasinn-example-mobilenet-image-named-model-aot.wasm) and replaces them with two new compilation commands. The impact of this change is unclear without further context. It may introduce new compilation errors or affect the functionality of the workflow.
  • The patch introduces a change in the command that runs the wasm files (wasmedge --dir .:. wasmedge-wasinn-example-mobilenet-image-aot.wasm mobilenet.pt input.jpg). Without further details or context, it is difficult to determine the impact of this change and whether it is intended.

Overall, the patch introduces several changes to the workflow that may have unintended consequences or may require further explanation or context to fully understand. It would be helpful to review the reasoning behind these changes and ensure that they are properly tested and documented.

Commit ed1957b05e35f165573732280a5d1cb7b63aa563

Key changes:

  • The script now upgrades the system with apt-get upgrade -y.
  • The script installs Python 3 with apt-get install -y python3.

Potential problems:

  • The script assumes that the system is running Ubuntu 20. If the system is running a different version, the installation may fail.
  • There is a potential issue with the apt-key add command. The script assumes that the necessary GPG key is available in the file GPG-PUB-KEY-INTEL-SW-PRODUCTS.PUB. If the file is missing or corrupted, the installation may fail.
  • The script does not handle errors or validate the success of commands, which could lead to issues if any of the commands fail.

Overall, the changes seem reasonable, but there are potential problems that should be addressed before merging the pull request.

Commit 0940389fbce3a53b130cec04f60e775df280de26

Key changes:

  • Refining the naming of the workflows in the .github/workflows directory. Specifically, the names of the llama.yml, pytorch.yml, and tflite.yml files have been updated.

Potential problems:

  • No obvious issues found in this patch.

Commit 4079288698c379c2d8836352d005cb00bc57c2d4

Key changes:

  • Removed the ldconfig command from the OpenVINO build steps in the workflows.
  • Modified the script install_openvino.sh to install OpenVINO version 2023.

Potential problems:

  • The removal of the ldconfig command from the workflows may cause issues if the ldconfig command is required for proper linking of libraries.
  • The script install_openvino.sh is modified to install a specific version of OpenVINO (2023) without any explanation or justification. This may cause compatibility issues if the project relies on a different version of OpenVINO.
  • The apt search openvino command in the script is unnecessary and may cause errors if the package is not found in the repository.
  • The export PATH=/usr/lib/x86_64-linux-gnu:$PATH command in the script may overwrite the existing PATH variable and cause issues with other dependencies or tools.

Commit 8f41152870a079a8c25ced02af463e310834f4ed

Key changes:

  • The script install_openvino.sh is modified to use the openvino-2023.0.0 version for installation.
  • The apt-get command is changed to install openvino-2023.0.0 instead of openvino.
  • The version information in the echo statement is updated to reflect 2023.0.0.

Potential problems:

  • The patch modifies the openvino installation script to use a specific version (2023.0.0), but it does not mention why this specific version is chosen. Without proper justification, it may be difficult for reviewers or maintainers to understand the reason behind this update.
  • The patch does not include any error handling or fallback mechanism if the wget command fails to download the GPG-PUB-KEY-INTEL-SW-PRODUCTS.PUB file.
  • The patch does not include any error handling if the apt-key command fails to add the GPG key.
  • The patch does not include any error handling if the apt update or apt upgrade commands fail.
  • The patch does not handle cases where openvino-2023.0.0 is not available in the specified repository.
  • The export command at the end of the script modifies the PATH environment variable. This change may have unintended consequences, especially if there are other components relying on the original PATH value.

Overall, the patch introduces specific changes to the version used in the openvino installation script. However, it lacks justification, error handling, and potential issues related to the updated version. These areas should be addressed or clarified in the Pull Request discussion.

Commit a1fd17c8a2c30f78ac91dc7ddb51187774886f27

Key changes:

  • Added a schedule for running the workflows daily at midnight.
  • Limited the workflow triggers to the master branch.
  • Updated the paths to include specific files and directories for triggering the workflows.
  • Removed the paths-ignore entry in the pull_request section.

Potential problems:

  • The schedule cron expression is set to "0 0 * * *", which means the workflows will run every day at midnight. This should be verified to ensure it aligns with the intended frequency.
  • The workflows are limited to triggering only on the master branch. This may be intentional, but it's important to confirm if this is the desired behavior.
  • The paths for workflow triggers have been updated. It's crucial to verify that the files and directories mentioned in the paths exist and are correct.
  • The paths-ignore entry in the pull_request section has been removed. This means that any changes to ".md" files will now trigger the workflow. It should be checked if this change aligns with the desired workflow behavior.

Overall, the changes seem to enable a daily automated build for OpenVINO workflows while restricting triggers to the master branch. However, a thorough review and testing should be performed to ensure the changes are implemented correctly and align with the intended workflow behavior.

Commit f5ecf3ab1dcc641e34faf69b49a5bdd3e550869a

Key changes:

  • The patch updates the OpenVINO version used in the script install_openvino.sh from openvino-2023.0.0 to openvino-2023.0.2.

Potential problems:

  • It is unclear why the OpenVINO version needs to be updated in this script. The commit message mentions that the change is made because the WASI-NN OpenVINO plugin uses version openvino-2023.0.2, but the script itself does not mention the plugin.
  • It would be beneficial to provide more context or explanation in the commit message regarding the reason for the change and what impact it may have on the overall project or workflow.

@hydai hydai force-pushed the hydai/enable_pytorch_ci branch 3 times, most recently from 8d8dca5 to a841b96 Compare September 20, 2023 02:41
Signed-off-by: hydai <z54981220@gmail.com>
Signed-off-by: hydai <z54981220@gmail.com>
Signed-off-by: hydai <z54981220@gmail.com>
Signed-off-by: hydai <z54981220@gmail.com>
Signed-off-by: hydai <z54981220@gmail.com>
Signed-off-by: hydai <z54981220@gmail.com>
Signed-off-by: hydai <z54981220@gmail.com>
Signed-off-by: hydai <z54981220@gmail.com>
Signed-off-by: hydai <z54981220@gmail.com>
…version

Signed-off-by: hydai <z54981220@gmail.com>
@hydai hydai requested a review from dm4 September 20, 2023 03:26
@hydai hydai changed the title [CI] Enable PyTorch workflow [CI] Enable daily run for each workflow Sep 20, 2023
@hydai hydai merged commit 717bb64 into master Sep 20, 2023
5 checks passed
@hydai hydai deleted the hydai/enable_pytorch_ci branch September 20, 2023 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.

None yet

3 participants