ML Context to create them all #1252

Zruty0 · 2018-10-13T01:52:31Z

Fixes #1098 .
Adds a couple extensions for transforms, and almost all trainers.
Adds text loading and saving, model loading.

sfilipi · 2018-10-13T04:06:08Z

using System;

copyright #Resolved

Refers to: src/Microsoft.ML.Data/DataLoadSave/DataLoadSaveCatalog.cs:1 in 26913ac. [](commit_id = 26913ac, deletion_comment = False)

sfilipi · 2018-10-13T04:07:52Z

src/Microsoft.ML.Data/DataLoadSave/Text/TextLoaderSaverCatalog.cs

+        /// </summary>
+        /// <param name="catalog">The catalog.</param>
+        /// <param name="args">The arguments to text reader, describing the data schema.</param>
+        /// <param name="dataSample">The optional data sample</param>


data sample, or dataSource?
#Resolved

It's a sample

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

sfilipi · 2018-10-13T04:08:15Z

src/Microsoft.ML.Data/DataLoadSave/Text/TextLoaderSaverCatalog.cs

+        /// <param name="catalog">The catalog.</param>
+        /// <param name="args">The arguments to text reader, describing the data schema.</param>
+        /// <param name="dataSample">The optional data sample</param>
+        /// <returns></returns>


[](start = 12, length = 19)

empty returns #Resolved

sfilipi · 2018-10-13T05:29:40Z

src/Microsoft.ML.Data/Model/ModelOperationsCatalog.cs

+        public void Save(ITransformer model, Stream stream) => model.SaveTo(Environment, stream);
+
+        /// <summary>
+        /// Load the model from the stream.


model [](start = 21, length = 5)

If we are saving and loading an ITransformer, should we call the class TransformerOperationsCatalog? #Resolved

I think the name should reflect the user-facing concept, rather than the implementation fact that 'trained model is a transformer'.

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

sfilipi · 2018-10-13T05:35:11Z

src/Microsoft.ML.Data/Training/TrainContext.cs

@@ -403,7 +372,7 @@ internal MulticlassClassificationTrainers(MulticlassClassificationContext ctx)
        /// the top-K accuracy, that is, the accuracy assuming we consider an example with the correct class within
        /// the top-K values as being stored "correctly."</param>
        /// <returns>The evaluation results for these calibrated outputs.</returns>
