Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addresses #3976 about using PFI with a model loaded from disk #4262

Merged
merged 15 commits into from
Oct 2, 2019

Conversation

antoniovs1029
Copy link
Member

With this pull request it is now possible to use PFI with some models loaded from disk. This is not yet a final solution to the problem, as described in the last section below.

The Problem

As explained in this comment of issue #3976 it was not possible to use PFI with a model loaded from disk, because the last transformer of the loaded model was not of the appropriate type to use it with the PermutationFeatureImportance method.

Specifically the problem occurred because when loading the last transformer, a 'create' method would be called and it would assign an inappropriate type to the last transformer, making it unusable for PFI. For example, if a model had RegressionPredictionTransformer<OlsModelParameters> as last transformer, and it was saved to disk, later on when loading it from disk the last transformer would be of type RegressionPredictionTransformer<IPredictorProducing<float>>. This would have happened because the Create method that is called when loading a RegressionPredictionTransformer would always return a RegressionPredictionTransformer<IPredictorProducing<float>> regardless of the actual TModel that should be loaded. In this case, it would be necessary to load the last transformer as RegressionPredictionTransformer<OlsModelParameters> in order to use it with PFI.

This was a problem also in the BinaryClassification, MulticlassClassification and Ranking prediction transformers which implemented a similar Create method. All of these classes are used with PFI.

The approach of the solution

The main obstacle was that the appropriate type TModel to be used when loading the last transformer couldn't be known at compile time, only at runtime once the last transformer was being loaded.

So to solve the problem, it was necessary to load first the internal model (e.g. the OlsModelParameters object) in the Create method of the prediction transformer, get its type to be used as TModel, make a generic type at runtime for the prediction transformer using the actual TModel, and instantiate that type with a constructor that would receive the internal model (previously loaded) to add it to the last transformer; this constructor would then continue to load the prediction transformer.

Changes implemented

  • The create method of the prediction transformers (binary, multiclass, ranking and regression) was modified to solve the problem. Different overloads of constructors were created to receive the internal model once it was loaded and then continue loading the prediction transformer.

  • Samples were added based on the original PFI samples, but now using a model that is saved and loaded from disk directly in the sample. This is provided for the multiclass, ranking and regression, but NOT for the binary classification case (see the final section below).

  • Test cases based on the original PFI test cases are also provided, but now using a model that is saved and loaded from disk directly in the test case. Again, this was done for multiclass, ranking and regression, but not for binary classification.

  • Changes were made to 2 tests in the LbfgsTests.cs file where PFI is not used, but prediction transformers are loaded from disk. The changes regard casts that were used in the tests, but can no longer be used. For example, as MulticlassPredictionTransformer<IPredictorProducing<VBuffer<float>>> was replaced for as MulticlassPredictionTransformer<MaximumEntropyModelParameters> in one test. This is done because now the create method returns the appropriate TModel type instead of the IPredictorProducing<> type. Notice that it is invalid to cast from MulticlassPredictionTransformer<MaximumEntropyModelParameters> to MulticlassPredictionTransformer<IPredictorProducing<VBuffer<float>>>... so those tests fail without the changes I made to the tests. Also notice that since IPredictorProducing<> is internal, a regular user using a nugget wouldn't be able to do the casts that were done in those tests.

Further problems

In this pull request I provide working samples and tests for regression, multiclass and ranking. Still, I've been unable to provide them for binary classification.

The existing sample for PFI with binary classification uses a last transformer of type BinaryPredictionTransformer<CalibratedModelParameterBase<LinearBinaryModelParameters,PlattCalibrator>> which is the type necessary to use it in the PFI method. When saving and then loading that model from disk, the last transformer was of type BinaryPredictionTransformer<IPredictorProducing<float>>; with the changes I made to the binary transformer, it now loads a last transformer of type BinaryPredictionTransformer<ParameterMixingCalibratorModelParameters<IPredictorProducing<float>, ICalibrator>>, which is still not the type necessary to use PFI.

