second-state / WasmEdge-WASINN-examples

Apache License 2.0
217 stars 36 forks source link

Updated docs related to openvino #26

Closed Mash707 closed 10 months ago

Mash707 commented 10 months ago

resolves #25

juntao commented 10 months ago

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


Overall, this pull request includes several updates related to OpenVINO documentation and dependencies. Among the changes, there are a few potential issues and errors that need to be addressed.

The first potential problem is in the GitHub workflows where the script is not sourcing setupvars.sh after installing OpenVINO. This may cause issues with correctly setting up the environment variables.

Another potential problem is in the installation script where the paths for the ldconfig file are not updated to reflect the new OpenVINO version. The script still uses the paths for OpenVINO 2021, which can lead to confusion and potential errors.

In terms of important findings, the removal of unnecessary code related to inspecting and modifying the /etc/ld.so.conf file is a positive change that does not introduce any potential problems.

Additionally, the addition of links to the official OpenVINO documentation in multiple README files is a valuable contribution without any potential problems.

Lastly, the update to the Wasmedge version is straightforward, but it is essential to ensure compatibility and avoid any compatibility issues or regressions with the codebase.

Overall, this pull request includes a mix of useful updates and potential issues that need to be resolved.

Details

Commit 41f079b0669f0a99bdfcf4f0990e4248835e36f1

Key changes:

Potential problems:

Commit 5243f9a1dbd7cbc1ce5abc173274bfdd72c90861

Key changes:

Potential problems:

Overall, this patch removes unnecessary code related to inspecting and modifying the /etc/ld.so.conf file. This removal does not introduce any potential problems.

Commit 9e20824ca6e1eb9b29a886d30431561cb27eeaef

Key changes:

Potential problems:

Commit 081a5d319d03432c8c5b7449ec8eca19e0076c4c

Key Changes:

Potential problems:

Overall, this patch adds useful links to the official documentation, which will help developers with the OpenVINO installation process and provide additional resources for understanding the project. It is a straightforward and valuable contribution.

Commit 705f1062f994336dc0c485398bc7965d3873b99a

Key Changes:

Potential Problems:

Mash707 commented 10 months ago

Hii @hydai could you please review the the PR. Also I have a question regarding the install_openvino.sh, what is the purpose of the code written from line 19 to 28? https://github.com/second-state/WasmEdge-WASINN-examples/blob/4f7ff0a7327602e4f513bab428495309e889a250/scripts/install_openvino.sh#L19-L28

hydai commented 10 months ago

Hii @hydai could you please review the the PR. Also I have a question regarding the install_openvino.sh, what is the purpose of the code written from line 19 to 28?

https://github.com/second-state/WasmEdge-WASINN-examples/blob/4f7ff0a7327602e4f513bab428495309e889a250/scripts/install_openvino.sh#L19-L28

These lines are registering the OpenVINO libraries because the previous ones are using a script to set related environments. Since we don't need to do this anymore when using the official API repo, we can remove them.

Mash707 commented 10 months ago

Hii @hydai I removed those lines. Should we add links to openvino official documents and wasmedge docs or the changes I made are enough?

hydai commented 10 months ago

Hii @hydai I removed those lines. Should we add links to openvino official documents and wasmedge docs or the changes I made are enough?

Yes, please

Mash707 commented 10 months ago

I have added links to the official openvino docs and WasmEdge docs in the readme files

Mash707 commented 10 months ago

I have added docs links in the Readme.md files of each example also. Should we keep the docs links in each Readme.md file ?

hydai commented 10 months ago

Looks like there is a conflict between this branch and the master branch. Please fix the conflicts. CleanShot 2023-07-26 at 01 48 21

Mash707 commented 10 months ago

Looks like there is a conflict between this branch and the master branch. Please fix the conflicts. CleanShot 2023-07-26 at 01 48 21

is this for me to fix ?

hydai commented 10 months ago

You may need to pull/rebase your branch to fix the conflicts.

Mash707 commented 10 months ago

You may need to pull/rebase your branch to fix the conflicts.

I tried pulling and rebasing my branch with master branch but the conflict is not fixed. image I used this for reference regarding pulling and rebasing https://stackoverflow.com/questions/7929369/how-to-rebase-local-branch-onto-remote-master.

Mash707 commented 10 months ago

Hii @hydai I have resolved the conflict. The PR is now ready to be merged.

apepkuss commented 10 months ago

@Mash707 Please check the failed CI workflows and resolve the failures. Thanks!

Mash707 commented 10 months ago

Hii @apepkuss , The workflow should work now because previously it was using wasmedge version 0.13.1 . I changed it to the latest release 0.13.2. The install script for openvino was updated in the 0.13.2 release.

apepkuss commented 10 months ago

@hydai @Mash707 LGTM. Thanks!