Fix the warnings found by VC++ static code analyzer and add it to the CPU CI build #7121

snnn · 2021-03-24T03:50:20Z

Description:

Fix the warnings found by VC++ static code analyzer and add it to the CPU CI build
Update pybind11 to a newer version because it contains a warning fix from me.
Remove the cancel function of the Eigen thread pool. It is not used anywhere, and we don't know if it ever works.

Motivation and Context

Why is this change required? What problem does it solve?

Before this change, we even didn't test any EPs(other than CPU) in the java code.

If it fixes an open issue, please link to the issue here.

onnxruntime/test/contrib_ops/shape_inference_test_helper.h

skottmckay · 2021-03-24T04:17:48Z

tools/ci_build/github/azure-pipelines/win-ci-pipeline.yml

+    condition: and(succeeded(), eq(variables['BuildConfig'], 'Debug'))
+    inputs:
+      scriptPath: '$(Build.SourcesDirectory)\tools\ci_build\build.py'
+      arguments: '--config Debug --build_dir $(Build.BinariesDirectory) --skip_submodule_sync --build_shared_lib --update --cmake_generator "Visual Studio 16 2019" --build_wheel --build_shared_lib --enable_onnx_tests --cmake_extra_defines onnxruntime_ENABLE_STATIC_ANALYSIS=ON'


--enable_onnx_tests [](start = 197, length = 19)

do we need stuff like enabling the onnx tests if we're doing static analysis, or can we just keep it to the core code (could maybe turn off building of all the unit tests to make it faster as well)

The "enable_onnx_tests" build flags doesn't control if the unit tests would be built. onnxruntime_BUILD_UNIT_TESTS does that. I think it's OK to keep it ON, it won't cost much time. Either works.

skottmckay · 2021-03-24T04:21:54Z

include/onnxruntime/core/platform/EigenNonBlockingThreadPool.h

 }

