Hide more of Microsoft.ML.Data #2842

TomFinley · 2019-03-05T00:09:29Z

Towards #1602. When performing what I hope is one of my final reviews of the public surface area of Microsoft.ML.Data I saw many "small" items, each too petty to warrant separate issues, but that we nonetheless do not want in the public surface. This is in the vein of #2300 and other similar PRs. Special emphasis was placed on making sure we don't have abstract protected members visible as part of the public surface, and other such things as this.

In my review of the assembly I did not address those types or members I knew were being taken care of through other channels. (E.g., the attribute bearing marker interfaces for IComponentFactory, or the calibrator.)

TomFinley · 2019-03-05T00:10:07Z

src/Microsoft.ML.Data/Commands/DefaultColumnNames.cs

+    /// uniformity on the public API surface, wherever it is judged where columns with default names should be consumed.
+    /// </summary>
+    [BestFriend]
+    internal static class DefaultColumnNames


While helpful for enabling a uniform public surface, this is of course not the same as it needing to be in the public surface.

(I agree we should internalize this - just raising as a general question in this area)

How do we imagine users to find out about these "magic" strings? Currently, one way is the default values in the MLContext APIs. But I wonder if that is enough. For example, when making a Prediction "output" class, how do users know to name/attribute their properties as PredictedLabel, Probability, Score? Just through documentation?

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

Yes, unfortunately I think both helpful displays in the debugger (for live discovery) and documentation (for offline discovery) are the only real answer we have right now -- both seem insufficient or at least suboptimal. This is a problem I think in most places we've chosen to believe that producing "default" columns names is more helpful than requiring the user to specify them. I am unsure how to address that problem. Something resembling the static typed API will I think be a solution to that problem, since the columns will be typed and addressable and right there.

Edit: Just to clarify, I am not saying that producing default column names is unhelpful or we should always insist that people provide them, just, this is one of the effects of that conscious choice.

I have seen quite a few instances where users use DefaultColumnNames. I don't understand why we want to hide it. We will always have default column names, why not allow users to access them instead of hard coding them in their applications?

searching the samples repo shows there are quite a few instances where people use DefaultColumnNames.
https://github.com/dotnet/machinelearning-samples/search?q=defaultcolumnnames&unscoped_q=defaultcolumnnames

or this other user provided code:
#2486 (comment)

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

In those places, what would have happened in those samples if instead of DefaultColumnNames.Features we instead wrote "Features"? I understand the appeal inside the tool of having a consistent "location" for them. For user code, I am less certain.

I am very uncomfortable with having as part of our public surface a class that is merely a collection of "magic strings" that we've used internally to denote certain concepts. That's not great API design.

The samples can and should change. To use this class of magic constants was, frankly, a bad move by the sample author, since it was totally needless and it obscures what's going on more than it clarifies it.

TomFinley · 2019-03-05T00:11:19Z

src/Microsoft.ML.Data/DataLoadSave/TransformWrapper.cs

@@ -148,11 +152,13 @@ IRowToRowMapper ITransformer.GetRowToRowMapper(DataViewSchema inputSchema)
    /// <summary>
    /// Estimator for trained wrapped transformers.
    /// </summary>
-    public abstract class TrainedWrapperEstimatorBase : IEstimator<TransformWrapper>
+    internal abstract class TrainedWrapperEstimatorBase : IEstimator<TransformWrapper>


This is a funny one. I was able to remove all subclasses of it, but some notes I read elsewhere led me to suspect there may be more than this than meets the eye. Started an issue about this for #2841. (Though that does not touch the public surface and is therefore inessential.)

TomFinley · 2019-03-05T00:12:03Z

