[WIP] Transformers for AutoML #4157

michaelgsharp · 2019-08-29T23:14:40Z

This is still a work in progress, but will be updated and finished over the next several hours.

In order for AutoML to take a full dependency on ML.NET, several pieces of functionality need to be added. This PR is the first step in that process adding in 3 new transformers. The actual implementation of the transformers is done in C++. This code is currently in another repo, but will be turned in a Nuget package that ML.Net can take a binary dependency on. The code going into ML.Net is mostly wrapper code dealing with data interop and, since the binary is exposed via C api's with no overloading, code to call the correct methods based on the data type.

ToStringTransformer - Takes in a column and converts it into an appropriate string representation. It currently supports all integer types, float, double, bool, and string. This is fully done, and should be reviewed first.

CategoryImputer - Fill in missing values with the most common value in the column. It currently supports all integer types, float, double, bool, and string. 0's are treated as missing for integer types. This is done except for a small piece of the c++ interop.

DateTimeTransformer - Splits a column into its appropriate date time components (20 columns total are added). Takes a Long that represents seconds since 1970. The c++ interop will be added for this tonight and can be reviewed last.

Until we get the binary added as an official dependency, the constructors of the estimator will throw an error if the correct .dll is not found. This allows us to do a private drop for now. The tests will also be disabled currently for that reason.

eerhardt · 2019-08-30T00:08:43Z

src/Microsoft.ML.Transforms/CategoryImputer.cs

@@ -0,0 +1,1829 @@
+using System;


(nit) copyright on all code files.

copyright has been added. Thanks for the reminder.

eerhardt · 2019-08-30T00:17:45Z

src/Microsoft.ML.Transforms/CategoryImputer.cs

+            internal override void CreateTransformerFromSavedData(byte[] data)
+            {
+                bool result;
+                GCHandle handle = GCHandle.Alloc(data, GCHandleType.Pinned);


You can just use a fixed statement in C#. This is slightly faster than allocing and freeing a GCHandle. See the rest of our code (like in CpuMath) where we get a pointer for an array.

Here's an example, from one of our tests:

machinelearning/test/Microsoft.ML.CpuMath.PerformanceTests/NativePerformanceTests.cs

Line 27 in b861b5d

fixed (float* pdst = dst)

But you get the idea.

Changed all occurrences to fixed.

src/Microsoft.ML.Transforms/CategoryImputer.cs

eerhardt · 2019-08-30T17:55:41Z

src/Microsoft.ML.Transforms/CategoryImputer.cs

+            }
+        }
+
+        [DllImport("Featurizers", EntryPoint = "GetErrorInfoString", CallingConvention = CallingConvention.Cdecl), SuppressUnmanagedCodeSecurity]


These new transforms should be split out of the main Microsoft.ML nuget package. Since you are adding them to the main nuget package, EVERY application that uses ML.NET will have a dependency on these native assemblies, even if they don't use these new transforms.

See what we did with the transforms that depend on MKL - we split them into their own nuget package, so only people who needed those components would be forced to bring MKL along with their application.

https://www.nuget.org/packages/Microsoft.ML.Mkl.Components/

ganik · 2019-08-30T18:41:20Z

    [DllImport("Featurizers", EntryPoint = "GetErrorInfoString"), SuppressUnmanagedCodeSecurity]

NuGet dependency should be added to use this native assembly.

Refers to: src/Microsoft.ML.Transforms/ToStringTransformer.cs:313 in 260ff25. [](commit_id = 260ff25, deletion_comment = False)

ganik · 2019-08-30T18:42:05Z

    [DllImport("Featurizers", EntryPoint = "DestroyErrorInfoString"), SuppressUnmanagedCodeSecurity]

As I am aware the NuGet for this native lib is not ready for various Linux flavors and Mac. I dont think it can go into master branch until the NuGet has them

Refers to: src/Microsoft.ML.Transforms/ToStringTransformer.cs:316 in 260ff25. [](commit_id = 260ff25, deletion_comment = False)

ganik · 2019-08-30T18:43:41Z

public sealed class ToStringTransformerEstimator : IEstimator<ToStringTransformer>

This ToStringTransformer could be easily added as an add-on functionality to TypeConverting transform. And TypeConverting transform has SaveAsOnnx(..) functionality already

Refers to: src/Microsoft.ML.Transforms/ToStringTransformer.cs:73 in 260ff25. [](commit_id = 260ff25, deletion_comment = False)

eerhardt

