Creation of components through MLContext: advanced options and other feedback

[TomFinley]

In our estimators and other similar components often have advanced settings, because, sometimes people have unusual circumstances. At the same time, there is a 95% or 99% scenario for "simple" usage that most people will be happy with. For this reason we have often made a distinction between common and advanced settings, as we see here.

machinelearning/src/Microsoft.ML.FastTree/TreeTrainersCatalog.cs

Lines 29 to 38 in cb37c7e

	public static FastTreeRegressionTrainer FastTree(this RegressionContext.RegressionTrainers ctx,
	string labelColumn = DefaultColumnNames.Label,
	string featureColumn = DefaultColumnNames.Features,
	string weights = null,
	int numLeaves = Defaults.NumLeaves,
	int numTrees = Defaults.NumTrees,
	int minDatapointsInLeaves = Defaults.MinDocumentsInLeaves,
	double learningRate = Defaults.LearningRates,
	Action<FastTreeRegressionTrainer.Arguments> advancedSettings = null)
	{

There are some possible things that excited feedback:

1. Echoing feedback seen in Rename mlContext.Data.TextReader() to mlContext.Data.CreateTextLoader() #1690, these things where we're making something should have the prefix Create, even in situations where this a catalog where we are always creating. Note: Create preferred to Make.

2. The worth of ASP.NET style configuration was questioned (seen above as Action<FastTreeRegressionTrainer.Arguments> advancedSettings), e.g., there may not be much purpose in having a delegate. The older style where it just takes the Arguments period was preferred.

3. Having this Arguments object as a nested class the component being created was viewed as positive, but this would be more idiomatically called Options -- Arguments was a holdover name from when these were exclusively for command line arguments, but for the API this is not a great name. So while keeping the general structure of how they are placed currently, they should probably be renamed to Options.

4. It is good to have the convenience for the simple arguments, however, if we have both simple and advanced settings, we should not mix them but have instead two distinct constructors/extension methods. (E.g., in the above, we would have two methods, one that took the advanced options.) To do otherwise is to invite confusion about which "wins" if we have the setting set in both.

   • Note that phase setting "set in both," which suggests that these settings object should retain the "simpler" settings in them. This reinforces feedback elsewhere as seen here.

5. If the simple arguments are totally sufficient, then there is no need to expose this Arguments class in hte public API. (For practical reasons relating to command line and entry-point usage, we still need to always have these Arguments objects, but if they serve no purpose for the API the class can simply be made internal.)

/cc @KrzysztofCwalina, @terrajobst , on whose feedback this list is primarily based, and who can correct me and provide clarification in case I misspoke.

[TomFinley]

One point that @glebuk brought up I wanted to make explicit, since I did not consider it before, is that by doing this, if we decide down the line to add more options to the "regular" options, we are free to do so. If we had the existing arrangement, then in order to maintain the API, if we made more regular default parameters. E.g., if we have this:

Create(int a, int b, int c, Action<Arguments> advancedSettings = null)

We could not have this, since this could break the signature.

Create(int a, int b, int c, int d, Action<Arguments> advancedSettings = null)

And the only way we see of not breaking the signature would be appending it to the end, which is very awkward and strange looking.

Create(int a, int b, int c, Action<Arguments> advancedSettings = null, int d = 1)

But if we had this:

Create(int a, int b, int c)
Create(Arguments advancedSettings)

Then we're free to do this:

Create(int a, int b, int c, int d = 1)
Create(Arguments advancedSettings)

[abgoswam]

marking as Done, and closing this isssue. All learners have been taken care of.

[Ivanidzo4ka]

Forgive me my intrusion into this wonderful conversation, but I have question from user, which I'm don't know how to answer.
So we have this issue #2165 and for now user can just call trival transformer constructors and avoid estimators and Fit method.

number 1 in Senja list states

For the transform estimators (former transforms) the following needs to happen:
1- Internalize the ctors

How we come up to this decision, and what is root cause behind hiding constructors for trivial transforms? /cc @JakeRadMSFT