-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add a project for functional tests without visibility into internals of ML.NET #2470
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.
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
Changes from 4 commits
7557a83
e27e20f
22c2f1f
363b083
2843389
f9f03c8
c1b7ffc
56ae524
8dd776e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,53 @@ | ||||||||||||
<Project Sdk="Microsoft.NET.Sdk"> | ||||||||||||
|
||||||||||||
<PropertyGroup> | ||||||||||||
<AssemblyName>Microsoft.ML.Functional.Tests</AssemblyName> | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no need to set |
||||||||||||
<AllowUnsafeBlocks>true</AllowUnsafeBlocks> | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need |
||||||||||||
<!-- We are turning off strong naming to ensure we never add `InternalsVisibleTo` for these tests --> | ||||||||||||
<SignAssembly>false</SignAssembly> | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You probably need to set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the tip! I couldn't figure out why those builds broke! In reply to: 255168732 [](ancestors = 255168732) |
||||||||||||
</PropertyGroup> | ||||||||||||
|
||||||||||||
<ItemGroup> | ||||||||||||
<ProjectReference Include="..\..\src\Microsoft.ML.Data\Microsoft.ML.Data.csproj" /> | ||||||||||||
<ProjectReference Include="..\..\src\Microsoft.ML.Ensemble\Microsoft.ML.Ensemble.csproj" /> | ||||||||||||
<ProjectReference Include="..\..\src\Microsoft.ML.EntryPoints\Microsoft.ML.EntryPoints.csproj" /> | ||||||||||||
<ProjectReference Include="..\..\src\Microsoft.ML.HalLearners\Microsoft.ML.HalLearners.csproj" /> | ||||||||||||
<ProjectReference Include="..\..\src\Microsoft.ML.ImageAnalytics\Microsoft.ML.ImageAnalytics.csproj" /> | ||||||||||||
<ProjectReference Include="..\..\src\Microsoft.ML.KMeansClustering\Microsoft.ML.KMeansClustering.csproj" /> | ||||||||||||
<ProjectReference Include="..\..\src\Microsoft.ML.SamplesUtils\Microsoft.ML.SamplesUtils.csproj" /> | ||||||||||||
<ProjectReference Include="..\..\src\Microsoft.ML.LightGBM\Microsoft.ML.LightGBM.csproj" /> | ||||||||||||
<ProjectReference Include="..\..\src\Microsoft.ML.Maml\Microsoft.ML.Maml.csproj" /> | ||||||||||||
<ProjectReference Include="..\..\src\Microsoft.ML.OnnxTransform\Microsoft.ML.OnnxTransform.csproj" /> | ||||||||||||
<ProjectReference Include="..\..\src\Microsoft.ML.PCA\Microsoft.ML.PCA.csproj" /> | ||||||||||||
<ProjectReference Include="..\..\src\Microsoft.ML.KMeansClustering\Microsoft.ML.KMeansClustering.csproj" /> | ||||||||||||
<ProjectReference Include="..\..\src\Microsoft.ML.Recommender\Microsoft.ML.Recommender.csproj" /> | ||||||||||||
<ProjectReference Include="..\..\src\Microsoft.ML.StandardLearners\Microsoft.ML.StandardLearners.csproj" /> | ||||||||||||
<ProjectReference Include="..\..\src\Microsoft.ML.Onnx\Microsoft.ML.Onnx.csproj" /> | ||||||||||||
<ProjectReference Include="..\..\src\Microsoft.ML.StaticPipe\Microsoft.ML.StaticPipe.csproj" /> | ||||||||||||
<ProjectReference Include="..\..\src\Microsoft.ML.TensorFlow.StaticPipe\Microsoft.ML.TensorFlow.StaticPipe.csproj" /> | ||||||||||||
<ProjectReference Include="..\..\src\Microsoft.ML.TensorFlow\Microsoft.ML.TensorFlow.csproj" /> | ||||||||||||
<ProjectReference Include="..\..\src\Microsoft.ML.TimeSeries\Microsoft.ML.TimeSeries.csproj" /> | ||||||||||||
<ProjectReference Include="..\Microsoft.ML.TestFramework\Microsoft.ML.TestFramework.csproj" /> | ||||||||||||
</ItemGroup> | ||||||||||||
|
||||||||||||
<ItemGroup> | ||||||||||||
<NativeAssemblyReference Include="CpuMathNative" /> | ||||||||||||
<NativeAssemblyReference Include="FastTreeNative" /> | ||||||||||||
<NativeAssemblyReference Include="FactorizationMachineNative" /> | ||||||||||||
<NativeAssemblyReference Include="MatrixFactorizationNative" /> | ||||||||||||
<NativeAssemblyReference Include="LdaNative" /> | ||||||||||||
<NativeAssemblyReference Include="SymSgdNative" /> | ||||||||||||
<NativeAssemblyReference Include="MklProxyNative" /> | ||||||||||||
<NativeAssemblyReference Include="MklImports" /> | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are those all needed a this point in time? #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||||
</ItemGroup> | ||||||||||||
|
||||||||||||
<!-- TensorFlow is 64-bit only --> | ||||||||||||
<ItemGroup Condition="'$(NativeTargetArchitecture)' == 'x64'"> | ||||||||||||
<NativeAssemblyReference Include="tensorflow" /> | ||||||||||||
<NativeAssemblyReference Condition="'$(OS)' != 'Windows_NT'" Include="tensorflow_framework" /> | ||||||||||||
</ItemGroup> | ||||||||||||
<ItemGroup> | ||||||||||||
<PackageReference Include="Microsoft.ML.TensorFlow.TestModels" Version="0.0.7-test" /> | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think Zeeshan is in the process of publishing version machinelearning/build/Dependencies.props Lines 42 to 46 in 834e471
Same for the Onnx TestModels below. #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've created global build variables for those and set them to the latest versions across the projects (they were out of sync). Will resolve once I see that the builds & tests pass. In reply to: 255165705 [](ancestors = 255165705) |
||||||||||||
<PackageReference Include="Microsoft.ML.Onnx.TestModels" Version="0.0.2-test" /> | ||||||||||||
</ItemGroup> | ||||||||||||
</Project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
// See the LICENSE file in the project root for more information. | ||
|
||
using Microsoft.ML.SamplesUtils; | ||
using Xunit; | ||
|
||
namespace Microsoft.ML.Functional.Tests | ||
{ | ||
public partial class PredictionScenarios | ||
{ | ||
/// <summary> | ||
/// Reconfigurable predictions: The following should be possible: A user trains a binary classifier, | ||
/// and through the test evaluator gets a PR curve, the based on the PR curve picks a new threshold | ||
/// and configures the scorer (or more precisely instantiates a new scorer over the same predictor) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
model Parameters #Resolved |
||
/// with some threshold derived from that. | ||
/// </summary> | ||
[Fact(Skip = "Blocked by issue #2465")] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test can still be run, right? Since you are commenting out the code that is being blocked. Maybe remove the Skip here, and add a TODO to the commented out line below. #Resolved |
||
public void ReconfigurablePrediction() | ||
{ | ||
var mlContext = new MLContext(seed: 789); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
this intentional? #Resolved |
||
|
||
// Get the dataset, create a train and test | ||
var dataset = DatasetUtils.LoadHousingRegressionDataset(mlContext); | ||
(var train, var test) = mlContext.BinaryClassification.TrainTestSplit(dataset, testFraction: 0.2); | ||
|
||
// Create a pipeline to train on the housing data | ||
var pipeline = mlContext.Transforms.Concatenate("Features", new string[] { | ||
"CrimesPerCapita", "PercentResidental", "PercentNonRetail", "CharlesRiver", "NitricOxides", "RoomsPerDwelling", | ||
"PercentPre40s", "EmploymentDistance", "HighwayDistance", "TaxRate", "TeacherRatio"}) | ||
.Append(mlContext.Transforms.CopyColumns("Label", "MedianHomeValue")) | ||
.Append(mlContext.Regression.Trainers.OrdinaryLeastSquares()); | ||
|
||
var model = pipeline.Fit(train); | ||
|
||
var scoredTest = model.Transform(test); | ||
var metrics = mlContext.Regression.Evaluate(scoredTest); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we be asserting the metrics are in a certain range? #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call on checking valid ranges. I added a Common library to add those sorts of checks to. In reply to: 255167478 [](ancestors = 255167478) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you missed checking that new function in. In reply to: 255228907 [](ancestors = 255228907,255167478) |
||
|
||
// This is no longer possible in the API | ||
//var newModel = new BinaryPredictionTransformer<IPredictorProducing<float>>(ml, model.Model, trainData.Schema, model.FeatureColumn, threshold: 0.01f, thresholdColumn: DefaultColumnNames.Probability); | ||
//var newScoredTest = newModel.Transform(pipeline.Transform(testData)); | ||
//var newMetrics = mlContext.BinaryClassification.Evaluate(scoredTest); | ||
|
||
// And the Threshold and ThresholdColumn properties are not settable. | ||
// var predictor = model.LastTransformer; | ||
// predictor.Threshold = 0.01; // Not possible | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
// See the LICENSE file in the project root for more information. | ||
|
||
using Microsoft.Data.DataView; | ||
using Microsoft.ML.Data; | ||
using Microsoft.ML.SamplesUtils; | ||
using Microsoft.ML.Trainers.HalLearners; | ||
using Xunit; | ||
|
||
namespace Microsoft.ML.Functional.Tests | ||
{ | ||
public partial class ValidationScenarios | ||
{ | ||
/// <summary> | ||
/// Cross-validation: Have a mechanism to do cross validation, that is, you come up with | ||
/// a data source (optionally with stratification column), come up with an instantiable transform | ||
/// and trainer pipeline, and it will handle (1) splitting up the data, (2) training the separate | ||
/// pipelines on in-fold data, (3) scoring on the out-fold data, (4) returning the set of | ||
/// evaluations and optionally trained pipes. (People always want metrics out of xfold, | ||
/// they sometimes want the actual models too.) | ||
/// </summary> | ||
[Fact] | ||
void CrossValidation() | ||
{ | ||
var mlContext = new MLContext(seed: 789); | ||
|
||
// Get the dataset, create a train and test | ||
var data = DatasetUtils.LoadHousingRegressionDataset(mlContext); | ||
|
||
// Create a pipeline to train on the sentiment data | ||
var pipeline = mlContext.Transforms.Concatenate("Features", new string[] { | ||
"CrimesPerCapita", "PercentResidental", "PercentNonRetail", "CharlesRiver", "NitricOxides", "RoomsPerDwelling", | ||
"PercentPre40s", "EmploymentDistance", "HighwayDistance", "TaxRate", "TeacherRatio"}) | ||
.Append(mlContext.Transforms.CopyColumns("Label", "MedianHomeValue")) | ||
.Append(mlContext.Regression.Trainers.OrdinaryLeastSquares()); | ||
|
||
// Compute the CV result | ||
var cvResult = mlContext.Regression.CrossValidate(data, pipeline, numFolds: 5); | ||
|
||
// Check that the results are valid | ||
Assert.IsType<RegressionMetrics>(cvResult[0].metrics); | ||
Assert.IsType<TransformerChain<RegressionPredictionTransformer<OlsLinearRegressionModelParameters>>>(cvResult[0].model); | ||
Assert.True(cvResult[0].scoredTestData is IDataView); | ||
Assert.Equal(5, cvResult.Length); | ||
} | ||
} | ||
} |
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you manually add these? They look wrong to me.
For example, this line should be:
and the
-netfx
ones should have-netfx
on the right side too.This is probably why CI is failing.
#Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Autogenerated. I'll fix these up.
In reply to: 255266128 [](ancestors = 255266128)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied the lines from Microsoft.ML.Tests/ and swapped in the correct GUID.
In reply to: 255266430 [](ancestors = 255266430,255266128)