NetCore3.1 generates different test result issue #5047

frank-dong-ms-zz · 2020-04-22T04:26:35Z

Address issues that NetCore 3.1 generates different test result:
#1096
#3856

The difference comes from CPUMath using different instruction set:

dotnet framework and dotnet core 2.1 uses CpuMathUtils.netstandard that uses SSE instruction set through CPUMath native library;
Since dotnet core 2.2, dotnet core has direct support for AVX and SSE hardware Intrinsics so dotnet core 3.1 use AVX or SSE instruction set directly. Dotnet core 3.1 uses CpuMathUtils.netcoreapp that uses AVX, SSE or direct floating point calculation depending on hardward avaibility.

AVX and SSE generates slightly different result due to nature of floating point math.
Use below issue to track: #5044

So in short term we are separating test baseline for netcore 3.1 until we add AVX support at CPUMath native library.

…to test-separate-baseline

…figuration

harishsk · 2020-04-22T17:09:59Z

test/Microsoft.ML.TestFramework/BaseTestBaseline.cs

+            // So Ideally we should adding AVX support at CPUMath native library, 
+            // use below issue to track: https://github.com/dotnet/machinelearning/issues/5044
+            if (AppDomain.CurrentDomain.GetData("FX_PRODUCT_VERSION") != null)
+                configurationDirs.Add("NetCore3.1Result");



This is a good change overall to move to multiple configuration dirs.

What happens if we have stored different results for win-x64-netcoreapp21, win-x64-netcoreapp31, win-x86-netcoreapp31, winx-x86-netcoreapp21?

Even if we store netcoreapp21 results by default into win-x64 and win-x86, we would still 2 separate directories for win-x64-netcoreapp31 and win winx86-netcoreapp31.

Does that sound correct? Or am I missing something?

Also can you please change the directory name to be just reflect the configuration (e.g. netcoreapp31 as opposed to NetCore3.1Result)? #Resolved

Yes, I can change the folder name to netcoreapp31.

What happens if we have stored different results for win-x64-netcoreapp21, win-x64-netcoreapp31, win-x86-netcoreapp31, winx-x86-netcoreapp21?

in that case, we have to return win-x64-netcoreapp21, win-x64-netcoreapp31, win-x86-netcoreapp31, winx-x86-netcoreapp21 as configuration directory and make sure we don't have any "expected" result file at folder win-64 or netcoreapp31. And during get file stage we will check one by one if file exists. Let me make the logic complete and send out new iteration.

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

frank-dong-ms-zz · 2020-04-22T17:10:19Z

/azp run #Resolved

azure-pipelines · 2020-04-22T17:10:28Z

Azure Pipelines successfully started running 1 pipeline(s). #Resolved

harishsk · 2020-04-22T18:45:15Z

test/Microsoft.ML.TestFramework/BaseTestBaseline.cs

            if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX))
-                return "osx-x64";
+                configurationDirs.Add("osx-x64");


I think the logic below should apply to these as well. We may have osx-x64, osx-x64-netcoreapp21 and osx-netcoreapp31 (and similarly for linux)

While we are at it, please extend it to include just the os as well. (i.e. separate folders for osx, linux, win)

yeah, let's add these combination if necessary

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

harishsk · 2020-04-22T18:46:03Z

test/Microsoft.ML.TestFramework/BaseTestBaseline.cs

+                    configurationDirs.Add("win-x64");
+
+                    if (AppDomain.CurrentDomain.GetData("FX_PRODUCT_VERSION") != null)
+                        configurationDirs.Add($"win-x64-netcoreapp31");


Since we will be applying this for all the OSes, this can be factored out and checked once outside this if block

yes, in theory we should apply this to all OSes but since we haven't investigate osx and linux case yet I don't want to make all the combination now because that seems little bit overkill and make all the combination will make the file exists check every time we try to reference baseline file that will likely impact our performance.
I'm considering add osx and linux if this is necessary after investigation, make sense to you?

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

Yes, there might be a small delay because we are checking for an extra directory. But this delay happens in CI verification tests and doesn't represent the true performance of the algorithms. Those are covered by the benchmark tests.
I think it is okay to add it right now. But if you wish to add it later after investigation, I am okay with that too.

In reply to: 413245167 [](ancestors = 413245167,413231007)

Yeah, I think we better add any specific combination in future if necessary to keep the logic simple

In reply to: 413247756 [](ancestors = 413247756,413245167,413231007)

harishsk · 2020-04-22T18:47:03Z

test/Microsoft.ML.TestFramework/BaseTestBaseline.cs

