Layer dev by walrusmcd · Pull Request #2406 · microsoft/onnxruntime

walrusmcd · 2019-11-15T19:27:20Z

temp

* Task 23998197: add winml_lib_core into onnnxruntime.dll * PR feedback build break on perf_test

#2382) this is a big PR. we are going to move it up to layer_dev , which is still a L3 so we are still safe to do work there agile. we are going to move this into the L3 so that ryan can start doing intergration testing. we will pause for a full code review and integration test result prior to going into the L2. >>>> raw comments from previous commits >>> * LearningModelSession is cleaned up to use the adapter, and parts of binding are. * moved everything in the winmladapter made it all nano-com using, WRL to construct objects in the ORT side. base interfaces for everythign for winml to call cleaned up a bunch of winml to use the base interfaces. * more pieces * GetData across the abi. * renamed some namepsace cleaned up OrtValue cleaned up Tensor cleaned up custom ops. everything *but* learnignmodel should be clean * make sure it's building. winml.dll is still a monolith.

…into layer_dev

everything builds clean. step !

* model moved over. everything builds clean. step ! * weak ref comment

martinb35 · 2019-11-15T22:36:43Z

cmake/CMakeLists.txt

-        set(CMAKE_${type}_LINKER_FLAGS_RELWITHDEBINFO "${CMAKE_${type}_LINKER_FLAGS_RELWITHDEBINFO} /OPT:REF,ICF,LBR")
-        set(CMAKE_${type}_LINKER_FLAGS_RELWITHDEBINFO "${CMAKE_${type}_LINKER_FLAGS_RELWITHDEBINFO} /INCREMENTAL:NO")
+      foreach(type EXE STATIC SHARED)
+        if (NOT type MATCHES STATIC)


STATIC [](start = 29, length = 6)

does this affect other binaries or just ours? #Closed

adding the STATIC in here makes it affect all static link libs. it seemed to make sense to apply everywhere. the problem is the linker has to take 2 passes if it needs to use LTCG and you didn't tell it before hand. so all the static link libs where starting their link phase, then resetting once they realized they needed LTCG.

In reply to: 347038839 [](ancestors = 347038839)

martinb35 · 2019-11-15T22:37:25Z

cmake/onnxruntime.cmake

    ${PROVIDERS_NUPHAR}
    ${PROVIDERS_DML}
    ${PROVIDERS_ACL}
+	${onnxruntime_winml}


[](start = 0, length = 1)

spaces #Closed

good catch !

In reply to: 347039014 [](ancestors = 347039014)

martinb35 · 2019-11-15T22:38:58Z

cmake/winml.cmake

+# add it to the onnxruntime shared library
+set(onnxruntime_winml windowsapp.lib -WHOLEARCHIVE:$<TARGET_FILE:winml_lib_core>)
+list(APPEND onnxruntime_EXTERNAL_DEPENDENCIES winml_lib_core)
+


is this going to live here in the final version? seems odd to add things to a different project #Closed

that was the idea. onnxruntime_EXTERNAL_DEPENDENCIES is a property setup just for this purpose. for other cmake files to add things to the the final shared_lib

In reply to: 347039511 [](ancestors = 347039511)

martinb35 · 2019-11-15T22:39:19Z

cmake/winml.cmake

+#target_link_libraries(winml_dll PRIVATE onnxruntime_providers_dml)
+#target_link_libraries(winml_dll PRIVATE onnxruntime_session)
+#target_link_libraries(winml_dll PRIVATE onnxruntime_util)
+#target_link_libraries(winml_dll PRIVATE onnx_proto)


why not just delete these instead of commenting? #Closed

martinb35 · 2019-11-15T22:39:37Z

cmake/winml.cmake

+target_link_libraries(winml_dll PRIVATE windowsapp.lib)
 target_link_libraries(winml_dll PRIVATE winml_lib_api)
-target_link_libraries(winml_dll PRIVATE winml_lib_core)
+#target_link_libraries(winml_dll PRIVATE winml_lib_core)


delete #Closed

martinb35 · 2019-11-15T22:40:09Z

cmake/winml.cmake