-        public MultiClassClassifierEvaluator.Result Evaluate(IDataView data, string label, string score = DefaultColumnNames.Score,
+        public MultiClassClassifierEvaluator.Result Evaluate(IDataView data, string label = DefaultColumnNames.Label, string score = DefaultColumnNames.Score,


string label = DefaultColumnNames.Label [](start = 77, length = 39)

would assigning defaults here make it error prone. This allows the users to trip and provide a name for the label column in the estimators, and leave it to the default name here? We have not assigned defaults in the estimators. #Resolved

Actually, all the trainer extensions also have default names for Label and Features. This is why I made this change as well.

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

sfilipi · 2018-10-13T05:38:25Z

src/Microsoft.ML.Data/Transforms/NormalizerCatalog.cs

+        /// </summary>
+        /// <param name="catalog">The transform catalog</param>
+        /// <param name="columnName">The column name</param>
+        /// <param name="mode">The normalization mode</param>


The normalization mod [](start = 31, length = 21)

cref maybe #Resolved

sfilipi · 2018-10-13T05:39:16Z

src/Microsoft.ML.Data/Transforms/TransformsCatalog.cs

+namespace Microsoft.ML.Runtime
+{
+    /// <summary>
+    /// Similar to training context, a transform context is an object serving as a 'catalog' of available transforms.


I like catalog. #Resolved

sfilipi · 2018-10-13T05:43:41Z

src/Microsoft.ML.KMeansClustering/KMeansCatalog.cs

+        /// <param name="weights">The optional example weights.</param>
+        /// <param name="clustersCount">The number of clusters to use for KMeans.</param>
+        /// <param name="advancedSettings">Algorithm advanced settings.</param>
+        public static KMeansPlusPlusTrainer KMeans(this ClusteringContext.ClusteringTrainers ctx,


KMeansPlusPlusTrainer [](start = 22, length = 21)

shall we add returns to all those new members, to distinguish them from the other extensions in the documentation? #Closed

hmm, I'm not sure why

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

sfilipi · 2018-10-13T05:44:22Z

src/Microsoft.ML.KMeansClustering/KMeansStatic.cs

 using Microsoft.ML.StaticPipe.Runtime;
 using System;

-namespace Microsoft.ML.Trainers
+namespace Microsoft.ML.StaticPipe


StaticPipe [](start = 23, length = 10)

why change namespace? I believe we decided to keep the trainers inside Microsoft.ML.Trainers.
The methods that return trainers iside the new catalog classes are being placed under Microsoft.ML.

@GalOshri what do you think? #Resolved

I made this sweeping change, because otherwise we are seeing 2 methods per trainer in the intellisense by default.

I think for now we want users to 'opt in' to the static API by using StaticPipe, and have the dynamic API available right off the bat

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

eerhardt · 2018-10-15T17:23:41Z

src/Microsoft.ML.Data/Training/MLContext.cs

+        /// <param name="conc">Concurrency level. Set to 1 to run single-threaded. Set to 0 to pick automatically.</param>
+        public MLContext(int? seed = null, int conc = 0)
+        {
+            _env = new LocalEnvironment(seed, conc);


I would have assumed that we just rename LocalEnvironment to MLContext. Why do we have both? #Resolved

Good point, let me think this through.

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

I would probably not bundle this change right now, but I'll write a REVIEW and a GitHib issue

In reply to: 225262172 [](ancestors = 225262172,225250207)

eerhardt · 2018-10-15T17:26:01Z

// Licensed to the .NET Foundation under one or more agreements.

Why is this file under the Training folder? #Resolved

Refers to: src/Microsoft.ML.Data/Training/MLContext.cs:1 in 26913ac. [](commit_id = 26913ac, deletion_comment = False)

eerhardt · 2018-10-15T17:37:38Z

src/Microsoft.ML.Data/Training/MLContext.cs

+        /// <summary>
+        /// Data processing operations.
+        /// </summary>
+        public TransformsCatalog Transform { get; }


I know the other properties are not plural, but for some reason the singular Transform sticks out to me. It feels like it should be Transforms. Even the class name TransformsCatalog has plural. #Resolved

Hmm, I like it the way it is. Maybe other commenters can swing the vote?

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

Ok, I renamed it

In reply to: 225262070 [](ancestors = 225262070,225254551)

Zruty0 · 2018-10-15T18:00:22Z

// Licensed to the .NET Foundation under one or more agreements.

It has to be in one of the folders :) It is mostly used for composing pipelines, so I picked Training. Do you have a better idea?

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

Refers to: src/Microsoft.ML.Data/Training/MLContext.cs:1 in 26913ac. [](commit_id = 26913ac, deletion_comment = False)

eerhardt · 2018-10-15T18:38:58Z

// Licensed to the .NET Foundation under one or more agreements.

What's wrong with the folder: https://github.com/dotnet/machinelearning/tree/master/src/Microsoft.ML.Data ?

In general, the .NET Core team follows folders-namespace alignment, so it is easier to find files. Since this type is in the root of the namespace: Microsoft.ML, I believe it should be in the root of the folders.

In reply to: 429953480 [](ancestors = 429953480,429941509)

Refers to: src/Microsoft.ML.Data/Training/MLContext.cs:1 in 26913ac. [](commit_id = 26913ac, deletion_comment = False)

Zruty0 · 2018-10-15T22:34:27Z

// Licensed to the .NET Foundation under one or more agreements.

I put it in the root.

In reply to: 429967563 [](ancestors = 429967563,429953480,429941509)

Refers to: src/Microsoft.ML.Data/Training/MLContext.cs:1 in 26913ac. [](commit_id = 26913ac, deletion_comment = False)

artidoro · 2018-10-16T17:46:02Z

docs/code/MlNetCookBook.md

@@ -293,11 +288,11 @@ In the file above, the last column (12th) is label that we predict, and all the
 ```csharp
 // Create a new environment for ML.NET operations. It can be used for exception tracking and logging, 


environment [](start = 16, length = 11)

Would be a good idea to replace with the above comments:

// Create a new context for ML.NET operations. It can be used for exception tracking and logging,
// as a catalog of available operations and as the source of randomness. #Resolved

artidoro · 2018-10-16T17:47:04Z

docs/code/MlNetCookBook.md

@@ -449,13 +437,13 @@ The prediction code now looks as follows:
 ```csharp
 // Create a new environment for ML.NET operations. It can be used for exception tracking and logging, 
 // as well as the source of randomness.


Same as above, would be a good idea to replace with the above comments:

// Create a new context for ML.NET operations. It can be used for exception tracking and logging,
// as a catalog of available operations and as the source of randomness. #Resolved

artidoro · 2018-10-16T17:47:22Z

docs/code/MlNetCookBook.md

@@ -797,10 +776,10 @@ Sentiment   SentimentText
 ```csharp
 // Create a new environment for ML.NET operations. It can be used for exception tracking and logging, 
 // as well as the source of randomness.


Would be a good idea to replace with the above comments:

// Create a new context for ML.NET operations. It can be used for exception tracking and logging,
// as a catalog of available operations and as the source of randomness. #Resolved

artidoro · 2018-10-16T17:49:56Z

docs/code/MlNetCookBook.md

@@ -345,7 +337,7 @@ Assuming the example above was used to train the model, here's how you calculate
 var testData = reader.Read(new MultiFileSource(testDataPath));
 // Calculate metrics of the model on the test data.
 // We are using the 'regression' context object here to perform evaluation.
-var metrics = regression.Evaluate(model.Transform(testData), label: r => r.Target, score: r => r.Prediction);
+var metrics = mlContext.Regression.Evaluate(model.Transform(testData), label: r => r.Target, score: r => r.Prediction);


Is this sentence still correct? #Resolved

eerhardt · 2018-10-17T15:41:59Z

src/Microsoft.ML.Data/MLContext.cs

+    /// </summary>
+    public sealed class MLContext : IHostEnvironment
+    {
+        // REVIEW: consider making LocalEnvironment and MLContext the same class instead of encapsulation.


I still don't fully understand our plan here with LocalEnvironment and ConsoleEnvironment. How could I ever use a ConsoleEnvironment with this new MLContext class?

And if the answer is "you can't - you'd use ConsoleEnvironment directly", I think that is a pretty poor answer because then my whole API paradigm changes. I can no longer use BinaryClassificationContext, Transforms, etc. from the MLContext. #Closed

What do you think about the answer: "you can't use ConsoleEnvironment because it is internal and only used by MAML"?

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

That would be fine with me. For now, I would think it's completely fine to have our custom console printing internal to the commandline tool

In reply to: 225989180 [](ancestors = 225989180,225986842)

#1284 #Closed

Thanks.

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

sfilipi

eerhardt · 2018-10-17T21:43:41Z

test/Microsoft.ML.Tests/Scenarios/Api/CookbookSamples/CookbookSamples.cs


            using (var stream = File.Create(modelPath))
            {
                // Saving and loading happens to 'dynamic' models, so the static typing is lost in the process.
-                model.AsDynamic.SaveTo(env, stream);
+                mlContext.Model.Save(model.AsDynamic, stream);


Thoughts on making this so I don't have to say model.AsDynamic? Can we open an issue to add that overload? #Pending

absolutely: #1286

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

eerhardt · 2018-10-17T21:46:52Z

test/Microsoft.ML.Tests/Scenarios/Api/CookbookSamples/CookbookSamples.cs

+                    new TextLoader.Column("Target", DataKind.R4, 10)
+                },
+                // Default separator is tab, but we need a comma.
+                s => s.Separator = ",");


(This is probably not exactly related to MLContext, but since you wrote this new test, I'll ask the question):

Do we like having s => here? Why isn't the separator just an argument in the TextReader() method? #Pending

The pattern we want to have is that we add the most important options as ctor options, and the rest 'advanced' inside the delegate.
The other pattern is that we want the extensions to replicate the signatures of constructors, to avoid any mapping logic in strange places.
Because of that, I'd rather change the signature of TextLoader ctor to add 'hasHeader' and 'separator', but we will need to do it in a separate PR.

Also 'separator' should not really be a string :)

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

eerhardt · 2018-10-17T21:56:23Z

src/Microsoft.ML.Transforms/CategoricalTransform.cs

@@ -128,7 +128,7 @@ public Arguments()
        public static IDataView Create(IHostEnvironment env, IDataView input, string name,
            string source = null, OutputKind outputKind = CategoricalEstimator.Defaults.OutKind)
        {
-            return new CategoricalEstimator(env, name, source, outputKind).Fit(input).Transform(input) as IDataView;
+            return new CategoricalEstimator(env, source, name, outputKind).Fit(input).Transform(input) as IDataView;


Maybe we should consider renaming and re-ordering the parameters to this method as well. name and source don't really help say what they are. Also, if new CategoricalEstimator has it one way, and CategoricalTransform.Create has it the other way, that is going to lead to confusion. #Resolved

Sure

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

eerhardt · 2018-10-17T21:57:14Z

src/Microsoft.ML.Transforms/Text/TextTransformCatalog.cs

+using System;
+using System.Collections.Generic;
+
+namespace Microsoft.ML


(nit) Since this is in the Microsoft.ML namespace, my preference would be for the file to not be under the Text folder. I think namespaces and folders should align. #Resolved

eerhardt · 2018-10-17T21:59:33Z

src/Microsoft.ML.FastTree/FastTreeCatalog.cs

+    /// <summary>
+    /// FastTree <see cref="TrainContextBase"/> extension methods.
+    /// </summary>
+    public static partial class RegressionTrainers


There already is a Microsoft.ML.RegressionTrainers class in the StandardLearners and LightGBM assemblies. We shouldn't have public types with the same namespace and name across multiple assemblies. It is only going to cause headaches for our users. #Closed

This applies to all the other extension classes as well.

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

I believe this was intentionally done by Senja for some documentation reasons?

In reply to: 226109272 [](ancestors = 226109272,226109180)

Let me keep it as is for now, and I'll sync with her and understand why it was done. For me, it would've been much easier to group by trainer, into FastTreeTrainerCatalog or something

In reply to: 226109747 [](ancestors = 226109747,226109272,226109180)

Let me keep it as is for now

I don't know .... this is a pretty strong "don't" rule in general. If we are going to fix this very soon (like the next few days), I'm OK with it. But please log an issue. We can't ship with conflicting types across our assemblies. #Closed

OK spoke to Senja, we should revert that :)

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

eerhardt · 2018-10-17T22:04:29Z

src/Microsoft.ML.Data/Transforms/NormalizerCatalog.cs

+    /// <summary>
+    /// Extensions for normalizer operations.
+    /// </summary>
+    public static class NormalizerCatalog


What are the use cases for NormalizerCatalog? I don't see any usages of it in this PR.

I see it is a static class, while all the other "catalog" classes are instance members of MLContext. Should it be designed the same as say TransformsCatalog?

Also, I don't see any tests using this class. #Resolved

Ah, I get it - this provides extension methods to the TransformsCatalog. How about we name these "Extension" only classes differently then? It is confusing in my opinion that NormalizerCatalog and TransformsCatalog are named similarly, but are designed differently.

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

This is a set of extensions to TransformsCatalog. I guess I will need to rename it

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

And add tests :D

In reply to: 226110673 [](ancestors = 226110673,226110284)

eerhardt · 2018-10-17T22:09:16Z

src/Microsoft.ML.Data/DataLoadSave/Text/TextLoader.cs

@@ -1320,6 +1337,8 @@ public void Save(ModelSaveContext ctx)

        public IDataView Read(IMultiStreamSource source) => new BoundLoader(this, source);

+        public IDataView Read(string path) => Read(new MultiFileSource(path));


❤️ #Closed

eerhardt

ML Context and a couple extensions

26913ac

Zruty0 added the API Issues pertaining the friendly API label Oct 13, 2018

Zruty0 added this to the 1018 milestone Oct 13, 2018

Zruty0 self-assigned this Oct 13, 2018

Zruty0 requested review from Ivanidzo4ka, TomFinley and sfilipi October 13, 2018 01:52

sfilipi reviewed Oct 13, 2018

View reviewed changes

sfilipi mentioned this pull request Oct 15, 2018

Training regression model from DataView #1235

Closed

Merge remote-tracking branch 'upstream/master' into feature/mlcontext

c6b5a67

eerhardt reviewed Oct 15, 2018

View reviewed changes

Pete Luferenko added 2 commits October 15, 2018 15:10

PR comments and cookbook update

e41fc21

Merge remote-tracking branch 'upstream/master' into feature/mlcontext

e548c63

artidoro reviewed Oct 16, 2018

View reviewed changes

Pete Luferenko added 2 commits October 16, 2018 17:11

PR comments

f7e99fd

Merge

65906d2

eerhardt reviewed Oct 17, 2018

View reviewed changes

eerhardt mentioned this pull request Oct 17, 2018

Make ConsoleEnvironment internal #1284

Closed

sfilipi approved these changes Oct 17, 2018

View reviewed changes

eerhardt reviewed Oct 17, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into feature/mlcontext

bbb7be4

eerhardt reviewed Oct 17, 2018

View reviewed changes

eerhardt approved these changes Oct 17, 2018

View reviewed changes

Pete Luferenko added 2 commits October 17, 2018 16:41

PR comments

8c485fc

Fixed tests and removed extra factory method

3062cb8

Zruty0 merged commit 9157cea into dotnet:master Oct 18, 2018

TomFinley mentioned this pull request Nov 30, 2018

Remove TextLoader.Read(string), promote IDataReader<MSS> extension #1797

Closed

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

		@@ -293,11 +288,11 @@ In the file above, the last column (12th) is label that we predict, and all the
		```csharp
		// Create a new environment for ML.NET operations. It can be used for exception tracking and logging,

		@@ -1320,6 +1337,8 @@ public void Save(ModelSaveContext ctx)

		public IDataView Read(IMultiStreamSource source) => new BoundLoader(this, source);

		public IDataView Read(string path) => Read(new MultiFileSource(path));

ML Context to create them all #1252

ML Context to create them all #1252

Conversation

Zruty0 commented Oct 13, 2018

sfilipi commented Oct 13, 2018 • edited by Zruty0 Loading

sfilipi Oct 13, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Oct 13, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

sfilipi Oct 13, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Oct 13, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Oct 13, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

sfilipi Oct 13, 2018 • edited Loading

Choose a reason for hiding this comment

sfilipi Oct 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Oct 13, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Oct 15, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt commented Oct 15, 2018 • edited by Zruty0 Loading

eerhardt Oct 15, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 commented Oct 15, 2018

eerhardt commented Oct 15, 2018

Zruty0 commented Oct 15, 2018

artidoro Oct 16, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

artidoro Oct 16, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

artidoro Oct 16, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

artidoro Oct 16, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

eerhardt Oct 17, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Oct 17, 2018 • edited by Zruty0 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

eerhardt Oct 17, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Zruty0 Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

eerhardt Oct 17, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Oct 17, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Oct 17, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

eerhardt Oct 17, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Oct 17, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Oct 17, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi commented Oct 13, 2018 •

edited by Zruty0

Loading

sfilipi Oct 13, 2018 •

edited by Zruty0

Loading

sfilipi Oct 13, 2018 •

edited by Zruty0

Loading

sfilipi Oct 13, 2018 •

edited by Zruty0

Loading

sfilipi Oct 13, 2018 •

edited by Zruty0

Loading

sfilipi Oct 13, 2018 •

edited by Zruty0

Loading

sfilipi Oct 13, 2018 •

edited

Loading

sfilipi Oct 13, 2018 •

edited

Loading

sfilipi Oct 13, 2018 •

edited by Zruty0

Loading

eerhardt Oct 15, 2018 •

edited by Zruty0

Loading

eerhardt commented Oct 15, 2018 •

edited by Zruty0

Loading

eerhardt Oct 15, 2018 •

edited by Zruty0

Loading

artidoro Oct 16, 2018 •

edited by Zruty0

Loading

artidoro Oct 16, 2018 •

edited by Zruty0

Loading

artidoro Oct 16, 2018 •

edited by Zruty0

Loading

artidoro Oct 16, 2018 •

edited by Zruty0

Loading

eerhardt Oct 17, 2018 •

edited by Zruty0

Loading

eerhardt Oct 17, 2018 •

edited by Zruty0

Loading

eerhardt Oct 17, 2018 •

edited by Zruty0

Loading

Zruty0 Oct 17, 2018 •

edited

Loading

eerhardt Oct 17, 2018 •

edited by Zruty0

Loading

eerhardt Oct 17, 2018 •

edited by Zruty0

Loading

eerhardt Oct 17, 2018 •

edited by Zruty0

Loading

eerhardt Oct 17, 2018 •

edited by Zruty0

Loading

eerhardt Oct 17, 2018 •

edited by Zruty0

Loading

eerhardt Oct 17, 2018 •

edited by Zruty0

Loading

eerhardt Oct 17, 2018 •

edited by Zruty0

Loading