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

The trainer API for MLContext is inconsistent for learner in external nugets #1827

Closed
singlis opened this issue Dec 5, 2018 · 5 comments
Closed
Labels
API Issues pertaining the friendly API question Further information is requested

Comments

@singlis
Copy link
Member

singlis commented Dec 5, 2018

APIs are discoverable via the MLContext. There are some learners that are declared on the MLContext where other learners define an MLContext extension in cases where the learner is in a separate nuget package.

This results inconsistent API calls when accessing APIs, for example BinaryClassification vs Recommendation():

var foo_bar = mlContext.BinaryClassification.Trainers;
var foo_moo_bar = mlContext.Recommendation().Trainers;

The API discover ability should be consistent and work across nuget pakages.
Original issue: #1806

This issue may be the solution #1319 as it creates extensions for the trainers.

@singlis singlis added the API Issues pertaining the friendly API label Dec 5, 2018
@Zruty0
Copy link
Contributor

Zruty0 commented Dec 5, 2018

This inconsistency was expected by design from the first day: we planned to have properties for 'standard' tasks (available in Microsoft.ML NuGet) and methods for tasks that are added in separate NuGets (time series, recommendation, etc.)

If you have any suggestion on how to make them consistent, please make it. The only way I see that we can reconcile the calls is to make all existing properties into methods:

var foo1 = mlContext.BinaryClassification().Trainers().LogisticRegression();
var foo2 = mlContext.Transforms().Categorical().OneHotEncoding();

etc.

I do not like this solution: a natural way to represent catalogs is by making them properties. I think sacrificing this natural design in order to make the calls to common and less-common parts of the API more similar is not a good idea.

@Zruty0
Copy link
Contributor

Zruty0 commented Dec 18, 2018

@singlis or anyone else: if we do not have a good suggestion for an alternative, I propose leaving the code as is.

@Zruty0 Zruty0 added the question Further information is requested label Dec 18, 2018
@glebuk
Copy link
Contributor

glebuk commented Jan 8, 2019

@singlis please review Pete's feedback. If agree with him, close.

@CESARDELATORRE
Copy link
Contributor

I think it should be consistent so if some of the learners have to be methods because they are extension methods, then let's make all of them methods.

From a user's point of view looks strange why some of them are methods and some of them are properties. We are propagating the way it is internally implemented to the way the API is designed (property vs. method) ...

Also, if something is a method, it should have a verb as part of the name...

I'm adding @eerhardt in case these approaches are changing due to his work on the catalog and removing MEF.

@singlis
Copy link
Member Author

singlis commented Mar 29, 2019

Very sorry for the late reply, I know I am well overdue.

I would rather not have an MLContext along with these discover-able methods for APIs and instead use namespaces. This problem would be solved across assemblies if the APIs were scoped by namespaces rather than within members of a class. This could then simply be

foo1 = Microsoft.ML.BinaryClassification.LogisticRegression();
var foo2 = Microsoft.ML.Transforms.OneHotEncoding();

But I know, I know, then we would have to pass MLContext around. That is a whole other discussion... that has been talked about a lot and I don't want to re-hash it here.

Since we are not there and we have mlContext, then I agree with @CESARDELATORRE in that we should always use parenthesis and be consistent. This would allow us to have MLContext extensions defined in their respective assembly and therefore the category would only show on mlContext if its available.

For example when I type mlContext.Ranking.Trainers -- nothing shows up in the pop-up window (with the exception of the defaults Equals, ToString). I then have to install a nuget that contains a Ranking trainer -- so if I install Microsoft.ML.LightGBM, Trainers then gets populated with LightGbm. This is a very disconnected experience -- how do I know I need to install a nuget? Why do I see Ranking at all when I dont have the proper assemblies yet?

Recommendation works more like how I would expect: if the Microsoft.ML.Recommender nuget is installed, I can now see Recommendation on mlContext (it is a function) and when I type mlContext.Recommendation().Trainers I can see a list of trainers to choose from.

@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 question Further information is requested
Projects
None yet
Development

No branches or pull requests

6 participants