Initial replacement of SubComponent with IComponentFactory #622

eerhardt · 2018-08-01T02:42:32Z

This is the first round of code changes necessary in order to remove SubComponent and instead use IComponentFactory.

This change removes SubComponent from the following public APIs:

CompositeDataLoader
- In order to completely remove it here, I needed to also remove it in some of the DataCommand classes as well - since they were calling methods on CompositeDataLoader that used SubComponent.
Ova and Pkpd
TermTransform

Working towards #585

And also a couple simple transforms.

…ands. This allows for CompositeDataLoader to no longer need to know about SubComponent.

TomFinley · 2018-08-01T18:44:52Z

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

-            [Argument(ArgumentType.Multiple, HelpText = "Transforms to apply prior to splitting the data into folds", ShortName = "prexf")]
-            public KeyValuePair<string, SubComponent<IDataTransform, SignatureDataTransform>>[] PreTransform;
+            [Argument(ArgumentType.Multiple, HelpText = "Transforms to apply prior to splitting the data into folds", ShortName = "prexf", SignatureType = typeof(SignatureDataTransform))]
+            public KeyValuePair<string, IComponentFactory<IDataView, IDataTransform>>[] PreTransform;


This is not necessarily how I'd prefer to do these things, for two reasons. Firstly, while IComponentFactory is an interface, usually when appearing in these arguments objects we actually are indicating a specific inheriting interface appropriate for this task... e.g., I would expect to see something like IDataTransformFactory.

Also: our goal would be to also get rid of this little "signature" concept.

Signature is used for two major purposes, which I list here and describe how I think they could be better accomplished.

It constrains the type based on the generic type (in the case where signatures are used. e.g., we might imagine an ITrainerFactory<IBinaryPredictor> in the pre-estimator world). This would I think suffice.

It indicates what parameters will be necessary to pass in to instantiate the object in the Create method or the constructor. However, in the case where the signature is indicating "globally appropriate" input (in this case, IDataTransform is always instantiated with the input data), it would be better for the subinterface to just have a method directly, without using all this reflection based stuff at all.

