Save per-iteration Output Tensor Results #113

laks0209 · 2018-12-18T06:42:10Z

Command line argument -saveTensorData dumps First or All iteration Output Tensor results to csv files
Output tensor corresponding to each iteration is saved in separate csv
files
Summary.csv file contains the final result of each iteration, hash of
output tensor and pointer to the dumped output tensor file

ryanlai2 · 2018-12-20T18:32:33Z

Tools/WinMLRunner/WinMLRunner.sln

 		{81EA9CC6-8A26-4583-B1A4-84740EF815C8} = {81EA9CC6-8A26-4583-B1A4-84740EF815C8}
 	EndProjectSection
 EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "ClassLibrary1", "..\ClassLibrary1\ClassLibrary1.csproj", "{12E5A5A7-E32C-4D2E-84DC-E937BE0A9DA8}"


Was a Csharp project supposed to be added to this solution in the PR with the name: ClassLibrary1?

ryanlai2 · 2018-12-20T18:41:14Z

Tools/WinMLRunner/WinMLRunner.vcxproj

    <Link>
      <SubSystem>Console</SubSystem>
-      <AdditionalDependencies>dxgi.lib;d3d12.lib;windowsapp.lib;%(AdditionalDependencies)</AdditionalDependencies>
+      <AdditionalDependencies>dxgi.lib;d3d12.lib;windowsapp.lib;mscoree.lib;%(AdditionalDependencies)</AdditionalDependencies>


Why is mscoree.lib needed as a dependency? From my understanding this is related to .NET Framework

ryanlai2 · 2018-12-20T18:44:58Z

Tools/WinMLRunner/WinMLRunner.vcxproj

    <UseDebugLibraries>true</UseDebugLibraries>
    <PlatformToolset>v141</PlatformToolset>
    <CharacterSet>Unicode</CharacterSet>
+    <CLRSupport>false</CLRSupport>


Why do we need to specify CLRSupport? Isn't this related to .NET?

laks0209

.NET framework support has been removed. Please review the new commit.

laks0209 · 2019-01-07T18:47:09Z

Made modifications according to the per-iteration performance dump. Now, the summary.csv file contains the per-iteration performance results and the final result. Please review the latest commit.

ryanlai2 · 2019-01-07T18:58:21Z

Tools/WinMLRunner/WinMLRunner.sln

 		{E9D4AC92-8295-4FB4-BF7D-3FAF74B564E8}.Debug|ARM64.ActiveCfg = Debug|Win32
-		{E9D4AC92-8295-4FB4-BF7D-3FAF74B564E8}.Debug|x64.ActiveCfg = Debug|x64
-		{E9D4AC92-8295-4FB4-BF7D-3FAF74B564E8}.Debug|x64.Build.0 = Debug|x64
+		{E9D4AC92-8295-4FB4-BF7D-3FAF74B564E8}.Debug|x64.ActiveCfg = Release|x64


When you change this, it builds release when debug is specified.

ryanlai2 · 2019-01-07T19:13:46Z

Tools/WinMLRunner/WinMLRunner.sln

 		Release|x86 = Release|x86
 	EndGlobalSection
 	GlobalSection(ProjectConfigurationPlatforms) = postSolution
+		{81EA9CC6-8A26-4583-B1A4-84740EF815C8}.Debug|Any CPU.ActiveCfg = Debug|Win32


With this change, When we specify Any CPU Debug, it'll build to Win32 Debug

Also, Debug|Any CPU.Build.0 is missing. If we click "Build solution" with the configuration: Any Debug | Any CPU, then WinMLRunner won't build.

ryanlai2 · 2019-01-07T19:14:39Z

Tools/WinMLRunner/WinMLRunner.sln

 		{81EA9CC6-8A26-4583-B1A4-84740EF815C8}.Debug|ARM64.Build.0 = Debug|ARM64
-		{81EA9CC6-8A26-4583-B1A4-84740EF815C8}.Debug|x64.ActiveCfg = Debug|x64
-		{81EA9CC6-8A26-4583-B1A4-84740EF815C8}.Debug|x64.Build.0 = Debug|x64
+		{81EA9CC6-8A26-4583-B1A4-84740EF815C8}.Debug|x64.ActiveCfg = Release|x64


When you change this, it builds release when debug is specified.

ryanlai2 · 2019-01-07T19:15:27Z

Tools/WinMLRunner/WinMLRunner.sln

+		{E9D4AC92-8295-4FB4-BF7D-3FAF74B564E8}.Debug|x64.Build.0 = Release|x64
 		{E9D4AC92-8295-4FB4-BF7D-3FAF74B564E8}.Debug|x86.ActiveCfg = Debug|Win32
 		{E9D4AC92-8295-4FB4-BF7D-3FAF74B564E8}.Debug|x86.Build.0 = Debug|Win32
