Hide IDataTransform Create methods #2031

Ivanidzo4ka · 2019-01-05T00:38:10Z

Step towards #1995.
Also add proper comments for Image transforms.
Remove Create methods in Onnx and Tensorflow (replace them with proper constructors).

Ivanidzo4ka · 2019-01-05T00:41:52Z

src/Microsoft.ML.OnnxTransform.StaticPipe/DnnImageFeaturizerStaticExtensions.cs


-namespace Microsoft.ML.OnnxTransform.StaticPipe
+namespace Microsoft.ML.Transforms.StaticPipe


Transforms [](start = 23, length = 10)

Reason why I make this change is because I don't like what I have to do this:
new Transforms.OnnxTransform(...);
instead of just
new OnnxTransform(...);
if we have Microsoft.ML in usings. (see OnnxTransformTests.cs)

Shall we just call it Microsoft.ML.StaticPipe without any additional sub namespaces? #Closed

I'd expect that static pipe would be something we'd nail down later, but I'd expect that inputs to to staticpipe would just be accomplished by taking a PiplineColumn instance, and applying the transform to it. There should not be any new Transforms.OnnxTransform in the static pipeline at all, since the static pipeline is more declarative. So I don't like either of the things you've described, if indeed that is now it iss set up. If that's not how it was set up, well, that's bad. :) So that would be something to file and issue on and fix. (Though I think static pipeline is something we're going to refine more post v1.0.)

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

Actually, from what I see below, it is set up as an extension method on the columns. So what is the problem?

In reply to: 245458003 [](ancestors = 245458003,245456891)

If you have your code and you use using Microsoft.ML; in it (which we do all the time), I can't longer just create OnnxTransform object via new OnnxTransform(...), because you will get collision of namespace and object.
So you have to directly specify what your transform is in Transform namespace like this: Transforms.OnnxTransform which I found annoying.

In reply to: 245458074 [](ancestors = 245458074,245458003,245456891)

Oh I see. Yes I agree then, I didn't know you were talking about namespace/object collision.

Ultimately all static extensions should be in Microsoft.ML namespace (since we must accept that inclusion of a specific nuget is license to use it, similar to our attitude about MLContext extensions), but not right now.

In reply to: 245458427 [](ancestors = 245458427,245458074,245458003,245456891)

But that happens only when ppl use both static and dynamic, right? Our tests do so, but idk how many ppl will mix ant match static and dynamic.

In reply to: 245458713 [](ancestors = 245458713,245458427,245458074,245458003,245456891)

TomFinley · 2019-01-05T00:42:48Z

src/Microsoft.ML.Data/Transforms/KeyToValue.cs

-        /// <summary>
-        /// Factory method for SignatureLoadModel.
-        /// </summary>
+        // Factory method for SignatureLoadModel.


// Factory method for SignatureLoadModel. [](start = 8, length = 41)

I see you doing this multiple places, is there any reason for it?

If we instead kept it an XML comment, we could improve this by having a <see tag on it, which overall seems like a good idea. We're not doing this now, but it seems like an unforced regression into a worse, not a better, place. #Resolved

only reason why we had
tag because it was requirement for public method, and since it's no longer public I just prefer mark this methods as comments since
a) This is most common pattern in our codebase.
b) I think about this as more as internal message for developer: "Hey, don't delete this one, even if no one uses it, we actually call it via our DI system." Summary is more like a description of what is going on in this method, and this is more like a mark/internal comment.

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

tags are a requirement for public methods. That ***does not make them a bad thing for private or internal methods***, especially if those comments explicitly reference other structures in the codebase, as this does.
Why would a developer prefer this comment, to the old comment? If you had instead had a <see tag, as suggested above, that developer could even F12 on it to see what the heck we're talking about. Right?

In reply to: 245457620 [](ancestors = 245457620,245456969)

This should probably just be a simple search-replace, I think, to put it into proper form. At least, so I'd think.

In reply to: 245458497 [](ancestors = 245458497,245457620,245456969)

I know I can't win argument with you so I reverted my changes to summary comments. Although I'm still adding comments for other places since this is common pattern in our code.
I hope this is not a blocker, since main intent of this PR is to hide IDataTransforms methods.

In reply to: 245459044 [](ancestors = 245459044,245458497,245457620,245456969)

TomFinley · 2019-01-05T00:43:49Z

src/Microsoft.ML.Data/Transforms/NormalizeColumn.cs

-        /// </summary>
-        public static IDataTransform Create(IHostEnvironment env, MeanVarArguments args, IDataView input)
+        // Factory method corresponding to SignatureDataTransform.
+        internal static IDataTransform Create(IHostEnvironment env, MeanVarArguments args, IDataView input)


