Skip to content

Command-line oriented arguments, that have more suitable alternatives for the API should be made internal #2133

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
sfilipi opened this issue Jan 11, 2019 · 5 comments
Assignees
Labels
API Issues pertaining the friendly API

Comments

@sfilipi
Copy link
Member

sfilipi commented Jan 11, 2019

In the arguments classes there are several arguments that get translated to the same parameter for the estimators/transforms.

One of the variant is a convenience for the command line version of ML.Net.

I think those can be made internal, and kept away from the user for v1.

Example:

ValueToKeyMappingTransformer.ArgumentsBase

public abstract class ArgumentsBase : TransformInputBase
{
     [Argument(ArgumentType.AtMostOnce, HelpText = "Maximum number of terms to keep per column when auto-training", ShortName = "max", SortOrder = 5)]
      public int MaxNumTerms = ValueToKeyMappingEstimator.Defaults.MaxNumTerms;

      [Argument(ArgumentType.AtMostOnce, HelpText = "Comma separated list of terms", SortOrder = 105, Visibility = ArgumentAttribute.VisibilityType.CmdLineOnly)]
      public string Terms;

      [Argument(ArgumentType.AtMostOnce, HelpText = "List of terms", SortOrder = 106, Visibility = ArgumentAttribute.VisibilityType.EntryPointsOnly)]
      public string[] Term;

I don't think our API users should see the first

public string Terms

@najeeb-kazmi
Copy link
Member

najeeb-kazmi commented Jan 12, 2019

The other two occurrences of this are:

CustomStopWordsRemovingTransform.ArgumentsBase

        public abstract class ArgumentsBase
        {
            [Argument(ArgumentType.AtMostOnce, HelpText = "Comma separated list of stopwords", Visibility = ArgumentAttribute.VisibilityType.CmdLineOnly)]
            public string Stopwords;

            [Argument(ArgumentType.AtMostOnce, HelpText = "List of stopwords", Visibility = ArgumentAttribute.VisibilityType.EntryPointsOnly)]
            public string[] Stopword;

and

TermLoaderArguments

    public sealed class TermLoaderArguments
    {
        [Argument(ArgumentType.AtMostOnce, HelpText = "Comma separated list of terms", SortOrder = 1, Visibility = ArgumentAttribute.VisibilityType.CmdLineOnly)]
        public string Terms;


        [Argument(ArgumentType.AtMostOnce, HelpText = "List of terms", SortOrder = 1, Visibility = ArgumentAttribute.VisibilityType.EntryPointsOnly)]
        public string[] Term;

@Zruty0
Copy link
Contributor

Zruty0 commented Jan 13, 2019

I would say that the name Term for a 'list of terms' is not ideal either.

@sfilipi sfilipi added the API Issues pertaining the friendly API label Jan 14, 2019
@glebuk
Copy link
Contributor

glebuk commented Jan 25, 2019

Related to #2079 and #2041

@artidoro
Copy link
Contributor

After further investigating, it seems that the above Arguments classes should be made internal. They are only used by the entrypoints API. We are using other objects to set the parameters of the estimators (either direct definition of the parameters in the constructors, or through a ColumnInfo object).

@artidoro
Copy link
Contributor

The only transform that still has public Options class (former Arguments class) is the TensorFlowTransformer and it does not have command line oriented arguments.

The remaining transform either use the ColumnInfo object or don't have advanced settings that cannot be set from the constructor directly. So we have made the Options class internal in those cases.

This issue can therefore be considered closed.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 25, 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

5 participants