Skip to content

Move towards being able to build for x86 #1008

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 10, 2018
Merged

Move towards being able to build for x86 #1008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,15 @@
<Configuration Condition="'$(Configuration)'==''">Debug</Configuration>
<Configurations>Debug;Release;Debug-Intrinsics;Release-Intrinsics</Configurations>
<Platform Condition="'$(Platform)'==''">AnyCPU</Platform>
<NativeTargetArchitecture Condition="'$(NativeTargetArchitecture)' == ''">x64</NativeTargetArchitecture>
<TargetArchitecture Condition="'$(TargetArchitecture)' == ''">x64</TargetArchitecture>
<NativeTargetArchitecture Condition="'$(NativeTargetArchitecture)' == ''">$(TargetArchitecture)</NativeTargetArchitecture>
<PlatformConfig>$(Platform).$(Configuration)</PlatformConfig>
</PropertyGroup>

<PropertyGroup>
<RestoreSources>
https://api.nuget.org/v3/index.json;
https://dotnetfeed.blob.core.windows.net/dotnet-core/index.json;
https://dotnet.myget.org/F/dotnet-core/api/v3/index.json;
</RestoreSources>
</PropertyGroup>
Expand Down
4 changes: 2 additions & 2 deletions pkg/Microsoft.ML/build/netstandard2.0/Microsoft.ML.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@
the PlatformTarget is empty, and you don't know until runtime (i.e. which dotnet.exe)
what processor architecture will be used.
-->
<Error Condition="'$(PlatformTarget)' != 'x64' AND
<Error Condition="('$(PlatformTarget)' != 'x64' AND '$(PlatformTarget)' != 'x86') AND
('$(OutputType)' == 'Exe' OR '$(OutputType)'=='WinExe') AND
!('$(TargetFrameworkIdentifier)' == '.NETCoreApp' AND '$(PlatformTarget)' == '')"
Text="Microsoft.ML currently supports 'x64' processor architectures. Please ensure your application is targeting 'x64'." />
Text="Microsoft.ML currently supports 'x64' and 'x86' processor architectures. Please ensure your application is targeting 'x64' or 'x86'." />
</Target>

</Project>
5 changes: 5 additions & 0 deletions pkg/common/CommonPackage.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
<Visible>false</Visible>
</Content>
<Content Include="$(MSBuildThisFileDirectory)\..\..\runtimes\win-x86\native\*.dll"
Condition="'$(PlatformTarget)' == 'x86'">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
<Visible>false</Visible>
</Content>
</ItemGroup>

</Project>
2 changes: 0 additions & 2 deletions src/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@
<WarningsNotAsErrors>$(WarningsNotAsErrors);1591</WarningsNotAsErrors>

<CodeAnalysisRuleSet>$(MSBuildThisFileDirectory)\Source.ruleset</CodeAnalysisRuleSet>

<TargetArchitecture Condition="'$(TargetArchitecture)' == ''">x64</TargetArchitecture>

<NativeAssetsBuiltPath>$(BaseOutputPath)$(TargetArchitecture).$(Configuration)\Native</NativeAssetsBuiltPath>

Expand Down
3 changes: 2 additions & 1 deletion src/Microsoft.ML.DnnAnalyzer/Microsoft.ML.DnnAnalyzer/Microsoft.ML.DnnAnalyzer.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
<ProjectReference Include="..\..\Microsoft.ML.TensorFlow\Microsoft.ML.TensorFlow.csproj" />
</ItemGroup>

<ItemGroup>
<!-- TensorFlow is 64-bit only -->
<ItemGroup Condition="'$(NativeTargetArchitecture)' == 'x64'">
<NativeAssemblyReference Include="tensorflow" />
<NativeAssemblyReference Condition="'$(OS)' != 'Windows_NT'" Include="tensorflow_framework" />
</ItemGroup>
Expand Down
2 changes: 1 addition & 1 deletion src/Native/Stdafx.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
#ifdef _WIN32
#include <intrin.h>

#define EXPORT_API(ret) extern "C" __declspec(dllexport) ret __stdcall
#define EXPORT_API(ret) extern "C" __declspec(dllexport) ret
#else
#include "UnixSal.h"

Expand Down
5 changes: 3 additions & 2 deletions test/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,9 @@

<ItemGroup>
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.8.0" />
<PackageReference Include="xunit" Version="2.3.1" />
<PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" />
<PackageReference Include="xunit" Version="2.4.0" />
<PackageReference Include="xunit.runner.visualstudio" Version="2.4.0" />
<PackageReference Include="Microsoft.DotNet.XUnitExtensions" Version="2.4.0-prerelease-63213-02" />
</ItemGroup>

</Project>
4 changes: 4 additions & 0 deletions test/Microsoft.ML.Core.Tests/Microsoft.ML.Core.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@
<NativeAssemblyReference Include="CpuMathNative" />
<NativeAssemblyReference Include="FastTreeNative" />
<NativeAssemblyReference Include="LdaNative" />
</ItemGroup>