src/Microsoft.ML.Data/DataLoadSave/TransformWrapper.cs

    {
-        protected readonly IHost Host;
+        [BestFriend]
+        private protected readonly IHost Host;


I have checked and in the public surface there is currently no protected member anywhere in Microsoft.ML.Data.

TomFinley · 2019-03-05T00:13:27Z

src/Microsoft.ML.Data/Model/ModelOperationsCatalog.cs

@@ -28,16 +28,6 @@ internal ModelOperationsCatalog(IHostEnvironment env)
            Explainability = new ExplainabilityTransforms(this);
        }

-        public abstract class SubCatalogBase


I may do more work to get rid of this sort of thing. These mysterious subclasses mean nothing to the user, and as part of the "central" object MLContext, we should probably choose some alternate mechanism. (I got rid of it here, but there's similar code elsewhere.)

I like this change. 👍

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

Very good, I have done as I suggested, and got rid of the rest. We typically hide infrastructure behind this using the tricks of explicit implementations of internal interfaces, so I used the same in introducing a new interface IInternalCatalog (not the best name, perhaps, but as it is internal we are free to improve it later). This also allowed me to simplify CatalogUtils considerably. So, now unless it has some other reason to exist (e.g., train catalog base does have a reason to exist, whereas SubCatalogBase did not), these mysterious "infrastructure" subclasses for catalogs are now gone.

TomFinley · 2019-03-05T00:14:21Z

src/Microsoft.ML.Data/Prediction/PredictionEngine.cs

@@ -103,7 +103,7 @@ public abstract class PredictionEngineBase<TSrc, TDst> : IDisposable
        /// <summary>
        /// Provides output schema.
        /// </summary>
-        public DataViewSchema OutputSchema;
+        public DataViewSchema OutputSchema { get; }


A mutable schema on the base class of the prediction engine. That would have been embarrassing. :)

TomFinley · 2019-03-05T00:17:40Z

src/Microsoft.ML.Transforms/Text/WrappedTextTransformers.cs

@@ -14,8 +15,9 @@ namespace Microsoft.ML.Transforms.Text
    /// Produces a bag of counts of ngrams (sequences of consecutive words) in a given text.
    /// It does so by building a dictionary of ngrams and using the id in the dictionary as the index in the bag.
    /// </summary>
-    public sealed class WordBagEstimator : TrainedWrapperEstimatorBase
+    public sealed class WordBagEstimator : IEstimator<ITransformer>


The TrainedWrapperEstimatorBase was meant to facilitate the translation from the legacy IDataTransform interface to the new ITransformer interface. It is intended to be a temporary situation; however, by making this class descend from that abstract base class, that temporary shim would have had to become permanent -- you cannot "undescend" from a class without it being a breaking change in your API. Therefore we would have had to live with it forever.

So we had to duplicate the logic that existed in that base class. Fortunately there were only two subclasses remaining (both in this file, as it happened), so it was not such a big problem.

This is a really good idea! until we do the proper translation

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

eerhardt

codecov · 2019-03-05T00:59:57Z

Codecov Report

Merging #2842 into master will decrease coverage by 0.01%.
The diff coverage is 82.78%.

@@            Coverage Diff             @@
##           master    #2842      +/-   ##
==========================================
- Coverage   71.71%   71.69%   -0.02%     
==========================================
  Files         809      809              
  Lines      142494   142522      +28     
  Branches    16114    16114              
==========================================
+ Hits       102185   102186       +1     
- Misses      35874    35907      +33     
+ Partials     4435     4429       -6