+		{E9D4AC92-8295-4FB4-BF7D-3FAF74B564E8}.Release|Any CPU.ActiveCfg = Release|Win32


With this change, When we specify Any CPU Release, it'll build to Win32 Release

ryanlai2 · 2019-01-08T20:02:02Z

Can we get some test(s) added to test that this works properly? Thanks!

smk2007 · 2019-01-10T23:02:01Z

Tools/WinMLRunner/BindingUtilities.h

+                    com_ptr<ITensorNative> itn = results.Lookup(desc.Name()).as<ITensorNative>();
+                    std::string* Tensor;
+                    uint32_t uCapacity;
+                    HRESULT(itn->GetBuffer(reinterpret_cast<BYTE**>(&Tensor), &uCapacity));


This is not supported for TensorString types. The GetBuffer method will return ERROR_INVALID_FUNCTION here.

smk2007 · 2019-01-10T23:03:21Z

Tools/WinMLRunner/BindingUtilities.h

+                    float* Tensor;
+                    uint32_t uCapacity;
+                    HRESULT(itn->GetBuffer(reinterpret_cast<BYTE**>(&Tensor), &uCapacity));
+                    hash = winrt::impl::hash_data(Tensor, uCapacity);


i dont think the winrt::impl namespace is safe here - the SDK team changes the impl internals quite frequently.

Thanks @smk2007 . Do you have any suggestions for hash function? Please feel free to comment/provide alternatives.

ryanlai2 · 2019-01-10T23:38:04Z

Tools/WinMLRunner/Main.cpp

    for (uint32_t i = 0; i < numIterations; i++)
    {
-        bool captureIterationPerf = (args.PerfCapture() && (!args.IgnoreFirstRun() || i > 0)) || (args.PerIterCapture());
+        bool captureIterationPerf = (args.PerfCapture() && (!args.IgnoreFirstRun() || i > 0)) || args.SaveTensor()  || args.PerIterCapture();;


Why do we need to capture performance when we want to save tensor output? What about the scenario when we just want to save tensor?

ryanlai2 · 2019-01-10T23:41:13Z

Tools/WinMLRunner/Main.cpp

        try
        {
-            model = LoadModel(path, args.PerfCapture() || args.PerIterCapture(), output, args, 0);
+            model = LoadModel(path, args.PerfCapture() || args.SaveTensor() || args.PerIterCapture(), output, args, 0);


Why do we want to capture load model performance if we only want to save tensor?

ryanlai2 · 2019-01-10T23:41:49Z

Tools/WinMLRunner/Main.cpp

                    for (auto deviceCreationLocation : deviceCreationLocations)
                    {
-                        if (args.PerfCapture() || args.PerIterCapture())
+                        if (args.PerfCapture() || args.SaveTensor() || args.PerIterCapture())


Why do we want to worry about performance when saving tensors?

ryanlai2 · 2019-01-10T23:49:52Z

Hey @laks0209, here are some tensor test ideas that @pmbrown1055 and I spoke about:

Running WinMLRunner and outputting tensor CSV files and compare against an expected tensor CSV file to make sure values are correct.

With combinations of

CPU / GPU
Input Image as png, Input Image as CSV, Garbage data that is seeded so output tensor is deterministic

There may be a % threshold of error tolerance needed

laks0209 · 2019-01-14T17:17:45Z

Thanks Ryan for the suggestions/corrections. I will update the code and add test cases as per the conversation.

laks0209 · 2019-01-24T19:03:45Z

Hi @ryanlai2 , Please review the latest commit with the changes and the tests.

ryanlai2 · 2019-02-01T19:47:08Z

Tools/WinMLRunner/src/Run.cpp

 {
    LearningModel model = nullptr;
    output.PrintLoadingInfo(path);
+	model = LoadModelCryptography(path);


Why do we need this for saving output tensor results? Also, won't the below line of code:

model = LearningModel::LoadFromFilePath(path);

overwrite the loading of the model here?

Have made correction in the latest commit.

ryanlai2 · 2019-02-01T19:48:52Z

Testing/WinMLRunnerTest/WinMLRunnerTest.cpp

            Assert::AreEqual(static_cast<size_t>(2), GetOutputCSVLineCount());
        }

+


Nit: Whitespace

Have made correction in the latest commit.

ryanlai2 · 2019-02-01T19:54:21Z

Tools/WinMLRunner/src/BindingUtilities.h

+                }
+                break;
+
+                case TensorKind::Int64:


Can we add a test to test this case if we're going to add it?

Is there a model that we can test for Int64? To verify that this code path works?

ryanlai2 · 2019-02-02T00:22:15Z

Tools/WinMLRunner/src/Run.cpp

    output.PrintLoadingInfo(path);
-	model = LoadModelCryptography(path);
-
+	model = LearningModel::LoadFromFilePath(path);


We already load the model below in line 25 so I think this line would be redundant. What do you think?

Yes thats right. Sorry. I have updated it.

ryanlai2 · 2019-02-04T19:36:56Z

Tools/WinMLRunner/src/OutputHelper.h

+    }

-    void SetDefaultCSVFileNamePerIteration()
+    void SetDefaultFolder()


Can we change this method name to capture the idea that it's for setting the folder for per iterations run data?

Maybe: "SetDefaultPerIterationFolder"?

ryanlai2

Would it be possible to include a model that takes Int64 tensorkind as input so that we can test that the codepath works?

…ft#136)

laks0209 · 2019-02-05T23:22:02Z

Removed Int64 tensorkind. And have changed the naming of the folder. Please review the latest commit :-)

