Redesign DnnCatalog methods API for ease of use and consistency. #4362

ashbhandare · 2019-10-22T00:06:44Z

Initially the methods in the DnnCatalog took a lot of optional arguments which would confuse users and adding features to the ImageClassification API would require breaking changes in the future.

To be consistent with the rest of ML .Net, this change follows the pattern where we have 2 overloads:
1)An overload that takes the required parameters, and optionally the most important/common parameters to a method.
2)An overload that takes an Options object, which contains all the options to the method

This changes also removes a couple of unused options for the ImageClassification API, while making RetrainDnnModel internal to enable further testing before releasing.

WIP to get initial feedback/add necessary tests.

eerhardt · 2019-10-22T14:42:58Z

@ashbhandare - please don't use WIP in the title anymore on PRs. If the PR isn't ready for review and merge, then open a Draft PR.

https://github.blog/2019-02-14-introducing-draft-pull-requests/ #Resolved

eerhardt · 2019-10-22T14:47:23Z

src/Microsoft.ML.Dnn/DnnCatalog.cs

+        /// <summary>
+        /// The options for the <see cref="ImageClassificationTransformer"/>.
+        /// </summary>
+        public sealed class Options


We don't need 2 "Options" classes. We only need one.

This appears to be a duplicate of the ImageClassificationEstimator.Options class. One of these should be removed. #Resolved

What should happen is the nested ImageClassificationEstimator.Options class should become public, and then this one can be deleted.

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

The two classes have most members in common, except that DnnCatalog.Options has featuresColumnName, and ImageClassificationEstimator.Options() takes in InputColumns and OutputColumns. Using ImageClassificationEstimator.Options directly would change the necessary arguments that ImageClassification() would take. I can make that change if that is acceptable.

In reply to: 337568602 [](ancestors = 337568602,337563694)

eerhardt · 2019-10-22T14:48:49Z

src/Microsoft.ML.Dnn/DnnCatalog.cs

            };

-            if (!File.Exists(options.ModelLocation))
+            var env = CatalogUtils.GetEnvironment(catalog);
+            return new ImageClassificationEstimator(env, options, DnnUtils.LoadDnnModel(env, options.Arch, true));


This method should just call into the ImageClassification overload below that takes an Options class. #Resolved

eerhardt · 2019-10-22T14:51:54Z

src/Microsoft.ML.Dnn/DnnCatalog.cs

@@ -83,6 +83,120 @@ public static class DnnCatalog
            return new DnnRetrainEstimator(env, options, DnnUtils.LoadDnnModel(env, modelPath, true));
        }

+        /// <summary>
+        /// The options for the <see cref="ImageClassificationTransformer"/>.


This should say it is the options for the Estimator, not the Transformer. These options are used to create the Estimator. #Resolved

eerhardt · 2019-10-22T14:52:32Z

src/Microsoft.ML.Dnn/DnnCatalog.cs

-        /// <param name="reuseTrainSetBottleneckCachedValues">Indicates to not re-compute cached trainset bottleneck values if already available in the bin folder.</param>
-        /// <param name="reuseValidationSetBottleneckCachedValues">Indicates to not re-compute validataionset cached bottleneck validationset values if already available in the bin folder.</param>
-        /// <param name="trainSetBottleneckCachedValuesFilePath">Indicates the file path to store trainset bottleneck values for caching.</param>
-        /// <param name="validationSetBottleneckCachedValuesFilePath">Indicates the file path to store validationset bottleneck values for caching.</param>
        /// <remarks>
        /// The support for image classification is under preview.


@codemzs - When should this remark get removed? I'm assuming we don't want to ship with this in the documentation. #Resolved

Removing the remark after talking to @codemzs offline.

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

eerhardt · 2019-10-22T14:53:15Z

src/Microsoft.ML.Dnn/DnnCatalog.cs

