Move towards being able to build for x86 by tannergooding · Pull Request #1008 · dotnet/machinelearning

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.

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

 }

- [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

 }

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


Similar question here - what requires x64?

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

 }

- [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.

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

 ///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

 }

- [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

 /// 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.

sfilipi

eerhardt

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.

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

 }

- [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

 }

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


Similar question here - what requires x64?

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

 }

- [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.

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

 ///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

 }

- [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

 /// 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.

sfilipi

eerhardt

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.

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

 }

- [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

 }

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


Similar question here - what requires x64?

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

 }

- [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.

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

 ///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

 }

- [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

 /// 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.

sfilipi

eerhardt

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.

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

 }

- [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

 }

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


Similar question here - what requires x64?

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

 }

- [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.

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

 ///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

 }

- [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

 /// 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.

sfilipi

eerhardt

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.

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

 }

- [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

 }

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


Similar question here - what requires x64?

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

 }

- [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.

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

 ///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

 }

- [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

 /// 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.

sfilipi

eerhardt

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.

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

 }

- [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

 }

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


Similar question here - what requires x64?

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

 }

- [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.

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

 ///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

 }

- [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

 /// 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.

sfilipi

eerhardt

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.

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

 }

- [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

 }

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


Similar question here - what requires x64?

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

 }

- [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.

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

 ///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

 }

- [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

 /// 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.

sfilipi

eerhardt

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.

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

 }

- [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

 }

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


Similar question here - what requires x64?

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

 }

- [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.

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

 ///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

 }

- [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

 /// 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.

sfilipi

eerhardt

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

danmoseley reviewed Sep 24, 2018
View reviewed changes

Comment threadDirectory.Build.props Outdated

danmoseley reviewed Sep 24, 2018
View reviewed changes

Comment threadsrc/Microsoft.ML.DnnAnalyzer/Microsoft.ML.DnnAnalyzer/Microsoft.ML.DnnAnalyzer.csproj

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

Comment threadDirectory.Build.props

eerhardt reviewed Oct 2, 2018
View reviewed changes

Comment threadtest/Microsoft.ML.FSharp.Tests/Microsoft.ML.FSharp.Tests.fsproj

eerhardt reviewed Oct 2, 2018
View reviewed changes

Comment threadtest/Microsoft.ML.Predictor.Tests/TestPipelineSweeper.cs Outdated

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

Comment threadtest/Microsoft.ML.TimeSeries.Tests/TimeSeriesDirectApi.cs

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:masterOct 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