Multiple inputs output support for OnnxTransform. #1586

jignparm · 2018-11-09T07:07:44Z

Fixes issue #1585.

This PR is to add support for Onnx models that have multiple input/outputs. The current version of the transform allows for only single input and single output.

shmoradims · 2018-11-12T22:25:07Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

+            bool isMultiOutput = ctx.Header.ModelVerReadable > 0x00010001;
+
+            //var inputColumn = ctx.LoadNonEmptyString();
+            //var outputColumn = ctx.LoadNonEmptyString();


delete #Closed

done

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

shmoradims · 2018-11-12T22:26:08Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

+            //    throw env.Except($"OnnxTransform supports Onnx models with one input. The provided model has ${modelInfo.InputsInfo.Length} input(s).");
+            //if (modelInfo.OutputsInfo.Length != 1)
+            //    throw env.Except($"OnnxTransform supports Onnx models with one output. The provided model has ${modelInfo.OutputsInfo.Length} output(s).");
+


delete #Closed

done.

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

shmoradims · 2018-11-12T22:33:23Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

+                        break;
+                    }
+                if (idx < 0)
+                    throw _host.Except($"Column {args.OutputColumns[i]} doesn't match output node names of model");


I would make OnnxModel._outputNames public and simplify code to:
if (!Model.OutputNames.Contains(args.OutputColumns[i])
throw ... #Closed

simplified .

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

why not use Contains() instead of IndexOf?

In reply to: 234085225 [](ancestors = 234085225,232835603)

The line of code just below uses the value -- we need to know the index since the user specified inputs might not be in the same order as Model.ModelInfo.OutputsInfo.

In reply to: 234305001 [](ancestors = 234305001,234085225,232835603)

shmoradims · 2018-11-12T22:38:39Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

+                    throw _host.Except($"Column {args.OutputColumns[i]} doesn't match output node names of model");
+                var outputNodeInfo = Model.ModelInfo.OutputsInfo[idx];
+                var shape = outputNodeInfo.Shape;
+                var dims = shape.Count > 0 ? shape.Skip(shape[0] < 0 ? 1 : 0).Select(x => (int)x).ToArray() : new[] { 0 };


var dims = shape.Count > 0 ? shape.Skip(shape[0] < 0 ? 1 : 0).Select(x => (int)x).ToArray() : new[] { 0 }; [](start = 16, length = 106)

This is too dense for me to follow. Please unpack the code into different cases and add comments about each case (meaning of negative count, why we skip, why skip 0 or 1, etc).

I've seen this later in the code to. If makes sense, please refactor to separate method. #Closed

refactored

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

shmoradims · 2018-11-12T22:40:51Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

-            ctx.SaveNonEmptyString(_args.InputColumn);
-            ctx.SaveNonEmptyString(_args.OutputColumn);
+            //ctx.SaveNonEmptyString(_args.InputColumn);
+            //ctx.SaveNonEmptyString(_args.OutputColumn);


delete #Closed

done

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

shmoradims · 2018-11-12T22:42:14Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

+                        if (model.ModelInfo.InputsInfo[j].Name == _parent.Inputs[i])
+                        {
+                            idx = j;
+                            break;


For inputs, you can do the same simplification you did with outputs. #Closed

done

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

same here. you can use Contains(), which doesn't involve dealing with indices.

In reply to: 234080355 [](ancestors = 234080355,232837640)

we need idexof -- resolved.

In reply to: 234307741 [](ancestors = 234307741,234080355,232837640)

shmoradims · 2018-11-12T22:43:12Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

+                    var inputNodeInfo = model.ModelInfo.InputsInfo[idx];
+
+                    var shape = inputNodeInfo.Shape;
+                    int[] inputdims = shape.Count > 0 ? shape.Skip(shape[0] < 0 ? 1 : 0).Select(x => (int)x).ToArray() : new[] { 0 };


t[] inputdims = shape.Count > 0 ? shape.Skip(shape[0] < 0 ? 1 : 0).Select(x => (int)x).ToArray() : new[] { 0 }; [](start = 22, length = 111)

Is this the same logic as before? Please refactor to separate method if so. #Closed

removed this logic as well as variable -- not needed here due to shape calculation below. The other one was to set the output dimension of vector type column.

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

shmoradims · 2018-11-12T22:47:03Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

+                _inputColIndices = new int[_parent.Inputs.Length];
+                _isInputVector = new bool[_parent.Inputs.Length];
+                _inputTensorShapes = new OnnxShape[_parent.Inputs.Length];
+                _inputOnnxTypes = new DataType[_parent.Inputs.Length];


does it make sense to bundle all these into a InputMetadata class and just have one array of that class instead of multiple arrays? #WontFix

This is a matter of readability -- bundling will introduce another data structure. Also, this pattern is what's there in the TF transform -- later on, if we need to consolidate into a single transform which does inferencing of all DNN models, it would be beneficial to keep it this way.

Example here :

machinelearning/src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

Line 810 in cb9effc

private readonly int[] _inputColIndices;

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

parallel arrays are considered an anti-pattern in OOD. Unfortunately, they're used often in our codebase. So, up to you if you want to keep it as is or refactor.

https://codeblog.jonskeet.uk/2014/06/03/anti-pattern-parallel-collections/

In reply to: 234077070 [](ancestors = 234077070,232838745)

shmoradims · 2018-11-12T22:54:06Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

+                        idx = j;
+                        break;
+                    }
+                if (idx < 0)


Same simplification as before #Closed

done

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

shmoradims · 2018-11-12T22:55:57Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

            {
                Contracts.Assert(toOutput.Length == 1);

                var outCol = (OutColumn)toOutput[0];


var outCol = (OutColumn)toOutput[0]; [](start = 16, length = 36)

Should we extend this also? It's only using 1 output. #Closed

TF seems to have the same reconciler logic. It works fine as-is, although we should double check it.

machinelearning/src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

Line 1237 in cb9effc

var outCol = (OutColumn)toOutput[0];

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

shmoradims · 2018-11-12T23:03:30Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

-            var inputColumn = ctx.LoadNonEmptyString();
-            var outputColumn = ctx.LoadNonEmptyString();
-            var args = new Arguments() { InputColumn = inputColumn, OutputColumn = outputColumn };
+            bool isMultiOutput = ctx.Header.ModelVerReadable > 0x00010001;


ModelVerReadable [](start = 44, length = 16)

Should be ModelVerWritten I think #Closed

Good catch. That sounds more correct -- updated. TF probably needs updating as well.

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

shmoradims · 2018-11-12T23:05:23Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

+
+            var numInputs = 1;
+            if (isMultiOutput)
+                numInputs = ctx.Reader.ReadInt32();


var numInputs = isMultiOutput ? ctx.Reader.ReadInt32() : 1; #Closed

condensed

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

shmoradims · 2018-11-12T23:09:24Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

-            var inputColumn = ctx.LoadNonEmptyString();
-            var outputColumn = ctx.LoadNonEmptyString();
-            var args = new Arguments() { InputColumn = inputColumn, OutputColumn = outputColumn };
+            bool isMultiOutput = ctx.Header.ModelVerReadable > 0x00010001;


isMultiOutput [](start = 17, length = 13)

supportsMultiInputOutput? #Closed

done

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

Ivanidzo4ka · 2018-11-13T23:48:19Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

+            //ctx.SaveNonEmptyString(_args.InputColumn);
+            //ctx.SaveNonEmptyString(_args.OutputColumn);
+
+            _host.AssertNonEmpty(Inputs);


AssertNonEmpty [](start = 18, length = 14)

CheckNonEmpty #Closed

done

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

Ivanidzo4ka · 2018-11-13T23:49:11Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

+            foreach (var colName in Inputs)
+                ctx.SaveNonEmptyString(colName);
+
+            _host.AssertNonEmpty(Outputs);


AssertNonEmpty [](start = 18, length = 14)

CheckNonEmpty, public method should use Check instead of assert #Closed

good catch

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

Ivanidzo4ka · 2018-11-13T23:49:28Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

-                verWrittenCur: 0x00010001, // Initial
-                verReadableCur: 0x00010001,
+                verWrittenCur: 0x00010002, // Initial
+                verReadableCur: 0x00010002,


verReadableCur: 0x00010002, [](start = 16, length = 27)

would be nice to add comment what exactly changed, like // Inputs and Outputs columns now array of strings. #Closed

done

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

Ivanidzo4ka · 2018-11-13T23:49:53Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

        }

-        public static IDataTransform Create(IHostEnvironment env, IDataView input, string modelFile, string inputColumn, string outputColumn)
+        public static IDataTransform Create(IHostEnvironment env, IDataView input, string modelFile, string[] inputColumns, string[] outputColumns)


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

can it be internal? #ByDesign

TF is same way (see below)-- I assume this is required to be public due to different ways it can be instantiated.

machinelearning/src/Microsoft.ML.TensorFlow/TensorflowTransform.cs

Line 195 in cb9effc

public static IDataTransform Create(IHostEnvironment env, IDataView input, string model, string[] names, string[] source)

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

Ivanidzo4ka · 2018-11-13T23:51:41Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

+            Outputs = args.OutputColumns;
+            //var type = OnnxUtils.OnnxToMlNetType(outputNodeInfo.Type);
+            //var shape = outputNodeInfo.Shape;
+            //var dims = shape.Count > 0 ? shape.Skip(shape[0] < 0 ? 1 : 0).Select( x => (int) x ).ToArray() : new[] { 0 };


delete as well? #Closed

Yes, this can be deleted.

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

Ivanidzo4ka · 2018-11-13T23:57:31Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

+        public OnnxTransform(IHostEnvironment env, string modelFile, string[] inputColumns, string[] outputColumns)
+            : this(env, new Arguments() { ModelFile = modelFile, InputColumns = inputColumns, OutputColumns = outputColumns })
        {
        }


I would leave old constructor, I don't see anything bad in having constructor for case in which you have only one input-output. #Closed

done.

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

Ivanidzo4ka · 2018-11-13T23:58:52Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

+            foreach (var input in Inputs)
+            {
+                if (!inputSchema.TryGetColumnIndex(input, out int srcCol))
+                    throw _host.ExceptSchemaMismatch(nameof(inputSchema), "input", input);


ExceptSchemaMismatch [](start = 32, length = 20)

At what place we check type of input column? during transform? #Closed

We check it in the other function GetOutputSchema, at line ~500. We check for shape mismatch, and type mismatch as well.

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

shmoradims · 2018-11-16T18:32:06Z

src/Microsoft.ML.OnnxTransform/OnnxUtils.cs

+        public string[] GetOutputNames()
+        {
+            return _outputNames.ToArray();
+        }


We don't need these new methods. Let just rename _inputNames to InputNames and make it readonly property. And List<> should be fine if you use Contains() #Closed

Done...made public. We need to use IndexOf, since we need the actual index (if it contains the item)

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

shmoradims · 2018-11-16T18:37:31Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs


+        private static int[] AdjustDimensions(OnnxShape shape)
+        {
+            if (shape.Count > 0)


shape.Count > 0 [](start = 16, length = 15)

is shape.Count == 0 a valid case or an error? right now it's being treated as a valid case. If it's valid please comment about what kind of models would have it. #Closed

Added a comment -- let me know if it makes sense.

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

Marge with master

shmoradims

Ivanidzo4ka · 2018-11-20T01:40:30Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

            base(Contracts.CheckRef(env, nameof(env)).Register(nameof(OnnxTransform)))
        {
+            //Contracts.CheckValue(env, nameof(env));
+            //Host = env.Register(RegistrationName);


feel free to delete, Host now get created in base class (and sorry for this changes)

Ivanidzo4ka · 2018-11-20T01:42:03Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

+                    if (type.ValueCount % valCount != 0)
+                        throw Contracts.Except($"Input shape mismatch: Input '{_parent.Inputs[i]}' has shape {String.Join(",", inputShape)}, but input data is of length {type.ValueCount}.");
+
+                    //Host.Assert(_outputItemRawType == _outputColType.ItemType.RawType);


//Host.Assert(_outputItemRawType == _outputColType.ItemType.RawType); [](start = 20, length = 69)

Do we need it or not?

Ivanidzo4ka · 2018-11-20T01:42:43Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

+            ////                disposer = null;
+            ////                return Utils.MarshalInvoke(MakeGetter<int>, _outputItemRawType, input);
+            ////>>>>>>> master
+            //            }


please delete :)

Ivanidzo4ka · 2018-11-20T01:44:58Z

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

+                {
+                    _srcgetter(ref _vBuffer);
+                    _vBuffer.CopyToDense(ref _vBufferDense);
+                    return OnnxUtils.CreateTensor(_vBufferDense.GetValues().ToArray(), _tensorShape);


_vBufferDense.GetValues().ToArray() [](start = 50, length = 35)

I would probably change CreateTensor to accept ReadonlySpan (or whatever GetValues returns) than cast every span to array.

Span is our new reality with VBuffers, you shouldn't expect arrays anymore. #Closed

Ivanidzo4ka

jignparm added 2 commits November 7, 2018 10:50

Added multiple input/output support for Onnxtransform

a9528de

Comment out multi-output test until model is ready

a7a9229

jignparm requested review from shmoradims, vaeksare, Zruty0, shauheen and Ivanidzo4ka November 9, 2018 07:08

jignparm changed the title ~~Jignparm/multiple inputs outputs~~ Multiple inputs output support for OnnxTransform. Nov 9, 2018

shmoradims reviewed Nov 12, 2018

View reviewed changes

Ivanidzo4ka reviewed Nov 13, 2018

View reviewed changes

addressed PR comments

4b689b3

shmoradims reviewed Nov 16, 2018

View reviewed changes

jignparm added 6 commits November 16, 2018 18:44

Merge branch 'master' into jignparm/multipleInputsOutputs

926cc35

merged with master and resolve conflict

f144251

merged again with master

6289485

Merge branch 'master' into jignparm/multipleInputsOutputs

6469985

Marge with master

update command line test to allow multi input/outputs

b4268cc

PR comments, plus activated 1 extra unit test for multi-in-out-puts

84da83c

shmoradims approved these changes Nov 20, 2018

View reviewed changes

merged with changes in master with rowtorowmapper

8cbd8bd

Ivanidzo4ka reviewed Nov 20, 2018

View reviewed changes

jignparm added 2 commits November 20, 2018 02:19

create tensor from span instead of array

c96778b

small change to kick off build

674c612

Ivanidzo4ka approved these changes Nov 20, 2018

View reviewed changes

remove commented code

66a2b55

jignparm merged commit dafa30c into dotnet:master Nov 20, 2018

jignparm deleted the jignparm/multipleInputsOutputs branch November 20, 2018 22:17

jignparm restored the jignparm/multipleInputsOutputs branch November 20, 2018 22:17

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

Multiple inputs output support for OnnxTransform. #1586

Multiple inputs output support for OnnxTransform. #1586

Uh oh!

Conversation

jignparm commented Nov 9, 2018

Uh oh!

shmoradims Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shmoradims Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shmoradims Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shmoradims Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shmoradims Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shmoradims Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shmoradims Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shmoradims Nov 12, 2018 • edited by jignparm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shmoradims Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shmoradims Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shmoradims Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shmoradims Nov 12, 2018 •

edited

Loading

shmoradims Nov 12, 2018 •

edited

Loading

shmoradims Nov 12, 2018 •

edited

Loading

shmoradims Nov 12, 2018 •

edited

Loading

shmoradims Nov 12, 2018 •

edited

Loading

shmoradims Nov 12, 2018 •

edited

Loading

shmoradims Nov 12, 2018 •

edited

Loading

shmoradims Nov 12, 2018 •

edited by jignparm

Loading

shmoradims Nov 12, 2018 •

edited

Loading

shmoradims Nov 12, 2018 •

edited

Loading

shmoradims Nov 12, 2018 •

edited

Loading

shmoradims Nov 12, 2018 •

edited

Loading

shmoradims Nov 12, 2018 •

edited

Loading

Ivanidzo4ka Nov 13, 2018 •

edited

Loading

Ivanidzo4ka Nov 13, 2018 •

edited

Loading

Ivanidzo4ka Nov 13, 2018 •

edited

Loading

Ivanidzo4ka Nov 13, 2018 •

edited by jignparm

Loading

Ivanidzo4ka Nov 13, 2018 •

edited

Loading