Flag	Coverage Δ
#Debug	`71.69% <82.78%> (-0.02%)`	⬇️
#production	`67.92% <82.78%> (-0.02%)`	⬇️
#test	`85.9% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
src/Microsoft.ML.StaticPipe/SchemaBearing.cs	`100% <ø> (ø)`	⬆️
src/Microsoft.ML.FastTree/FastTree.cs	`80.77% <ø> (ø)`	⬆️
src/Microsoft.ML.Data/DataLoadSave/FakeSchema.cs	`83.33% <ø> (ø)`	⬆️
src/Microsoft.ML.Data/Transforms/ValueMapping.cs	`84.37% <ø> (ø)`	⬆️
...Microsoft.ML.Data/Scorers/PredictionTransformer.cs	`97.02% <ø> (ø)`	⬆️
...ML.Data/Transforms/ValueToKeyMappingTransformer.cs	`78.21% <ø> (ø)`	⬆️
src/Microsoft.ML.Transforms/OneHotEncoding.cs	`85.44% <ø> (ø)`	⬆️
...Microsoft.ML.Data/DataLoadSave/TransformWrapper.cs	`22% <0%> (-11%)`	⬇️
...soft.ML.Data/Transforms/OneToOneTransformerBase.cs	`95.08% <100%> (ø)`	⬆️
...Microsoft.ML.Data/Training/TrainerEstimatorBase.cs	`78.26% <100%> (ø)`	⬆️
... and 21 more

artidoro · 2019-03-05T01:21:04Z

src/Microsoft.ML.Transforms/Text/WrappedTextTransformers.cs

+        }
+
+        public SchemaShape GetOutputSchema(SchemaShape inputSchema)
+        {


Can you add a summary for this new public method?

I suppose I could. While acknowledging the worth of documentation, I would point out documentation can be added any time. We have three weeks and only three weeks to finalize the public shape of the API. Anything that is there must remain there forever.

This method is not really "new" as such. It is merely copied from the existing base class, where it has precisely the same documentation as you see now.

If other people are making similar requests on other PRs, I might advise them to desist.

I think there’s an “inheritdoc” syntax you can use.

Aspnet uses it:

https://github.com/aspnet/AspNetCore/blob/c565386a3ed135560bc2e9017aa54a950b4e35dd/src/Mvc/Mvc.Core/src/Infrastructure/NonDisposableStream.cs

It looks like some doc tools respect these. Do ours?

Here’s the C# Lang feature request to add it as an official support in the language.

dotnet/csharplang#313

artidoro · 2019-03-05T01:21:13Z

src/Microsoft.ML.Transforms/Text/WrappedTextTransformers.cs

+            return new TransformWrapper(_host, WordHashBagProducingTransformer.Create(_host, options, input), true);
+        }
+
+        public SchemaShape GetOutputSchema(SchemaShape inputSchema)


SchemaShape [](start = 15, length = 11)

Same here: Can you add a summary for this new public method?

…ractically unsubclassable.

… literals.

…lic surface. Simplify CatalogUtils.

…tSchema.

artidoro · 2019-03-05T01:48:16Z

    /// </summary>

Should this be more specific to calibrators? #Resolved

Refers to: src/Microsoft.ML.Data/TrainCatalog.cs:381 in 392cee6. [](commit_id = 392cee6, deletion_comment = False)

artidoro · 2019-03-05T02:03:50Z

    /// </summary>

Why is this showing as a change? I see it was added as part of ivan's PR #2766

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

Refers to: src/Microsoft.ML.Data/TrainCatalog.cs:381 in 392cee6. [](commit_id = 392cee6, deletion_comment = False)

artidoro

🕐

artidoro

TomFinley · 2019-03-05T02:13:26Z

    /// </summary>

The documentation is wrong, but this line (381) is not at least showing as a change for me either on the Github web review tool, nor in the non-web tools we use to review, so, maybe something somehow got screwed up on your end?

The last line it shows that I've changed is 258, and the next line is 409. This line (381) definitely did not change.

I agree that the documentation should be changed to actually reflect reality, but I hope we can refine that later.

In reply to: 469504933 [](ancestors = 469504933,469501477)

Refers to: src/Microsoft.ML.Data/TrainCatalog.cs:381 in 658a128. [](commit_id = 658a128, deletion_comment = False)

TomFinley requested review from codemzs, Ivanidzo4ka, eerhardt, artidoro and sfilipi March 5, 2019 00:09

TomFinley commented Mar 5, 2019