internal [](start = 8, length = 8)

If the comment is correct, then it should suffice for it to be private, right? So is the comment incorrect? #Closed

Everytime we have Entrypoint for transform I have to mark this method as internal.
As alternative I can refactor conversion of argument class to ColumnInfo array and use it here and in entrypoint, but oh, boy, it would be lot of changes across codebase.

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

Oh entrypoints. Fair enough.

In reply to: 245457335 [](ancestors = 245457335,245457063)

TomFinley · 2019-01-05T00:44:42Z

src/Microsoft.ML.Data/Transforms/OneToOneTransformerBase.cs

@@ -15,7 +15,7 @@ public abstract class OneToOneTransformerBase : RowToRowTransformerBase
    {
        protected readonly (string input, string output)[] ColumnPairs;

-        protected OneToOneTransformerBase(IHost host, (string input, string output)[] columns) : base(host)
+        protected OneToOneTransformerBase(IHost host, params (string input, string output)[] columns) : base(host)


params [](start = 54, length = 6)

Is this related? It might be a good idea, just not sure? It doesn't at least at first glance seem to have anything to do with the issue. (Not suggesting we can't do it, just, not sure what the purpose of the change is.) #Closed

I think I replaced one of Create methods with constructor which accept only one input and output column, and I didn't like idea what I need to cast that pair into array of pairs.
This is why I changed it. Not sure that constructor is still in part of codereview, but I don't think it's in general bad thing to have.

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

All right, thanks Ivan.

In reply to: 245457798 [](ancestors = 245457798,245457186)

TomFinley · 2019-01-05T00:50:46Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

@@ -100,32 +100,8 @@ private static VersionInfo GetVersionInfo()
            loaderAssemblyName: typeof(OnnxTransform).Assembly.FullName);
        }

-        public static IDataTransform Create(IHostEnvironment env, IDataView input, string modelFile, int? gpuDeviceId = null, bool fallbackToCpu = false)


public static IDataTransform Create(IHostEnvironment env, IDataView input, string modelFile, int? gpuDeviceId = null, bool fallbackToCpu = false) [](start = 8, length = 145)

This is an "odd" method, so it seems good to remove, so are we instead working via the extension methods and MLContext? #Closed

I'm not sure about extension methods, but in tests I switch to using OnnxTransform() constructor with same parameters and then calling Transform method on DataView.
I think it's better to get rid of this Create methods, and use either estimator, or since estimator is trivial, just call directly transform consturctor.

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

Yes, probably.

In reply to: 245458012 [](ancestors = 245458012,245457717)

TomFinley · 2019-01-05T00:56:35Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

+        /// <param name="modelFile">Model file path.</param>
+        /// <param name="inputColumn">The name of the input data column. Must match model input name.</param>
+        /// <param name="outputColumn">The output columns to generate. Names must match model specifications. Data types are inferred from model.</param>
+        public TensorFlowTransform(IHostEnvironment env, string modelFile, string inputColumn, string outputColumn)


TensorFlowTransform [](start = 15, length = 19)

The naming I find incomprehensible, similar to the ONNX thing. It is called a Transform, yet it appears to be a Transformer going by the class relationship? Do we know what is going on here?

If it is in fact a transform and not a transformer, as the name suggests, then I'd prefer our public surface area be smaller, not larger? But maybe the name is just bad? #Resolved

It's a transformer now

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

TomFinley · 2019-01-05T01:03:55Z

public sealed class OnnxTransform : RowToRowTransformerBase

This naming is very strange... it is called OnnxTransform, but, is it a IDataTransform, or an ITransformer? I know it's not your code, just wondering if we know what was the author's intent. #Resolved

Refers to: src/Microsoft.ML.OnnxTransform/OnnxTransform.cs:58 in 185099d. [](commit_id = 185099d, deletion_comment = False)

Ivanidzo4ka · 2019-01-05T01:16:17Z

public sealed class OnnxTransform : RowToRowTransformerBase

It's a ITransformer, I actually converted it to ITransformer some time ago, but didn't rename it.
I push another iteration with renaming for TensorFlow, Onnx, and Image transforms.

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

Refers to: src/Microsoft.ML.OnnxTransform/OnnxTransform.cs:58 in 185099d. [](commit_id = 185099d, deletion_comment = False)

sfilipi · 2019-01-08T21:19:04Z

src/Microsoft.ML.Data/Transforms/ColumnCopying.cs

@@ -39,7 +39,7 @@ public sealed class ColumnCopyingEstimator : TrivialEstimator<ColumnCopyingTrans
        {
        }

-        public ColumnCopyingEstimator(IHostEnvironment env, params (string source, string name)[] columns)