Please split these new Transformers out into a separate nuget package. We cannot add new dependencies in the core Microsoft.ML NuGet package. Not every ML.NET application will need these new dependencies, so they shouldn't be in the core. New dependencies need to be opted in to.

ebarsoumMS

Replace Cpp with Native.
Why are you mixing DLLImport with the rest of the code?
Add comment to the public transformer, describing what it does.

ebarsoumMS · 2019-09-03T23:26:26Z

src/Microsoft.ML.Transforms/CategoryImputer.cs

+            }
+
+            public override void Dispose()
+            {


Why manually? Can we use SafeHandler?

I am using a SafeHandler for all handles themselves. This is for the main transformer class itself, and it just calls dispose on any SafeHandles it has.

ebarsoumMS · 2019-09-04T00:06:05Z

src/Microsoft.ML.Transforms/CategoryImputer.cs

+        }
+        #endregion
+
+        // Using this to confirm DLL exists. If does it will just return false since no parameters are being passed.


Why do we need that? Isn't the DLL is part of the package?

It will be a dependency we get through nuget, but until we get the nuget up and running this is in there just as a check. Once the nuget is ready we will remove this check.

ebarsoumMS · 2019-09-04T00:07:15Z

src/Microsoft.ML.Transforms/CategoryImputer.cs

+
+        internal delegate bool DestroyCppTransformerEstimator(IntPtr estimator, out IntPtr errorHandle);
+        internal delegate bool DestroyTransformerSaveData(IntPtr buffer, IntPtr bufferSize, out IntPtr errorHandle);
+        internal delegate bool DestroyTransformedDataCpp(IntPtr output, IntPtr outputSize, out IntPtr errorHandle);


Use Native instead of Cpp

Done, changed all occurrences from Cpp to Native.

eerhardt · 2019-09-10T23:02:05Z

pkg/Microsoft.ML.AutoMLFeaturizers/Microsoft.ML.Mkl.Redist.nupkgproj

+<Project Sdk="Microsoft.NET.Sdk" DefaultTargets="Pack">
+
+  <PropertyGroup>
+    <Authors>Intel</Authors>


You need to rename this file to be the name of the NuGet package.

Also, remove this Authors property.

I fixed the files. It is creating separate nuget packages now in the bin/packages folder. It is not creating a folder under packages in the root though. Should it be creating them there as well?

It's a little confusing, but $RepoRoot\packages are the NuGet packages that our code depends on - what are downloaded when we do a restore. $RepoRoot\bin\packages are the NuGet packages that our build produces.

Ok, that makes sense. I have the $RepoRoot\bin\packages creating the NuGet package correctly and tested it with a local import into another solution. So we should be good then. Thanks for the instructions!

eerhardt · 2019-09-11T14:30:22Z

src/Microsoft.ML.Transforms/Properties/Resources.Designer.cs

@@ -8,7 +8,7 @@
 // </auto-generated>
 //------------------------------------------------------------------------------

-namespace Microsoft.ML.Transforms.Properties
+namespace Microsoft.ML.AutoMLFeaturizers.Properties


I don't think this was an intended change, was it?

Nope, good catch. Reverting that in next push.

eerhardt · 2019-09-11T14:32:21Z

src/Microsoft.ML.AutoMLFeaturizers/Properties/AssemblyInfo.cs

+using System.Runtime.CompilerServices;
+using Microsoft.ML;
+
+[assembly: InternalsVisibleTo(assemblyName: "Microsoft.ML.Tests" + PublicKey.TestValue)]


What internals are the tests using? I think you should be able to test everything from the public API, right? #Resolved

I was testing the constructor that took in the Options class, but looking at the other Transformers and their tests they are going about it differently. Let me fix it and remove that VisibleTo tag. #Resolved

So I just double checked, and I was testing the entry point that AutoML uses and it takes in the Options class which is internal. Is it ok to leave everything as is? #Resolved

Yeah, all our tests have IVT, so this is fine to leave. I was mostly just curious why it was needed here. IMO it is best to test without Internals Visible To, but if you need it don't worry. #Resolved

eerhardt · 2019-09-11T14:32:51Z

src/Microsoft.ML.AutoMLFeaturizers/Microsoft.ML.AutoMLFeaturizers.csproj

+    <TargetFramework>netstandard2.0</TargetFramework>
+    <IncludeInPackage>Microsoft.ML.Featurizers</IncludeInPackage>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <DefineConstants>CORECLR</DefineConstants>


I don't think this DefineConstant is being used in the new assembly. It can be removed. #Resolved

