Added the assembly name of the custom transform to the model file #4989

harishsk · 2020-03-31T21:50:03Z

Fixes #4965
Right now when a custom transformer is used, the model file retains the name of the transformer as a string. This is not enough information to re-instantiate the custom transformer when the model is loaded from file. If the assembly containing the custom transformer is not already registered with the component catalog, then the model will fail to load.
To fix this, I have incremented the model version and am now saving the assembly name of the transform with the model.

antoniovs1029 · 2020-04-01T08:06:39Z

test/Microsoft.ML.Core.Tests/UnitTests/TestCustomTypeRegister.cs

@@ -192,7 +196,7 @@ public void RegisterTypeWithAttribute()
            Assert.Equal(tribeEnumerable[0].Merged.HandCount, tribe[0].One.HandCount + tribe[0].Two.HandCount);

            // Build prediction engine from the trained pipeline.
-            var engine = ML.Model.CreatePredictionEngine<AlienHero, SuperAlienHero>(model);
+            var engine = ML.Model.CreatePredictionEngine<AlienHero, SuperAlienHero>(modelSaved);


It might be good to have this test both for the model before saving it, and for the model that is loaded from disk. Ideally both models should behave the same, but if a bug is introduced that only appears in one of both cases, it might be good to have both of the tests.

The InlineDataAttribute could become handy for this to avoid code duplication, as used by the PermutationFeatureImportanceTests:

machinelearning/test/Microsoft.ML.Tests/PermutationFeatureImportanceTests.cs

Lines 30 to 51 in cacc72f

[Theory]

[InlineData(true)]

[InlineData(false)]

public void TestPfiRegressionOnDenseFeatures(bool saveModel)

{

var data = GetDenseDataset();

var model = ML.Regression.Trainers.OnlineGradientDescent().Fit(data);

ImmutableArray<RegressionMetricsStatistics> pfi;

if(saveModel)

{

var modelAndSchemaPath = GetOutputPath("TestPfiRegressionOnDenseFeatures.zip");

ML.Model.Save(model, data.Schema, modelAndSchemaPath);

var loadedModel = ML.Model.Load(modelAndSchemaPath, out var schema);

var castedModel = loadedModel as RegressionPredictionTransformer<LinearRegressionModelParameters>;

pfi = ML.Regression.PermutationFeatureImportance(castedModel, data);

}

else

{

pfi = ML.Regression.PermutationFeatureImportance(model, data);

}

#Resolved

Fixed.

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

antoniovs1029 · 2020-04-01T08:10:20Z

test/Microsoft.ML.Tests/Transformers/CustomMappingTests.cs

-                if (!ex.IsMarked())
-                    throw;
-            }
-            ML.ComponentCatalog.RegisterAssembly(typeof(MyLambda).Assembly);


I wonder, what happens (and what should happen) if the user actually registers the assembly as done here and then tries to load the model? Will it throw an exception, or will it work anyway with the fix on this PR? #Resolved

It will work. The assembly can be registered multiple times. #Resolved

Great.

