From ec265188d2be002bdde402635eff86c9c1542a00 Mon Sep 17 00:00:00 2001 From: Frank Dong Date: Tue, 1 Oct 2019 15:16:53 -0700 Subject: [PATCH 1/8] fix issue 4120, add reasonable exception when user try to use OnnxSequenceType attribute without specify sequence type --- .../OnnxSequenceType.cs | 6 +++ ...ributesTest.cs => OnnxSequenceTypeTest.cs} | 42 +++++++++++++++++-- 2 files changed, 44 insertions(+), 4 deletions(-) rename test/Microsoft.ML.Tests/{OnnxSequenceTypeWithAttributesTest.cs => OnnxSequenceTypeTest.cs} (62%) diff --git a/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs b/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs index c8b310b45c..6dca134c6d 100644 --- a/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs +++ b/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs @@ -94,6 +94,12 @@ public override int GetHashCode() /// public override void Register() { + // this happens when use OnnxSequenceType attribute without specify sequence type + if (_elemType == null) + { + throw new Exception("Please specify sequence type when use OnnxSequenceType Attribute."); + } + var enumerableType = typeof(IEnumerable<>); var type = enumerableType.MakeGenericType(_elemType); DataViewTypeManager.Register(new OnnxSequenceType(_elemType), type, this); diff --git a/test/Microsoft.ML.Tests/OnnxSequenceTypeWithAttributesTest.cs b/test/Microsoft.ML.Tests/OnnxSequenceTypeTest.cs similarity index 62% rename from test/Microsoft.ML.Tests/OnnxSequenceTypeWithAttributesTest.cs rename to test/Microsoft.ML.Tests/OnnxSequenceTypeTest.cs index e8090aef81..683e6a22c4 100644 --- a/test/Microsoft.ML.Tests/OnnxSequenceTypeWithAttributesTest.cs +++ b/test/Microsoft.ML.Tests/OnnxSequenceTypeTest.cs @@ -13,10 +13,11 @@ using System.Linq; using System.IO; using Microsoft.ML.TestFramework.Attributes; +using System; namespace Microsoft.ML.Tests { - public class OnnxSequenceTypeWithAttributesTest : BaseTestBaseline + public class OnnxSequenceTypeTest : BaseTestBaseline { public class OutputObj { @@ -24,6 +25,16 @@ public class OutputObj [OnnxSequenceType(typeof(IDictionary))] public IEnumerable> Output; } + + public class ProblematicOutputObj + { + + [ColumnName("output")] + // incorrect usage, should always specify sequence type when using OnnxSequenceType attribute + [OnnxSequenceType] + public IEnumerable> Output; + } + public class FloatInput { [ColumnName("input")] @@ -31,12 +42,12 @@ public class FloatInput public float[] Input { get; set; } } - public OnnxSequenceTypeWithAttributesTest(ITestOutputHelper output) : base(output) + public OnnxSequenceTypeTest(ITestOutputHelper output) : base(output) { } - public static PredictionEngine LoadModel(string onnxModelFilePath) + + private static OnnxTransformer PrepareModel(string onnxModelFilePath, MLContext ctx) { - var ctx = new MLContext(); var dataView = ctx.Data.LoadFromEnumerable(new List()); var pipeline = ctx.Transforms.ApplyOnnxModel( @@ -44,6 +55,13 @@ public static PredictionEngine LoadModel(string onnxModel outputColumnNames: new[] { "output" }, inputColumnNames: new[] { "input" }); var model = pipeline.Fit(dataView); + return model; + } + + public static PredictionEngine LoadModel(string onnxModelFilePath) + { + var ctx = new MLContext(); + var model = PrepareModel(onnxModelFilePath, ctx); return ctx.Model.CreatePredictionEngine(model); } @@ -64,5 +82,21 @@ public void OnnxSequenceTypeWithColumnNameAttributeTest() } } + + private static PredictionEngine CreatePredictor() + { + var onnxModelFilePath = Path.Combine(Directory.GetCurrentDirectory(), "zipmap", "TestZipMapString.onnx"); + + var ctx = new MLContext(); + var model = PrepareModel(onnxModelFilePath, ctx); + return ctx.Model.CreatePredictionEngine(model); + } + + [OnnxFact] + public void OnnxSequenceTypeWithouSpecifySequenceTypeTest() + { + Exception ex = Assert.Throws(() => CreatePredictor()); + Assert.Equal("Please specify sequence type when use OnnxSequenceType Attribute.", ex.Message); + } } } From 795892255f5d37455e280d29d1774d01f61f0695 Mon Sep 17 00:00:00 2001 From: Frank Dong Date: Tue, 1 Oct 2019 15:22:36 -0700 Subject: [PATCH 2/8] rename function for better readibility --- test/Microsoft.ML.Tests/OnnxSequenceTypeTest.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/Microsoft.ML.Tests/OnnxSequenceTypeTest.cs b/test/Microsoft.ML.Tests/OnnxSequenceTypeTest.cs index 683e6a22c4..9f26783597 100644 --- a/test/Microsoft.ML.Tests/OnnxSequenceTypeTest.cs +++ b/test/Microsoft.ML.Tests/OnnxSequenceTypeTest.cs @@ -83,7 +83,7 @@ public void OnnxSequenceTypeWithColumnNameAttributeTest() } - private static PredictionEngine CreatePredictor() + private static PredictionEngine CreatePredictorWithPronlematicOutputObj() { var onnxModelFilePath = Path.Combine(Directory.GetCurrentDirectory(), "zipmap", "TestZipMapString.onnx"); @@ -95,7 +95,7 @@ private static PredictionEngine CreatePredicto [OnnxFact] public void OnnxSequenceTypeWithouSpecifySequenceTypeTest() { - Exception ex = Assert.Throws(() => CreatePredictor()); + Exception ex = Assert.Throws(() => CreatePredictorWithPronlematicOutputObj()); Assert.Equal("Please specify sequence type when use OnnxSequenceType Attribute.", ex.Message); } } From 6c90cb2a9d29579f359a614be906d1143feb4623 Mon Sep 17 00:00:00 2001 From: Frank Dong Date: Wed, 2 Oct 2019 11:11:16 -0700 Subject: [PATCH 3/8] take comments and modify accordingly --- src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs | 4 +--- test/Microsoft.ML.Tests/OnnxSequenceTypeTest.cs | 7 ++++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs b/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs index 6dca134c6d..bac48478fd 100644 --- a/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs +++ b/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs @@ -96,9 +96,7 @@ public override void Register() { // this happens when use OnnxSequenceType attribute without specify sequence type if (_elemType == null) - { - throw new Exception("Please specify sequence type when use OnnxSequenceType Attribute."); - } + throw new InvalidOperationException("Please specify sequence type when use OnnxSequenceType Attribute."); var enumerableType = typeof(IEnumerable<>); var type = enumerableType.MakeGenericType(_elemType); diff --git a/test/Microsoft.ML.Tests/OnnxSequenceTypeTest.cs b/test/Microsoft.ML.Tests/OnnxSequenceTypeTest.cs index 9f26783597..222f76ce81 100644 --- a/test/Microsoft.ML.Tests/OnnxSequenceTypeTest.cs +++ b/test/Microsoft.ML.Tests/OnnxSequenceTypeTest.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System; using System.Collections.Generic; using System.Drawing; using Microsoft.ML.Data; @@ -13,7 +14,7 @@ using System.Linq; using System.IO; using Microsoft.ML.TestFramework.Attributes; -using System; + namespace Microsoft.ML.Tests { @@ -83,7 +84,7 @@ public void OnnxSequenceTypeWithColumnNameAttributeTest() } - private static PredictionEngine CreatePredictorWithPronlematicOutputObj() + private static PredictionEngine CreatePredictorWithProblematicOutputObj() { var onnxModelFilePath = Path.Combine(Directory.GetCurrentDirectory(), "zipmap", "TestZipMapString.onnx"); @@ -95,7 +96,7 @@ private static PredictionEngine CreatePredicto [OnnxFact] public void OnnxSequenceTypeWithouSpecifySequenceTypeTest() { - Exception ex = Assert.Throws(() => CreatePredictorWithPronlematicOutputObj()); + InvalidOperationException ex = Assert.Throws(() => CreatePredictorWithProblematicOutputObj()); Assert.Equal("Please specify sequence type when use OnnxSequenceType Attribute.", ex.Message); } } From b201e172df42b44b746282b5398e5dafb4c8fb4b Mon Sep 17 00:00:00 2001 From: Frank Dong Date: Wed, 2 Oct 2019 14:36:25 -0700 Subject: [PATCH 4/8] take eric's comments, comment out default constructor instead of throw exception --- .../OnnxSequenceType.cs | 15 ++---- ... => OnnxSequenceTypeWithAttributesTest.cs} | 47 +++---------------- 2 files changed, 11 insertions(+), 51 deletions(-) rename test/Microsoft.ML.Tests/{OnnxSequenceTypeTest.cs => OnnxSequenceTypeWithAttributesTest.cs} (59%) diff --git a/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs b/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs index bac48478fd..38521e02a5 100644 --- a/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs +++ b/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs @@ -56,12 +56,11 @@ public sealed class OnnxSequenceTypeAttribute : DataViewTypeAttribute { private Type _elemType; - /// - /// Create a sequence type. - /// - public OnnxSequenceTypeAttribute() - { - } + // Comment out default constructor + // Use default contructor will left the _elemType field empty and cause exception in methods using _elemType + //public OnnxSequenceTypeAttribute() + //{ + //} /// /// Create a -sequence type. @@ -94,10 +93,6 @@ public override int GetHashCode() /// public override void Register() { - // this happens when use OnnxSequenceType attribute without specify sequence type - if (_elemType == null) - throw new InvalidOperationException("Please specify sequence type when use OnnxSequenceType Attribute."); - var enumerableType = typeof(IEnumerable<>); var type = enumerableType.MakeGenericType(_elemType); DataViewTypeManager.Register(new OnnxSequenceType(_elemType), type, this); diff --git a/test/Microsoft.ML.Tests/OnnxSequenceTypeTest.cs b/test/Microsoft.ML.Tests/OnnxSequenceTypeWithAttributesTest.cs similarity index 59% rename from test/Microsoft.ML.Tests/OnnxSequenceTypeTest.cs rename to test/Microsoft.ML.Tests/OnnxSequenceTypeWithAttributesTest.cs index 222f76ce81..14d0fbddfe 100644 --- a/test/Microsoft.ML.Tests/OnnxSequenceTypeTest.cs +++ b/test/Microsoft.ML.Tests/OnnxSequenceTypeWithAttributesTest.cs @@ -2,7 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. -using System; using System.Collections.Generic; using System.Drawing; using Microsoft.ML.Data; @@ -15,10 +14,9 @@ using System.IO; using Microsoft.ML.TestFramework.Attributes; - namespace Microsoft.ML.Tests { - public class OnnxSequenceTypeTest : BaseTestBaseline + public class OnnxSequenceTypeWithAttributesTest : BaseTestBaseline { public class OutputObj { @@ -26,16 +24,6 @@ public class OutputObj [OnnxSequenceType(typeof(IDictionary))] public IEnumerable> Output; } - - public class ProblematicOutputObj - { - - [ColumnName("output")] - // incorrect usage, should always specify sequence type when using OnnxSequenceType attribute - [OnnxSequenceType] - public IEnumerable> Output; - } - public class FloatInput { [ColumnName("input")] @@ -43,12 +31,12 @@ public class FloatInput public float[] Input { get; set; } } - public OnnxSequenceTypeTest(ITestOutputHelper output) : base(output) + public OnnxSequenceTypeWithAttributesTest(ITestOutputHelper output) : base(output) { } - - private static OnnxTransformer PrepareModel(string onnxModelFilePath, MLContext ctx) + public static PredictionEngine LoadModel(string onnxModelFilePath) { + var ctx = new MLContext(); var dataView = ctx.Data.LoadFromEnumerable(new List()); var pipeline = ctx.Transforms.ApplyOnnxModel( @@ -56,13 +44,6 @@ private static OnnxTransformer PrepareModel(string onnxModelFilePath, MLContext outputColumnNames: new[] { "output" }, inputColumnNames: new[] { "input" }); var model = pipeline.Fit(dataView); - return model; - } - - public static PredictionEngine LoadModel(string onnxModelFilePath) - { - var ctx = new MLContext(); - var model = PrepareModel(onnxModelFilePath, ctx); return ctx.Model.CreatePredictionEngine(model); } @@ -77,27 +58,11 @@ public void OnnxSequenceTypeWithColumnNameAttributeTest() var onnx_out = output.Output.FirstOrDefault(); Assert.True(onnx_out.Count == 3, "Output missing data."); var keys = new List(onnx_out.Keys); - for(var i =0; i < onnx_out.Count; ++i) + for (var i = 0; i < onnx_out.Count; ++i) { Assert.Equal(onnx_out[keys[i]], input.Input[i]); } } - - private static PredictionEngine CreatePredictorWithProblematicOutputObj() - { - var onnxModelFilePath = Path.Combine(Directory.GetCurrentDirectory(), "zipmap", "TestZipMapString.onnx"); - - var ctx = new MLContext(); - var model = PrepareModel(onnxModelFilePath, ctx); - return ctx.Model.CreatePredictionEngine(model); - } - - [OnnxFact] - public void OnnxSequenceTypeWithouSpecifySequenceTypeTest() - { - InvalidOperationException ex = Assert.Throws(() => CreatePredictorWithProblematicOutputObj()); - Assert.Equal("Please specify sequence type when use OnnxSequenceType Attribute.", ex.Message); - } } -} +} \ No newline at end of file From 188598777a077a78a9ef202c3d3b085c16260895 Mon Sep 17 00:00:00 2001 From: Frank Dong Date: Wed, 2 Oct 2019 15:08:12 -0700 Subject: [PATCH 5/8] refine comments --- src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs b/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs index 38521e02a5..0235f5bf47 100644 --- a/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs +++ b/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs @@ -57,7 +57,8 @@ public sealed class OnnxSequenceTypeAttribute : DataViewTypeAttribute private Type _elemType; // Comment out default constructor - // Use default contructor will left the _elemType field empty and cause exception in methods using _elemType + // Use default constructor will left the _elemType field empty and cause exception in methods using _elemType + // Comment out default constructor will give user compile error when user try to use [OnnxSequenceType] attribute directly without specify sequence type //public OnnxSequenceTypeAttribute() //{ //} From 18eff2dab10d773b9503c44929868c1c22894d3f Mon Sep 17 00:00:00 2001 From: Frank Dong Date: Wed, 2 Oct 2019 15:09:10 -0700 Subject: [PATCH 6/8] remove unnecessary code change --- test/Microsoft.ML.Tests/OnnxSequenceTypeWithAttributesTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Microsoft.ML.Tests/OnnxSequenceTypeWithAttributesTest.cs b/test/Microsoft.ML.Tests/OnnxSequenceTypeWithAttributesTest.cs index 14d0fbddfe..11849c3372 100644 --- a/test/Microsoft.ML.Tests/OnnxSequenceTypeWithAttributesTest.cs +++ b/test/Microsoft.ML.Tests/OnnxSequenceTypeWithAttributesTest.cs @@ -58,7 +58,7 @@ public void OnnxSequenceTypeWithColumnNameAttributeTest() var onnx_out = output.Output.FirstOrDefault(); Assert.True(onnx_out.Count == 3, "Output missing data."); var keys = new List(onnx_out.Keys); - for (var i = 0; i < onnx_out.Count; ++i) + for(var i =0; i < onnx_out.Count; ++i) { Assert.Equal(onnx_out[keys[i]], input.Input[i]); } From 1c71e9988afa18a46f7a17c32279c33c30ac40d2 Mon Sep 17 00:00:00 2001 From: Frank Dong Date: Thu, 3 Oct 2019 11:20:30 -0700 Subject: [PATCH 7/8] Make default constructor of OnnxSequenceType attribute obsolete --- .../OnnxSequenceType.cs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs b/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs index 0235f5bf47..1c2d5a77d8 100644 --- a/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs +++ b/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs @@ -56,12 +56,13 @@ public sealed class OnnxSequenceTypeAttribute : DataViewTypeAttribute { private Type _elemType; - // Comment out default constructor - // Use default constructor will left the _elemType field empty and cause exception in methods using _elemType - // Comment out default constructor will give user compile error when user try to use [OnnxSequenceType] attribute directly without specify sequence type - //public OnnxSequenceTypeAttribute() - //{ - //} + // Make default constructor obsolete. + // Use default constructor will left the _elemType field empty and cause exception in methods using _elemType. + // User will receive compile error when try to use [OnnxSequenceType] attribute directly without specify sequence type + [Obsolete("Please specify sequence type when use OnnxSequenceType Attribute", true)] + public OnnxSequenceTypeAttribute() + { + } /// /// Create a -sequence type. From aea2c84cc7a4d9629ef06286b1a419af61a025a6 Mon Sep 17 00:00:00 2001 From: Frank Dong Date: Fri, 4 Oct 2019 14:52:20 -0700 Subject: [PATCH 8/8] hide default constructor of OnnxSequenceType attribute from intellisense and make obsolete not cause compiler error just to be safe --- src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs b/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs index 1c2d5a77d8..a41fa2cd2c 100644 --- a/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs +++ b/src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; +using System.ComponentModel; using Microsoft.ML.Data; namespace Microsoft.ML.Transforms.Onnx @@ -58,8 +59,9 @@ public sealed class OnnxSequenceTypeAttribute : DataViewTypeAttribute // Make default constructor obsolete. // Use default constructor will left the _elemType field empty and cause exception in methods using _elemType. - // User will receive compile error when try to use [OnnxSequenceType] attribute directly without specify sequence type - [Obsolete("Please specify sequence type when use OnnxSequenceType Attribute", true)] + // User will receive compile warning when try to use [OnnxSequenceType] attribute directly without specify sequence type + [Obsolete("Please specify sequence type when use OnnxSequenceType Attribute", false)] + [EditorBrowsable(EditorBrowsableState.Never)] public OnnxSequenceTypeAttribute() { }