Enabled option to get multiple outputs from TF graphs #814

zeahmed · 2018-09-04T22:44:22Z

This PR fixes #712.

Update Fork...

…_output

… graph.

…_output

…form.

zeahmed · 2018-09-04T22:45:17Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

+            private readonly string[] _outputColNames;
+            private readonly ColumnType[] _outputColTypes;
+            private readonly TFDataType[] _tfOutputTypes;
+            private IDictionary<string, TFTensor> _cachedOutputs;


@yaeldekel, is it the right place to cache data here? #Resolved

The dictionary holding the cached outputs needs to be instantiated on each call to CreateGetters.

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

cachedPosition as well.

In reply to: 215387466 [](ancestors = 215387466,215089636)

Ivanidzo4ka · 2018-09-05T17:43:52Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

@@ -326,7 +384,7 @@ public static IDataTransform Create(IHostEnvironment env, Arguments args, IDataV
            host.CheckUserArg(File.Exists(args.ModelFile), nameof(args.ModelFile));

            var modelBytes = File.ReadAllBytes(args.ModelFile);
-            var mapper = new TensorFlowMapper(host, input.Schema, modelBytes, args.InputColumns, args.OutputColumn);
+            var mapper = new TensorFlowMapper(host, input.Schema, modelBytes, args.InputColumns, args.OutputColumns);


OutputColumns [](start = 102, length = 13)

Do you want to validate uniqueness of output columns? #Resolved

yaeldekel · 2018-09-05T18:48:40Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

                _host.Check(inputColNames.All(name => _session.Graph[name] != null), "One of the input does not exist in the model");
+                _host.Check(outputColNames.All(name => _session.Graph[name] != null), "One of the output does not exist in the model");


[name] [](start = 69, length = 6)

Could this throw if "name" is null? If it does, then we need to add a check of each item in outputColNames, in addition to the CheckNonEmpty on the array itself. #Closed

yaeldekel · 2018-09-05T18:49:45Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

+                Contracts.AssertNonEmpty(_outputColNames);
+                ctx.Writer.Write(_outputColNames.Length);
+                foreach (var colName in _outputColNames)
+                    ctx.SaveNonEmptyString(colName);


We need to update the version. #Closed

yaeldekel · 2018-09-05T18:50:25Z

            Contracts.AssertNonEmpty(_inputColNames);

_host. #Closed

Refers to: src/Microsoft.ML.TensorFlow/TensorflowTransform.cs:125 in b57a5c9. [](commit_id = b57a5c9, deletion_comment = False)

yaeldekel · 2018-09-05T18:51:21Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

@@ -116,7 +127,10 @@ public void Save(ModelSaveContext ctx)
                foreach (var colName in _inputColNames)
                    ctx.SaveNonEmptyString(colName);

-                ctx.SaveNonEmptyString(_outputColName);
+                Contracts.AssertNonEmpty(_outputColNames);


Contracts [](start = 16, length = 9)

_host. #Closed

yaeldekel · 2018-09-05T18:56:42Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

            }

-            private static (ColumnType, TFDataType) GetOutputTypes(TFGraph graph, string columnName)


static [](start = 20, length = 6)

Can this still be static? #Resolved

I am using the _session.Graph[name] in this method.

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

You can use the graph that is passed as an argument instead.

In reply to: 215390484 [](ancestors = 215390484,215388432)

yaeldekel · 2018-09-05T18:59:18Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

-                return new[] { new RowMapperColumnInfo(_outputColName, _outputColType, null) };
+                var info = new RowMapperColumnInfo[_outputColNames.Length];
+                for (int i = 0; i < _outputColNames.Length; i++)
+                {


{ [](start = 16, length = 1)

nit: don't need the curly braces. #Closed

yaeldekel · 2018-09-05T19:01:37Z

            return col => activeOutput(0) && _inputColIndices.Any(i => i == col);

This should be updated to reflect that if any of the output columns are active, then all the inputs need to active. #Closed

Refers to: src/Microsoft.ML.TensorFlow/TensorflowTransform.cs:251 in b57a5c9. [](commit_id = b57a5c9, deletion_comment = False)

yaeldekel · 2018-09-05T19:05:01Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

@@ -187,28 +217,30 @@ private Delegate MakeGetter<T>(IRow input, ColumnType columnType)
                        runner.AddInput(inputName, srcTensorGetters[i].GetTensor());
                    }

-                    var tensors = runner.Fetch(_outputColName).Run();
-
+                    var tensors = runner.Fetch(_outputColNames).Run();


_outputColNames [](start = 47, length = 15)

Does it make sense to create an array that only has the active outputs so that we don't need to compute and cache all of them if only some are active? #Closed

This is basically an array passed by the user. I think any output column requested by the user must be active. Do you think a scenario where user specified column wont be active?

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

yaeldekel · 2018-09-05T19:11:28Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

+                        values = new T[_outputColTypes[iinfo].VectorSize];
+
+                    TensorFlowUtils.FetchData<T>(_cachedOutputs[_outputColNames[iinfo]].Data, values);
+                    dst = new VBuffer<T>(values.Length, values);


); [](start = 62, length = 2)