The problem is that the Create method of ParameterMixingCalibratorModelParameters returns a CalibratedModelParameterBase object. This is similar to the problem that the prediction transformers had, but there's a key difference in that ParameterMixingCalibratorModelParameters is internal, and its create method returns an object of its public superclass CalibratedModelParameterBase. This key difference has stopped me from solving the problem using the same approach used to fix the prediction transformers. Once this pull request is accepted, I will open another issue with this specific use case, explaining it in more detail.

Nonetheless, notice that the problem is not in the BinaryPredictionTransformer<TModel> but rather in its TModel for this specific case. Having changed the BinaryPredictionTransformer<TModel> actually works as expected, in that it no longer returns a fixed <IPredictorProducing<float>> as TModel, and so the problem is actually in the ParameterMixingCalibratorModelParameters class.

Also notice that this is a signal that there might be other generic classes where the Create method returns an object with fixed type parameters that aren't the ones actually being used. This might become a problem for users trying to use PFI with a model that uses one of such classes.

…ression, Ranking and Multiclass Classification. Still working in Binary Classification.
…s so that it uses the appropiate casts now that the PredictionTransformers have been updated.
…del, since problems arise because of the ParameterMixingCalibratedModelParameters
@antoniovs1029 antoniovs1029 requested a review from a team as a code owner September 28, 2019 01:36
loadedModel = mlContext.Model.Load(modelAndSchemaPath, out var schema);

var transformedData = loadedModel.Transform(data);
var linearPredictor = (loadedModel as TransformerChain<ITransformer>).LastTransformer as RegressionPredictionTransformer<LinearRegressionModelParameters>;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RegressionPredictionTransformer [](start = 101, length = 64)

Wouldn't this work if we write RegressionPredictionTransformer<object>?

Copy link
Member Author

@antoniovs1029 antoniovs1029 Sep 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't work since the cast to RegressionPredictionTransformer<object> returns null, and having a null linearPredictor makes it unusable to use the PFI method, and the test fails with a NullReferenceException.

I've just tested this, and this happens both with or without the changes I've made to the Prediction Transformers in this pull request.

Notice that originally the .LastTransformer would return a RegressionPredictionTransformer<IPredictorProducing<float>> object, but with the changes in this pull request it now returns a RegressionPredictionTransformer<LinearRegressionModelParameters> object. So, as I've stated, in both cases a cast to RegressionPredictionTransformer<object> returns null.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. Would it work with ISingleFeaturePredictionTransformer<object>? There is a test in the functional tests project that loads a model and extracts the predictor:
https://github.com/dotnet/machinelearning/blob/master/test/Microsoft.ML.Functional.Tests/ModelFiles.cs#L193

Would something similar work here?


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

Copy link
Member Author

@antoniovs1029 antoniovs1029 Oct 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a cast to ISingleFeaturePredictionTransformer<object> works in this test.

I've tested this with the changes I've made in this pull request, and without them. And it works in both cases, and the test passes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, should I change the cast in here to ISingleFeaturePredictionTransformer<object> for some reason?

Or are there other casts you want me to try out?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it works without the change, then why do we need this change at all?

Is it because we don't expect the user to know which version of PermutationFeatureImportance to call (regression/classification)?


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

Copy link
Member

@eerhardt eerhardt Oct 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it works without the change, then why do we need this change at all?

I don't believe users should be using ISingleFeaturePredictionTransformer<object> in a mainline scenario, like using PFI. They should be able to use the exact same type as what they got back when they created the model in the first place.

TransformerChain<RegressionPredictionTransformer<OlsModelParameters>> model = pipeline.Fit(data);

When I save and then load the above model, I should get back a TransformerChain<RegressionPredictionTransformer<OlsModelParameters>> object. But with the current code, the loaded object is a TransformerChain<RegressionPredictionTransformer<IPredictorProducing<float>>>

The root issue here is that the type of object that we get back when loading a model from disk is not the same as the type of object that is created in the first place. Thus, certain casts that work when you haven't saved the model to disk will work, and then fail for the same model after it is saved and then loaded.

