-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Addresses #3976 about using PFI with a model loaded from disk #4262
Conversation
…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
loadedModel = mlContext.Model.Load(modelAndSchemaPath, out var schema); | ||
|
||
var transformedData = loadedModel.Transform(data); | ||
var linearPredictor = (loadedModel as TransformerChain<ITransformer>).LastTransformer as RegressionPredictionTransformer<LinearRegressionModelParameters>; |
There was a problem hiding this comment.
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>
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
…class. - Added static class to hold DirModel string
Codecov Report
@@ 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
|
...amples/Dynamic/Trainers/MulticlassClassification/PermutationFeatureImportanceLoadFromDisk.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…to myissue03reflection3
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 typeRegressionPredictionTransformer<IPredictorProducing<float>>.
This would have happened because the Create method that is called when loading aRegressionPredictionTransformer
would always return aRegressionPredictionTransformer<IPredictorProducing<float>>
regardless of the actualTModel
that should be loaded. In this case, it would be necessary to load the last transformer asRegressionPredictionTransformer<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 asTModel
, make a generic type at runtime for the prediction transformer using the actualTModel
, 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 foras MulticlassPredictionTransformer<MaximumEntropyModelParameters>
in one test. This is done because now the create method returns the appropriate TModel type instead of theIPredictorProducing<>
type. Notice that it is invalid to cast fromMulticlassPredictionTransformer<MaximumEntropyModelParameters>
toMulticlassPredictionTransformer<IPredictorProducing<VBuffer<float>>>
... so those tests fail without the changes I made to the tests. Also notice that sinceIPredictorProducing<>
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 typeBinaryPredictionTransformer<IPredictorProducing<float>>
; with the changes I made to the binary transformer, it now loads a last transformer of typeBinaryPredictionTransformer<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 thatParameterMixingCalibratorModelParameters
is internal, and its create method returns an object of its public superclassCalibratedModelParameterBase
. 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 itsTModel
for this specific case. Having changed theBinaryPredictionTransformer<TModel>
actually works as expected, in that it no longer returns a fixed<IPredictorProducing<float>>
as TModel, and so the problem is actually in theParameterMixingCalibratorModelParameters
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.