Fix column purpose for PipelineSweeperMacro #461

ghost · 2018-06-30T02:03:42Z

Fixes #460

Adding arguments to the PipelineSweeperMacro. Users will be able to specify the columns for a particular purpose.
Updating the methods in the PurposeInference and TransformInference classes to pass the user provided column purposes as optional arguments.
Added unit tests for the sweeping engines we have currently (Defaults, Rocket, UniformRandom)

Sync with latest ML.NET

Merge with latest master of dotnet/machinelearning

Merge with latest dotnet master

dnfclas · 2018-06-30T02:03:54Z

All CLA requirements met. #Closed

ghost · 2018-06-30T02:13:54Z

@justinormont is added to the review. #Closed

ghost · 2018-06-30T02:16:52Z

@yaeldekel . Could you kindly take a look at this PR ? #Closed

justinormont · 2018-06-30T20:00:07Z

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

+            var allPipelines = autoMlState.GetAllEvaluatedPipelines();
+            var bestPipeline = autoMlState.GetBestPipeline();
+            Assert.Equal(allPipelines.Length, numIterations);
+            Assert.True(bestPipeline.PerformanceSummary.MetricValue > 0.87);


I'd recommend a range. Perhaps 87-89?

In the future, we may cause a Label to be leaked, and I'd like that caught. #Resolved

thanks Justin. To catch issues with leaky label I have added ranges on the training AUC.

Also added ranges for the test AUC.

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

… GroupId and Name columns

Ivanidzo4ka · 2018-07-02T18:50:37Z

src/Microsoft.ML.PipelineInference/AutoInference.cs

@@ -370,19 +371,21 @@ public void UpdateTerminator(ITerminator terminator)
                TransformInference.SuggestedTransform[] existingTransforms = null)
            {
                // Infer transforms using experts
-                var levelTransforms = TransformInference.InferTransforms(_env, data, args);
+                var levelTransforms = TransformInference.InferTransforms(_env, data, args, this._columnPurpose);


this [](start = 91, length = 4)

we tend to omit usage of "this" word. #Resolved

Fixed

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

Ivanidzo4ka · 2018-07-02T18:52:22Z

src/Microsoft.ML.PipelineInference/Macros/PipelineSweeperMacro.cs

@@ -36,6 +36,33 @@ public sealed class Arguments

            [Argument(ArgumentType.AtMostOnce, HelpText = "Output datasets from previous iteration of sweep.", SortOrder = 7, Hide = true)]
            public IDataView[] CandidateOutputs;
+
+            [Argument(ArgumentType.MultipleUnique, HelpText = "Column(s) to use as purpose 'Ignore'", SortOrder = 8, Hide = true)]


Hide = true [](start = 117, length = 11)

why you hide them? #Resolved

Fixed. Any thoughts on what's the implication of the 'Hide' argument for a Macro? Wasn't clear to me.

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

Ivanidzo4ka · 2018-07-02T18:54:16Z

src/Microsoft.ML.PipelineInference/Macros/PipelineSweeperMacro.cs

@@ -98,6 +186,9 @@ public static Output ExtractSweepResult(IHostEnvironment env, ResultInput input)
                "Must have a valid AutoML State, or pass arguments to create one.");
            env.Check(input.BatchSize > 0, "Batch size must be > 0.");

+            // Get the user-defined column purposes (if any)
+            var columnPurpose = SetColumnPurpose(env, input);


SetColumnPurpose [](start = 32, length = 16)

as you mention in comment you use this method to "Get' purposes, so it's better to name this method GetColumnPurpose #Resolved

Fixed

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

Ivanidzo4ka · 2018-07-02T19:00:23Z

src/Microsoft.ML.PipelineInference/AutoInference.cs

+                        foreach (var transform in pipeline.Transforms)
+                        {
+                            transformK += 1;
+                            ch.Info($"Transform {transformK} : {transform.Transform.ToString()}");


.ToString() [](start = 83, length = 11)

i'm not sure you need to call ToString here. #Resolved

Thanks.

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

Ivanidzo4ka · 2018-07-02T19:01:55Z

src/Microsoft.ML.PipelineInference/AutoInference.cs

+                    currentBatchSize,
+                    this._columnPurpose);
+
+                var h = _env.Register("AutoMlMlState");