Same comment as the prior one. This was removed last night, but is not showing up in the PR for some reason even though it is in my branch. Will rebase and check again. #Resolved

eerhardt · 2019-09-11T14:33:42Z

src/Microsoft.ML.AutoMLFeaturizers/Microsoft.ML.AutoMLFeaturizers.csproj

+
+  <PropertyGroup>
+    <TargetFramework>netstandard2.0</TargetFramework>
+    <IncludeInPackage>Microsoft.ML.Featurizers</IncludeInPackage>


IncludeInPackage needs to match the name of the NuGet package. Or else this assembly won't be included in the right NuGet package. #Resolved

I am not sure why it is showing up like that. Last night I pushed a fix that had that change to the correct name. It looks like somehow the branch in my repo and this PR aren't syncing up correctly. The number of commits this PR shows is not the same as the number of commits in my branch itself? I will rebase and see if that fixes the issue. #Resolved

eerhardt · 2019-09-11T14:37:27Z

pkg/Microsoft.ML.AutoMLFeaturizers/Microsoft.ML.AutoMLFeaturizers.nupkgproj

+  </ItemGroup>
+
+  <ItemGroup> 
+    <Content Include="..\common\CommonPackage.props" Pack="true" PackagePath="build\netstandard2.0\$(MSBuildProjectName).props" />


This line isn't necessary. This CommonPackage.props file probably should have a better name, but it is needed for NuGet packages that contain native assets inside of them. This one doesn't, so it is unnecessary. #Resolved

Sounds good. Removing in the next push. #Resolved

eerhardt · 2019-09-11T14:39:43Z

pkg/Microsoft.ML.AutoMLFeaturizers/Microsoft.ML.AutoMLFeaturizers.nupkgproj

@@ -0,0 +1,16 @@
+<Project Sdk="Microsoft.NET.Sdk" DefaultTargets="Pack">


I wonder if AutoMLFeaturizers is the best name for this package. Technically, anyone can use these featurizers without using AutoML.

What is going to be the name of the underlying NuGet package that contains the native assemblies? We should probably align with that name. So if that NuGet package is going to be named "AutoMLFeaturizers", then I guess that would be a good name here as well.

I will sync with my team today and figure that out. Part of the reason I picked this name is that these Transformers are added specifically for the AutoML team. So while anyone can use them, they are custom tailored for their use. #Resolved

How about naming it NativeFeaturizers? Hinting that implementation is in native code

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

eerhardt · 2019-09-11T14:40:46Z

src/Microsoft.ML.AutoMLFeaturizers/ToStringTransformer.cs

+        [DllImport("Featurizers", EntryPoint = "GetErrorInfoString"), SuppressUnmanagedCodeSecurity]
+        private static extern bool CheckIfDllExists(IntPtr error, out IntPtr errorHandleString, out IntPtr errorHandleStringSize);
+
+        public static ToStringTransformerEstimator Create(IHostEnvironment env, string outputColumnName, string inputColumnName)


These methods shouldn't be public. External users should only create the estimators from the Catalog APIs.

Changed them all to internal. Fixed in next push.

eerhardt · 2019-09-11T14:42:49Z

src/Microsoft.ML.AutoMLFeaturizers/ToStringTransformer.cs

+            }
+        }
+
+        internal class TransformedDataSafeHandler : SafeHandleZeroOrMinusOneIsInvalid


(minor) The .NET type is a SafeHandle, not SafeHandler. I think we should drop the r here and just be TransformedDataSafeHandle.

Removed the extra r from all the SafeHandle classes. Fixed in next push.

michaelgsharp · 2019-09-18T22:12:18Z

CodeFlow for GitHub does not download executable files. If you trust this file and would like to run it, please download it yourself through GitHub.

This is temporary and will be removed before this PR is merged into master. This is just so that we can build using the nuget package.

Refers to: temp-nuget-folder/MicrosoftMLFeaturizers.0.1.0.nupkg:1 in efdf22a. [](commit_id = efdf22a, deletion_comment = False)

eerhardt · 2019-09-19T15:04:23Z

pkg/Microsoft.ML.AutoMLFeaturizers/Microsoft.ML.AutoMLFeaturizers.nupkgproj

@@ -7,6 +7,8 @@

  <ItemGroup>
    <ProjectReference Include="../Microsoft.ML/Microsoft.ML.nupkgproj" />
+
+    <PackageReference Include="MicrosoftMLFeaturizers" Version="0.1.0" />


