Move towards being able to build for x86 #1008

tannergooding · 2018-09-24T22:23:34Z

This is work towards #97

It allows you to build using build.cmd -buildArch=x86 and, if you manually set the correct testhost, it allows the tests to pass.

tannergooding · 2018-09-24T22:23:44Z

CC. @eerhardt

tannergooding · 2018-09-24T22:24:39Z

Most of the skipped tests are due to a x64 only dependency (TensorFlow or LightGBM). Some of them were caused by differences in the baseline vs actual output.

tannergooding · 2018-09-24T22:25:28Z

In order to be able to run tests from the command line, we will need some kind of "test-host" logic, possibly similar to what CoreFX has. Right now, init-tools.cmd only pulls down the x64 CLI and it uses that when attempting to run the tests.

Directory.Build.props

src/Microsoft.ML.DnnAnalyzer/Microsoft.ML.DnnAnalyzer/Microsoft.ML.DnnAnalyzer.csproj

eerhardt · 2018-09-25T14:02:18Z

test/Microsoft.ML.Tests/Transformers/NormalizerTests.cs

@@ -18,7 +19,7 @@ public NormalizerTests(ITestOutputHelper output) : base(output)
        {
        }

-        [Fact]
+        [ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))]


What about this test and the below test require x64? I don't see tensorflow or lightgbm in the tests. Are there other things that only work on x64?

As mentioned here: #1008 (comment), there are a few tests which have different outputs from the baseline.

These need further investigation and potentially fixes.

Let's put that in the code comments then, so we know which ones need to be investigated, and which ones don't.

eerhardt · 2018-09-25T14:03:21Z

test/Microsoft.ML.TestFramework/TestCommandBase.cs

@@ -812,7 +812,7 @@ public void CommandCrossValidation()
            Done();
        }

-        [Fact]
+        [ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))]


Similar question here - what requires x64?

Directory.Build.props

test/Microsoft.ML.FSharp.Tests/Microsoft.ML.FSharp.Tests.fsproj

eerhardt · 2018-10-02T14:56:30Z

test/Microsoft.ML.OnnxTransformTest/OnnxTransformTests.cs

@@ -57,7 +57,7 @@ public OnnxTransformTests(ITestOutputHelper output) : base(output)
        {
        }