+#target_link_libraries(winml_dll PRIVATE winml_lib_core)
 target_link_libraries(winml_dll PRIVATE winml_lib_image)
 target_link_libraries(winml_dll PRIVATE winml_lib_telemetry)
-target_link_libraries(winml_dll PRIVATE onecoreuap_apiset.lib)


onecoreuap_apiset [](start = 40, length = 17)

intentional? you replaced onecoreuap_apiset.lib with windowsapp.lib

yeah, not sure why i did that.

In reply to: 347039801 [](ancestors = 347039801)

this doesn't seem resolved yet...

In reply to: 347539379 [](ancestors = 347539379,347039801)

martinb35 · 2019-11-15T22:41:36Z

include/onnxruntime/core/common/status.h

+    default:
+        return E_FAIL;
+    }
+}


this exists somewhere else, too right? in winml somewhere. can we share it instead of copying it? #Closed

nm. I see now that you removed it from the winml spot

In reply to: 347040246 [](ancestors = 347040246)

martinb35 · 2019-11-15T22:43:55Z

winml/api/Windows.AI.MachineLearning.idl

+                TensorKind KeyKind,
+                ILearningModelFeatureDescriptor ValueDescriptor
+            );
+        }


do we need to do API review? #Closed

also can you explain what these new methods are for?

In reply to: 347040835 [](ancestors = 347040835)

yep. we will need an API review. these new methogs allow creation of feature descriptors outside the runtime (the code comment there). basically before this change you could never create these on their own but had to have the runtime make then inside of models and return them. this change adds a factory so that other people can create them. ORT creates them in this PR. and in the future people like Vision Skills could create then also. it makes them reusable currencies.

In reply to: 347040969 [](ancestors = 347040969,347040835)

martinb35 · 2019-11-15T22:47:46Z

winml/lib/Api/LearningModel.h

      bool& use_fp16);

-  void
-  OverrideShapeInferenceMethods();


OverrideShapeInferenceMethods [](start = 2, length = 29)

where did this method go? #Closed

IWinMLAdapter::OverrideSchemaInferenceFunctions()

In reply to: 347041742 [](ancestors = 347041742)

martinb35 · 2019-11-15T22:49:06Z

winml/lib/Api/LearningModelBinding.cpp

