TensorFlow estimator #840

Ivanidzo4ka · 2018-09-05T21:21:34Z

Converts TensorFlow transform to estimator/transformer + tests for them

Ivanidzo4ka · 2018-09-05T21:23:33Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

+        public static IRowMapper Create(IHostEnvironment env, ModelLoadContext ctx, ISchema inputSchema)
+            => Create(env, ctx).MakeRowMapper(inputSchema);
+
+        private TFSession LoadTFSession(byte[] modelBytes, string modelArg = null)


string modelArg = null [](start = 59, length = 22)

I couldn't find any usage of this, so I tempted to delete this #Resolved

Ivanidzo4ka · 2018-09-05T21:24:23Z

test/Microsoft.ML.Tests/CSharpCodeGen.cs

@@ -16,7 +16,7 @@ public CSharpCodeGen(ITestOutputHelper output) : base(output)
        {
        }

-        [Fact(Skip = "Execute this test if you want to regenerate CSharpApi file")]
+        [Fact]


] [](start = 13, length = 1)

revert #Resolved

Ivanidzo4ka · 2018-09-05T21:24:29Z

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

@@ -234,7 +234,7 @@ private string GetBuildPrefix()
 #endif
        }

-        [Fact(Skip = "Execute this test if you want to regenerate ep-list and _manifest.json")]
+        [Fact]


Fact [](start = 9, length = 4)

revert #Resolved

Ivanidzo4ka · 2018-09-05T21:26:02Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

+                var info = new RowMapperColumnInfo[_parent.Outputs.Length];
+                for (int i = 0; i < _parent.Outputs.Length; i++)
+                    info[i] = new RowMapperColumnInfo(_parent.Outputs[i], _parent.OutputTypes[i], null);
+                return info;
            }

            private static (string[], int[], bool[], TFShape[], TFDataType[]) GetInputMetaData(TFGraph graph, string[] source, ISchema inputSchema)


not needed anymore #Resolved

are we doing the shape checks somewhere ? e.g. lines 451-158

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

They all got moved to TensorFlowMapper constructor.

In reply to: 215438936 [](ancestors = 215438936,215430955)

zeahmed · 2018-09-05T23:28:00Z

I have bunch of changes in my PR, is it going to affect this PR? #Resolved

Ivanidzo4ka · 2018-09-06T01:10:12Z

They all part of this PR.

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

yaeldekel · 2018-09-06T17:59:41Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

            }

-            private Delegate MakeGetter<T>(IRow input, ColumnType columnType)
+            private Delegate[] MakeGetter(IRow input, Func<int, bool> activeOutput)


MakeGetter [](start = 31, length = 10)

nit: Change to MakeGetters. #Closed

sfilipi · 2018-09-06T18:03:58Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

+            return new TensorFlowTransform(env, File.ReadAllBytes(modelFile), source, names).MakeDataTransform(input);
+        }
+
+        // Factory method for SignatureLoadModel.


// Fact [](start = 8, length = 7)

xml style documentation, since it is a public method #ByDesign

OR we could make that stuff internal (and change ComponentCatalog to look for internal.

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

Maybe later though.

In reply to: 215728658 [](ancestors = 215728658,215723539)

It's public because of flaws in our dependency injection.

In reply to: 215728692 [](ancestors = 215728692,215728658,215723539)

Zruty0 · 2018-09-06T18:16:37Z

test/Microsoft.ML.Tests/TensorFlowEstimatorTests.cs

+            var invalidDataWrongNames = ComponentCreation.CreateDataView(Env, xyData);
+            var invalidDataWrongTypes = ComponentCreation.CreateDataView(Env, stringData);
+            var invalidDataWrongVectorSize = ComponentCreation.CreateDataView(Env, sizeData);
+            TestEstimatorCore(pipe, dataView, invalidInput: invalidDataWrongNames, validSchemaCantFitInput: invalidDataWrongVectorSize);


validSchemaCantFitInput [](start = 83, length = 23)

I think it's unique enough to not be in the TestEstimatorCore. You can just check that schema propagation works and Fit fails in this test. #Closed

yaeldekel · 2018-09-06T18:19:52Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

-            var mapper = new TensorFlowMapper(host, input.Schema, modelBytes, args.InputColumns, args.OutputColumn);
-            return new RowToRowMapperTransform(host, input, mapper);
+            _host = env.Register(nameof(TensorFlowEstimator));
+            _transformer = new TensorFlowTransform(env, File.ReadAllBytes(modelFile), inputs, outputs);


File [](start = 56, length = 4)

Should we check that the file exists? #Closed

Actually, the ctors for the trivial estimator should match the ctors for the transformer.

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

Zruty0 · 2018-09-06T18:20:21Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

+        {
+            Contracts.CheckValue(env, nameof(env));
+            env.CheckValue(ctx, nameof(ctx));
+            ctx.CheckAtModel(GetVersionInfo());


CheckAtModel [](start = 16, length = 12)

where's the binary format comment? #Closed

Zruty0 · 2018-09-06T18:21:39Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

-
-                ctx.SaveNonEmptyString(_outputColName);
-            }
+        private sealed class TensorFlowMapper : IRowMapper


TensorFlowMapper [](start = 29, length = 16)

maybe just Mapper to follow the pattern? #Closed

yaeldekel · 2018-09-06T18:22:08Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

-                return TFTensor.Create(_vBufferDense.Values, _tfShape);
-            }
+            for (var i = 0; i < _transformer.Outputs.Length; i++)
+                //IVAN: not sure about VectorKind.