Will this be the final package name of the native featurizers? If yes, then I find it confusing that the ML.NET package has the word Auto in it, but the underlying native package doesn't. What about the ML.NET package makes it "AutoML", but the underlying native package isn't "AutoML"?

I will renaming the ML.NET package to be called NativeFeaturizers per Gani's suggestion. Or are you thinking since the native package is MicrosoftMLFeaturizers that I should name the ML.NET package the same thing?

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

Totally just my opinion, but I don't really like the word Native in the public API. I don't think it conveys any real information, and is an implementation detail.

It almost feels like the native assemblies, and the managed assemblies belong in the same NuGet package. But if we need to split them, I think their names should align in an understandable way.

Sounds good to me. I'll rename the ML.NET package than to just be Microsoft.ML.Featurizers.

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

codecov · 2019-09-30T19:46:47Z

Codecov Report

❗ No coverage uploaded for pull request base (master@0b20a99). Click here to learn what that means.
The diff coverage is 81.97%.

@@            Coverage Diff            @@
##             master    #4157   +/-   ##
=========================================
  Coverage          ?   74.69%           
=========================================
  Files             ?      888           
  Lines             ?   157644           
  Branches          ?    17193           
=========================================
  Hits              ?   117753           
  Misses            ?    35018           
  Partials          ?     4873

Flag	Coverage Δ
#Debug	`74.69% <81.97%> (?)`
#production	`70.24% <74.97%> (?)`
#test	`89.81% <100%> (?)`

Impacted Files	Coverage Δ
src/Microsoft.ML.Featurizers/CategoryImputer.cs	`69.31% <ø> (ø)`
...irectInstantiation/IrisPlantClassificationTests.cs	`100% <ø> (ø)`
...ft.ML.Tests/Transformers/TimeSeriesImputerTests.cs	`100% <100%> (ø)`
....ML.Tests/Transformers/ToStringTransformerTests.cs	`100% <100%> (ø)`
...crosoft.ML.Core.Tests/UnitTests/TestEntryPoints.cs	`98.71% <100%> (ø)`
....ML.Tests/Transformers/DateTimeTransformerTests.cs	`100% <100%> (ø)`
...soft.ML.Tests/Transformers/CategoryImputerTests.cs	`100% <100%> (ø)`
src/Microsoft.ML.Featurizers/Common.cs	`17.3% <17.3%> (ø)`
...rc/Microsoft.ML.Featurizers/ToStringTransformer.cs	`70.53% <70.53%> (ø)`
...rosoft.ML.Featurizers/TimeSeriesImputerDataView.cs	`71.88% <71.88%> (ø)`
... and 2 more

eerhardt · 2019-10-24T14:27:42Z

CategoryImputer

Why are we creating a whole new transform here, when ML.NET already has ReplaceMissingValues ?

I think this feature should just be an option to the existing ReplaceMissingValues transform. Today ML.NET supports:

machinelearning/src/Microsoft.ML.Transforms/MissingValueReplacing.cs

Lines 913 to 934 in 6d192b6

    
           /// <summary> 
        
           /// The possible ways to replace missing values. 
        
           /// </summary> 
        
           public enum ReplacementMode : byte 
        
           { 
        
               /// <summary> 
        
               /// Replace with the default value of the column based on its type. 
        
               /// </summary> 
        
               DefaultValue = 0, 
        
               /// <summary> 
        
               /// Replace with the mean value of the column. 
        
               /// </summary> 
        
               Mean = 1, 
        
               /// <summary> 
        
               /// Replace with the minimum value of the column. 
        
               /// </summary> 
        
               Minimum = 2, 
        
               /// <summary> 
        
               /// Replace with the maximum value of the column. 
        
               /// </summary> 
        
               Maximum = 3, 
        
           }

We should just be adding a new value to that enum called MostCommon (or similar).

/cc @codemzs @ebarsoumMS

ToStringTransformer is done. CatagoryImputer is done. TimeSeriesImputer is done. RobustScaler is done. Adding in samples and documentation. General code cleanup. Made the RowToRowMapperTransform create a new mapper if possible for each cursor.

michaelgsharp · 2019-12-18T22:01:33Z

Closing this as we are merging in them one featurizer at a time instead of all together.

michaelgsharp requested review from codemzs, harishsk, eerhardt, ganik, anujshrotriya and wangyems August 29, 2019 23:14

michaelgsharp requested a review from a team as a code owner August 29, 2019 23:14

ganik changed the title ~~Transformers for AutoML~~ [WIP] Transformers for AutoML Aug 29, 2019