ColumnCopyingEstimator [](start = 15, length = 22)

I'll bug the next person about XML comments, or do it:) #Closed

Would you trust me if I say what I plan to do that in other PR?
This one is purely IDataTransform hiding.

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

yep!

In reply to: 246156509 [](ancestors = 246156509,246154879)

sfilipi · 2019-01-08T21:19:42Z

src/Microsoft.ML.Data/Transforms/ColumnCopying.cs

-        // Factory method to create from arguments
-        public static IDataTransform Create(IHostEnvironment env, Arguments args, IDataView input)
+        // Factory method corresponding to SignatureDataTransform.
+        internal static IDataTransform Create(IHostEnvironment env, Arguments args, IDataView input)


internal [](start = 8, length = 8)

double-checking: can't be private? #Closed

If it's internal, it's mean we have entrypoint somewhere which invokes that create method.
I'm quite confident we have entrypoint for Column Copying.

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

thanks.

In reply to: 246156795 [](ancestors = 246156795,246155060)

sfilipi · 2019-01-08T21:32:51Z

src/Microsoft.ML.ImageAnalytics/ImageResizerTransform.cs

+        /// <param name="env">The host environment.</param>
+        /// <param name="input">Name of the input column.</param>
+        /// <param name="output">Name of the column resulting from the transformation of <paramref name="input"/>.</param>
+        /// <param name="imageWidth">Widht of resized image.</param>


Widht [](start = 37, length = 5)

width.

sfilipi · 2019-01-08T21:33:14Z

src/Microsoft.ML.ImageAnalytics/ImageResizerTransform.cs

-            int imageWidth, int imageHeight, ImageResizerTransform.ResizingKind resizing = ImageResizerTransform.ResizingKind.IsoCrop, ImageResizerTransform.Anchor cropAnchor = ImageResizerTransform.Anchor.Center)
-            : this(env, new ImageResizerTransform(env, inputColumn, outputColumn, imageWidth, imageHeight, resizing, cropAnchor))
+        /// <summary>
+        /// Estimator which resize images.


resize [](start = 28, length = 6)

resizes

sfilipi · 2019-01-08T21:33:57Z

src/Microsoft.ML.ImageAnalytics/ImageResizerTransform.cs

+        /// <param name="output">Name of the column resulting from the transformation of <paramref name="input"/>.</param>
+        /// <param name="imageWidth">Widht of resized image.</param>
+        /// <param name="imageHeight">Height of resized image.</param>
+        /// <param name="resizing">What resize method to use.</param>


resize method [](start = 40, length = 13)

sfilipi · 2019-01-08T22:02:54Z

src/Microsoft.ML.Transforms/MissingValueHandlingTransformer.cs

@@ -115,7 +116,7 @@ public bool TryUnparse(StringBuilder sb)
        /// <param name="name">Name of the output column.</param>
        /// <param name="source">Name of the column to be transformed. If this is null '<paramref name="name"/>' will be used.</param>
        /// <param name="replaceWith">The replacement method to utilize.</param>
-        public static IDataTransform Create(IHostEnvironment env, IDataView input, string name, string source = null, ReplacementKind replaceWith = ReplacementKind.DefaultValue)
+        public static IDataView Create(IHostEnvironment env, IDataView input, string name, string source = null, ReplacementKind replaceWith = ReplacementKind.DefaultValue)


public [](start = 8, length = 6)

Internal? Actually, does this need a constructor? it is not a transformer, and not on the catalogs, but it will still pop up if public.

sfilipi · 2019-01-08T22:06:19Z

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

@@ -279,8 +281,10 @@ public TransformApplierParams(TextFeaturizingEstimator  parent)
            advancedSettings?.Invoke(AdvancedSettings);

            _dictionary = null;
-            _wordFeatureExtractor = new NgramExtractorTransform.NgramExtractorArguments();
-            _charFeatureExtractor = new NgramExtractorTransform.NgramExtractorArguments() { NgramLength = 3, AllLengths = false };
+            if (AdvancedSettings.UseWordExtractor)


if (AdvancedSettings.UseWordExtractor) [](start = 11, length = 39)

Pr title should have been: Hide the Create methods, change the name of the ImageTransformers to match the conventions used elsewhere, and fix every bug, whitespace and whatnot i see as i work :D #WontFix

sfilipi · 2019-01-08T22:11:51Z

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

-                    Seed = args.Seed,
-                    Ordered = false,
-                    Column = hashColumns.ToArray(),
-                    InvertHash = args.InvertHash


args.InvertHash [](start = 33, length = 15)

are we losing this and the args.Seed? #Closed

line 363: column.Seed ?? args.Seed
I use args.Seed

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