I would have an actual subinterface type (that is, IDataTransformFactory or something similarly named. Then this interface would itself have a method to create the transform, and that method would have at least two parameters (in the current world): the env and the input data.

Also: our goal would be to also get rid of this little "signature" concept.

I 100% agree. But it is too baked into the system right now to remove it in a single commit. Moving to IComponentFactory will lessen the dependence on the "signature" concept, but it will have to stay for a while until we can completely revamp the ComponentCatalog system.

usually when appearing in these arguments objects we actually are indicating a specific inheriting interface appropriate for this task... e.g., I would expect to see something like IDataTransformFactory.

I would have an actual subinterface type (that is, IDataTransformFactory or something similarly named. Then this interface would itself have a method to create the transform, and that method would have at least two parameters (in the current world): the env and the input data.

I'd need to think about this a bit more. My initial reaction is that this would cause the following disadvantages:

We would have a lot of different IComponentFactory interfaces instead of the 3 we have so far. As we added new types of components, we would constantly be needing to create new interfaces.

I think this is analogous with delegates in .NET 1.0 and 2.0. If you needed a delegate that took in an int and returned a bool, you needed to make a brand new delegate type. But in .NET 3.5, we added Action<> and Func<> delegates. And now you barely ever need to define custom delegate types. Instead you can just use Func<int, bool>.

We couldn't have the small set of SimpleComponentFactory classes that just takes a delegate to create the component. We would have to define a new class for each subinterface.

Similarly as (2) above, the CmdParser class would have to (1) know about (2) define a new class, and (3) create different objects for each of the subinterfaces the API defines.

(Obviously I know the command line isn't a high priority, or selling reason, but we still need to support it and other tooling scenarios) #Pending

/cc @Zruty0 - any thoughts here? According to history you were the original author #Resolved

I agree with @[email protected] 's reasoning. I think we could check it in as is.

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

Yea, I am OK with this solution.

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

Ivanidzo4ka · 2018-08-02T21:52:07Z

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

+                                {
+                                    Name ="Term",
+                                    Type = DataKind.TX,
+                                    KeyRange = new KeyRange() { Min = 0 }


KeyRange = new KeyRange() { Min = 0 } [](start = 36, length = 37)

It should be Source = new[] {new TextLoader.Range() { Min = 0 } }
instead of current line #Resolved

Ivanidzo4ka · 2018-08-02T22:00:51Z

            .Select(x => new KeyValuePair<string, string>(x.Key, x.Value.ToString()))

this can throw exception. #Resolved

Refers to: src/Microsoft.ML.Data/DataLoadSave/CompositeDataLoader.cs:136 in 05141a8. [](commit_id = 05141a8, deletion_comment = False)

Ivanidzo4ka · 2018-08-02T22:17:36Z

            .Select(x => new KeyValuePair<string, string>(x.Key, x.Value.ToString()))

so before I can had something like: <key:"", value:"ProduceId"> and value.ToString() return me "ProduceId", and now i'm not even sure what it contains.

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

Refers to: src/Microsoft.ML.Data/DataLoadSave/CompositeDataLoader.cs:136 in 05141a8. [](commit_id = 05141a8, deletion_comment = False)

eerhardt · 2018-08-03T22:36:18Z

            .Select(x => new KeyValuePair<string, string>(x.Key, x.Value.ToString()))

I don't understand the "this can throw exception" comment. Can you explain further?

As for the 2nd comment, I think I see what was happening here. Before, when it was a SubComponent, we would ToString() the SubComponent (which produces the command-line style "name + args" of the transform) and later write that string to the model.

However, here we are introducing a new scenario where you can new up any transform using this factory, and you may not have the "name + args" string available to write to the model. But when using the command-line (and GUI, etc) we still should be writing the string as before (which we can do from the CmdParser factory classes).

To solve this, I'll check for ICommandLineFactory interface . If the x.Value implements this interface, use its ToString(). If it doesn't, then just write null for the tag value, like we are doing here:

machinelearning/src/Microsoft.ML.Data/DataLoadSave/CompositeDataLoader.cs

Line 517 in 52cc874

.Select((xf, i) => new TransformEx(GenerateTag(i), null, xf))

I considered adding a new interface ICanProduceTagData, but I don't know of any usages of it, so for now I am not introducing it.

In reply to: 410085392 [](ancestors = 410085392,410081350)

Refers to: src/Microsoft.ML.Data/DataLoadSave/CompositeDataLoader.cs:136 in 05141a8. [](commit_id = 05141a8, deletion_comment = False)

Ivanidzo4ka · 2018-08-03T22:47:57Z

            .Select(x => new KeyValuePair<string, string>(x.Key, x.Value.ToString()))

Sorry for this cryptic messages, I just took your code, move it into internal repo, run bunch of tests, and notice what it fails, but since I had only one version of code, and I wasn't sure what happened originally, I just mark this place, revert code, and then tried to explain why it fails. Sometimes. As example it fails if in command line we call text loader, and then ProduceId transform with no arguments.

I think @Zruty0 is person who come up with this tags, so maybe he can elaborate is it still required, and why we accept nulls as tags. Or maybe we can use just use name of class instead of implementing interface

In reply to: 410393424 [](ancestors = 410393424,410085392,410081350)

Refers to: src/Microsoft.ML.Data/DataLoadSave/CompositeDataLoader.cs:136 in 05141a8. [](commit_id = 05141a8, deletion_comment = False)

Fix NRE. Use existing PlattCalibratorTrainerFactory.

Zruty0 · 2018-08-06T23:04:42Z

src/Microsoft.ML.Core/CommandLine/ArgumentAttribute.cs

@@ -139,5 +140,11 @@ public bool IsRequired
        {
            get { return ArgumentType.Required == (_type & ArgumentType.Required); }
        }
+
+        public Type SignatureType


SignatureType [](start = 20, length = 13)

Does it have to be here? Let me see why it would be needed #Closed

Ooh, I see now. You need to tell the arg parser what's the signature type to invoke the ctor properly.
Hmm, this is unfortunate. Are you sure there's no better way?

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

I haven't found one yet...

We could have some sort of mapping from IComponentFactory<...> to SignatureType. But that would seem fragile to me.

My other thinking (that seems to be aligned with @TomFinley's above) is that we should be moving away from SignatureTypes. This at least puts the Signature type into the attribute, and out of the API directly. Hopefully, eventually we can remove signature types all together. #Pending

Yea, the fact that it's prominent in the attribute makes the whole approach less desirable. But I think it's an OK intermediate step

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

Zruty0 · 2018-08-07T17:06:43Z

            .Select(x => new KeyValuePair<string, string>(x.Key, x.Value.ToString()))

Tags are still required, and we shouldn't have failing tests after this change. If the failing tests are in TLC and not ML.NET, it's not reasonable to expect Eric to write code that doesn't break it :)
I would still like to see the error occurrence to understand what's going on.

In reply to: 410395240 [](ancestors = 410395240,410393424,410085392,410081350)

Refers to: src/Microsoft.ML.Data/DataLoadSave/CompositeDataLoader.cs:136 in 05141a8. [](commit_id = 05141a8, deletion_comment = False)

Zruty0 · 2018-08-07T17:07:41Z

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

@@ -37,6 +37,21 @@ public interface IComponentFactory<in TArg1, out TComponent> : IComponentFactory
        TComponent CreateComponent(IHostEnvironment env, TArg1 argument1);
    }

+    public class SimpleComponentFactory<TArg1, TComponent> : IComponentFactory<TArg1, TComponent>


please add a summary comment to public class #Closed

Done.

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

Zruty0

🕐

Zruty0

eerhardt · 2018-08-07T22:37:21Z

            .Select(x => new KeyValuePair<string, string>(x.Key, x.Value.ToString()))

I've fixed the NullReferenceException (NRE) in the latest commits. It was caused by the ComponentFactory.ToString() method I added to CmdParser.cs.

In reply to: 411130583 [](ancestors = 411130583,410395240,410393424,410085392,410081350)

Refers to: src/Microsoft.ML.Data/DataLoadSave/CompositeDataLoader.cs:136 in 05141a8. [](commit_id = 05141a8, deletion_comment = False)

Zruty0 · 2018-08-08T15:06:56Z

@codemzs @Ivanidzo4ka please take a look as well.

Ivanidzo4ka · 2018-08-08T17:41:27Z

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

@@ -451,8 +451,8 @@ public sealed class TermLoaderArguments
        [Argument(ArgumentType.AtMostOnce, IsInputFileName = true, HelpText = "Data file containing the terms", ShortName = "data", SortOrder = 2, Visibility = ArgumentAttribute.VisibilityType.CmdLineOnly)]
        public string DataFile;

-        [Argument(ArgumentType.Multiple, HelpText = "Data loader", NullName = "<Auto>", SortOrder = 3, Visibility = ArgumentAttribute.VisibilityType.CmdLineOnly)]
-        public SubComponent<IDataLoader, SignatureDataLoader> Loader;
+        [Argument(ArgumentType.Multiple, HelpText = "Data loader", NullName = "<Auto>", SortOrder = 3, Visibility = ArgumentAttribute.VisibilityType.CmdLineOnly, SignatureType = typeof(SignatureDataLoader))]


Visibility = ArgumentAttribute.VisibilityType.CmdLineOnly [](start = 103, length = 57)

unrelated to your PR, but we need to look at some point on this visibility. It's CmdLineOnly now because Subcomponent can't be exposed to EntryPoints, but since you moving to IComponentFactory, maybe we should remove this visibility restriction.

Ivanidzo4ka · 2018-08-08T17:41:48Z

src/Microsoft.ML.StandardLearners/Standard/MultiClass/MetaMulticlassTrainer.cs

@@ -10,6 +10,7 @@
 using Microsoft.ML.Runtime.Internal.Calibration;
 using Microsoft.ML.Runtime.Internal.Internallearn;
 using Microsoft.ML.Runtime.Training;
+using Microsoft.ML.Runtime.EntryPoints;



nit: sorting

Ivanidzo4ka

Ivanidzo4ka · 2018-08-08T23:37:39Z

src/Microsoft.ML.StandardLearners/Standard/MultiClass/MetaMulticlassTrainer.cs

            [TGUI(Label = "Predictor Type", Description = "Type of underlying binary predictor")]
-            public SubComponent<TScalarTrainer, SignatureBinaryClassifierTrainer> PredictorType =
-                new SubComponent<TScalarTrainer, SignatureBinaryClassifierTrainer>(LinearSvm.LoadNameValue);
+            public IComponentFactory<TScalarTrainer> PredictorType;


PredictorType [](start = 53, length = 13)

This is force us to go and put ITrainer<IPredictorProducing> to all our binary learners.
For example FastTree is just ITrainer in current moment.
Which I think is a good thing, but we need to do it, overwise we can't use OVA in same way as before this change.

Actually, I agree it's a good thing that we need to make our trainers produce IPredictorProducing

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

Did an internal test catch this? Or did you find it some other way?

All internal tests use old way of instantiating data, via string :)
I've tried to write OVA example with new changes, and found I can't pass anything here, because nothing satisfy to this interface.

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

That was the idea with the SimpleComponentFactory class(es):

using (var env = new TlcEnvironment()) { var ovaArgs = new Ova.Arguments(); ovaArgs.PredictorType = new SimpleComponentFactory<ITrainer<IPredictorProducing<float>>>( (e) => new FastTreeBinaryClassificationTrainer(e, new FastTreeBinaryClassificationTrainer.Arguments())); var ova = new Ova(env, ovaArgs); }

can you put this code into setter for PredictorType?
public IComponentFactory PredictorType = new SimpleComponentFactory<ITrainer<IPredictorProducing>>((e)=> new LinearSvm(e, new LinearSvm.Arguments())));

So we will have example of how it's done.

Also I personally prefer to throw exception if user set PredictorType to null, rather than falling to default value.

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

The reason I did it the way I did was because it skips using the Factory all together in the default case. No need to create an extra factory object or a delegate - if the user didn't provide a PredictorType, we just use LinearSvm.

It also solves the case where the user set the field to null, as an added bonus. There's no reason to throw an exception in this case. We have a default we can use if the user doesn't give us one.

eerhardt requested review from codemzs, Ivanidzo4ka, TomFinley and Zruty0 August 1, 2018 02:42

eerhardt added 5 commits August 1, 2018 11:50

Initial change to replace SubComponent with IComponentFactory.

cf7207f

Replace SubComponent with IComponentFactory in DataCommand.

0dec10c

And also a couple simple transforms.

Replace SubComponent with IComponentFactory in CompositeDataLoader

a0a5da2

Remove SubComponent from pre and post transforms in Score and CV comm…

d702353

…ands. This allows for CompositeDataLoader to no longer need to know about SubComponent.

Clean up code in preparation for PR.

05141a8

eerhardt force-pushed the SubComponent branch from e8f61dc to 05141a8 Compare August 1, 2018 16:52

TomFinley reviewed Aug 1, 2018

View reviewed changes

Ivanidzo4ka reviewed Aug 2, 2018

View reviewed changes

PR feedback

5fa843e

eerhardt added 2 commits August 3, 2018 17:54

Respond to PR feedback.

12f1798

Minor cleanup

69c55c9

Fix NRE. Use existing PlattCalibratorTrainerFactory.

Zruty0 reviewed Aug 6, 2018

View reviewed changes

Zruty0 reviewed Aug 7, 2018

View reviewed changes

Zruty0 suggested changes Aug 7, 2018

View reviewed changes

Zruty0 approved these changes Aug 7, 2018

View reviewed changes

PR feedback

4cd6a29

Ivanidzo4ka reviewed Aug 8, 2018

View reviewed changes

Ivanidzo4ka approved these changes Aug 8, 2018

View reviewed changes

eerhardt merged commit 1b2f181 into dotnet:master Aug 8, 2018

Ivanidzo4ka reviewed Aug 8, 2018

View reviewed changes

Ivanidzo4ka mentioned this pull request Aug 8, 2018

Binary learners need to have IComponentFactory implementation in order to work with OVA. #669

Closed

eerhardt mentioned this pull request Aug 9, 2018

WIP Introduce I*PredictionKind*TrainerFactory and propagate them #670

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial replacement of SubComponent with IComponentFactory #622

Initial replacement of SubComponent with IComponentFactory #622

eerhardt commented Aug 1, 2018

TomFinley Aug 1, 2018

eerhardt Aug 1, 2018 •

edited by Zruty0

Loading

eerhardt Aug 1, 2018 •

edited by Zruty0

Loading

Zruty0 Aug 6, 2018

Zruty0 Aug 6, 2018

Ivanidzo4ka Aug 2, 2018 •

edited by eerhardt

Loading

Ivanidzo4ka commented Aug 2, 2018 •

edited by eerhardt

Loading

Ivanidzo4ka commented Aug 2, 2018 •

edited

Loading

eerhardt commented Aug 3, 2018 •

edited

Loading

Ivanidzo4ka commented Aug 3, 2018 •

edited

Loading

Zruty0 Aug 6, 2018 •

edited

Loading

Zruty0 Aug 6, 2018

eerhardt Aug 6, 2018 •

edited by Zruty0

Loading

Zruty0 Aug 7, 2018

Zruty0 commented Aug 7, 2018

Zruty0 Aug 7, 2018 •

edited

Loading

eerhardt Aug 7, 2018

Zruty0 left a comment

Zruty0 left a comment

eerhardt commented Aug 7, 2018

Zruty0 commented Aug 8, 2018

Ivanidzo4ka Aug 8, 2018

Ivanidzo4ka Aug 8, 2018

Ivanidzo4ka left a comment

Ivanidzo4ka Aug 8, 2018

Zruty0 Aug 8, 2018

eerhardt Aug 9, 2018

Ivanidzo4ka Aug 9, 2018

eerhardt Aug 9, 2018 •

edited

Loading

Ivanidzo4ka Aug 9, 2018

eerhardt Aug 9, 2018

Initial replacement of SubComponent with IComponentFactory #622

Initial replacement of SubComponent with IComponentFactory #622

Conversation

eerhardt commented Aug 1, 2018

Choose a reason for hiding this comment

eerhardt Aug 1, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

eerhardt Aug 1, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Aug 2, 2018 • edited by eerhardt Loading

Choose a reason for hiding this comment

Ivanidzo4ka commented Aug 2, 2018 • edited by eerhardt Loading

Ivanidzo4ka commented Aug 2, 2018 • edited Loading

eerhardt commented Aug 3, 2018 • edited Loading

Ivanidzo4ka commented Aug 3, 2018 • edited Loading

Zruty0 Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Aug 6, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 commented Aug 7, 2018

Zruty0 Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 left a comment

Choose a reason for hiding this comment

Zruty0 left a comment

Choose a reason for hiding this comment

eerhardt commented Aug 7, 2018

Zruty0 commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka left a 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

eerhardt Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Aug 1, 2018 •

edited by Zruty0

Loading

eerhardt Aug 1, 2018 •

edited by Zruty0

Loading

Ivanidzo4ka Aug 2, 2018 •

edited by eerhardt

Loading

Ivanidzo4ka commented Aug 2, 2018 •

edited by eerhardt

Loading

Ivanidzo4ka commented Aug 2, 2018 •

edited

Loading

eerhardt commented Aug 3, 2018 •

edited

Loading

Ivanidzo4ka commented Aug 3, 2018 •

edited

Loading

Zruty0 Aug 6, 2018 •

edited

Loading

eerhardt Aug 6, 2018 •

edited by Zruty0

Loading

Zruty0 Aug 7, 2018 •

edited

Loading

eerhardt Aug 9, 2018 •

edited

Loading