+            /// <summary>
+            /// The names of the model inputs.
+            /// </summary>
+            [Argument(ArgumentType.Multiple | ArgumentType.Required, HelpText = "The names of the model inputs", ShortName = "inputs", SortOrder = 1)]


Why is this marked with ArgumentType.Multiple? We only support a single Features column name, correct? #Resolved

eerhardt · 2019-10-22T14:54:09Z

src/Microsoft.ML.Dnn/DnnCatalog.cs

+            /// Validation set.
+            /// </summary>
+            [Argument(ArgumentType.AtMostOnce, HelpText = "Validation set.", SortOrder = 15)]
+            public IDataView ValidationSet = null;


Reference types are null by default. You don't need to explicitly put = null at the end here, and below. #Resolved

harishsk · 2019-10-22T15:23:24Z

docs/samples/Microsoft.ML.Samples/Program.cs

@@ -15,7 +15,7 @@ internal static void RunAll()
            {
                var sample = type.GetMethod("Example", BindingFlags.Public | BindingFlags.Static | BindingFlags.FlattenHierarchy);

-                if (sample != null)
+                if (sample != null && type.Name == "ResnetV2101TransferLearningTrainTestSplit")
                {


Did this get accidentally left behind? #Resolved

That's right, thank you!

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

… addressed review comments.

eerhardt · 2019-10-22T18:48:28Z

src/Microsoft.ML.Dnn/DnnCatalog.cs

@@ -46,7 +46,7 @@ public static class DnnCatalog
        /// <remarks>
        /// The support for retraining is under preview.
        /// </remarks>
-        public static DnnRetrainEstimator RetrainDnnModel(
+        internal static DnnRetrainEstimator RetrainDnnModel(


Should the DnnRetrainTransformer and DnnRetrainEstimator classes also be marked as internal? #Resolved

eerhardt · 2019-10-22T18:57:11Z

src/Microsoft.ML.Dnn/ImageClassificationTransform.cs

+            /// The names of the model input features.
+            /// </summary>
+            [Argument(ArgumentType.AtMostOnce | ArgumentType.Required, HelpText = "The names of the model inputs", ShortName = "features", SortOrder = 1)]
+            public string FeaturesColumnName;


Why do we have both InputColumns and FeaturesColumnName? We should only have 1. #Resolved

The same applies for LabelColumnName.

And also for OutputColumns and ScoreColumnName.

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

OutputColumns is an array of ScoreColumn and PredictedLabelColumn. Users can have a choice of setting either one or both of these names. I could make OutputColumns internal if that's cleaner.

In reply to: 337694812 [](ancestors = 337694812,337694012)

Would this API take more than one input at any point, as InputColumn is an array? I could either make InputColumn internal or get rid of one of InputColumns and FeaturesColumnName.

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

Making InputColumns and OutputColumns internal after talking with @[email protected] offline.

In reply to: 337712848 [](ancestors = 337712848,337694012)

eerhardt · 2019-10-22T18:58:31Z

src/Microsoft.ML.Dnn/ImageClassificationTransform.cs

@@ -1341,31 +1340,31 @@ public enum Dataset
        /// <summary>
        /// The options for the <see cref="ImageClassificationTransformer"/>.
        /// </summary>
-        internal sealed class Options : TransformInputBase
+        public sealed class Options : TransformInputBase


This shouldn't inherit from TransformInputBase. That class is for data transforms, and this is a trainer. #Resolved

done.

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

eerhardt · 2019-10-22T19:01:58Z

docs/samples/Microsoft.ML.Samples/Dynamic/ImageClassification/ImageClassificationDefault.cs

+                return false;
+            }
+
+            var wc = new WebClient();


We need to fix this pattern. This is becoming pervasive in our repo.

It is not recommended to use WebClient. We should use HttpClient instead.

https://docs.microsoft.com/en-us/dotnet/api/system.net.webclient?view=netframework-4.8#remarks #Resolved

This change is included in another PR #4314, which is failing the CI test intermittently. I would like to make that change separate from this if that's okay.

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

eerhardt · 2019-10-22T19:02:25Z

docs/samples/Microsoft.ML.Samples/Dynamic/ImageClassification/ImageClassificationDefault.cs

+            if (File.Exists(Path.Combine(destFolder, flag))) return;
+
+            Console.WriteLine($"Extracting.");
+            var task = Task.Run(() =>


There is no reason to do a Task.Run here, and then Thread.Sleep later. Just call ZipFile.ExtractToDirectory inline in the method. #Resolved

eerhardt · 2019-10-22T19:03:31Z

src/Microsoft.ML.Dnn/DnnCatalog.cs

@@ -6,11 +6,11 @@
 using System.IO;
 using System.IO.Compression;
 using System.Net;
+using Microsoft.ML.CommandLine;


Do we still need this using? #Resolved

eerhardt · 2019-10-22T19:04:08Z

src/Microsoft.ML.Dnn/DnnCatalog.cs

-                    }
-                }
-            }
+        public static ImageClassificationEstimator ImageClassification(


This should have XML docs since it is a public method. #Resolved

eerhardt · 2019-10-22T19:05:43Z

src/Microsoft.ML.Dnn/ImageClassificationTransform.cs

-            [Argument(ArgumentType.AtMostOnce, HelpText = "Training labels.", ShortName = "label", SortOrder = 4)]
-            public string LabelColumn;
+            [Argument(ArgumentType.AtMostOnce | ArgumentType.Required, HelpText = "Training labels.", ShortName = "label", SortOrder = 4)]
+            public string LabelColumnName;


What's the difference between this and TensorFlowLabel? #Resolved

As far as I can understand, TensorFlowLabel is the name of the TF node in the model that contains the label, while the LabelColumn is the name of the column in the IDataView. They are assigned the same value, so working on removing one of them.

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

codemzs · 2019-10-22T21:46:27Z

src/Microsoft.ML.Dnn/ImageClassificationTransform.cs

-            [Argument(ArgumentType.Multiple | ArgumentType.Required, HelpText = "The names of the model inputs", ShortName = "inputs", SortOrder = 1)]
-            public string[] InputColumns;
+            [Argument(ArgumentType.Multiple , HelpText = "The names of the model inputs", ShortName = "inputs", SortOrder = 1)]
+            internal string[] InputColumns;


internal string[] InputColumns; [](start = 11, length = 32)

probably don't need this or Output coliumns but I will remove this in my PR where I will refactor all this code and convert to ITrainerEstimator

codemzs · 2019-10-22T21:48:48Z

@eerhardt I need this change for my AutoML integration change and also the change where I convert this API from IEstimator to ITrainerEstimator, hence I will try to check-in as it mostly looks good and if there is any other feedback from you I will take it as part of my PR.

codecov · 2019-10-22T21:57:09Z

Codecov Report

Merging #4362 into master will increase coverage by <.01%.
The diff coverage is 93.29%.

@@            Coverage Diff             @@
##           master    #4362      +/-   ##
==========================================
+ Coverage   74.61%   74.62%   +<.01%     
==========================================
  Files         882      882              
  Lines      154848   154899      +51     
  Branches    16913    16914       +1     
==========================================
+ Hits       115539   115591      +52     
+ Misses      34563    34557       -6     
- Partials     4746     4751       +5

Flag	Coverage Δ
#Debug	`74.62% <93.29%> (ø)`	⬆️
#production	`70.19% <91.04%> (ø)`	⬆️
#test	`89.55% <94.84%> (ø)`	⬆️

Impacted Files	Coverage Δ
src/Microsoft.ML.Dnn/DnnRetrainTransform.cs	`57.26% <ø> (ø)`	⬆️
src/Microsoft.ML.Dnn/DnnCatalog.cs	`100% <100%> (+20.48%)`	⬆️
src/Microsoft.ML.Dnn/DnnUtils.cs	`69.4% <89.47%> (+3.31%)`	⬆️
...c/Microsoft.ML.Dnn/ImageClassificationTransform.cs	`88.54% <90.47%> (+0.72%)`	⬆️
...cenariosWithDirectInstantiation/TensorflowTests.cs	`90.52% <94.84%> (+0.04%)`	⬆️
...icrosoft.ML.AutoML/Experiment/SuggestedPipeline.cs	`88.17% <0%> (-4.31%)`	⬇️
....ML.AutoML/PipelineSuggesters/PipelineSuggester.cs	`79.83% <0%> (-4.21%)`	⬇️
...rosoft.ML.AutoML/ColumnInference/TextFileSample.cs	`59.6% <0%> (-2.65%)`	⬇️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs	`84.7% <0%> (-0.61%)`	⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	`86.1% <0%> (-0.16%)`	⬇️
... and 2 more

codemzs

eerhardt · 2019-10-23T16:37:53Z

    public enum DnnFramework

@codemzs - do we still need this public enum? I don't think it is used anymore.

Refers to: src/Microsoft.ML.Dnn/ImageClassificationTransform.cs:1115 in f956a07. [](commit_id = f956a07, deletion_comment = False)

eerhardt · 2019-10-23T16:45:48Z

src/Microsoft.ML.Dnn/DnnCatalog.cs

+        public static ImageClassificationEstimator ImageClassification(
+            this ModelOperationsCatalog catalog, Options options)
+        {
+            options.EarlyStoppingCriteria = options.DisableEarlyStopping ? null : options.EarlyStoppingCriteria ?? new EarlyStopping();


I still think this acts in a surprising behavior. What if we changed it to the following?

Remove options.DisableEarlyStopping.

Default options.EarlyStoppingCriteria = new EarlyStopping();

Then, by default the EarlyStopping is enabled to the default EarlyStopping. If someone wants to disable EarlyStopping, they set options.EarlyStoppingCriteria = null; when calling this API. If they want to change EarlyStopping to a different criteria, they can just set options.EarlyStoppingCriteria = new EarlyStopping(metric: EarlyStoppingMetric.Loss) (or whatever criteria they need).

This feels like a better API design choice because then the caller doesn't have 2 properties to deal with - DisableEarlyStopping and EarlyStoppingCriteria. There is only 1 to consider, that either they leave to the default, or set it as appropriate. If they want to disable EarlyStopping - they just set it to null.

eerhardt · 2019-10-23T16:46:46Z

if there is any other feedback from you I will take it as part of my PR.

I have left the above 2 pieces of feedback. Can you address them in your PR? Thanks.

…net#4362) * WIP initial change * Changed API design, changed tests and samples to use new API * Combined DnnCatalog.Options and ImageClassificationEstimator.Options, addressed review comments. * Added unit test and sample * Removed duplicate members in Options class, addresses PR comments * Removed preview remark for ImageClassification.

ashbhandare added 2 commits October 21, 2019 15:26

WIP initial change

7b035b8

Changed API design, changed tests and samples to use new API

280a878

ashbhandare requested a review from a team as a code owner October 22, 2019 00:06

eerhardt reviewed Oct 22, 2019

View reviewed changes

harishsk reviewed Oct 22, 2019

View reviewed changes

ashbhandare changed the title ~~WIP: Redesign DnnCatalog methods API for ease of use and consistency.~~ Redesign DnnCatalog methods API for ease of use and consistency. Oct 22, 2019

ashbhandare added 2 commits October 22, 2019 11:13

Combined DnnCatalog.Options and ImageClassificationEstimator.Options,…

b0dafb8

… addressed review comments.

Added unit test and sample

b17c016

eerhardt reviewed Oct 22, 2019

View reviewed changes

ashbhandare added 2 commits October 22, 2019 13:44

Removed duplicate members in Options class, addresses PR comments

2c0897d

Removed preview remark for ImageClassification.

f956a07

codemzs reviewed Oct 22, 2019

View reviewed changes

codemzs approved these changes Oct 22, 2019

View reviewed changes

codemzs merged commit 5f7527f into dotnet:master Oct 22, 2019

ashbhandare deleted the DnnCatalogAPI branch October 23, 2019 16:44

eerhardt reviewed Oct 23, 2019

View reviewed changes

eerhardt mentioned this pull request Oct 24, 2019

Make ImageClassification API an ITrainerEstimator and refactor code. #4372

Merged

yamachu mentioned this pull request Dec 4, 2019

[DNN Training] Cannot execute retrain DNN model(Tensorflow model) manually since ML.NET 1.4.0-stable #4520

Closed

najeeb-kazmi mentioned this pull request Dec 30, 2019

How to create NN model for NEAT in ML dot net ?? #4605

Closed

frank-dong-ms-zz mentioned this pull request Aug 25, 2020

how to retrain the image classify model incrementally? #5355

Open

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redesign DnnCatalog methods API for ease of use and consistency. #4362

Redesign DnnCatalog methods API for ease of use and consistency. #4362

ashbhandare commented Oct 22, 2019 •

edited

Loading

eerhardt commented Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019

ashbhandare Oct 22, 2019

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

ashbhandare Oct 22, 2019

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

harishsk Oct 22, 2019 •

edited by ashbhandare

Loading

ashbhandare Oct 22, 2019

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019

ashbhandare Oct 22, 2019

ashbhandare Oct 22, 2019

ashbhandare Oct 22, 2019

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

ashbhandare Oct 22, 2019

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

ashbhandare Oct 22, 2019

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

ashbhandare Oct 22, 2019

codemzs Oct 22, 2019

codemzs commented Oct 22, 2019

codecov bot commented Oct 22, 2019

codemzs left a comment

eerhardt commented Oct 23, 2019

eerhardt Oct 23, 2019

eerhardt commented Oct 23, 2019

Redesign DnnCatalog methods API for ease of use and consistency. #4362

Redesign DnnCatalog methods API for ease of use and consistency. #4362

Conversation

ashbhandare commented Oct 22, 2019 • edited Loading

eerhardt commented Oct 22, 2019 • edited by ashbhandare Loading

eerhardt Oct 22, 2019 • edited by ashbhandare Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Oct 22, 2019 • edited by ashbhandare Loading

Choose a reason for hiding this comment

eerhardt Oct 22, 2019 • edited by ashbhandare Loading

Choose a reason for hiding this comment

eerhardt Oct 22, 2019 • edited by ashbhandare Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Oct 22, 2019 • edited by ashbhandare Loading

Choose a reason for hiding this comment

eerhardt Oct 22, 2019 • edited by ashbhandare Loading

Choose a reason for hiding this comment

harishsk Oct 22, 2019 • edited by ashbhandare Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Oct 22, 2019 • edited by ashbhandare Loading

Choose a reason for hiding this comment

eerhardt Oct 22, 2019 • edited by ashbhandare 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

Choose a reason for hiding this comment

eerhardt Oct 22, 2019 • edited by ashbhandare Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Oct 22, 2019 • edited by ashbhandare Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Oct 22, 2019 • edited by ashbhandare Loading

Choose a reason for hiding this comment

eerhardt Oct 22, 2019 • edited by ashbhandare Loading

Choose a reason for hiding this comment

eerhardt Oct 22, 2019 • edited by ashbhandare Loading

Choose a reason for hiding this comment

eerhardt Oct 22, 2019 • edited by ashbhandare Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codemzs commented Oct 22, 2019

codecov bot commented Oct 22, 2019

Codecov Report

codemzs left a comment

Choose a reason for hiding this comment

eerhardt commented Oct 23, 2019

Choose a reason for hiding this comment

eerhardt commented Oct 23, 2019

ashbhandare commented Oct 22, 2019 •

edited

Loading

eerhardt commented Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

harishsk Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading

eerhardt Oct 22, 2019 •

edited by ashbhandare

Loading