+        private IEnumerable<string> GetConfigurationDirs()
+        {
+            // Sometimes different configuration will have combination.
+            // for example: one test can run on windows, have different result both


Thanks for explaining the rationale in detail. #WontFix

harishsk · 2020-04-22T18:51:14Z

test/Microsoft.ML.TestFramework/BaseTestBaseline.cs

                else
-                    return "win-x86";
+                {
+                    configurationDirs.Add("win-x86");



The order of these should be reversed.
We should check from the most specific to the least specific. The structure in the folder names we have is --. So we should check in the following order:

win-x86-netcoreapp31
win-x86
win

yes, that make sense, thanks

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

azure-pipelines · 2020-04-22T21:24:53Z

No pipelines are associated with this pull request.

harishsk

frank-dong-ms added 4 commits April 21, 2020 15:42

fix difference by using different result

84b4e9f

add netcore3.1 baseline results

82e74fa

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

840546d

…to test-separate-baseline

sync latest and merge

a8e16db

frank-dong-ms-zz requested a review from a team as a code owner April 22, 2020 04:26

frank-dong-ms-zz requested a review from harishsk April 22, 2020 04:26

frank-dong-ms added 2 commits April 21, 2020 21:31

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

3bb2e1f

…to test-separate-baseline

modify configuration specific baselines to support combination of con…

6a7f08d

…figuration

harishsk reviewed Apr 22, 2020

View reviewed changes

frank-dong-ms added 2 commits April 22, 2020 11:32

take comments

2e2d2df

remove unnecessary changes

e80a1aa

harishsk reviewed Apr 22, 2020

View reviewed changes

take comments

f2263b2

harishsk approved these changes Apr 22, 2020

View reviewed changes

frank-dong-ms-zz merged commit adad3e4 into dotnet:master Apr 23, 2020

filipnavara mentioned this pull request Apr 25, 2020

Delete FX_PRODUCT_VERSION host key/value pair dotnet/runtime#35455

Merged

ghost locked as resolved and limited conversation to collaborators Mar 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NetCore3.1 generates different test result issue #5047

NetCore3.1 generates different test result issue #5047

frank-dong-ms-zz commented Apr 22, 2020 •

edited

Loading

harishsk Apr 22, 2020 •

edited by frank-dong-ms-zz

Loading

frank-dong-ms-zz Apr 22, 2020

frank-dong-ms-zz commented Apr 22, 2020 •

edited

Loading

azure-pipelines bot commented Apr 22, 2020 •

edited by frank-dong-ms-zz

Loading

harishsk Apr 22, 2020 •

edited

Loading

frank-dong-ms-zz Apr 22, 2020

harishsk Apr 22, 2020

frank-dong-ms-zz Apr 22, 2020

harishsk Apr 22, 2020

frank-dong-ms-zz Apr 22, 2020

harishsk Apr 22, 2020 •

edited by frank-dong-ms-zz

Loading

harishsk Apr 22, 2020 •

edited

Loading

frank-dong-ms-zz Apr 22, 2020

azure-pipelines bot commented Apr 22, 2020

harishsk left a comment

NetCore3.1 generates different test result issue #5047

NetCore3.1 generates different test result issue #5047

Conversation

frank-dong-ms-zz commented Apr 22, 2020 • edited Loading

harishsk Apr 22, 2020 • edited by frank-dong-ms-zz Loading

Choose a reason for hiding this comment

frank-dong-ms-zz Apr 22, 2020

Choose a reason for hiding this comment

frank-dong-ms-zz commented Apr 22, 2020 • edited Loading

azure-pipelines bot commented Apr 22, 2020 • edited by frank-dong-ms-zz Loading

harishsk Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

frank-dong-ms-zz Apr 22, 2020

Choose a reason for hiding this comment

harishsk Apr 22, 2020

Choose a reason for hiding this comment

frank-dong-ms-zz Apr 22, 2020

Choose a reason for hiding this comment

harishsk Apr 22, 2020

Choose a reason for hiding this comment

frank-dong-ms-zz Apr 22, 2020

Choose a reason for hiding this comment

harishsk Apr 22, 2020 • edited by frank-dong-ms-zz Loading

Choose a reason for hiding this comment

harishsk Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

frank-dong-ms-zz Apr 22, 2020

Choose a reason for hiding this comment

azure-pipelines bot commented Apr 22, 2020

harishsk left a comment

Choose a reason for hiding this comment

frank-dong-ms-zz commented Apr 22, 2020 •

edited

Loading

harishsk Apr 22, 2020 •

edited by frank-dong-ms-zz

Loading

frank-dong-ms-zz commented Apr 22, 2020 •

edited

Loading

azure-pipelines bot commented Apr 22, 2020 •

edited by frank-dong-ms-zz

Loading

harishsk Apr 22, 2020 •

edited

Loading

harishsk Apr 22, 2020 •

edited by frank-dong-ms-zz

Loading

harishsk Apr 22, 2020 •

edited

Loading