Pass dst.Indices here too. #Closed

abgoswam · 2018-09-05T19:13:10Z

src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

+            private readonly ColumnType[] _outputColTypes;
+            private readonly TFDataType[] _tfOutputTypes;
+            private IDictionary<string, TFTensor> _cachedOutputs;
+            private long _cachedPosition;


question: what do the _cachedOutputs and _cachedPosition represent ?

Are they only needed for the multiple inputs/outputs scenario ? #Closed

Actually, output getters work independent of each other and in each getter we have call to compute operations on TF graph.

What we are doing here is for each row in the input dataview, cache the results of all the requested operations on the first output getter call. Then for subsequent output getter calls for that row, serve the results from this _cachedOutputs dictionary.

If the row position is changed the computation is done again and the current position is saved in cachedPosition.

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

zeahmed · 2018-09-05T20:14:39Z

@dotnet-bot, test it please. #Closed

abgoswam · 2018-09-05T20:33:26Z

// The .NET Foundation licenses this file to you under the MIT license.

Should we add a simple test showcasing the multiple input / multiple output scenario -- perhaps just taking 2 numbers as inputs and producing their {sum, product} as outputs ? #Closed

Refers to: test/Microsoft.ML.Tests/ScenariosWithDirectInstantiation/TensorflowTests.cs:2 in b57a5c9. [](commit_id = b57a5c9, deletion_comment = False)

zeahmed · 2018-09-05T22:37:29Z

// The .NET Foundation licenses this file to you under the MIT license.

I have already changed the test TensorFlowTransformMNISTConvTest at line 74 on this file that shows how to use TensorflowTransform for multiple outputs. Let me know if you are thinking of different scenario?

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

Refers to: test/Microsoft.ML.Tests/ScenariosWithDirectInstantiation/TensorflowTests.cs:2 in b57a5c9. [](commit_id = b57a5c9, deletion_comment = False)

yaeldekel

zeahmed · 2018-09-06T21:16:31Z

// The .NET Foundation licenses this file to you under the MIT license.

I am planning to do it in next PR because it will cause the model nuget to be updated. I have created an issue for this #850

In reply to: 418903839 [](ancestors = 418903839,418871731)

Refers to: test/Microsoft.ML.Tests/ScenariosWithDirectInstantiation/TensorflowTests.cs:2 in b57a5c9. [](commit_id = b57a5c9, deletion_comment = False)

abgoswam

Ivanidzo4ka

zeahmed and others added 18 commits May 23, 2018 15:03

Merge pull request #1 from dotnet/master

21d2b48

Update Fork...

Merge remote-tracking branch 'upstream/master'

405ffcf

Merge remote-tracking branch 'upstream/master'

966b091

Merge remote-tracking branch 'upstream/master'

0a8ec57

Merge remote-tracking branch 'upstream/master'

a5e9a1d

Merge remote-tracking branch 'upstream/master'

37959d0

Merge remote-tracking branch 'upstream/master'

fba10a5

Merge remote-tracking branch 'upstream/master'

bb1e63d

Merge remote-tracking branch 'upstream/master'

740ad4b

Merge remote-tracking branch 'upstream/master'

9f5c45d

Merge remote-tracking branch 'upstream/master'

4bbf76a

Merge remote-tracking branch 'upstream/master'

d6248be

Enabled option to get multiple outputs from TF graphs

55683f7

Merge remote-tracking branch 'upstream/master' into TFTransform_multi…

1826769

…_output

Updated CSharpAPI and relevants tests to reflect multi-ouputs from TF…

2ad6470

… graph.

Merge remote-tracking branch 'upstream/master' into TFTransform_multi…

ca7f38c

…_output

Merge remote-tracking branch 'upstream/master' into TFTransform_multi…

e2e0e14

…_output

Updated cache position to -1 when creating getters in TensorflowTrans…

5316655

…form.

zeahmed requested review from abgoswam, ericstj, yaeldekel and Zruty0 September 4, 2018 22:44

zeahmed commented Sep 4, 2018

View reviewed changes

Updated doc.xml to reflect the multi-outputs.

b57a5c9

Ivanidzo4ka reviewed Sep 5, 2018

View reviewed changes

yaeldekel reviewed Sep 5, 2018

View reviewed changes

abgoswam reviewed Sep 5, 2018

View reviewed changes

Addressed reviewers' comments.

76ea346

zeahmed mentioned this pull request Sep 5, 2018