If anything, I would recommend using the InlineData trick I mentioned in another comment to test both cases: when the user registers the assembly manually (even if it's not necessary) and when they don't register it. #Resolved

Done in the next iteration.

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

antoniovs1029 · 2020-04-01T08:11:52Z

src/Microsoft.ML.Transforms/CustomMappingTransformer.cs

@@ -58,6 +60,7 @@ public sealed class CustomMappingTransformer<TSrc, TDst> : ITransformer
               : InternalSchemaDefinition.Create(typeof(TDst), outputSchemaDefinition);

            _contractName = contractName;
+            _contractAssembly = _mapAction.Method.DeclaringType.Assembly.FullName;


Is there any case where the loaded model would actually require having a different name registered from the "FullName" retrieved from here? #Resolved

Or any case where trying to access that member of _mapAction would throw? #Resolved

The call to Method can throw a MemberAccessException. But that would be up to the caller to fix in their code and the exception would help with that.

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

I think we should enforce the idea that the same transform that is used in training should be used in prediction as well. If they were to be different, then they are not the same pipelines and not the same models.

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

antoniovs1029 · 2020-04-01T08:27:19Z

In general, it LGTM, I just left some comments and also it would be good to update the docs and samples in here to say that it's no longer necessary for the user to register the assembly when loading back the model:

https://docs.microsoft.com/en-us/dotnet/api/microsoft.ml.custommappingcatalog.custommapping?view=ml-dotnet

antoniovs1029 · 2020-04-01T08:33:07Z

src/Microsoft.ML.Transforms/LambdaTransform.cs

@@ -3,7 +3,9 @@
 // See the LICENSE file in the project root for more information.

 using System;
+using System.Diagnostics.Contracts;


Just curious, why do we need System.Diagnostics.Contracts in here? #Resolved

Fixed.

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

antoniovs1029

LGTM It only needs to update the docs and the samples to show that it is no longer necessary to register the assembly

harishsk requested a review from a team as a code owner March 31, 2020 21:50

harishsk requested review from luisquintanilla, LittleLittleCloud, JakeRadMSFT, frank-dong-ms-zz and antoniovs1029 March 31, 2020 21:54

harishsk and others added 4 commits March 31, 2020 18:06

Added the assembly name of the custom transform to the model file

2a2b655

Backed out unnecessary change

87eb370

Fixed failing test

c77a09e

Disabled two tensorflow tests that are hanging

47966e7

antoniovs1029 reviewed Apr 1, 2020

View reviewed changes

antoniovs1029 mentioned this pull request Apr 1, 2020

Add support for stateful custom mappers, and for custom filters. #4676

Merged

luisquintanilla approved these changes Apr 1, 2020

View reviewed changes

harishsk requested a review from yaeldekel April 1, 2020 17:18

harishsk added 2 commits April 1, 2020 11:25

Addressed code review comments and added another test for backcompat

db3dcb7

Removed unnecessary using statements

2fd1d40

antoniovs1029 self-requested a review April 1, 2020 20:57

antoniovs1029 approved these changes Apr 1, 2020

View reviewed changes

Updated docs

019d7cf

harishsk merged commit 4b3cc7e into dotnet:master Apr 1, 2020

antoniovs1029 mentioned this pull request Sep 28, 2020

CustomMapping causes problem when used inside an Autodesk plugin #5405

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added the assembly name of the custom transform to the model file #4989

Added the assembly name of the custom transform to the model file #4989

harishsk commented Mar 31, 2020

antoniovs1029 Apr 1, 2020 •

edited by harishsk

Loading

harishsk Apr 1, 2020

antoniovs1029 Apr 1, 2020 •

edited by harishsk

Loading

harishsk Apr 1, 2020 •

edited

Loading

antoniovs1029 Apr 1, 2020 •

edited by harishsk

Loading

harishsk Apr 1, 2020

antoniovs1029 Apr 1, 2020 •

edited by harishsk

Loading

antoniovs1029 Apr 1, 2020 •

edited by harishsk

Loading

harishsk Apr 1, 2020

harishsk Apr 1, 2020

antoniovs1029 commented Apr 1, 2020 •

edited

Loading

antoniovs1029 Apr 1, 2020 •

edited by harishsk

Loading

harishsk Apr 1, 2020

antoniovs1029 left a comment

	[Theory]
	[InlineData(true)]
	[InlineData(false)]
	public void TestPfiRegressionOnDenseFeatures(bool saveModel)
	{
	var data = GetDenseDataset();
	var model = ML.Regression.Trainers.OnlineGradientDescent().Fit(data);

	ImmutableArray<RegressionMetricsStatistics> pfi;
	if(saveModel)
	{
	var modelAndSchemaPath = GetOutputPath("TestPfiRegressionOnDenseFeatures.zip");
	ML.Model.Save(model, data.Schema, modelAndSchemaPath);

	var loadedModel = ML.Model.Load(modelAndSchemaPath, out var schema);
	var castedModel = loadedModel as RegressionPredictionTransformer<LinearRegressionModelParameters>;
	pfi = ML.Regression.PermutationFeatureImportance(castedModel, data);
	}
	else
	{
	pfi = ML.Regression.PermutationFeatureImportance(model, data);
	}

Added the assembly name of the custom transform to the model file #4989

Added the assembly name of the custom transform to the model file #4989

Conversation

harishsk commented Mar 31, 2020

antoniovs1029 Apr 1, 2020 • edited by harishsk Loading

Choose a reason for hiding this comment

harishsk Apr 1, 2020

Choose a reason for hiding this comment

antoniovs1029 Apr 1, 2020 • edited by harishsk Loading

Choose a reason for hiding this comment

harishsk Apr 1, 2020 • edited Loading

Choose a reason for hiding this comment

antoniovs1029 Apr 1, 2020 • edited by harishsk Loading

Choose a reason for hiding this comment

harishsk Apr 1, 2020

Choose a reason for hiding this comment

antoniovs1029 Apr 1, 2020 • edited by harishsk Loading

Choose a reason for hiding this comment

antoniovs1029 Apr 1, 2020 • edited by harishsk Loading

Choose a reason for hiding this comment

harishsk Apr 1, 2020

Choose a reason for hiding this comment

harishsk Apr 1, 2020

Choose a reason for hiding this comment

antoniovs1029 commented Apr 1, 2020 • edited Loading

antoniovs1029 Apr 1, 2020 • edited by harishsk Loading

Choose a reason for hiding this comment

harishsk Apr 1, 2020

Choose a reason for hiding this comment

antoniovs1029 left a comment

Choose a reason for hiding this comment

antoniovs1029 Apr 1, 2020 •

edited by harishsk

Loading

antoniovs1029 Apr 1, 2020 •

edited by harishsk

Loading

harishsk Apr 1, 2020 •

edited

Loading

antoniovs1029 Apr 1, 2020 •

edited by harishsk

Loading

antoniovs1029 Apr 1, 2020 •

edited by harishsk

Loading

antoniovs1029 Apr 1, 2020 •

edited by harishsk

Loading

antoniovs1029 commented Apr 1, 2020 •

edited

Loading

antoniovs1029 Apr 1, 2020 •

edited by harishsk

Loading