UEP 4.1 release #11834

sfatimar · 2022-06-13T15:38:55Z

Update File Version Details in Onnxruntime_providers_openvino.dll
Update PyPi Package information for OpenVINO
Include Training Package in OpenVINO PyPi Package

Motivation and Context

This changes is required to ensure that we are able to host signed binaries in public domain
If it fixes an open issue, please link to the issue here.

lgtm-com · 2022-06-13T17:12:57Z

This pull request introduces 4 alerts when merging 02b90ff into 7582644 - view on LGTM.com

new alerts:

1 for Use of the return value of a procedure
1 for Unused local variable
1 for Unused global variable
1 for Module is imported with 'import' and 'import from'

preetha-intel · 2022-06-14T05:21:38Z

setup.py

@@ -15,6 +16,8 @@
 from shutil import copyfile

 from setuptools import Extension, setup
+from setuptools.command.install import install as InstallCommandBase
+from wheel.vendored.packaging.tags import sys_tags

 nightly_build = False


Variables used in master. Should we change it to upper case or disable the pylint warning for this file

preetha-intel · 2022-06-14T05:23:17Z

setup.py

@@ -52,6 +55,7 @@ def parse_arg_remove_string(argv, arg_name_equal):
 cuda_version = None
 rocm_version = None
 is_rocm = False
+is_openvino = False
 # The following arguments are mutually exclusive
 if wheel_name_suffix == "gpu":
    # TODO: how to support multiple CUDA versions?


Warning is thrown from the master branch comment. It should be safely ignored.

preetha-intel · 2022-06-14T05:28:12Z

setup.py

                self._rewrite_ld_preload(to_preload)
                self._rewrite_ld_preload_cuda(to_preload_cuda)
                self._rewrite_ld_preload_tensorrt(to_preload_tensorrt)
            _bdist_wheel.run(self)
-            if is_manylinux and not disable_auditwheel_repair:
+            if is_manylinux and not disable_auditwheel_repair and not is_openvino:
                file = glob(path.join(self.dist_dir, "*linux*.whl"))[0]


self.dist_dir is member of _bdist_wheel which gets its value during runtime. At compilation its set to None. Pylint is throwing error for adding path to dist_dir. Should we set a path to this member variable ?

setup.py

lgtm-com · 2022-06-14T17:36:50Z

This pull request introduces 4 alerts when merging ad8670a into f6d2b62 - view on LGTM.com

new alerts:

1 for Use of the return value of a procedure
1 for Unused local variable
1 for Unused global variable
1 for Module is imported with 'import' and 'import from'

jywu-msft · 2022-06-14T18:01:27Z

preetha-intel

@preetha-intel can you see if these alerts can be addressed as well?

jywu-msft · 2022-06-14T18:01:40Z

/azp run Linux OpenVINO CI Pipeline

azure-pipelines · 2022-06-14T18:01:50Z

Azure Pipelines successfully started running 1 pipeline(s).

setup.py

lgtm-com · 2022-06-15T10:53:58Z

This pull request introduces 1 alert when merging de834d5 into a805a49 - view on LGTM.com

new alerts:

1 for Use of the return value of a procedure

jywu-msft · 2022-06-16T03:06:59Z

please fix the Lint / Python format errors. there are still 3 or 4 issues left to fix.

Signed-off-by: Preetha Veeramalai <[email protected]>

lgtm-com · 2022-06-16T13:51:37Z

This pull request introduces 1 alert when merging 1a63a21 into 3f8c914 - view on LGTM.com

new alerts:

1 for Use of the return value of a procedure

jywu-msft · 2022-06-16T16:59:22Z

/azp run Linux OpenVINO CI Pipeline

azure-pipelines · 2022-06-16T16:59:32Z

Azure Pipelines successfully started running 1 pipeline(s).

lgtm-com · 2022-06-17T06:39:55Z

This pull request introduces 1 alert when merging d8e3843 into 10478a0 - view on LGTM.com

new alerts:

1 for Use of the return value of a procedure

sfatimar · 2022-06-17T12:44:11Z

@jywu-msft The pipelines failed with the same error. Can we modify the requirements.txt in https://github.com/intel/onnxruntime/blob/master/tools/ci_build/github/linux/docker/scripts/

Else we can find another solutions like modifying Dockerfile.ubuntu_openvino to install the wheel package directly...

jywu-msft · 2022-06-17T14:37:23Z

@jywu-msft The pipelines failed with the same error. Can we modify the requirements.txt in https://github.com/intel/onnxruntime/blob/master/tools/ci_build/github/linux/docker/scripts/

Else we can find another solutions like modifying Dockerfile.ubuntu_openvino to install the wheel package directly...