<!-- TensorFlow is 64-bit only -->
<ItemGroup Condition="'$(NativeTargetArchitecture)' == 'x64'">
<NativeAssemblyReference Include="tensorflow" />
<NativeAssemblyReference Condition="'$(OS)' != 'Windows_NT'" Include="tensorflow_framework" />
</ItemGroup>
Expand Down
2 changes: 1 addition & 1 deletion test/Microsoft.ML.Core.Tests/UnitTests/TestCSharpApi.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -959,7 +959,7 @@ public void TestOvaMacroWithUncalibratedLearner()
}
}

[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // TensorFlow is 64-bit only
public void TestTensorFlowEntryPoint()
{
var dataPath = GetDataPath("Train-Tiny-28x28.txt");
Expand Down
6 changes: 3 additions & 3 deletions test/Microsoft.ML.Core.Tests/UnitTests/TestEntryPoints.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1787,14 +1787,14 @@ public void EntryPointEvaluateRanking()
}
}

[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // LightGBM is 64-bit only
public void EntryPointLightGbmBinary()
{
Env.ComponentCatalog.RegisterAssembly(typeof(LightGbmBinaryPredictor).Assembly);
TestEntryPointRoutine("breast-cancer.txt", "Trainers.LightGbmBinaryClassifier");
}

[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // LightGBM is 64-bit only
public void EntryPointLightGbmMultiClass()
{
Env.ComponentCatalog.RegisterAssembly(typeof(LightGbmBinaryPredictor).Assembly);
Expand Down Expand Up @@ -3710,7 +3710,7 @@ public void EntryPointWordEmbeddings()
}
}

[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // TensorFlow is 64-bit only
public void EntryPointTensorFlowTransform()
{
Env.ComponentCatalog.RegisterAssembly(typeof(TensorFlowTransform).Assembly);
Expand Down
2 changes: 1 addition & 1 deletion test/Microsoft.ML.FSharp.Tests/Microsoft.ML.FSharp.Tests.fsproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
<NoWarn>2003;$(NoWarn)</NoWarn>
<PublicSign>false</PublicSign>
<SourceLink></SourceLink>
<PlatformTarget>x64</PlatformTarget>
<PlatformTarget>$(TargetArchitecture)</PlatformTarget>
</PropertyGroup>

<ItemGroup>
Expand Down
8 changes: 4 additions & 4 deletions test/Microsoft.ML.OnnxTransformTest/OnnxTransformTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@eerhardt eerhardt Oct 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are multiple advantages to taking the above approach:

  1. It is self-documenting, like @danmosemsft said.
  2. It is centralized, so if it ever needs to be updated, only need to update a single spot.
  3. We can add any rules we want into the method, where you can't do this in a single attribute.
  4. 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's fine with me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1220

void TestSimpleCase()
{
if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
Expand Down Expand Up @@ -100,7 +100,7 @@ void TestSimpleCase()
catch (InvalidOperationException) { }
}

[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // x86 fails with "An attempt was made to load a program with an incorrect format."
void TestOldSavingAndLoading()
{
if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
Expand Down Expand Up @@ -158,7 +158,7 @@ void TestOldSavingAndLoading()
}
}

[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // x86 fails with "An attempt was made to load a program with an incorrect format."
public void OnnxStatic()
{
if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
Expand Down Expand Up @@ -205,7 +205,7 @@ public void OnnxStatic()
}
}

[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // x86 output differs from Baseline
void TestCommandLine()
{
if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
Expand Down
46 changes: 23 additions & 23 deletions test/Microsoft.ML.Predictor.Tests/TestPredictors.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1215

[TestCategory("Multiclass")]
[TestCategory("Logistic Regression")]
[TestCategory("FastTree")]
Expand Down Expand Up @@ -239,7 +239,7 @@ public void KMeansClusteringTest()
Done();
}

[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // x86 output differs from Baseline
[TestCategory("Binary")]
[TestCategory("SDCA")]
public void LinearClassifierTest()
Expand All @@ -260,7 +260,7 @@ public void LinearClassifierTest()
/// <summary>
///A test for binary classifiers
///</summary>
[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // x86 output differs from Baseline
[TestCategory("Binary")]
public void BinaryClassifierLogisticRegressionTest()
{
Expand All @@ -270,7 +270,7 @@ public void BinaryClassifierLogisticRegressionTest()
Done();
}

[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // x86 output differs from Baseline
[TestCategory("Binary")]
public void BinaryClassifierSymSgdTest()
{
Expand All @@ -282,7 +282,7 @@ public void BinaryClassifierSymSgdTest()
Done();
}

[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // x86 output differs from Baseline
[TestCategory("Binary")]
public void BinaryClassifierTesterThresholdingTest()
{
Expand All @@ -308,7 +308,7 @@ public void BinaryClassifierLogisticRegressionNormTest()
/// <summary>
///A test for binary classifiers with non-negative coefficients
///</summary>
[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // x86 output differs from Baseline
[TestCategory("Binary")]
public void BinaryClassifierLogisticRegressionNonNegativeTest()
{
Expand All @@ -334,7 +334,7 @@ public void BinaryClassifierLogisticRegressionBinNormTest()
/// <summary>
///A test for binary classifiers
///</summary>
[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // x86 output differs from Baseline
[TestCategory("Binary")]
public void BinaryClassifierLogisticRegressionGaussianNormTest()
{
Expand Down Expand Up @@ -373,7 +373,7 @@ public void BinaryClassifierFastRankClassificationTest()
/// <summary>
///A test for binary classifiers
///</summary>
[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // x86 output differs from Baseline
[TestCategory("Binary")]
[TestCategory("FastForest")]
public void FastForestClassificationTest()
Expand Down Expand Up @@ -440,7 +440,7 @@ public void WeightingFastForestRegressionPredictorsTest()
Done();
}

[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // x86 output differs from Baseline
[TestCategory("Binary")]
[TestCategory("FastTree")]
public void FastTreeBinaryClassificationTest()
Expand All @@ -459,7 +459,7 @@ public void FastTreeBinaryClassificationTest()
Done();
}

[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // LightGBM is 64-bit only
[TestCategory("Binary")]
[TestCategory("LightGBM")]
public void LightGBMClassificationTest()
Expand All @@ -475,7 +475,7 @@ public void LightGBMClassificationTest()
Done();
}

[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // LightGBM is 64-bit only
[TestCategory("Binary"), TestCategory("LightGBM")]
public void GossLightGBMTest()
{
Expand All @@ -485,7 +485,7 @@ public void GossLightGBMTest()
Done();
}

[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // LightGBM is 64-bit only
[TestCategory("Binary")]
[TestCategory("LightGBM")]
public void DartLightGBMTest()
Expand All @@ -499,7 +499,7 @@ public void DartLightGBMTest()
/// <summary>
/// A test for multi class classifiers.
/// </summary>
[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // LightGBM is 64-bit only
[TestCategory("Multiclass")]
[TestCategory("LightGBM")]
public void MultiClassifierLightGBMKeyLabelTest()
Expand All @@ -513,7 +513,7 @@ public void MultiClassifierLightGBMKeyLabelTest()
/// <summary>
/// A test for multi class classifiers.
/// </summary>
[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // LightGBM is 64-bit only
[TestCategory("Multiclass")]
[TestCategory("LightGBM")]
public void MultiClassifierLightGBMKeyLabelU404Test()
Expand All @@ -527,7 +527,7 @@ public void MultiClassifierLightGBMKeyLabelU404Test()
/// <summary>
/// A test for regression.
/// </summary>
[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // LightGBM is 64-bit only
[TestCategory("Regression")]
[TestCategory("LightGBM")]
public void RegressorLightGBMTest()
Expand All @@ -541,7 +541,7 @@ public void RegressorLightGBMTest()
/// <summary>
/// A test for regression.
/// </summary>
[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // LightGBM is 64-bit only
[TestCategory("Regression")]
[TestCategory("LightGBM")]
public void RegressorLightGBMMAETest()
Expand All @@ -555,7 +555,7 @@ public void RegressorLightGBMMAETest()
/// <summary>
/// A test for regression.
/// </summary>
[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // LightGBM is 64-bit only
[TestCategory("Regression")]
[TestCategory("LightGBM")]
public void RegressorLightGBMRMSETest()
Expand Down Expand Up @@ -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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be investigated and fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there a follow up issue for this?

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1216

public void TestTreeEnsembleCombiner()
{
var dataPath = GetDataPath("breast-cancer.txt");
Expand All @@ -611,7 +611,7 @@ public void TestTreeEnsembleCombiner()
CombineAndTestTreeEnsembles(dataView, fastTrees);
}

[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // x86 fails with "Unknown command: 'train'; Format error at (83,3)-(83,4011): Illegal quoting"
public void TestTreeEnsembleCombinerWithCategoricalSplits()
{
var dataPath = GetDataPath("adult.tiny.with-schema.txt");
Expand Down Expand Up @@ -705,7 +705,7 @@ private void CombineAndTestTreeEnsembles(IDataView idv, IPredictorModel[] fastTr
}
}

[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // x86 output differs from Baseline
[TestCategory("Binary")]
[TestCategory("FastTree")]
public void FastTreeBinaryClassificationCategoricalSplitTest()
Expand Down Expand Up @@ -744,7 +744,7 @@ public void FastTreeRegressionCategoricalSplitTest()
Done();
}

[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // x86 output differs from Baseline
[TestCategory("Binary")]
[TestCategory("FastTree")]
public void FastTreeBinaryClassificationNoOpGroupIdTest()
Expand All @@ -764,7 +764,7 @@ public void FastTreeBinaryClassificationNoOpGroupIdTest()
Done();
}

[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // x86 output differs from Baseline
[TestCategory("Binary")]
[TestCategory("FastTree")]
public void FastTreeHighMinDocsTest()
Expand Down Expand Up @@ -1363,7 +1363,7 @@ public void PAVCalibratorPerceptronTest()
/// <summary>
///A test for random calibrators
///</summary>
[Fact]
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // x86 output differs from Baseline
[TestCategory("Calibrator")]
public void RandomCalibratorPerceptronTest()
{
Expand Down
Loading