laks0209 requested a review from a team as a code owner December 18, 2018 06:42

laks0209 force-pushed the feature/OutputTensor branch from ea8b5ea to c217107 Compare December 18, 2018 06:45

ryanlai2 reviewed Dec 20, 2018

View reviewed changes

laks0209 force-pushed the feature/OutputTensor branch 7 times, most recently from 11e29f8 to aecbc38 Compare December 26, 2018 20:12

laks0209 commented Dec 26, 2018

View reviewed changes

laks0209 force-pushed the feature/OutputTensor branch 5 times, most recently from ae33af0 to c9de335 Compare January 7, 2019 18:42

ryanlai2 reviewed Jan 7, 2019

View reviewed changes

laks0209 force-pushed the feature/OutputTensor branch 7 times, most recently from d5254f3 to 21c5fd3 Compare January 7, 2019 22:36

smk2007 reviewed Jan 10, 2019

View reviewed changes

ryanlai2 reviewed Jan 10, 2019

View reviewed changes

laks0209 force-pushed the feature/OutputTensor branch 4 times, most recently from 7af8c3e to ac2e9de Compare January 24, 2019 19:01

laks0209 force-pushed the feature/OutputTensor branch from ac2e9de to 7b6cb13 Compare January 30, 2019 23:05

ryanlai2 reviewed Feb 1, 2019

View reviewed changes

ryanlai2 reviewed Feb 2, 2019

View reviewed changes

laks0209 force-pushed the feature/OutputTensor branch 2 times, most recently from fae6075 to 28967ed Compare February 2, 2019 00:45

ryanlai2 approved these changes Feb 4, 2019

View reviewed changes

ryanlai2 reviewed Feb 4, 2019

View reviewed changes

ryanlai2 suggested changes Feb 5, 2019

View reviewed changes

laks0209 force-pushed the feature/OutputTensor branch 2 times, most recently from b347e38 to 517a544 Compare February 5, 2019 23:05

Changed formatting for Stringify types to be more consistent (microso…

253771b

…ft#136)

laks0209 force-pushed the feature/OutputTensor branch from 517a544 to 253771b Compare February 5, 2019 23:16

ryanlai2 approved these changes Feb 6, 2019

View reviewed changes

ryanlai2 merged commit d2b1522 into microsoft:master Feb 6, 2019

		Assert::AreEqual(static_cast<size_t>(2), GetOutputCSVLineCount());
		}

Save per-iteration Output Tensor Results #113

Save per-iteration Output Tensor Results #113

Uh oh!

Conversation

laks0209 commented Dec 18, 2018

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!

laks0209 left a comment

Choose a reason for hiding this comment

Uh oh!

laks0209 commented Jan 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanlai2 Jan 7, 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!

ryanlai2 Jan 7, 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!

ryanlai2 commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ryanlai2 Jan 10, 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!

ryanlai2 commented Jan 10, 2019

Uh oh!

laks0209 commented Jan 14, 2019

Uh oh!

laks0209 commented Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ryanlai2 Feb 1, 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!

Choose a reason for hiding this comment

Uh oh!

ryanlai2 Feb 4, 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!

ryanlai2 left a comment

Choose a reason for hiding this comment

Uh oh!

ryanlai2 Jan 7, 2019 •

edited

Loading

ryanlai2 Jan 7, 2019 •

edited

Loading

ryanlai2 commented Jan 8, 2019 •

edited

Loading

ryanlai2 Jan 10, 2019 •

edited

Loading

laks0209 commented Jan 24, 2019 •

edited

Loading

ryanlai2 Feb 1, 2019 •

edited

Loading

ryanlai2 Feb 4, 2019 •

edited

Loading