eerhardt reviewed Aug 30, 2019

View reviewed changes

harishsk reviewed Aug 30, 2019

View reviewed changes

src/Microsoft.ML.Transforms/CategoryImputer.cs Outdated Show resolved Hide resolved

eerhardt reviewed Aug 30, 2019

View reviewed changes

eerhardt requested changes Aug 30, 2019

View reviewed changes

ebarsoumMS suggested changes Sep 4, 2019

View reviewed changes

eerhardt reviewed Sep 10, 2019

View reviewed changes

eerhardt reviewed Sep 11, 2019

View reviewed changes

michaelgsharp force-pushed the AutoMLTransformers branch 3 times, most recently from 4d6434f to e657947 Compare September 17, 2019 22:05

eerhardt reviewed Sep 19, 2019

View reviewed changes

michaelgsharp force-pushed the AutoMLTransformers branch from 4b44abf to eabf20e Compare September 30, 2019 18:54

michaelgsharp force-pushed the AutoMLTransformers branch from eabf20e to 3258b6d Compare October 7, 2019 22:09

DateTimeTransformer is done.

93a994f

ToStringTransformer is done. CatagoryImputer is done. TimeSeriesImputer is done. RobustScaler is done. Adding in samples and documentation. General code cleanup. Made the RowToRowMapperTransform create a new mapper if possible for each cursor.

michaelgsharp force-pushed the AutoMLTransformers branch from f4b4dd9 to 93a994f Compare October 28, 2019 21:39

michaelgsharp mentioned this pull request Oct 29, 2019

Initial featurizers project #4413

Merged

michaelgsharp closed this Dec 18, 2019

michaelgsharp deleted the AutoMLTransformers branch February 24, 2020 21:03

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

		@@ -0,0 +1,16 @@
		<Project Sdk="Microsoft.NET.Sdk" DefaultTargets="Pack">

[WIP] Transformers for AutoML #4157

[WIP] Transformers for AutoML #4157

Conversation

michaelgsharp commented Aug 29, 2019

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

ganik commented Aug 30, 2019

ganik commented Aug 30, 2019

ganik commented Aug 30, 2019 • edited Loading

eerhardt left a comment

Choose a reason for hiding this comment

ebarsoumMS left a 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

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

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 Sep 11, 2019 • edited by michaelgsharp Loading

Choose a reason for hiding this comment

michaelgsharp Sep 11, 2019 • edited Loading

Choose a reason for hiding this comment

michaelgsharp Sep 11, 2019 • edited Loading

Choose a reason for hiding this comment

eerhardt Sep 11, 2019 • edited by michaelgsharp Loading

Choose a reason for hiding this comment

eerhardt Sep 11, 2019 • edited by michaelgsharp Loading

Choose a reason for hiding this comment

michaelgsharp Sep 11, 2019 • edited Loading

Choose a reason for hiding this comment

eerhardt Sep 11, 2019 • edited by michaelgsharp Loading

Choose a reason for hiding this comment

michaelgsharp Sep 11, 2019 • edited Loading

Choose a reason for hiding this comment

eerhardt Sep 11, 2019 • edited by michaelgsharp Loading

Choose a reason for hiding this comment

michaelgsharp Sep 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelgsharp Sep 11, 2019 • edited 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

Choose a reason for hiding this comment

michaelgsharp commented Sep 18, 2019

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

codecov bot commented Sep 30, 2019 • edited Loading

Codecov Report

eerhardt commented Oct 24, 2019

michaelgsharp commented Dec 18, 2019

ganik commented Aug 30, 2019 •

edited

Loading

eerhardt Sep 11, 2019 •

edited by michaelgsharp

Loading

michaelgsharp Sep 11, 2019 •

edited

Loading

michaelgsharp Sep 11, 2019 •

edited

Loading

eerhardt Sep 11, 2019 •

edited by michaelgsharp

Loading

eerhardt Sep 11, 2019 •

edited by michaelgsharp

Loading

michaelgsharp Sep 11, 2019 •

edited

Loading

eerhardt Sep 11, 2019 •

edited by michaelgsharp

Loading

michaelgsharp Sep 11, 2019 •

edited

Loading

eerhardt Sep 11, 2019 •

edited by michaelgsharp

Loading

michaelgsharp Sep 11, 2019 •

edited

Loading

michaelgsharp Sep 11, 2019 •

edited

Loading

codecov bot commented Sep 30, 2019 •

edited

Loading