Copy link
Member Author

@antoniovs1029 antoniovs1029 Oct 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it works without the change, then why do we need this change at all?

I hadn't tried to cast to ISingleFeaturePredictionTransformer<object> until you've mentioned it in here, so I didn't know PFI would work that way, even without my changes. So now I am unsure if the changes I've done in this pull request are actually necessary.

There is still a problem that isn't present in the test cases, but that arises in the sample case of PFI with Regression. Specifically what happens is that the code tries to acces linearPredictor.Model.Weights[i] and if linearPredictor was created using the cast you are proposing:
var linearPredictor = (model as TransformerChain<ITransformer>).LastTransformer as ISingleFeaturePredictionTransformer<object>

then linearPredictor doesn't have access to the Model.Weights member, and the compiler throws an error.

If I change the cast to ISingleFeaturePredictionTransformer<OlsModelParameters> then the compiler doesn't complain, and it works with the changes I've made in this pull request... but it doesn't work without them (the cast returns a null reference).

So, in short, it seems to me that if the user casts the last transformer to ISingleFeaturePredictionTransformer<object> then s/he will be able to use PFI even without my changes, but s/he might not be able to access the members of the parameters.

I don't know if there's a workaround I am not seeing for this. But I will keep on trying other casts and see if I find one that works.

Copy link
Member Author

@antoniovs1029 antoniovs1029 Oct 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe users should be using ISingleFeaturePredictionTransformer<object> in a mainline scenario, like using PFI. They should be able to use the exact same type as what they got back when they created the model in the first place.

So I understand that the way to go is to keep my changes and not expect the user to use the ISingleFeaturePredictionTransformer<object> cast, right?

Should I also add a test where loading a model from disk and casting it to ISingleFeaturePredictionTransformer<object> doesn't work? Such as the situation I've described in my last comment in here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree on "They should be able to use the exact same type as what they got back when they created the model in the first place.". I think we should be expecting users to cast it to the same type they saved it not some generic type with object ...


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

@codecov
Copy link

codecov bot commented Oct 1, 2019

Codecov Report

Merging #4262 into master will increase coverage by 0.01%.
The diff coverage is 95.74%.

@@            Coverage Diff             @@
##           master    #4262      +/-   ##
==========================================
+ Coverage   74.48%   74.49%   +0.01%     
==========================================
  Files         877      877              
  Lines      153663   153831     +168     
  Branches    16828    16838      +10     
==========================================
+ Hits       114457   114600     +143     
- Misses      34474    34498      +24     
- Partials     4732     4733       +1
Flag Coverage Δ
#Debug 74.49% <95.74%> (+0.01%) ⬆️
#production 70.08% <91.48%> (ø) ⬆️
#test 89.5% <100%> (+0.01%) ⬆️
Impacted Files Coverage Δ
...soft.ML.Tests/PermutationFeatureImportanceTests.cs 100% <100%> (ø) ⬆️
...st/Microsoft.ML.Functional.Tests/Explainability.cs 100% <100%> (ø) ⬆️
...Microsoft.ML.Tests/TrainerEstimators/LbfgsTests.cs 98.02% <100%> (ø) ⬆️
...Microsoft.ML.Data/Scorers/PredictionTransformer.cs 92.13% <91.48%> (-4.9%) ⬇️
...c/Microsoft.ML.FastTree/Utils/ThreadTaskManager.cs 79.48% <0%> (-20.52%) ⬇️
...soft.ML.TestFramework/DataPipe/TestDataPipeBase.cs 73.6% <0%> (-0.33%) ⬇️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs 84.7% <0%> (-0.21%) ⬇️
src/Microsoft.ML.TensorFlow/TensorflowCatalog.cs 100% <0%> (ø) ⬆️
src/Microsoft.ML.Dnn/DnnCatalog.cs 78.37% <0%> (ø) ⬆️
... and 2 more

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

Copy link
Member

@codemzs codemzs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants