Skip to content

Public Constructors for ModelParameters #1866

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

Closed
najeeb-kazmi opened this issue Dec 12, 2018 · 16 comments
Closed

Public Constructors for ModelParameters #1866

najeeb-kazmi opened this issue Dec 12, 2018 · 16 comments
Labels
API Issues pertaining the friendly API

Comments

@najeeb-kazmi
Copy link
Member

During the course of fixing #1698 and deriving work items (e.g. #1699, #1701, #1702), we created public constructors for the various ModelParameter objects (formerly Predictor objects).

@sfilipi raised the question whether these should be public or be made internal as the only thing that can create ModelParameters (Predictors) is training a model. What use case are we supporting with this? When might a user want to create say a LinearRegressionModelParameters object with bias and weights instead of training a linear regression model?

@najeeb-kazmi najeeb-kazmi added the API Issues pertaining the friendly API label Dec 12, 2018
@Ivanidzo4ka
Copy link
Contributor

I think it make sense to some degree for me.
We have Feature contribution functionality which accept IPredictor and let you give feature importance. And without public constructors only way to create IPredictor is via training.
I personally would like ability to create IPredictor by hands which would give me ability to tweak it.

@sfilipi
Copy link
Member

sfilipi commented Dec 12, 2018

Curious, how would you create an IPredictor by hand? Where would you get the values for the model parameters?

@Ivanidzo4ka
Copy link
Contributor

With @najeeb-kazmi changes, like this:
new LinearModelParameters(env, "some_name_have_no_idea_why", in my_beatuiful_weights, yes_im_biased);

@Ivanidzo4ka
Copy link
Contributor

I can always explore predictor from training, and decide to manually boost certain weight or bias.

@Ivanidzo4ka
Copy link
Contributor

I believe in general this positive changes.
One thing I think we can improve is that name parameter we ask from user.
It's confusing and doesn't make any sense from user perspective.
We still can provide it, we just need to create separate internal constructor and pass nameof(class) in it (or whatever we pass right now).

@rogancarr
Copy link
Contributor

I think it would be super useful to be able to create some predictors by hand. I do this quite a bit to compare models across implementations and languages. This would make it way easier. This would be a more advanced usage, though.

An interesting use-case that I could see becoming popular would be to create importers of non-ML.NET models for ML.NET predictor classes. For example, if you had an in-house tree learner, you could write custom code to import your models as a TreeEnsembleModelParameters class for deployment. Similarly, we could allow NimbusML to pass compatible sklearn predictors to ML.NET for use with tools like Feature Contribution Calculator and Permutation Feature Importance.

@Zruty0
Copy link
Contributor

Zruty0 commented Dec 13, 2018

I can always explore predictor from training, and decide to manually boost certain weight or bias.

What @Ivanidzo4ka says is the primary reason I like public constructors. Being able to tweak the model and then plug it back into the system is a useful functionality.

@Zruty0
Copy link
Contributor

Zruty0 commented Dec 18, 2018

I suppose we can close it now

@Zruty0 Zruty0 closed this as completed Dec 18, 2018
@abgoswam
Copy link
Member

abgoswam commented Jan 25, 2019

re-opening this discussion.

  • if we do want to keep it public we should have some unit tests for it (currently there are no unit tests)

@TomFinley @glebuk @eerhardt

@rogancarr
Copy link
Contributor

Some scenarios where it's important to have public constructors:

Adjusting model parameters: Often there are situations where you may want to modify some parameters based on prior knowledge, as @Ivanidzo4ka and @Zruty0 mention above.

Importing models: Many people need to train models in custom systems, but need to deploy them in .NET. Imagine if you train a large-scale model in Spark or Hive and need to deploy it in .NET. Or your data scientists are using Python or R. You could hand-code some model, or you could simply plug in your linear model parameters, gradient-boosted-trees, or k-means parameters, etc. into ML.NET and use our framework for scoring.

@TomFinley
Copy link
Contributor

Can you write some code demonstrating how this then becomes used @rogancarr? Don't be afraid to be specific; links to actual code would be most informative.

If I take your statements as mere theoretical benefits, it is difficult to see how they're actually become practical. You can't just take one of these models and make a transformer out of it. The model parameters are something you get out of the transformer you get by training. The reason why we have the trainers return a transformer rather than just having the user get the predictor and make a transformer out of it, is because it is kind of tricky, and highly scenario dependent.

My impression is, that it's not useful right now, but with a hypothetical new surface area for creating transformers out of these things that does not exist, it might become useful. Is that so?

@rogancarr
Copy link
Contributor

Hi @TomFinley ,

Let me give you some examples where I go around the system and actually do this in ML.NET.

First let's talk about Use Case No. 2: I do the following when I bring in models trained in different systems:

public static ITransformer GetLinearModelScorerForFeature(MLContext mlContext)
{
    Action<ScaleMapInputRow, ScaleMapOutputRow> mapping = (input, output) =>
    {
        output.Score = LinearModel.Score(input.features);
    };
    // Make a custom transformer and transform the data.
    var transformer = mlContext.Transforms.CustomMappingTransformer(mapping, null);
    return transformer;
}

Here, I use a CustomMappingTransformer using a custom LinearModel class that I wrote. You could do the same for any model class you understood. Note that this totally works with our evaluators because I have a Score column. To use the actual model parameters classes in this solution, we would need IValueMapper to be public.

Now, when you do have proper model parameters, you can totally make transformers. I have code that does this too. In my case, I was instantiating models directly so that I could train with validation sets or do weird things to the training data outside of pipelines. When you do this, you receive the model parameters back (once called predictors) instead of a transformer. Here are some examples (v0.9 naming conventions):

// OLS (needed to to strange non-pipeline operations on the training set)
var linearTransformer = new RegressionPredictionTransformer<OlsLinearRegressionPredictor>(mlContext, linearPredictor, trainSet.Schema, featureName);

// Binary classification linear models  (needed to to strange non-pipeline operations on the training set)
if (returnLogit)
    linearTransformer = new RegressionPredictionTransformer<IPredictorProducing<float>>(mlContext, linearPredictor.SubPredictor, trainSet.Schema, featureName);
else
    linearTransformer = new BinaryPredictionTransformer<ParameterMixingCalibratedPredictor>(mlContext, linearPredictor, trainSet.Schema, featureName);

// Binary prediction GAM (needed validation)
gamTransformer = new BinaryPredictionTransformer<CalibratedPredictor>(mlContext, gamPredictor, trainSet.Schema, "Features");

// Regression GAM (needed validation)
gamTransformer = new RegressionPredictionTransformer<RegressionGamPredictor>(mlContext, gamPredictor, trainSet.Schema, "Features");

One of the beautiful things about ML.NET is how much expressivity you have. It allows you to be super creative in building solutions. This does result in rather hacky code sometimes (see above), but one thing that I worry about in closing down a lot of our APIs is losing the ability to do these creative workarounds.

@TomFinley
Copy link
Contributor

TomFinley commented Jan 25, 2019

I see. As you've observed, IValueMapper, have been made internal. They are not public -- they are meant as internal conveniences to allow for the easy creation of the ITranformer implementors, and that is what is public.

But as you've also observed, there are lots of things that are still public that should not be. E.g., these constructors for RegressionPredictionTransformer and the like. These APIs, though technically public, will not be public for v1. They are intended for component support only. (A semi-reliable but hardly perfect way to tell whether an API is meant for component support is whether it takes IHostEnvironment or IHost or IChannel as a parameter , since these are only component-level constructs, not user-level constructs.)

However, this work is not completed. To give an example, I have this PR currently opened. #2245. Ivan points out correctly here that the constructor should not be public, instead we should work via a mechanism of MLContext. Indeed, as we see in #2100, we explicitly have an item to remove such constructors. So in the short term, we are going to be more restrictive, not less, in this area. This code will not work beyond a certain point, until such a time as we design an acceptable public surface for it. (Post v1, to be clear.)

There are also more red flags, like the current-but-not-forever exposure of things like marker interfaces like IPredictorProducing<> and whatnot, which as marker interfaces are probably not going to make it as part of the public API.

Note that it is insufficient at this stage for something to merely be useful, since we currently have a problem where we have a limited amount of time before we must "freeze" the API. Anything that is part of that API at that time must stay forever. Therefore the focus in the immediate term will be on actually closing off the public surface as much as possible, since we literally do not have capacity to do anything else (if we even have capacity to do this). If you wish to help with that, that might be a good way to help us get more capacity.

@rogancarr
Copy link
Contributor

Hi @TomFinley. That makes a lot of sense. I suppose that the first workaround that I use — creating a custom transformer with a custom model — will be a fine workaround for the next little bit then.

@Ivanidzo4ka
Copy link
Contributor

So conclusion is following. We don't do it for v1.0, but will explore that option post release.
I'm moving this issue to backlog.

@rogancarr
Copy link
Contributor

@Ivanidzo4ka We will need a new issue for Project 13 to re-internalize these APIs

@codemzs codemzs closed this as completed Jun 30, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API Issues pertaining the friendly API
Projects
None yet
Development

No branches or pull requests

8 participants