+_winmla::IIOBinding* LearningModelBinding::BindingCollection() {
+    _winmla::IIOBinding* p;
+    m_lotusBinding.copy_to(&p);
+    return p;


is this a copy or an add-ref? #Closed

copy_to bumps the ref, so it is returning a strong ref. the caller needs to handle it. in this case there is one caller, in learningmodelsession.cpp line #206

In reply to: 347042063 [](ancestors = 347042063)

martinb35 · 2019-11-15T22:50:34Z

winml/lib/Api/LearningModelSession.cpp

-  auto& io_binding = binding_impl->BindingCollection();
-  auto& bound_output_names = io_binding.GetOutputNames();
+    com_ptr<_winmla::IIOBinding> io_binding;
+    io_binding.attach(binding_impl->BindingCollection());


spacing #Closed

good catch !

In reply to: 347042505 [](ancestors = 347042505)

martinb35 · 2019-11-15T22:51:18Z

winml/lib/Api/LearningModelSession.cpp


  // Invoke run on the ORT session.
-  WINML_THROW_IF_NOT_OK(inference_session_->Run(run_options, io_binding));
+  WINML_THROW_IF_FAILED(inference_session_->Run(&run_options, io_binding.get()));


WINML_THROW_IF_FAILED [](start = 2, length = 21)

NOT_OK is not the same as FAILED - why this change? #Closed

nope. not the same. NOT_OK is for status.IsOK() checks. This moved from an InferenceSession (ort type) to a IInferenceSession (com_ptr) and uses HRESULTS now across the ABI. So it had to change from NOT_OK to FAILED.

In reply to: 347042739 [](ancestors = 347042739)

martinb35 · 2019-11-15T22:55:39Z

winml/lib/Api.Core/WinMLAdapter.cpp

+
+// class AbiSafeTensor
+//
+class AbiSafeTensor : public Microsoft::WRL::RuntimeClass <


AbiSafe [](start = 6, length = 7)

are these names temporary? I don't love the "ABISafe" prefix... #Closed

temporary.

In reply to: 347043931 [](ancestors = 347043931)

martinb35 · 2019-11-15T22:57:04Z

winml/lib/Api.Core/WinMLAdapter.cpp

+    ITensor> {
+private:
+    onnxruntime::Tensor& tensor_;   // weak ref
+    ComPtr<IOrtValue> value_;       // strong ref


I believe our code convention puts data members at the end of a class, right? #Closed

probably. i changed it to the top when the classes are coded inside the CPP file (also not something we normally do) . when you put them at the bottom they are much harder to find.

In reply to: 347044267 [](ancestors = 347044267)

martinb35 · 2019-11-15T22:58:22Z

winml/lib/Api.Core/WinMLAdapter.cpp

+        THROW_HR_IF_MSG(
+            E_FAIL,
+            0 > file_descriptor,
+            "Failed");  //errno


can you integrate the file not found change I just made? #Closed

how do i do that?

In reply to: 347044532 [](ancestors = 347044532)

i figured it out. the change is here: https://dev.azure.com/microsoft/WindowsAI/_git/WindowsAI/pullrequest/4014934?_a=files&path=%2Fwinml%2Flib%2FCore%2FModelInfo.cpp

In reply to: 347543900 [](ancestors = 347543900,347044532)

martinb35 · 2019-11-15T22:59:23Z

winml/lib/Api.Core/WinMLAdapter.cpp

+HRESULT STDMETHODCALLTYPE OrtGetWinMLAdapter(IWinMLAdapter** adapter) {
+    // make an adapter instance
+    Microsoft::WRL::ComPtr<WinMLAdapter> adapterptr = wil::MakeOrThrow<WinMLAdapter>();
+    return adapterptr.CopyTo(__uuidof(IWinMLAdapter), (void **)adapter);


(void **) [](start = 54, length = 9)

static_cast? #Closed

static_cast doesn't work to void ** . fun, right?

https://stackoverflow.com/questions/3625410/c-static-cast-from-float-to-void

In reply to: 347044742 [](ancestors = 347044742)

reinterpret! that's what I meant. my bad.

In reply to: 347542211 [](ancestors = 347542211,347044742)

martinb35

🕐

…r creating winml objects. fixes model load.

* model moved over. everything builds clean. step ! * weak ref comment * added a wrapper for RoGetActivationFactory to hook back into winml for creating winml objects. fixes model load.

ryanlai2 · 2019-11-18T19:30:17Z

cmake/onnxruntime.cmake

    ${PROVIDERS_NUPHAR}
    ${PROVIDERS_DML}
    ${PROVIDERS_ACL}
+    ${onnxruntime_winml}


onnxruntime_winml [](start = 6, length = 17)

Should the naming have adapter / core in there? I had to check in winml.cmake to double check which lib(s) were getting included

ryanlai2 · 2019-11-19T01:52:42Z

winml/lib/Api.Core/WinMLAdapter.cpp

+    }
+    std::vector<IOrtValue*>& STDMETHODCALLTYPE GetOutputs() override {
+        auto output_inner = binding_->GetOutputs();
+        outputs_.clear();


Also need to clear outputs_weak vector holding a vector of weak ptrs :) #Closed

* model moved over. everything builds clean. step ! * weak ref comment * added a wrapper for RoGetActivationFactory to hook back into winml for creating winml objects. fixes model load. * fixed some lifetime management. fixed the debug build. squeezenet passes using winmlrunner for CPU and GPU * PR feedback. * couple of fixes and coded getmutabledata()

* model moved over. everything builds clean. step ! * weak ref comment * added a wrapper for RoGetActivationFactory to hook back into winml for creating winml objects. fixes model load. * fixed some lifetime management. fixed the debug build. squeezenet passes using winmlrunner for CPU and GPU * PR feedback. * couple of fixes and coded getmutabledata() * fixed 2 more heap corruptions

found a leak in nvidia driver, but skipped it. all winmlapitests pass now

ryanlai2 · 2019-11-21T18:38:05Z

winml/lib/Api.Core/WinMLAdapter.cpp

+#include "ImageFeatureDescriptor.h"
+#include "api.image/inc/D3DDeviceCache.h"
+
+#include "PheonixSingleton.h"


Already included "PheonixSingleton.h" up above #Closed

* Register WinML TraceLogging provider on Onnxruntime.dll * Add ifdef to make sure trace logging provider has telemetry option when LAYERING_DONE * No need for ifdef for TraceLoggingOptionMicrosoftTelemetry * PR feedback * Move etw registration into lotus environment constructor and deresgister in lotus environment destructor

* allow building winml cpu without dml.

* fix some more breaks * learning model doesn't need lotusEnvironment and CPU shouldn't include dmlEP headers * move dml checks out of winml and into the adapter * better error handling

* learning model doesn't need lotusEnvironment and CPU shouldn't include dmlEP headers * User/xianz/win ml telemetry (#2410) * add option to enable winml telemetry * add option to enable winml telemetry * clean logs while developping * clean the log of GUID * compile onnxruntime_common with winml telemetry * use option for use_telemetry * rename option winml_use_telemetry to onnxruntime_use_telemetry * little change * Add opset and IR check when loading model (#2413) * Add opset and IR check. * Add test case for future opsets. #2371 * WinML CI (#2412) * Pass flags to build/test WinML in CI * Add initial CMake config for unit tests in WinML * Set winml_unittests standard to C++17 * Add WinML API tests and port them to googletest * Install WinML test collateral * Add LearningModelSessionAPITests ported to googletest * Fix WinML test files encoding * Add GPU tests * Add parameterized test, skip GPU tests * Enable precompiled header * Remove unused code and collateral * Remove brand images * Add dllload.cpp * Remove images not used in API tests * Add LICENSE.md to image collaterals * Add models with licenses * Remove FNS Candy tests * Add API test models * Add ModelInSubdirectory * Install collaterals post-build with copy_if_different, split common lib * fix warnings * Link to gtest_main * fix bad merge

…in parrallel

* add missing ir version to dictvectorizer-string.onnx * add missing ir version to relu.onnx * add missing ir version to zipmap*onnx * add IR version to manually generated models * remove an unnecessary ifdef dml

* Add scenario tests * Remove TODO from model license * Add winml_api test dependency

* commetns for dml graph transformer fixed ort value passing using the allocatir info * fixed and coded maps and sequences across the abi

* Support test parameters through CLI arguments * Add WinML do Windows x86/ARM CI builds * Code style fixes * Update googletest Remove GPUTEST macros everywhere now that GTEST_SKIP is supported * Refactor main.cpp * Build scenario tests without DML

Ryan Lai and others added 7 commits November 7, 2019 16:50

Initial changes for layering

444bfcc

more snipping to get core into ort

b6f5eef

Task 23998197: add winml_lib_core into onnnxruntime.dll (#2368)

5406801

* Task 23998197: add winml_lib_core into onnnxruntime.dll * PR feedback build break on perf_test

Merge remote-tracking branch 'origin/windowsai' into layer_dev

8b37bd0

Merge branch 'layer_dev' of https://github.com/microsoft/onnxruntime …

00cee34

…into layer_dev

model moved over.

f07fdf9

everything builds clean. step !

walrusmcd requested a review from a team as a code owner November 15, 2019 19:27

walrusmcd added 3 commits November 15, 2019 13:15

weak ref comment

f32bbd5

Layer dev paulm (#2408)

2bfa3c6

* model moved over. everything builds clean. step ! * weak ref comment

Merge remote-tracking branch 'origin/layer_dev' into layer_dev_paulm

8f95b77

martinb35 reviewed Nov 15, 2019

View reviewed changes

martinb35 requested changes Nov 15, 2019

View reviewed changes

walrusmcd added 2 commits November 15, 2019 16:47

added a wrapper for RoGetActivationFactory to hook back into winml fo…

7f9a7f5

…r creating winml objects. fixes model load.

Layer dev paulm (#2414)

5bd2c1e

* model moved over. everything builds clean. step ! * weak ref comment * added a wrapper for RoGetActivationFactory to hook back into winml for creating winml objects. fixes model load.

ryanlai2 reviewed Nov 18, 2019

View reviewed changes

ryanlai2 reviewed Nov 19, 2019

View reviewed changes

walrusmcd added 9 commits November 18, 2019 18:31

couple of fixes and coded getmutabledata()

acc6ea5

merge

9253e23

Merge remote-tracking branch 'origin/layer_dev' into layer_dev_paulm

72b3a91

fixed 2 more heap corruptions

3da841b

fixed map and sequence when passing stl types across the ABI .

d8941f1

found a leak in nvidia driver, but skipped it. all winmlapitests pass now

merge layer_dev_paulm

94fc7bc

Moved SessionOptions over to the abi

44a4fc0

ryanlai2 reviewed Nov 21, 2019

View reviewed changes

ryanlai2 and others added 17 commits November 22, 2019 07:23

Brianma/cpuwinml (#2466)

d738bdd

* allow building winml cpu without dml.

Brianma/breaks (#2469)

1bc2ca6

* fix some more breaks * learning model doesn't need lotusEnvironment and CPU shouldn't include dmlEP headers * move dml checks out of winml and into the adapter * better error handling

Checking in a staging checkpoint point so that Ryan can work with me …

974ee0b

…in parrallel

merged in brian's changes

78c3c12

build break.

befe933

Brianma/testfails (#2473)

34b1bf2

* add missing ir version to dictvectorizer-string.onnx * add missing ir version to relu.onnx * add missing ir version to zipmap*onnx * add IR version to manually generated models * remove an unnecessary ifdef dml

Add scenario tests (#2457)

f40d5d8

* Add scenario tests * Remove TODO from model license * Add winml_api test dependency

Merge branch 'windowsai' into layer_dev

da24d96

fix bad merge

1d7111f

fix bad model merge

8346ddf

Layer dev paulm (#2492)

c27f56c

* commetns for dml graph transformer fixed ort value passing using the allocatir info * fixed and coded maps and sequences across the abi

Rename ambiguous header (#2489)

0688afa

fix one more missing IR version model (#2500)

15acc7d

add missing IR version to 4 more models used by scenario tests (#2501)

0f9c6a5

walrusmcd closed this Nov 27, 2019

Conversation

walrusmcd commented Nov 15, 2019

Uh oh!

martinb35 Nov 15, 2019 • 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!

martinb35 Nov 15, 2019 • 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!

martinb35 Nov 15, 2019 • 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!

martinb35 Nov 15, 2019 • 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!

martinb35 Nov 15, 2019 • 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!

martinb35 Nov 15, 2019 • 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!

martinb35 Nov 15, 2019 • 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!

martinb35 Nov 15, 2019 • 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!

martinb35 Nov 15, 2019 • 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!

martinb35 Nov 15, 2019 • 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!

martinb35 Nov 15, 2019 • 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!

martinb35 Nov 15, 2019 • 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!

martinb35 Nov 15, 2019 • 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

martinb35 Nov 15, 2019 •

edited

Loading

martinb35 Nov 15, 2019 •

edited

Loading

martinb35 Nov 15, 2019 •

edited

Loading

martinb35 Nov 15, 2019 •

edited

Loading

martinb35 Nov 15, 2019 •

edited

Loading

martinb35 Nov 15, 2019 •

edited

Loading

martinb35 Nov 15, 2019 •

edited

Loading

martinb35 Nov 15, 2019 •

edited

Loading

martinb35 Nov 15, 2019 •

edited

Loading

martinb35 Nov 15, 2019 •

edited

Loading

martinb35 Nov 15, 2019 •

edited

Loading

martinb35 Nov 15, 2019 •

edited

Loading

martinb35 Nov 15, 2019 •

edited

Loading

martinb35 Nov 15, 2019 •

edited

Loading

martinb35 Nov 15, 2019 •

edited

Loading

martinb35 Nov 15, 2019 •

edited

Loading

ryanlai2 Nov 19, 2019 •

edited

Loading

ryanlai2 Nov 21, 2019 •

edited

Loading