I think you would need to update requirements.txt since that import would be invoked for all other python package builds right?
can you find the specific version of wheel that needs to be added to requirements.txt?

sfatimar · 2022-06-17T16:56:49Z

@jywu-msft The pipelines failed with the same error. Can we modify the requirements.txt in https://github.com/intel/onnxruntime/blob/master/tools/ci_build/github/linux/docker/scripts/
Else we can find another solutions like modifying Dockerfile.ubuntu_openvino to install the wheel package directly...

I think you would need to update requirements.txt since that import would be invoked for all other python package builds right? can you find the specific version of wheel that needs to be added to requirements.txt?

@jywu-msft wheel>=0.35.1

jywu-msft · 2022-06-17T17:06:56Z

@jywu-msft The pipelines failed with the same error. Can we modify the requirements.txt in https://github.com/intel/onnxruntime/blob/master/tools/ci_build/github/linux/docker/scripts/
Else we can find another solutions like modifying Dockerfile.ubuntu_openvino to install the wheel package directly...

I think you would need to update requirements.txt since that import would be invoked for all other python package builds right? can you find the specific version of wheel that needs to be added to requirements.txt?

@jywu-msft wheel>=0.35.1

make the change and i'll kick off the CI again

jywu-msft · 2022-06-17T18:05:02Z

/azp run Linux OpenVINO CI Pipeline

azure-pipelines · 2022-06-17T18:05:21Z

Azure Pipelines successfully started running 1 pipeline(s).

jywu-msft · 2022-06-17T18:58:06Z

/azp run Windows GPU TensorRT CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline, onnxruntime-python-checks-ci-pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed

jywu-msft · 2022-06-17T18:58:45Z

/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux Nuphar CI Pipeline, MacOS CI Pipeline, ONNX Runtime Web CI Pipeline, Windows CPU CI Pipeline, Windows GPU CI Pipeline

azure-pipelines · 2022-06-17T18:58:47Z

Azure Pipelines successfully started running 6 pipeline(s).

azure-pipelines · 2022-06-17T18:59:44Z

Azure Pipelines successfully started running 9 pipeline(s).

lgtm-com · 2022-06-17T19:42:41Z

This pull request introduces 1 alert when merging 3842635 into 4ac72e3 - view on LGTM.com

new alerts:

1 for Use of the return value of a procedure

snnn · 2022-07-13T05:50:35Z

tools/ci_build/github/linux/docker/Dockerfile_manylinux2014_openvino_multipython

+RUN yum -y install wget git
+
+# libusb1.0.22
+RUN cd /home/ && wget https://github.com/libusb/libusb/archive/v1.0.22.zip && \


libusb is GPL. It implies obligations. I will consult our compliance team on whether we need to register this component to our security&compliance database or not. @jywu-msft , FYI.

This is to enable VPU for OpenVINO Execution Provider..

snnn · 2022-07-13T05:52:49Z

tools/ci_build/github/linux/docker/Dockerfile_manylinux2014_openvino_multipython

@@ -0,0 +1,82 @@
+FROM quay.io/pypa/manylinux2014_x86_64:latest


So what is this file for? It looks like it is very different than the other docker files in this dir because it takes dependency on ORT code, and build arbitrary branches, not the current code. It makes feel that you put this file in wrong place.

This file is to build OpenVINO Execution Provider for ORT. Last time we needed to build a special branch but going forward we can build on master. Anyways the input branch is a Docker argument so by default we can keep it as master

But the other docker files in this directory do not do "git checkout https://github.com/Microsoft/onnxruntime". Why does this one need to?
And, for you information, I'm asked to remove all references to "quay.io/pypa/:". For now I will not touch this file. But I want to know more about this file, how is it used and how to test it when I need to make changes to it?

snnn · 2022-07-13T05:56:42Z

cmake/onnxruntime.cmake