VectorKind [](start = 39, length = 10)

For now it is Vector. There will soon be a PR for enabling VariableVector as well. #Resolved

Zruty0 · 2018-09-06T18:23:48Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

-        public const string UserName = "TensorFlowTransform";
-        public const string ShortName = "TFTransform";
-        private const string RegistrationName = "TensorFlowTransform";
+    public sealed class TensorFlowEstimator : IEstimator<TensorFlowTransform>


IEstimator [](start = 46, length = 10)

can we do TrivialEstimator ? #Closed

Zruty0 · 2018-09-06T18:25:23Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

+                    throw _host.ExceptSchemaMismatch(nameof(inputSchema), "input", input);
+                var tfInput = new TFOutput(_transformer.Graph[input]);
+                if (!TensorFlowUtils.IsTypeSupported(tfInput.OutputType))
+                    throw _host.Except($"Input type '{tfInput.OutputType}' of input column '{input}' is not supported in TensorFlow");


Should this be checked when the graph is loaded? It doesn't look like we need inputSchema for this check #Closed

Zruty0 · 2018-09-06T18:26:01Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

+                if (!TensorFlowUtils.IsTypeSupported(tfInput.OutputType))
+                    throw _host.Except($"Input type '{tfInput.OutputType}' of input column '{input}' is not supported in TensorFlow");
+                var tfShape = _transformer.Graph.GetTensorShape(tfInput);
+                var shape = tfShape.ToIntArray().Skip(tfShape[0] == -1 ? TensorFlowTransform.BatchSize : 0);


shape [](start = 20, length = 5)

is shape used? #Closed

Zruty0 · 2018-09-06T18:27:45Z

I sign off on the estimator conversion. I did not look at the multiple inputs change, so I'm not marking as approved.

Zruty0 · 2018-09-06T19:39:12Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

+                    throw _host.ExceptParam(nameof(outputs), $"Input column '{input}'  does not exist in the model");
+                var tfInput = new TFOutput(Session.Graph[input]);
+                if (!TensorFlowUtils.IsTypeSupported(tfInput.OutputType))
+                    throw _host.Except($"Input type '{tfInput.OutputType}' of input column '{input}' is not supported in TensorFlow");


Except [](start = 32, length = 6)

ExceptParam on modelStream #Closed

Zruty0 · 2018-09-06T19:39:58Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs


-            public TensorFlowMapper(IHostEnvironment env, ISchema inputSchema, byte[] modelBytes, string[] inputColNames, string outputColName)
+        private TensorFlowTransform(IHostEnvironment env, byte[] modelStream, string[] inputs, string[] outputs)


modelStream [](start = 65, length = 11)

nit: I would call that modelBytes, since it's not actually a stream #Closed

Ivanidzo4ka · 2018-09-06T19:46:00Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