-        [Fact]
+        [ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // x86 fails with "An attempt was made to load a program with an incorrect format."


I assume Onnx support (i.e. Microsoft.ML.Scoring) is 64-bit only. @shmoradims @jignparm - can you confirm?

If so it might be more self documenting to have something like [ConditionalFact(typeof(TestUtilities), nameof(TestUtilities.IsOnnxScoringSupported))] (and similar for TF)

Was there a response to this feedback? I think this seems like a good idea. Or perhaps even deriving a few fact attributes like OnnxFact or TensorflowFact, etc.

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

I'm not sure its worth the added overhead to declare a new fact, which itself calls Environment.Is64BitProcess.

The current conditional fact requires no "new" code and clearly defines that it is only supported on 64-bit processes.

And when we try to add Aarch64 (i.e. arm64)?

Then we will need to re-review any of the currently disabled tests and possibly change this based on whether they support ARM and/or ARM64.

There are multiple advantages to taking the above approach:

It is self-documenting, like @danmosemsft said.

It is centralized, so if it ever needs to be updated, only need to update a single spot.

We can add any rules we want into the method, where you can't do this in a single attribute.

It sets a good pattern for other tests to follow.

The only disadvantage I see is the small amount of effort it is going to take to make this change.

@eerhardt, do you have any problem with logging a tracking issue to improve this?

I'd like to get this merged in order to unblock people and I don't think it's going to be as trivial as just defining some new attributes, since we will need to do something different for .NET Core (where we need to instead check the S.R.InteropServices.Architecture of the process)

that's fine with me.

test/Microsoft.ML.Predictor.Tests/TestPipelineSweeper.cs

eerhardt · 2018-10-02T14:57:45Z

test/Microsoft.ML.Predictor.Tests/TestPredictors.cs

@@ -102,7 +102,7 @@ public IList<TestDataset> GetDatasetsForClassificationWeightingPredictorsTest()
        /// <summary>
        ///A test for binary classifiers
        ///</summary>
-        [Fact]
+        [ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // x86 output differs from Baseline


I think we should fix/adjust the baseline mechanism so tests like this can be executed. We shouldn't disable tests just because the baseline differs.

I agree, but it requires more investigation to determine what differs and why. I saw a number of other tests that are already disabled due to baseline differences as well (for all targets), that also need investigation.

Can you, at minimum, log an issue to address the x86 baseline differences?

other tests that are already disabled

Those tests have never been enabled in dotnet/machinelearning. They were ported to this repo and immediately disabled.

IMO, the existing test shortcomings in this repo aren't valid reasons for not following good practices going forward. We need to actively make things better, or else it won't get better.

Looking at a handful of the differences, they generally seem to be minor differences in the computed result (for example: 1.4157174495771871 vs 1.4157174495771872). This is likely fallout from alignment differences and algorithm differences for various System.Math (and other) functions.

The comparison logic, in general, likely needs an overhaul (to properly account for these differences cross-architecture and cross-platform).

CC. @GalOshri, @shauheen, @TomFinley, @Zruty0

eerhardt · 2018-10-02T14:58:26Z

test/Microsoft.ML.Predictor.Tests/TestPredictors.cs

@@ -587,7 +587,7 @@ public void RankingLightGBMTest()
            Done();
        }

-        [Fact]
+        [ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // x86 fails with "Unknown command: 'train'; Format error at (83,3)-(83,4011): Illegal quoting"


This should be investigated and fixed.

Could perhaps be a follow up issue if @tannergooding isn't the right person to investigate.

Was there a follow up issue for this?

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

I've not logged issues for anything that requires followup yet.

eerhardt · 2018-10-02T19:08:29Z

We will need to update:

https://github.com/dotnet/machinelearning/blob/master/pkg/common/CommonPackage.props
and
https://github.com/dotnet/machinelearning/blob/master/pkg/Microsoft.ML/build/netstandard2.0/Microsoft.ML.targets#L16

so we can support x86 on .NET Framework.

tannergooding · 2018-10-09T19:01:49Z

Updated. Significantly fewer tests disabled now. The ones that are still disabled depend on a 64-bit only native component or currently have results that don't match for digitsOfPrecision: 4.

tannergooding · 2018-10-09T20:18:47Z

@sfilipi, would you be able to give this a quick review?

As per above, there are still a few tests that are disabled due to having less than 4 matching significant digits. From a brief look, some of these are caused by bugs in the parser/formatter code for full framework that have been fixed (or are being fixed) in .NET Core.

sfilipi · 2018-10-09T20:31:57Z

Taking a look shortly!

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

sfilipi · 2018-10-10T17:14:55Z

test/Microsoft.ML.Predictor.Tests/TestPredictors.cs

@@ -192,7 +192,7 @@ public void MulticlassSdcaTest()
        /// <summary>
        /// Multiclass Logistic Regression test with a tree featurizer.
        /// </summary>
-        [Fact]
+        [ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // x86 output differs from Baseline


[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // x86 output differs from Baseline [](start = 8, length = 110)

Let's add an issue to create baselines for those x86 tests, after checking that the difference is ok.

test/Microsoft.ML.TimeSeries.Tests/TimeSeriesDirectApi.cs

sfilipi

eerhardt

danmoseley requested a review from eerhardt September 24, 2018 23:37

danmoseley reviewed Sep 24, 2018

View reviewed changes

Directory.Build.props Outdated Show resolved Hide resolved

danmoseley reviewed Sep 24, 2018

View reviewed changes

src/Microsoft.ML.DnnAnalyzer/Microsoft.ML.DnnAnalyzer/Microsoft.ML.DnnAnalyzer.csproj Show resolved Hide resolved

danmoseley approved these changes Sep 24, 2018

View reviewed changes

eerhardt reviewed Sep 25, 2018

View reviewed changes

tannergooding closed this Oct 1, 2018

tannergooding reopened this Oct 1, 2018

eerhardt reviewed Oct 2, 2018

View reviewed changes

Directory.Build.props Show resolved Hide resolved

eerhardt reviewed Oct 2, 2018

View reviewed changes

test/Microsoft.ML.FSharp.Tests/Microsoft.ML.FSharp.Tests.fsproj Show resolved Hide resolved

eerhardt reviewed Oct 2, 2018

View reviewed changes

test/Microsoft.ML.Predictor.Tests/TestPipelineSweeper.cs Outdated Show resolved Hide resolved

eerhardt reviewed Oct 2, 2018

View reviewed changes

danmoseley mentioned this pull request Oct 4, 2018

Fix MatchNumberWithTolerance to better compare floating-point values #1145

Merged

Move towards being able to build for x86

898f447

sfilipi reviewed Oct 10, 2018

View reviewed changes

test/Microsoft.ML.TimeSeries.Tests/TimeSeriesDirectApi.cs Show resolved Hide resolved

sfilipi approved these changes Oct 10, 2018

View reviewed changes

eerhardt approved these changes Oct 10, 2018

View reviewed changes

tannergooding mentioned this pull request Oct 10, 2018

Clean up [ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] #1220

Closed

tannergooding merged commit c25e45f into dotnet:master Oct 10, 2018

tannergooding mentioned this pull request Oct 10, 2018

Update CI to support building and testing x86 #1221

Closed

antoniovs1029 mentioned this pull request Feb 1, 2020

Enable Conditional Numerical Reproducibility for tests #4569

Merged

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

Move towards being able to build for x86 #1008

Move towards being able to build for x86 #1008

Conversation

tannergooding commented Sep 24, 2018

tannergooding commented Sep 24, 2018

tannergooding commented Sep 24, 2018

tannergooding commented Sep 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt commented Oct 2, 2018

tannergooding commented Oct 9, 2018

tannergooding commented Oct 9, 2018

sfilipi commented Oct 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi left a comment

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

eerhardt Oct 10, 2018 •

edited

Loading