-void Cancel() override {
-  cancelled_ = true;


Would we need this if we did anything more for parallel and/or async execution?

I think it is mainly needed for fast failing. Suppose you have summited a "parallel for" code block with 4 threadpool tasks to a thread pool, and one of the tasks failed. If you want to quickly cancel the other 3 without continue to wait them being finished, then a cancel method would help. If everything was fine, then we don't need to cancel the pending or running tasks.

onnxruntime/test/mlas/unittest.cpp

snnn · 2021-03-25T23:45:11Z

I'm still working on this.

snnn · 2021-04-09T00:32:19Z

Now it's ready for review.

snnn · 2021-04-09T00:33:30Z

cmake/onnxruntime_python.cmake

-endif()
-
-if (onnxruntime_USE_OPENVINO)
+if(NOT onnxruntime_ENABLE_STATIC_ANALYSIS)


I added a "if() ... endif()". The code between them is unchanged.

snnn · 2021-04-09T00:34:07Z

java/src/main/native/OrtJniUtil.c

    switch (code) {
        case ORT_OK:
-			return 0;
+            return 0;


I changed tabs to spaces.

Craigacp · 2021-04-14T12:37:48Z

cmake/onnxruntime_java.cmake

+onnxruntime_add_shared_library_module(onnxruntime4j_jni ${onnxruntime4j_native_src})
 set_property(TARGET onnxruntime4j_jni PROPERTY CXX_STANDARD 11)

-# Tell the JNI code about the requested providers


Why is this code removed? It's used to cause the JNI code to emit a well formed exception if the provider isn't available.

I moved it to CMakeLists.txt. And I think the things like "USE_OPENVINO" should not be used in the JNI project. I suggest the macro should be always on.

Now that the provider list exists in the C API I guess I can check that and throw the exception, but if you look in SessionOptions.c it's used to either compile the JNI method so it always throws a Java exception, or to compile it so it adds the requested provider. I'm not sure how stable that provider list is though, as it's not in the C API itself, I've got to manually copy the string constants into Java and check them.

Craigacp · 2021-04-14T12:47:29Z

java/src/main/java/ai/onnxruntime/OrtSession.java

-     * @param deviceId The id of the OpenVINO execution device.
-     * @throws OrtException If there was an error in native code.
-     */
-    public void addOpenVINO(String deviceId) throws OrtException {


I realise these need shared library support which isn't currently possible in Java, but is there some other reason they are being removed? Are these two providers deprecated?

What do you mean isn't currently possible ?

The changes which broke out OpenVINO, DNNL and TensorRT into separate shared libraries broke the Java API wrappers around those providers. I opened #6553 as we'll package those shared libraries into the jar, and so need to be able to load them from a non-standard place, but as there's been no progress there they are broken in the current release and it's been broken since those providers are converted into shared libraries. When this (https://communities.sas.com/t5/SAS-Communities-Library/SAS-and-Microsoft-collaborate-to-democratize-the-use-of-Deep/ta-p/730072) lands it'll break CUDA too.

BTW I've got the code ready to go to move these providers over to the new API point (with the struct), but I can't test it because Java can't load the library.

jywu-msft · 2021-04-14T15:35:57Z

include/onnxruntime/core/providers/openvino/openvino_provider_factory.h

- * \param device_type openvino device type and precision. Could be any of
- * CPU_FP32, GPU_FP32, GPU_FP16, MYRIAD_FP16, VAD-M_FP16 or VAD-F_FP32.
- */
-ORT_API_STATUS(OrtSessionOptionsAppendExecutionProvider_OpenVINO,


why are these api's being removed for OpenVINO and TensorRT?

This is the deprecated one, and it misses features.

okay. originally i was going to let Intel handle it, but looks like they didn't remove this yet.

snnn · 2021-04-14T19:31:41Z

csharp/src/Microsoft.ML.OnnxRuntime/NativeMethods.cs

+        // End of Version 6 - DO NOT MODIFY ABOVE (see above text for more information)
+
        public IntPtr ModelMetadataGetGraphDescription;
+        public IntPtr SessionOptionsAppendExecutionProvider_TensorRT;


This is the new API we should use

snnn · 2021-04-14T19:35:21Z

I'll wait #7315 merge first.

snnn · 2021-04-15T23:08:18Z

I will split it to smaller pieces.

snnn requested a review from a team as a code owner March 24, 2021 03:50

skottmckay reviewed Mar 24, 2021

View reviewed changes

onnxruntime/test/contrib_ops/shape_inference_test_helper.h Outdated Show resolved Hide resolved

skottmckay reviewed Mar 24, 2021

View reviewed changes

tracysh reviewed Mar 24, 2021

View reviewed changes

onnxruntime/test/mlas/unittest.cpp Outdated Show resolved Hide resolved

snnn closed this Mar 29, 2021

snnn reopened this Mar 29, 2021

snnn force-pushed the snnn/vs2019 branch from 267c989 to a3c19b2 Compare March 29, 2021 19:11

snnn force-pushed the snnn/vs2019 branch 2 times, most recently from c35ff35 to f1839b4 Compare April 8, 2021 20:41

Changming Sun added 4 commits April 8, 2021 16:16

update

e5e5eb3

nodejs

f784b4c

update

0269c70

update

0ce6767

snnn force-pushed the snnn/vs2019 branch from f1839b4 to 0ce6767 Compare April 8, 2021 23:16

snnn commented Apr 9, 2021

View reviewed changes

java/src/main/native/OrtJniUtil.c

switch (code) {

case ORT_OK:

return 0;

return 0;

Copy link

Contributor Author

snnn Apr 9, 2021

Choose a reason for hiding this comment

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

I changed tabs to spaces.

revert

42c525b

snnn requested a review from skottmckay April 9, 2021 14:42

Changming Sun added 9 commits April 9, 2021 07:52

revert

af5989a

Merge remote-tracking branch 'origin/master' into snnn/vs2019

aaaa762

Merge remote-tracking branch 'origin/master' into snnn/vs2019

87b16af

update

c41167e

merge

f0ec067

update

82ebd94

Merge remote-tracking branch 'origin/master' into snnn/vs2019

bd2f376

update

540ce45

update

0cc97ae

Changming Sun added 2 commits April 14, 2021 00:47

update

500db5e

fix

d365021

snnn mentioned this pull request Apr 14, 2021

C# console app crash upon appending OpenVino execution provider #7330

Closed

Changming Sun added 2 commits April 14, 2021 01:19

update

49ea81f

update

d530ed2

Craigacp reviewed Apr 14, 2021

View reviewed changes

jywu-msft reviewed Apr 14, 2021

View reviewed changes

Changming Sun added 3 commits April 14, 2021 09:10

Merge remote-tracking branch 'origin/master' into snnn/vs2019

bacfc13

update

45eea68

update

4ae1613

snnn commented Apr 14, 2021

View reviewed changes

Changming Sun and others added 11 commits April 14, 2021 13:27

update

2960868

fix

21e2f27

update

6f22a1b

fix

9214a63

fix

d897e38

update

a4f68ea

TRT

12f2481

update

5e9b1ea

Merge remote-tracking branch 'origin/master' into snnn/vs2019

be3e5f4

Merge remote-tracking branch 'origin/master' into snnn/vs2019

4a83b6a

update

4f0dd4f

snnn force-pushed the snnn/vs2019 branch from b5a9175 to 4f0dd4f Compare April 15, 2021 19:27

Changming Sun added 3 commits April 15, 2021 13:01

update

172745c

update

ecdad2d

update

3aa73ef

snnn closed this Apr 15, 2021

Fix the warnings found by VC++ static code analyzer and add it to the CPU CI build #7121

Fix the warnings found by VC++ static code analyzer and add it to the CPU CI build #7121

Uh oh!

Conversation

snnn commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

snnn commented Mar 25, 2021

Uh oh!

snnn commented Apr 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snnn Apr 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jywu-msft Apr 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snnn commented Apr 14, 2021

Uh oh!

snnn commented Apr 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

snnn commented Mar 24, 2021 •

edited

Loading

snnn Apr 14, 2021 •

edited

Loading

jywu-msft Apr 14, 2021 •

edited

Loading