@@ -105,6 +105,7 @@ target_compile_definitions(onnxruntime PRIVATE VER_MINOR=${VERSION_MINOR_PART})
 target_compile_definitions(onnxruntime PRIVATE VER_BUILD=${VERSION_BUILD_PART})
 target_compile_definitions(onnxruntime PRIVATE VER_PRIVATE=${VERSION_PRIVATE_PART})
 target_compile_definitions(onnxruntime PRIVATE VER_STRING=\"${VERSION_STRING}\")
+target_compile_definitions(onnxruntime PRIVATE FILE_NAME=\"onnxruntime.dll\")


Why was this change made? How does it relate to #11058 ?

this change was needed because the onnxruntime.rc was hard coding onnxruntime.dll and we needed to change to ensure that filedetails for onnxruntime_providers_shared.dll were updated properly

The file was created by Microsoft Windows team, and it was specific for that use. You can see it claims the file is a part of Windows. But Windows does not ship onnxruntime_providers_shared.dll and the other DLLs.

snnn · 2022-07-13T06:08:03Z

cmake/onnxruntime_providers.cmake

@@ -794,7 +802,7 @@ if (onnxruntime_USE_OPENVINO)
  endif()

  source_group(TREE ${ONNXRUNTIME_ROOT}/core FILES ${onnxruntime_providers_openvino_cc_srcs})
-  onnxruntime_add_shared_library_module(onnxruntime_providers_openvino ${onnxruntime_providers_openvino_cc_srcs})
+  onnxruntime_add_shared_library_module(onnxruntime_providers_openvino ${onnxruntime_providers_openvino_cc_srcs} "${ONNXRUNTIME_ROOT}/core/dll/onnxruntime.rc")


You can't say Microsoft is the author of the OpenVino EP DLL. Unless it meets Microsoft compliance requirements, it can't be published under the name of Microsoft. For example, at very minimum Microsoft's legal team needs to know if this DLL contains any GPL code.

I guess if it is breaking a security code we could create a new rc document for it. @jywu-msft can comment

This changes is required to ensure that we are able to host signed binaries in public domain

I have concerns on this. You can't publish binaries under the name of Microsoft and let Microsoft sign the binaries, when Microsoft's security and compliance team doesn't know anything about the binaries. If you do both of the things with your company's identity, then I won't have any concern because it is your business.

Microsoft is signing the onnxruntime.dll and onnxruntime_providers_shared.dll and Intel is signing the onnxruntime_providers_openvino.dll... that is the current process... It is the one we are following with MSFT and communicated to George too...

will follow up about this offline.

I guess if it is breaking a security code we could create a new rc document for it. @jywu-msft can comment

this part seems like a problem then. it shouldn't be marked as created by microsoft.

snnn · 2022-07-13T06:17:20Z

setup.py

+
+        if is_openvino and is_manylinux:
+
+            def get_tag(self):


Why this part is needed? manylinux's audithwheel tool only supports very a few tags(glibc verions). Here you force changing their tag, which means this code won't support most Linux distro versions. It would only work in very specific environments.

Why do you need to change the tag? Why can't let the docker image provide this?

We have training components also included so auditwheel repair is disabled. And we were not getting a manylinux tag if we did not run auditwheel repair

When you do not want to run auditwheel, you can see we have a flag to disable it.
When you want to run it, the tool get tag information from env. It uses AUDITWHEEL_PLAT environment variable as a default option to --plat. Usually the env is set correctly in manylinux docker images. So, may I know in which case you need to manually set it by using the information from ldd?

snnn · 2022-07-13T06:22:36Z

setup.py

+
+class InstallCommand(InstallCommandBase):
+    def finalize_options(self):
+        ret = InstallCommandBase.finalize_options(self)


Why this class is needed?
Our linter reports an error on this line. It says InstallCommandBase.finalize_options doesn't return any value.

I guess we can try removing this and check if it works out...

snnn · 2022-07-13T06:39:45Z

This changes is required to ensure that we are able to host signed binaries in public domain

~~I have concerns on this.~~ ~~You can't publish binaries under the name of Microsoft and let Microsoft sign the binaries, when Microsoft's security and compliance team doesn't know anything about the binaries.~~ ~~If you do both of the things with your company's identity, then I won't have any concern because it is your business.~~

sfatimar · 2022-07-13T06:49:41Z

This changes is required to ensure that we are able to host signed binaries in public domain

I have concerns on this. You can't publish binaries under the name of Microsoft and let Microsoft sign the binaries, when Microsoft's security and compliance team doesn't know anything about the binaries. If you do both of the things with your company's identity, then I won't have any concern because it is your business.

Microsoft is signing the onnxruntime.dll and onnxruntime_providers_shared.dll and Intel is signing the onnxruntime_providers_openvino.dll... that is the current process... It is the one we are following with MSFT and communicated to George too...

sfatimar · 2022-07-13T06:50:34Z

The change will still stand because onnxruntime_providers_shared.dll had incorrect File Details Info

* Update Python dependencies following upstream [1]. * Rebase patches for devendoring after upstream changes [2]. * Avoid wheel.vendored, which is needed since [3] while devendored in Arch [4]. [1] microsoft/onnxruntime#11522 [2] microsoft/onnxruntime#11146 [3] microsoft/onnxruntime#11834 [4] archlinux/svntogit-community@e691288

preetha-intel reviewed Jun 14, 2022

View reviewed changes

jywu-msft reviewed Jun 14, 2022

View reviewed changes