var h = _env.Register("AutoMlMlState"); [](start = 15, length = 40)

you should have _host field, no reason to create new host variable. #Resolved

Ivanidzo4ka · 2018-07-02T19:02:57Z

src/Microsoft.ML.PipelineInference/AutoInference.cs

+                    this._columnPurpose);
+
+                var h = _env.Register("AutoMlMlState");
+                using (var ch = h.Start("GetNextCandidates"))


GetNextCandidates [](start = 41, length = 17)

Print suggested transforms? #Resolved

Ivanidzo4ka · 2018-07-02T19:03:49Z

src/Microsoft.ML.PipelineInference/AutoInference.cs

+                {
+                    foreach (var pipeline in BatchCandidates)
+                    {
+                        ch.Info("AutoInference Suggested Transforms.");


AutoInference Suggested Transforms. [](start = 33, length = 35)

either add pipeline identification, or remove it, what's the point to repeat same phrase all the time? #Resolved

sfilipi · 2018-07-02T19:05:03Z

src/Microsoft.ML.PipelineInference/PurposeInference.cs

        /// <returns>The result includes the array of auto-detected column purposes.</returns>
-        public static InferenceResult InferPurposes(IHostEnvironment env, IDataView data, IEnumerable<int> columnIndices, Arguments args)
+        public static InferenceResult InferPurposes(IHostEnvironment env, IDataView data, IEnumerable<int> columnIndices, Arguments args,
+            Dictionary<string, ColumnPurpose> columnPurpose = null)


Dictionary<string, ColumnPurpose> columnPurpose = null [](start = 12, length = 54)

Why is this needed here? Should we intersect the result of this, with the purposes we have, in the places that call this? #Resolved

We need it here because we want to first honor the column purpose set by the user -- In line #344 we set the 'SuggestedPurpose' to be what the user provided. By doing this we ensure the 'Experts' below do not do purpose inference (and hence transform inference) on those user provided columns. Auto inference will only be done on columns which do not have user intent

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

sfilipi · 2018-07-02T19:07:16Z

test/BaselineOutput/Common/EntryPoints/core_manifest.json

+          "Default": null
+        },
+        {
+          "Name": "NumericFeatureColumn",


NumericFeatureColumn [](start = 19, length = 20)

I'd keep Column plural: NumericFeatureColumns, in all of those. #Resolved

Using plural form as suggested

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

Ivanidzo4ka · 2018-07-02T19:08:47Z

src/Microsoft.ML.PipelineInference/AutoMlEngines/UniformRandomEngine.cs

@@ -30,12 +30,13 @@ public UniformRandomEngine(IHostEnvironment env)
            : base(env, env.Register("UniformRandomEngine(AutoML)"))
        {}

-        public override PipelinePattern[] GetNextCandidates(IEnumerable<PipelinePattern> history, int numberOfCandidates)
+        public override PipelinePattern[] GetNextCandidates(IEnumerable<PipelinePattern> history, int numberOfCandidates,
+            Dictionary<string, ColumnPurpose> columnPurpose = null)


columnPurpose [](start = 46, length = 13)

any chance you can move them to base class PipelineOptimizerBase so you don't have to pass them around all the time? #Resolved

For the engines, I have created a field in PipelineOptimizerBase. This reduces some clutter.

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

Ivanidzo4ka · 2018-07-02T19:13:40Z

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

+                new ImportTextData.Input { InputFile = inputFileTest, CustomSchema = schema }).Data.Take(numOfSampleRows);
+#pragma warning restore 0618
+            // Define entrypoint graph
+            string inputGraph = @"


I know what most of tests written in way of "Create manually json string and execute it", but it doesn't mean it has to be this way.
check TestSimpleExperiment
any chance I can convince you to use C# classes instead of composing JSON string manually? #Resolved

I added a 4th test case (TestPipelineSweeperMacroColumnPurpose) which follows this paradigm. Does it look good ?

The other 3 tests I would like to keep as is if possible

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

I also vote to keep some of them as strings, because they test the codepath of translating from string to components.

In reply to: 199963125 [](ancestors = 199963125,199594792)

TomFinley · 2018-07-03T01:22:44Z

src/Microsoft.ML.PipelineInference/AutoMlEngines/UniformRandomEngine.cs

@@ -30,8 +30,10 @@ public UniformRandomEngine(IHostEnvironment env)
            : base(env, env.Register("UniformRandomEngine(AutoML)"))
        {}

-        public override PipelinePattern[] GetNextCandidates(IEnumerable<PipelinePattern> history, int numberOfCandidates)
+        public override PipelinePattern[] GetNextCandidates(IEnumerable<PipelinePattern> history, int numberOfCandidates,
+            Dictionary<string, ColumnPurpose> colPurpose = null)


colPurpose [](start = 46, length = 10)

I am somewhat unenthusiastic about this, since it basically seems like role-mappings, except in reverse (which also makes it, incidentally, less capable). Is there any possibility of re-using that concept, rather than inventing another thing that acts more or less just like it? #Resolved

It seems the 'recipe framework' added the concept of 'Purpose' previously. Not quite sure why the decision was made at that time to introduce Purpose as an additional concept, instead of re-using the concept of Roles and extending it.

<snip from InferenceUtil.cs>
public enum ColumnPurpose
{
Ignore = 0,
Name = 1,
Label = 2,
NumericFeature = 3,
CategoricalFeature = 4,
TextFeature = 5,
Weight = 6,
GroupId = 7,
ImagePath = 8
}

Is there a simple way to extend Roles to include purpose (e.g. 'TextFeature', 'Ignore') ? My understanding is that it would be a bigger change that warrants a separate design / PR. Let me know if that sounds reasonable.

This PR is to address the particular case where 'recipe framework' inferred the purpose incorrectly. In sich cases the user has no way of overriding the inferred purpose when using the PipelineSweeperMacro. This PR unblocks such scenarios.

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

You don't extend roles, you simply use them. If you want to introduce roles appropriate for your task, that is totally fine and supported. I wonder though if the issue is that you do not have an actual ISchema implementation we're working on? #Resolved

Is ColumnPurpose and Roles the same things? Feels to me they are not. There is a ColumnPurpose of "ImagePath" but not such a Role, and @justin has ideas about adding more purposes. Purposes would be used to just communicate to Experts what transforms to suggest for the pipelines. Learners would not neccessarly need to know about them.

I have been wondering whether it makes more sense to extend the Loader columns to have a field/property of type ColumnPurpose, rather than pass yet another list of purposes, so they become part of the Schema, and whatever needs the purposes (the inference code) can just look it up in the columns.

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

RecipeInference uses ColumnPurpose to infer the set of transforms to use (and also the columns used for each transform) .

In the GUI users can modify the ColumnPurpose through the wizard. We are adding arguments to the Macro so users can specify the purpose when using the macro programmatically.

Am not sure which ISchema implementation is being referred to. If you are referring to an ISchema implementation at the end of all transformations then yes, we do not have the final ISchema implemenation at this point in the computation. Hence the need to pass ColumnPurpose so we can infer the set of transforms and get to the final ISchema implementation.

Or, is the suggestion to completely do away with the notion of ColumnPurpose ?

In reply to: 200410413 [](ancestors = 200410413,200406010)

Made a couple of changes to address this:

Dropped ‘Ignore’ from Arguments to the PipelineSweepMacro.

Using RoleMappedData in the API (not exposing ColumnPurpose anymore in the APIs)

In reply to: 200723741 [](ancestors = 200723741,200410413,200406010)

TomFinley · 2018-07-03T01:28:44Z

src/Microsoft.ML.PipelineInference/Interfaces/IPipelineOptimizer.cs

@@ -44,6 +45,7 @@ public abstract class PipelineOptimizerBase : IPipelineOptimizer
        protected IDataView OriginalData;
        protected IDataView FullyTransformedData;
        protected AutoInference.DependencyMap DependencyMapping;
+        protected Dictionary<string, ColumnPurpose> columnPurpose;


columnPurpose; [](start = 52, length = 14)

C# naming conventions, please. #Resolved

Fixed.

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

justinormont

This PR is simply copying what we do in the GUI Wizard to the PipelineSweeperMacro; the overall style for this was laid out in Recipes previously.

@codemzs : Since you created Recipes, can you take a look?

Merge latest code from dotnet/master

Merge latest code from agoswamiazureml/master

…amiazureml/machinelearning into agoswami/purpose_inference

sfilipi · 2018-07-12T20:26:15Z

src/Microsoft.ML.PipelineInference/Macros/PipelineSweeperMacro.cs

@@ -36,6 +36,30 @@ public sealed class Arguments

            [Argument(ArgumentType.AtMostOnce, HelpText = "Output datasets from previous iteration of sweep.", SortOrder = 7, Hide = true)]
            public IDataView[] CandidateOutputs;
+
+            [Argument(ArgumentType.MultipleUnique, HelpText = "Column(s) to use as purpose 'Label'", SortOrder = 8)]


purpose [](start = 83, length = 7)

do we want to use "role" rather than purpose. #Resolved

… consistent with Role 'Group'

merge with dotnet/master

update to latest master

sfilipi · 2018-07-13T17:57:59Z

src/Microsoft.ML.PipelineInference/Macros/PipelineSweeperMacro.cs

+            public string[] TextFeatureColumns;
+
+            [Argument(ArgumentType.MultipleUnique, HelpText = "Column(s) to use as Role 'ImagePath'", SortOrder = 15)]
+            public string[] ImagePathColumns;


Shall we also log a bug about finding a more scalable solution? #Resolved

Yes, there is a bug for that already

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

sfilipi · 2018-07-13T17:58:48Z

src/Microsoft.ML.PipelineInference/Macros/PipelineSweeperMacro.cs

+        {
+            var roles = new List<KeyValuePair<RoleMappedSchema.ColumnRole, string>>();
+
+            if (input.LabelColumns != null)


if (input.LabelColumns != null) [](start = 12, length = 31)

would you want to make specifying this mandatory? #ByDesign

one scenario that needs to be supported is when user does not specify anything. in that case case we will do AutoInference on all columns.

That's why these arguments (including Label) are not mandatory. These arguments are meant to give user the ability to override the auto inference (if needed)

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

sfilipi

sfilipi

* Adding arguments to PipelineSweep Macro * updating the unit tests * taking care of review comments; adding validations for Label, Weight, GroupId and Name columns * taking care of some review comments * some code cleanup * addressing PR comments * API changes to use RoleMappedData * taking care of review comments * using pipeline.UniqueId * taking care of review comments. update ColumnPurpose 'Group' so it is consistent with Role 'Group'

abgoswam added 5 commits June 20, 2018 11:32

Merge pull request #1 from dotnet/master

e1fa741

Sync with latest ML.NET

Merge pull request #2 from dotnet/master

703cb38

Merge with latest master of dotnet/machinelearning

Merge pull request #3 from dotnet/master

c3c2125

Merge with latest dotnet master

Adding arguments to PipelineSweep Macro

17026a4

updating the unit tests

c76181c

justinormont requested review from justinormont and yaeldekel June 30, 2018 19:01

justinormont reviewed Jun 30, 2018

View reviewed changes

taking care of review comments; adding validations for Label, Weight,…

28371fe

… GroupId and Name columns

Ivanidzo4ka reviewed Jul 2, 2018

View reviewed changes

sfilipi reviewed Jul 2, 2018

View reviewed changes

Ivanidzo4ka reviewed Jul 2, 2018

View reviewed changes

abgoswam added 2 commits July 2, 2018 22:58

taking care of some review comments

310b395

some code cleanup

4fbaf14

TomFinley reviewed Jul 3, 2018

View reviewed changes

addressing PR comments

3ca4da9

justinormont approved these changes Jul 6, 2018

View reviewed changes

shauheen requested a review from codemzs July 10, 2018 07:45

abgoswam added 6 commits July 11, 2018 15:49

Merge pull request #4 from dotnet/master

678d727

Merge latest code from dotnet/master

Merge pull request #5 from agoswamiazureml/master

7f94a87

Merge latest code from agoswamiazureml/master

API changes to use RoleMappedData

d55b1e0

Merge branch 'agoswami/purpose_inference' of https://github.com/agosw…

c624415

…amiazureml/machinelearning into agoswami/purpose_inference

taking care of review comments

d4f6102

using pipeline.UniqueId

99172e2

sfilipi reviewed Jul 12, 2018

View reviewed changes

abgoswam added 3 commits July 12, 2018 21:29

taking care of review comments. update ColumnPurpose 'Group' so it is…

fb5ca79

… consistent with Role 'Group'

Merge pull request #6 from dotnet/master

6b34d71

merge with dotnet/master

Merge pull request #7 from agoswamiazureml/master

3ad0c08

update to latest master

sfilipi reviewed Jul 13, 2018

View reviewed changes

sfilipi approved these changes Jul 13, 2018

View reviewed changes

ghost merged commit 3053f3d into dotnet:master Jul 13, 2018

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

This pull request was closed.

Fix column purpose for PipelineSweeperMacro #461

Fix column purpose for PipelineSweeperMacro #461

Conversation

ghost commented Jun 30, 2018

dnfclas commented Jun 30, 2018 • edited by ghost Loading

ghost commented Jun 30, 2018 • edited by ghost Loading

ghost commented Jun 30, 2018 • edited by ghost Loading

justinormont Jun 30, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Jul 2, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Jul 2, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Jul 3, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Ivanidzo4ka Jul 2, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Jul 2, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Jul 2, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Ivanidzo4ka Jul 2, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Ivanidzo4ka Jul 2, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

sfilipi Jul 2, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Jul 2, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Jul 2, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Jul 2, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Ivanidzo4ka Jul 2, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Jul 3, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Jul 3, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Jul 3, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

TomFinley Jul 5, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

sfilipi Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

ghost Jul 6, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Jul 3, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinormont left a comment

Choose a reason for hiding this comment

sfilipi Jul 12, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

sfilipi Jul 13, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Jul 13, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi left a comment

Choose a reason for hiding this comment

sfilipi left a comment

Choose a reason for hiding this comment

dnfclas commented Jun 30, 2018 •

edited by ghost

Loading

ghost commented Jun 30, 2018 •

edited by ghost

Loading

ghost commented Jun 30, 2018 •

edited by ghost

Loading

justinormont Jun 30, 2018 •

edited by ghost

Loading

Ivanidzo4ka Jul 2, 2018 •

edited by ghost

Loading

Ivanidzo4ka Jul 2, 2018 •

edited by ghost

Loading

ghost Jul 3, 2018 •

edited by ghost

Loading

Ivanidzo4ka Jul 2, 2018 •

edited by ghost

Loading

Ivanidzo4ka Jul 2, 2018 •

edited by ghost

Loading

Ivanidzo4ka Jul 2, 2018 •

edited by ghost

Loading

Ivanidzo4ka Jul 2, 2018 •

edited by ghost

Loading

Ivanidzo4ka Jul 2, 2018 •

edited by ghost

Loading

sfilipi Jul 2, 2018 •

edited by ghost

Loading

sfilipi Jul 2, 2018 •

edited by ghost

Loading

Ivanidzo4ka Jul 2, 2018 •

edited by ghost

Loading

ghost Jul 2, 2018 •

edited by ghost

Loading

Ivanidzo4ka Jul 2, 2018 •

edited by ghost

Loading

ghost Jul 3, 2018 •

edited by ghost

Loading

TomFinley Jul 3, 2018 •

edited by ghost

Loading

ghost Jul 3, 2018 •

edited by ghost

Loading

TomFinley Jul 5, 2018 •

edited by ghost

Loading

sfilipi Jul 5, 2018 •

edited

Loading

ghost Jul 6, 2018 •

edited by ghost

Loading

TomFinley Jul 3, 2018 •

edited by ghost

Loading

sfilipi Jul 12, 2018 •

edited by ghost

Loading

sfilipi Jul 13, 2018 •

edited by ghost

Loading

sfilipi Jul 13, 2018 •

edited by ghost

Loading