sfilipi · 2019-01-08T22:18:12Z

test/Microsoft.ML.Core.Tests/UnitTests/TestEntryPoints.cs

-                        }
-                    }
-                }, data);
+                data = new ColumnConcatenatingTransformer(Env, "Features", new[] { "Features1", "Features2" }).Transform(data);


data = new ColumnConcatenatingTransformer(Env, "Features", new[] { "Features1", "Features2" }).Transform(data); [](start = 16, length = 111)

me looking at this and thinking that we should make the Transformer constructors internal.

Is there any benefit to leaving them public? (i know we've had the conversation for the Predictors/ModelParams and that made sense.)

If it is more than a yes/no answer, i can create an issue and we can discuss it there. #Closed

I think idea was if it's a trivial estimator, it's ok to have public constructor for it's transformer.
Estimator is become pointless for them unless you try to build chain of estimators.

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

ok, thanks.

In reply to: 246172551 [](ancestors = 246172551,246171719)

sfilipi · 2019-01-08T22:26:19Z

test/Microsoft.ML.Tests/ScenariosWithDirectInstantiation/TensorflowTests.cs

@@ -28,7 +28,7 @@ private class TestData
        [ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // TensorFlow is 64-bit only
        public void TensorFlowTransformMatrixMultiplicationTest()
        {
-            var model_location = "model_matmul/frozen_saved_model.pb";


model_location [](start = 16, length = 14)

:) #WontFix

sfilipi

TomFinley

Thanks @Ivanidzo4ka !

Hide IDataTransform Create methods

185099d

Ivanidzo4ka requested review from TomFinley, abgoswam, sfilipi and artidoro January 5, 2019 00:38

Ivanidzo4ka commented Jan 5, 2019

View reviewed changes

TomFinley reviewed Jan 5, 2019

View reviewed changes

Transform ->Transformer

fe6b3c7

Ivan Matantsev added 9 commits January 4, 2019 17:35

merge with master

a0791bd

Transformer!

96d5578

fix loading order

09ecc19

merge with master

0adcfc7

last tests

21081be

and CSharpApi!

052d448

return back summary

3de085f

one more missing summary

f957aed

build

1157384

sfilipi reviewed Jan 8, 2019

View reviewed changes

sfilipi approved these changes Jan 8, 2019

View reviewed changes

TomFinley approved these changes Jan 9, 2019

View reviewed changes

Ivan Matantsev added 3 commits January 9, 2019 16:06

Merge branch 'master' into Ivanidze/hideCreateIDataTransform

13fc705

Address Senja comments in resizer transform

bdf61bd

Merge branch 'master' into Ivanidze/hideCreateIDataTransform

7b95004

Ivanidzo4ka merged commit 95f1530 into dotnet:master Jan 10, 2019

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


		namespace Microsoft.ML.OnnxTransform.StaticPipe
		namespace Microsoft.ML.Transforms.StaticPipe

Hide IDataTransform Create methods #2031

Hide IDataTransform Create methods #2031

Conversation

Ivanidzo4ka commented Jan 5, 2019

Ivanidzo4ka Jan 5, 2019 • edited by TomFinley 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

Choose a reason for hiding this comment

TomFinley Jan 5, 2019 • edited by Ivanidzo4ka 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 Jan 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

TomFinley Jan 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

TomFinley Jan 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

TomFinley Jan 5, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley commented Jan 5, 2019 • edited by Ivanidzo4ka Loading

Ivanidzo4ka commented Jan 5, 2019

sfilipi Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Jan 8, 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

Choose a reason for hiding this comment

sfilipi Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

sfilipi Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

sfilipi Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

sfilipi left a comment

Choose a reason for hiding this comment

TomFinley left a comment

Choose a reason for hiding this comment

Ivanidzo4ka Jan 5, 2019 •

edited by TomFinley

Loading

TomFinley Jan 5, 2019 •

edited by Ivanidzo4ka

Loading

TomFinley Jan 5, 2019 •

edited

Loading

TomFinley Jan 5, 2019 •

edited

Loading

TomFinley Jan 5, 2019 •

edited

Loading

TomFinley Jan 5, 2019 •

edited by Ivanidzo4ka

Loading

TomFinley commented Jan 5, 2019 •

edited by Ivanidzo4ka

Loading

sfilipi Jan 8, 2019 •

edited

Loading

sfilipi Jan 8, 2019 •

edited

Loading

sfilipi Jan 8, 2019 •

edited

Loading

sfilipi Jan 8, 2019 •

edited

Loading

sfilipi Jan 8, 2019 •

edited

Loading

sfilipi Jan 8, 2019 •

edited

Loading

sfilipi Jan 8, 2019 •

edited

Loading