View reviewed changes

eerhardt approved these changes Mar 5, 2019

View reviewed changes

artidoro reviewed Mar 5, 2019

View reviewed changes

TomFinley force-pushed the Hide1 branch from 392cee6 to 7b714fd Compare March 5, 2019 01:22

TomFinley added 8 commits March 4, 2019 17:41

Get rid of unnecessary SubCatalogBase base class.

5d05983

Clean up unintentional usage of protected internal.

45bd2fe

ValueToKeyMappingEstimator.ColumnOptions now not publicly subclassable.

d2844c9

Have the public abstract classes for estimator/transformers/etc. be p…

5b38404

…ractically unsubclassable.

Internalize the infrastructure surrounding TransformWrapper.

501a318

Internalize DefaultColumnNames utility class for storing const string…

3755f6e

… literals.

Use explicit IInternalCatalog, thereby avoiding SubCatalogBase in pub…

ac0fe82

…lic surface. Simplify CatalogUtils.

Copy documentation from IEstimator to two implementations of GetOutpu…

658a128

…tSchema.

TomFinley force-pushed the Hide1 branch from 7b714fd to 658a128 Compare March 5, 2019 01:42

artidoro suggested changes Mar 5, 2019

View reviewed changes

artidoro approved these changes Mar 5, 2019

View reviewed changes

TomFinley merged commit a4bfd93 into dotnet:master Mar 5, 2019

TomFinley deleted the Hide1 branch March 6, 2019 16:31

CESARDELATORRE mentioned this pull request Apr 9, 2019

Breaking code documentation is missing breaking change 'DefaultColumnNames' is inaccessible due to its protection level" dotnet/machinelearning-samples#358

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide more of Microsoft.ML.Data #2842

Hide more of Microsoft.ML.Data #2842

TomFinley commented Mar 5, 2019

TomFinley Mar 5, 2019

eerhardt Mar 5, 2019

TomFinley Mar 5, 2019 •

edited

Loading

artidoro Mar 5, 2019 •

edited

Loading

TomFinley Mar 5, 2019 •

edited

Loading

TomFinley Mar 5, 2019

TomFinley Mar 5, 2019

TomFinley Mar 5, 2019

eerhardt Mar 5, 2019

TomFinley Mar 5, 2019 •

edited

Loading

TomFinley Mar 5, 2019

TomFinley Mar 5, 2019

artidoro Mar 5, 2019

eerhardt left a comment

codecov bot commented Mar 5, 2019 •

edited

Loading

artidoro Mar 5, 2019

TomFinley Mar 5, 2019

eerhardt Mar 5, 2019

artidoro Mar 5, 2019

artidoro commented Mar 5, 2019 •

edited

Loading

artidoro commented Mar 5, 2019

artidoro left a comment •

edited

Loading

artidoro left a comment

TomFinley commented Mar 5, 2019

Hide more of Microsoft.ML.Data #2842

Hide more of Microsoft.ML.Data #2842

Conversation

TomFinley commented Mar 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Mar 5, 2019 • edited Loading

Choose a reason for hiding this comment

artidoro Mar 5, 2019 • edited Loading

Choose a reason for hiding this comment

TomFinley Mar 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Mar 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 5, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro commented Mar 5, 2019 • edited Loading

artidoro commented Mar 5, 2019

artidoro left a comment • edited Loading

Choose a reason for hiding this comment

artidoro left a comment

Choose a reason for hiding this comment

TomFinley commented Mar 5, 2019

TomFinley Mar 5, 2019 •

edited

Loading

artidoro Mar 5, 2019 •

edited

Loading

TomFinley Mar 5, 2019 •

edited

Loading

TomFinley Mar 5, 2019 •

edited

Loading

codecov bot commented Mar 5, 2019 •

edited

Loading

artidoro commented Mar 5, 2019 •

edited

Loading

artidoro left a comment •

edited

Loading