+            {
+                _host.CheckNonWhiteSpace(input, nameof(inputs));
+                if (Session.Graph[input] == null)
+                    throw _host.ExceptParam(nameof(outputs), $"Input column '{input}'  does not exist in the model");


outputs [](start = 51, length = 7)

inputs #Resolved

Ivanidzo4ka · 2018-09-06T19:46:09Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

+            {
+                _host.CheckNonWhiteSpace(input, nameof(inputs));
+                if (Session.Graph[input] == null)
+                    throw _host.ExceptParam(nameof(outputs), $"Input column '{input}'  does not exist in the model");


[](start = 85, length = 2)

HORROR #Resolved

yaeldekel

sfilipi · 2018-09-06T21:04:27Z

src/Microsoft.ML.TensorFlow/doc.xml

-        The TensorflowTransform extracts the specified output from the operation computed on the graph (given the input(s)) using a pre-trained <a href="https://www.tensorflow.org">Tensorflow</a> model.
-        The transform takes as input the Tensorflow model together with the names of the inputs to the model and name of the operation for which output values will be extracted from the model.
+        The TensorflowTransform extracts the specified outputs from the operations computed on the graph (given the input(s)) using a pre-trained <a href="https://www.tensorflow.org">Tensorflow</a> model.
+        The transform takes as input the Tensorflow model together with the names of the inputs to the model and names of the operations for which output values will be extracted from the model.

        This transform requires the <a href="https://dotnet.myget.org/feed/dotnet-core/package/nuget/Microsoft.ML.TensorFlow/0.5.0-preview-26830-5">Microsoft.ML.TensorFlow</a> nuget to be installed.


This [](start = 8, length = 4)

if you want real paragraphs, i think you have to wrap the sentence in #Resolved

sfilipi · 2018-09-06T21:06:56Z

src/Microsoft.ML.TensorFlow/doc.xml


        This transform requires the <a href="https://dotnet.myget.org/feed/dotnet-core/package/nuget/Microsoft.ML.TensorFlow/0.5.0-preview-26830-5">Microsoft.ML.TensorFlow</a> nuget to be installed.
-		
+
        The TensorflowTransform has following assumptions regarding the input, output and processing of data.
        <list type="number">
          <item>


[](start = 10, length = 6)

not part of your PR, but i have had trouble with list items not displaying if not inside #Resolved

Zruty0

still need tests

36a6069

Ivanidzo4ka requested review from abgoswam, Zruty0, yaeldekel and zeahmed September 5, 2018 21:21

Ivanidzo4ka commented Sep 5, 2018

View reviewed changes

Ivan Matantsev added 3 commits September 5, 2018 17:08

get Zeeshan changes, add tests.

4e58354

some code cleanup

0501920

sync zeeshan changes

d8f5870

Ivanidzo4ka changed the title ~~WIP TensorFlow estimator~~ TensorFlow estimator Sep 6, 2018

Ivanidzo4ka self-assigned this Sep 6, 2018

Ivanidzo4ka added the API Issues pertaining the friendly API label Sep 6, 2018

Ivanidzo4ka added this to the 0918 milestone Sep 6, 2018

Zruty0 mentioned this pull request Sep 6, 2018

New API for ML.NET #754

Closed

change slash

b6465c3

yaeldekel reviewed Sep 6, 2018

View reviewed changes

sfilipi reviewed Sep 6, 2018

View reviewed changes

Zruty0 reviewed Sep 6, 2018

View reviewed changes

yaeldekel reviewed Sep 6, 2018

View reviewed changes

Zruty0 reviewed Sep 6, 2018

View reviewed changes

yaeldekel reviewed Sep 6, 2018

View reviewed changes

Zruty0 reviewed Sep 6, 2018

View reviewed changes

address comments

d66ddb6

Zruty0 reviewed Sep 6, 2018

View reviewed changes

more comments

26bcaf7

Ivanidzo4ka commented Sep 6, 2018

View reviewed changes

some more comments

7095798

yaeldekel approved these changes Sep 6, 2018

View reviewed changes

sfilipi reviewed Sep 6, 2018

View reviewed changes

merge with master

bcc2c95

Zruty0 approved these changes Sep 7, 2018

View reviewed changes

Ivanidzo4ka merged commit 44c6e90 into dotnet:master Sep 7, 2018

sfilipi mentioned this pull request Oct 10, 2018

API reference - Samples for Transforms #1209

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

TensorFlow estimator #840

TensorFlow estimator #840

Ivanidzo4ka commented Sep 5, 2018 •

edited

Loading

Ivanidzo4ka Sep 5, 2018 •

edited

Loading

Ivanidzo4ka Sep 5, 2018 •

edited

Loading

Ivanidzo4ka Sep 5, 2018 •

edited

Loading

Ivanidzo4ka Sep 5, 2018 •

edited

Loading

abgoswam Sep 5, 2018

Ivanidzo4ka Sep 5, 2018

zeahmed commented Sep 5, 2018 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka commented Sep 6, 2018

yaeldekel Sep 6, 2018 •

edited

Loading

sfilipi Sep 6, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Sep 6, 2018

Zruty0 Sep 6, 2018

Ivanidzo4ka Sep 6, 2018

Zruty0 Sep 6, 2018 •

edited

Loading

yaeldekel Sep 6, 2018 •

edited by Zruty0

Loading

Zruty0 Sep 6, 2018

Zruty0 Sep 6, 2018 •

edited

Loading

Zruty0 Sep 6, 2018 •

edited

Loading

yaeldekel Sep 6, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Sep 6, 2018 •

edited

Loading

Zruty0 Sep 6, 2018 •

edited

Loading

Zruty0 Sep 6, 2018 •

edited

Loading

Zruty0 commented Sep 6, 2018

Zruty0 Sep 6, 2018 •

edited

Loading

Zruty0 Sep 6, 2018 •

edited

Loading

Ivanidzo4ka Sep 6, 2018 •

edited

Loading

Ivanidzo4ka Sep 6, 2018 •

edited

Loading

yaeldekel left a comment

sfilipi Sep 6, 2018 •

edited by Ivanidzo4ka

Loading

sfilipi Sep 6, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 left a comment


		public TensorFlowMapper(IHostEnvironment env, ISchema inputSchema, byte[] modelBytes, string[] inputColNames, string outputColName)
		private TensorFlowTransform(IHostEnvironment env, byte[] modelStream, string[] inputs, string[] outputs)

TensorFlow estimator #840

TensorFlow estimator #840

Conversation

Ivanidzo4ka commented Sep 5, 2018 • edited Loading

Ivanidzo4ka Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

abgoswam Sep 5, 2018

Choose a reason for hiding this comment

Ivanidzo4ka Sep 5, 2018

Choose a reason for hiding this comment

zeahmed commented Sep 5, 2018 • edited by Ivanidzo4ka Loading

Ivanidzo4ka commented Sep 6, 2018

yaeldekel Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

sfilipi Sep 6, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Zruty0 Sep 6, 2018

Choose a reason for hiding this comment

Zruty0 Sep 6, 2018

Choose a reason for hiding this comment

Ivanidzo4ka Sep 6, 2018

Choose a reason for hiding this comment

Zruty0 Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

yaeldekel Sep 6, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Zruty0 Sep 6, 2018

Choose a reason for hiding this comment

Zruty0 Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

yaeldekel Sep 6, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Zruty0 Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 commented Sep 6, 2018

Zruty0 Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

yaeldekel left a comment

Choose a reason for hiding this comment

sfilipi Sep 6, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

sfilipi Sep 6, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Zruty0 left a comment

Choose a reason for hiding this comment

Ivanidzo4ka commented Sep 5, 2018 •

edited

Loading

Ivanidzo4ka Sep 5, 2018 •

edited

Loading

Ivanidzo4ka Sep 5, 2018 •

edited

Loading

Ivanidzo4ka Sep 5, 2018 •

edited

Loading

Ivanidzo4ka Sep 5, 2018 •

edited

Loading

zeahmed commented Sep 5, 2018 •

edited by Ivanidzo4ka

Loading

yaeldekel Sep 6, 2018 •

edited

Loading

sfilipi Sep 6, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Sep 6, 2018 •

edited

Loading

yaeldekel Sep 6, 2018 •

edited by Zruty0

Loading

Zruty0 Sep 6, 2018 •

edited

Loading

Zruty0 Sep 6, 2018 •

edited

Loading

yaeldekel Sep 6, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Sep 6, 2018 •

edited

Loading

Zruty0 Sep 6, 2018 •

edited

Loading

Zruty0 Sep 6, 2018 •

edited

Loading

Zruty0 Sep 6, 2018 •

edited

Loading

Zruty0 Sep 6, 2018 •

edited

Loading

Ivanidzo4ka Sep 6, 2018 •

edited

Loading

Ivanidzo4ka Sep 6, 2018 •

edited

Loading

sfilipi Sep 6, 2018 •

edited by Ivanidzo4ka

Loading

sfilipi Sep 6, 2018 •

edited by Ivanidzo4ka

Loading