Mark EntryPoints classes and APIs as internal #2674

ganik · 2019-02-21T00:58:27Z

Merge

Ivanidzo4ka · 2019-02-21T01:06:12Z

src/Microsoft.ML.StandardLearners/Standard/Online/LinearSvm.cs

@@ -69,7 +69,8 @@ public sealed class Options : OnlineLinearOptions
            /// Column to use for example weight.
            /// </summary>
            [Argument(ArgumentType.AtMostOnce, HelpText = "Column to use for example weight", ShortName = "weight", SortOrder = 4, Visibility = ArgumentAttribute.VisibilityType.EntryPointsOnly)]
-            public Optional<string> WeightColumn = Optional<string>.Implicit(DefaultColumnNames.Weight);
+            [BestFriend]
+            internal Optional<string> WeightColumn = Optional<string>.Implicit(DefaultColumnNames.Weight);


internal Optional WeightColumn = Optional.Implicit(DefaultColumnNames.Weight); [](start = 12, length = 94)

How user suppose to specify weight column during training?
This algorithm supports weights column in dataview see line 149 #Resolved

@Ivanidzo4ka is correct... this must continue to be supported.

To remind and possibly rephrase earlier conversations since they were somewhat informal, and I may not have expressed as clearly as might be desirable:

You will rename this field WeightColumn something appropriate, like, say, WeightColumnOptional. To maintain the entry-point name, the Argument attribute will have its Name property set to WeightColumn. (Thereby making the entry-points remain unchanged.)

You will introduce another field or property named WeightColumn intended for use from the API. This will be a public field, unadorned by an argument attribute, as it is not intended for Entry Point/Command Line use. This will be a string, not an Optional<string>.

