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

Updated docs related to openvino #26

Merged
merged 6 commits into from
Jul 31, 2023

Conversation

Mash707
Copy link
Contributor

@Mash707 Mash707 commented Jul 21, 2023

resolves #25

Signed-off-by: Divyanshu GUpta <[email protected]>
Copy link
Member

juntao commented Jul 21, 2023

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:

  • Updated the OpenVINO version in the GitHub workflows for building OpenVINO models to "2023.0.0"
  • Updated the OpenVINO installation instructions in the README and examples to use version "2023.0.0"

Potential problems:

  • In the GitHub workflows, the script is not sourcing setupvars.sh after installing OpenVINO. This may cause issues with correctly setting up the environment variables.
  • In the installation script, the paths for the ldconfig file are not updated to reflect the new OpenVINO version. The script still uses the paths for OpenVINO 2021.

Commit 5243f9a1dbd7cbc1ce5abc173274bfdd72c90861

Key changes:

  • Removed unnecessary code related to inspecting and adding new paths to /etc/ld.so.conf file.

Potential problems:

  • None.

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:

  • Added links to the official OpenVINO documentation in multiple README files

Potential problems:

  • There are no potential problems in this patch. It is a simple update to include links to the official OpenVINO documentation.

Commit 081a5d319d03432c8c5b7449ec8eca19e0076c4c

Key Changes:

  • Added links to official documentation in the README files of the following directories: openvino-mobilenet-image, openvino-mobilenet-raw, openvino-road-segmentation-adas.
  • The links point to the WasmEdge Docs and OpenVINO 2023 installation guide.

Potential problems:

  • No potential problems were identified in this patch.

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:

  • The patch updates the Wasmedge version from 0.13.1 to 0.13.2 in various files.

Potential Problems:

  • There are no significant potential problems with this patch. It is a straightforward update to the Wasmedge version used in multiple files. However, it would be helpful to ensure that the new version 0.13.2 is compatible with the codebase and doesn't introduce any compatibility issues or regressions.

@Mash707
Copy link
Contributor Author

Mash707 commented Jul 21, 2023

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?

echo "*** inspect /etc/ld.so.conf"
cat /etc/ld.so.conf
echo "*** add new paths to /etc/ld.so.conf"
echo "include /etc/ld.so.conf.d/*.conf" >> /etc/ld.so.conf
echo "/opt/intel/openvino_2021/deployment_tools/ngraph/lib/" >> /etc/ld.so.conf
echo "/opt/intel/openvino_2021/deployment_tools/inference_engine/lib/intel64/" >> /etc/ld.so.conf
echo "*** inspect /etc/ld.so.conf"
cat /etc/ld.so.conf

@hydai
Copy link
Member

hydai commented Jul 23, 2023

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?

echo "*** inspect /etc/ld.so.conf"
cat /etc/ld.so.conf
echo "*** add new paths to /etc/ld.so.conf"
echo "include /etc/ld.so.conf.d/*.conf" >> /etc/ld.so.conf
echo "/opt/intel/openvino_2021/deployment_tools/ngraph/lib/" >> /etc/ld.so.conf
echo "/opt/intel/openvino_2021/deployment_tools/inference_engine/lib/intel64/" >> /etc/ld.so.conf
echo "*** inspect /etc/ld.so.conf"
cat /etc/ld.so.conf

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.

Signed-off-by: Divyanshu GUpta <[email protected]>
@Mash707
Copy link
Contributor Author

Mash707 commented Jul 24, 2023

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
Copy link
Member

hydai commented Jul 25, 2023

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

Signed-off-by: Divyanshu GUpta <[email protected]>
@Mash707
Copy link
Contributor Author

Mash707 commented Jul 25, 2023

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

Signed-off-by: Divyanshu GUpta <[email protected]>
@Mash707
Copy link
Contributor Author

Mash707 commented Jul 25, 2023

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 ?

Copy link
Member

@hydai hydai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @apepkuss PTAL.

@hydai
Copy link
Member

hydai commented Jul 25, 2023

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
Copy link
Contributor Author

Mash707 commented Jul 25, 2023

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
Copy link
Member

hydai commented Jul 25, 2023

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

@Mash707
Copy link
Contributor Author

Mash707 commented Jul 26, 2023

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
Copy link
Contributor Author

Mash707 commented Jul 27, 2023

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

@apepkuss
Copy link
Member

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

Signed-off-by: Divyanshu GUpta <[email protected]>
@Mash707
Copy link
Contributor Author

Mash707 commented Jul 27, 2023

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
Copy link
Member

@hydai @Mash707 LGTM. Thanks!

@hydai hydai merged commit 931b46e into second-state:master Jul 31, 2023
3 checks passed
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.

Update the OpenVINO document after WasmEdge 0.13.2 released
4 participants