TensorFlow estimator #840

Merged

yaeldekel approved these changes Sep 6, 2018

View reviewed changes

Addressed reviewers' comments.

23677c8

abgoswam approved these changes Sep 6, 2018

View reviewed changes

Ivanidzo4ka approved these changes Sep 6, 2018

View reviewed changes

zeahmed merged commit c502615 into dotnet:master Sep 6, 2018

zeahmed deleted the TFTransform_multi_output branch January 30, 2019 21:30

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

Enabled option to get multiple outputs from TF graphs #814

Enabled option to get multiple outputs from TF graphs #814

zeahmed commented Sep 4, 2018

zeahmed Sep 4, 2018 •

edited

Loading

yaeldekel Sep 5, 2018

yaeldekel Sep 5, 2018

Ivanidzo4ka Sep 5, 2018 •

edited by zeahmed

Loading

yaeldekel Sep 5, 2018 •

edited

Loading

yaeldekel Sep 5, 2018 •

edited

Loading

yaeldekel commented Sep 5, 2018 •

edited

Loading

yaeldekel Sep 5, 2018 •

edited

Loading

yaeldekel Sep 5, 2018 •

edited by zeahmed

Loading

zeahmed Sep 5, 2018

yaeldekel Sep 6, 2018

yaeldekel Sep 5, 2018 •

edited

Loading

yaeldekel commented Sep 5, 2018 •

edited

Loading

yaeldekel Sep 5, 2018 •

edited

Loading

zeahmed Sep 5, 2018

yaeldekel Sep 5, 2018 •

edited

Loading

abgoswam Sep 5, 2018 •

edited

Loading

zeahmed Sep 5, 2018

zeahmed commented Sep 5, 2018 •

edited

Loading

abgoswam commented Sep 5, 2018 •

edited

Loading

zeahmed commented Sep 5, 2018 •

edited

Loading

yaeldekel left a comment

zeahmed commented Sep 6, 2018

abgoswam left a comment

Ivanidzo4ka left a comment

		_host.Check(inputColNames.All(name => _session.Graph[name] != null), "One of the input does not exist in the model");
		_host.Check(outputColNames.All(name => _session.Graph[name] != null), "One of the output does not exist in the model");

		}

		private static (ColumnType, TFDataType) GetOutputTypes(TFGraph graph, string columnName)

Enabled option to get multiple outputs from TF graphs #814

Enabled option to get multiple outputs from TF graphs #814

Conversation

zeahmed commented Sep 4, 2018

zeahmed Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

yaeldekel Sep 5, 2018

Choose a reason for hiding this comment

yaeldekel Sep 5, 2018

Choose a reason for hiding this comment

Ivanidzo4ka Sep 5, 2018 • edited by zeahmed Loading

Choose a reason for hiding this comment

yaeldekel Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

yaeldekel Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

yaeldekel commented Sep 5, 2018 • edited Loading

yaeldekel Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

yaeldekel Sep 5, 2018 • edited by zeahmed Loading

Choose a reason for hiding this comment

zeahmed Sep 5, 2018

Choose a reason for hiding this comment

yaeldekel Sep 6, 2018

Choose a reason for hiding this comment

yaeldekel Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

yaeldekel commented Sep 5, 2018 • edited Loading

yaeldekel Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

zeahmed Sep 5, 2018

Choose a reason for hiding this comment

yaeldekel Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

abgoswam Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

zeahmed Sep 5, 2018

Choose a reason for hiding this comment

zeahmed commented Sep 5, 2018 • edited Loading

abgoswam commented Sep 5, 2018 • edited Loading

zeahmed commented Sep 5, 2018 • edited Loading

yaeldekel left a comment

Choose a reason for hiding this comment

zeahmed commented Sep 6, 2018

abgoswam left a comment

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

zeahmed Sep 4, 2018 •

edited

Loading

Ivanidzo4ka Sep 5, 2018 •

edited by zeahmed

Loading

yaeldekel Sep 5, 2018 •

edited

Loading

yaeldekel Sep 5, 2018 •

edited

Loading

yaeldekel commented Sep 5, 2018 •

edited

Loading

yaeldekel Sep 5, 2018 •

edited

Loading

yaeldekel Sep 5, 2018 •

edited by zeahmed

Loading

yaeldekel Sep 5, 2018 •

edited

Loading

yaeldekel commented Sep 5, 2018 •

edited

Loading

yaeldekel Sep 5, 2018 •

edited

Loading

yaeldekel Sep 5, 2018 •

edited

Loading

abgoswam Sep 5, 2018 •

edited

Loading

zeahmed commented Sep 5, 2018 •

edited

Loading

abgoswam commented Sep 5, 2018 •

edited

Loading

zeahmed commented Sep 5, 2018 •

edited

Loading