The code in LightGBM will appropriately adjust to the presence of either of these, thereby responding appropriately whether one or the other is set (or, as it may happen, not set). This may require some additional (hidden!) parameters somewhere to distinguish whether something was called from the command line/entry-points (in which case we'd expect the argument attribute field to be set), or the public surface of the API. There are many obvious ways of doing this, I'll trust we can figure out which is best.

The same pattern should be followed in other places where you are providing an "alternative" to the options. We want our Options object, where exposed, to expose all available options. (This in accordance of our usual attitude towards how these objects should behave, as captured in #1798.) #Resolved

As agreed made Weight and GroupId columns explicit strings

In reply to: 258773450 [](ancestors = 258773450)

Ivanidzo4ka · 2019-02-21T01:07:54Z

src/Microsoft.ML.Core/EntryPoints/ModuleArgs.cs

@@ -133,7 +133,8 @@ public sealed class OutputAttribute : Attribute
        /// A node can be run without optional input fields.
        /// </summary>
        [AttributeUsage(AttributeTargets.Field | AttributeTargets.Property)]
-        public sealed class OptionalInputAttribute : Attribute { }
+        [BestFriend]
+        internal sealed class OptionalInputAttribute : Attribute { }


OptionalInputAttribute [](start = 30, length = 22)

why only this get changed?
And it's a part of internal static class, isn't it internal already? #Resolved

TomFinley · 2019-02-21T03:55:22Z

src/Microsoft.ML.StandardLearners/Standard/Online/LinearSvm.cs

@@ -69,7 +69,8 @@ public sealed class Options : OnlineLinearOptions
            /// Column to use for example weight.
            /// </summary>
            [Argument(ArgumentType.AtMostOnce, HelpText = "Column to use for example weight", ShortName = "weight", SortOrder = 4, Visibility = ArgumentAttribute.VisibilityType.EntryPointsOnly)]
-            public Optional<string> WeightColumn = Optional<string>.Implicit(DefaultColumnNames.Weight);
+            [BestFriend]


[BestFriend] [](start = 12, length = 12)

I see no particular reason for this to be best-friended with anything. Your public surface will not involve this attribute. This field will only be used via entry-points and suchlike. #Resolved

TomFinley

The functionality currently exposed as optional arguments must continue to be supported through alternate means, they must not just be hidden altogether.

eerhardt · 2019-02-21T17:03:40Z

src/Microsoft.ML.Data/Properties/AssemblyInfo.cs

@@ -26,6 +26,7 @@
 [assembly: InternalsVisibleTo(assemblyName: "Microsoft.ML.HalLearners" + PublicKey.Value)]
 [assembly: InternalsVisibleTo(assemblyName: "Microsoft.ML.KMeansClustering" + PublicKey.Value)]
 [assembly: InternalsVisibleTo(assemblyName: "Microsoft.ML.LightGBM" + PublicKey.Value)]
+[assembly: InternalsVisibleTo(assemblyName: "Microsoft.ML.LightGBM.StaticPipe" + PublicKey.Value)]


Will we still need this after @TomFinley's comments are addressed? (Basically to keep a public string WeightColumn property) #Resolved

… effect

codecov · 2019-02-22T02:00:16Z

Codecov Report

Merging #2674 into master will increase coverage by <.01%.
The diff coverage is 74.28%.

@@            Coverage Diff             @@
##           master    #2674      +/-   ##
==========================================
+ Coverage   71.57%   71.57%   +<.01%     
==========================================
  Files         800      805       +5     
  Lines      141789   141993     +204     
  Branches    16107    16119      +12     
==========================================
+ Hits       101488   101637     +149     
- Misses      35851    35918      +67     
+ Partials     4450     4438      -12

Flag	Coverage Δ
#Debug	`71.57% <74.28%> (ø)`	⬆️
#production	`67.87% <68.96%> (-0.01%)`	⬇️
#test	`85.73% <100%> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
...rc/Microsoft.ML.Core/EntryPoints/PredictorModel.cs	`100% <ø> (ø)`	⬆️
src/Microsoft.ML.Core/EntryPoints/ModuleArgs.cs	`57.32% <ø> (-0.98%)`	⬇️
...rc/Microsoft.ML.Data/EntryPoints/EntryPointNode.cs	`68.58% <ø> (-0.27%)`	⬇️
src/Microsoft.ML.Data/Training/TrainerUtils.cs	`70.48% <ø> (-0.3%)`	⬇️
...rc/Microsoft.ML.Core/EntryPoints/TransformModel.cs	`100% <ø> (ø)`	⬆️
src/Microsoft.ML.Data/EntryPoints/InputBuilder.cs	`81.65% <ø> (ø)`	⬆️
...ML.LightGBM.StaticPipe/LightGbmStaticExtensions.cs	`46.42% <0%> (ø)`	⬆️
...actorizationMachine/FactorizationMachineTrainer.cs	`88.18% <0%> (ø)`	⬆️
src/Microsoft.ML.StaticPipe/LbfgsStatic.cs	`53.15% <100%> (ø)`	⬆️
src/Microsoft.ML.LightGBM/LightGbmTrainerBase.cs	`62% <100%> (+0.18%)`	⬆️
... and 42 more
#Resolved

Ivanidzo4ka · 2019-02-22T02:00:28Z

#Resolved

Ivanidzo4ka · 2019-02-22T02:03:03Z

src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs

@@ -147,7 +147,7 @@ internal FieldAwareFactorizationMachineTrainer(IHostEnvironment env, Options opt
                FeatureColumns[i + 1] = new SchemaShape.Column(options.ExtraFeatureColumns[i], SchemaShape.Column.VectorKind.Vector, NumberDataViewType.Single, false);

            LabelColumn = new SchemaShape.Column(options.LabelColumn, SchemaShape.Column.VectorKind.Scalar, BooleanDataViewType.Instance, false);
-            WeightColumn = options.WeightColumn.IsExplicit ? new SchemaShape.Column(options.WeightColumn, SchemaShape.Column.VectorKind.Scalar, NumberDataViewType.Single, false) : default;
+            WeightColumn = options.WeightColumn != null ? new SchemaShape.Column(options.WeightColumn, SchemaShape.Column.VectorKind.Scalar, NumberDataViewType.Single, false) : default;


ptions.WeightColumn != null ? new SchemaShape.Column(options.WeightColumn, SchemaShape.Column.VectorKind.Scalar, NumberDataViewType.Single, false) : default [](start = 28, length = 156)

TrainerUtils.MakeR4ScalarWeightColumn?

Ivanidzo4ka

TomFinley

Thank you @ganik !!

Make public classes in Entrypoints namespace internal

d843005

ganik requested review from TomFinley, Ivanidzo4ka and eerhardt February 21, 2019 00:58

ganik and others added 2 commits February 20, 2019 17:00

Merge pull request #1 from dotnet/master

d43d94a

Merge

merged

0409e23

Ivanidzo4ka reviewed Feb 21, 2019

View reviewed changes

TomFinley reviewed Feb 21, 2019

View reviewed changes

TomFinley suggested changes Feb 21, 2019

View reviewed changes

eerhardt reviewed Feb 21, 2019

View reviewed changes

ganik added 5 commits February 21, 2019 15:46

Make WeightColumn as a string. It has to be provided in order to take…

adb985a

… effect

roll back

6f4b026

fix test build

4897f34

fix tests

54fe04b

fix tests

08fc16e

Ivanidzo4ka reviewed Feb 22, 2019

View reviewed changes

Ivanidzo4ka approved these changes Feb 22, 2019

View reviewed changes

Ivanidzo4ka requested a review from TomFinley February 22, 2019 19:01

TomFinley approved these changes Feb 22, 2019

View reviewed changes

Ivanidzo4ka merged commit b604b07 into dotnet:master Feb 22, 2019

eerhardt mentioned this pull request Mar 2, 2019

WeightColumn name defaults to "Weights" in one of the constructors of the estimators #1065

Closed

ghost locked as resolved and limited conversation to collaborators Mar 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark EntryPoints classes and APIs as internal #2674

Mark EntryPoints classes and APIs as internal #2674

ganik commented Feb 21, 2019 •

edited

Loading

Ivanidzo4ka Feb 21, 2019 •

edited by ganik

Loading

TomFinley Feb 21, 2019 •

edited by ganik

Loading

ganik Feb 22, 2019

Ivanidzo4ka Feb 21, 2019 •

edited by ganik

Loading

TomFinley Feb 21, 2019 •

edited by ganik

Loading

TomFinley left a comment

eerhardt Feb 21, 2019 •

edited by ganik

Loading

codecov bot commented Feb 22, 2019 •

edited by ganik

Loading

Ivanidzo4ka commented Feb 22, 2019 •

edited by ganik

Loading

Ivanidzo4ka Feb 22, 2019

Ivanidzo4ka left a comment

TomFinley left a comment

Mark EntryPoints classes and APIs as internal #2674

Mark EntryPoints classes and APIs as internal #2674

Conversation

ganik commented Feb 21, 2019 • edited Loading

Ivanidzo4ka Feb 21, 2019 • edited by ganik Loading

Choose a reason for hiding this comment

TomFinley Feb 21, 2019 • edited by ganik Loading

Choose a reason for hiding this comment

ganik Feb 22, 2019

Choose a reason for hiding this comment

Ivanidzo4ka Feb 21, 2019 • edited by ganik Loading

Choose a reason for hiding this comment

TomFinley Feb 21, 2019 • edited by ganik Loading

Choose a reason for hiding this comment

TomFinley left a comment

Choose a reason for hiding this comment

eerhardt Feb 21, 2019 • edited by ganik Loading

Choose a reason for hiding this comment

codecov bot commented Feb 22, 2019 • edited by ganik Loading

Codecov Report

Ivanidzo4ka commented Feb 22, 2019 • edited by ganik Loading

Ivanidzo4ka Feb 22, 2019

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

TomFinley left a comment

Choose a reason for hiding this comment

ganik commented Feb 21, 2019 •

edited

Loading

Ivanidzo4ka Feb 21, 2019 •

edited by ganik

Loading

TomFinley Feb 21, 2019 •

edited by ganik

Loading

Ivanidzo4ka Feb 21, 2019 •

edited by ganik

Loading

TomFinley Feb 21, 2019 •

edited by ganik

Loading

eerhardt Feb 21, 2019 •

edited by ganik

Loading

codecov bot commented Feb 22, 2019 •

edited by ganik

Loading

Ivanidzo4ka commented Feb 22, 2019 •

edited by ganik

Loading