Skip to content
6 changes: 6 additions & 0 deletions src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,12 @@ public override int GetHashCode()
/// </summary>
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.");
}

@codemzs codemzs Oct 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please do not use braces for single line if condition #Resolved

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this is best practice to always use braces for if condition, in my previous team it is also suggested to always use braces for if condition to avoid further problem.

There is also some discussion online, please check out and see whether this make sense to you: https://softwareengineering.stackexchange.com/questions/16528/single-statement-if-block-braces-or-no


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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Offline discussed with Zeeshan, this can be debatable, I will just follow the coding style in this team, not a big issue


In reply to: 330672086 [](ancestors = 330672086,330344007)

@harishsk harishsk Oct 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it is better to throw ArgumentNullException here? #Resolved

@eerhardt eerhardt Oct 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing just a plain Exception is not recommended.

https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/using-standard-exception-types

X DO NOT throw System.Exception or System.SystemException.

However, ArgumentNullException is not correct either, since there is no argument to this method.

✓ DO throw ArgumentException or one of its subtypes if bad arguments are passed to a member.

In this case, InvalidOperationException is the most correct:

✓ DO throw an InvalidOperationException if the object is in an inappropriate state. #Resolved

@eerhardt eerhardt Oct 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO - it the best approach would be to remove the empty constructor and verify the Type elemType in the other constructor is never null.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, very good suggestion, InvalidOperationException is the most proper one to use here


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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have a similar discussion with Harish about this yesterday, we agreed that remove empty constructor seems to be more ambiguous because when our customer use [OnnxSequenceType] attribute directly he/she will get error message like "method is inaccessible due to protection level". By throwing exception we can give user more specific error message. Make sense to you?


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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to take Eric's suggestion and use InvalidOperationException here.


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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when our customer use [OnnxSequenceType] attribute directly he/she will get error message like "method is inaccessible due to protection level"

That's a compile time error - which is exactly what we want. We want to raise the error as early as possible, so the user knows they are doing something wrong.

Also, when I get a different error message than what you are displaying:

    class Program
    {
        [My]
        private int foo;
    }

    class MyAttribute : Attribute
    {
        public MyAttribute(int required)
        { }
    }
Error	CS7036	There is no argument given that corresponds to the required formal parameter 'required' of 'MyAttribute.MyAttribute(int)'	ConsoleApp80	C:\Users\eerhardt\source\repos\ConsoleApp80\Program.cs	13	Active

Pretty obvious what to do to fix it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Eric's suggestion is the better approach here. Please modify it as per his suggestion.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me, will do


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


var enumerableType = typeof(IEnumerable<>);
var type = enumerableType.MakeGenericType(_elemType);
DataViewTypeManager.Register(new OnnxSequenceType(_elemType), type, this);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,37 +13,55 @@
using System.Linq;
using System.IO;
using Microsoft.ML.TestFramework.Attributes;
using System;

@codemzs codemzs Oct 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please place "using System" at the top and sort alphabetical order as done in source files. System libraries take preference over non-system libraries like Microsoft.ML.* #Resolved

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, will do


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


namespace Microsoft.ML.Tests
{
public class OnnxSequenceTypeWithAttributesTest : BaseTestBaseline
public class OnnxSequenceTypeTest : BaseTestBaseline
{
public class OutputObj
{
[ColumnName("output")]
[OnnxSequenceType(typeof(IDictionary<string, float>))]
public IEnumerable<IDictionary<string, float>> Output;
}

public class ProblematicOutputObj
{

[ColumnName("output")]
// incorrect usage, should always specify sequence type when using OnnxSequenceType attribute
[OnnxSequenceType]
public IEnumerable<IDictionary<string, float>> Output;
}

public class FloatInput
{
[ColumnName("input")]
[VectorType(3)]
public float[] Input { get; set; }
}

public OnnxSequenceTypeWithAttributesTest(ITestOutputHelper output) : base(output)
public OnnxSequenceTypeTest(ITestOutputHelper output) : base(output)
{
}
public static PredictionEngine<FloatInput, OutputObj> LoadModel(string onnxModelFilePath)

private static OnnxTransformer PrepareModel(string onnxModelFilePath, MLContext ctx)
{
var ctx = new MLContext();
var dataView = ctx.Data.LoadFromEnumerable(new List<FloatInput>());

var pipeline = ctx.Transforms.ApplyOnnxModel(
modelFile: onnxModelFilePath,
outputColumnNames: new[] { "output" }, inputColumnNames: new[] { "input" });

var model = pipeline.Fit(dataView);
return model;
}

public static PredictionEngine<FloatInput, OutputObj> LoadModel(string onnxModelFilePath)
{
var ctx = new MLContext();
var model = PrepareModel(onnxModelFilePath, ctx);
return ctx.Model.CreatePredictionEngine<FloatInput, OutputObj>(model);
}

Expand All @@ -64,5 +82,21 @@ public void OnnxSequenceTypeWithColumnNameAttributeTest()
}

}

private static PredictionEngine<FloatInput, ProblematicOutputObj> CreatePredictorWithPronlematicOutputObj()
{
var onnxModelFilePath = Path.Combine(Directory.GetCurrentDirectory(), "zipmap", "TestZipMapString.onnx");

var ctx = new MLContext();
var model = PrepareModel(onnxModelFilePath, ctx);
return ctx.Model.CreatePredictionEngine<FloatInput, ProblematicOutputObj>(model);
}

[OnnxFact]
public void OnnxSequenceTypeWithouSpecifySequenceTypeTest()
{
Exception ex = Assert.Throws<Exception>(() => CreatePredictorWithPronlematicOutputObj());

@harishsk harishsk Oct 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CreatePredictorWithPronlematicOutputObj [](start = 58, length = 39)

A small typo here with Problematic #Resolved

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, will do, thanks


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

Assert.Equal("Please specify sequence type when use OnnxSequenceType Attribute.", ex.Message);
}

@harishsk harishsk Oct 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to assert on the type of exception rather than the specific message? #Resolved

@frank-dong-ms-zz frank-dong-ms-zz Oct 2, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the exception type is assert in previous line Assert.Throws, after change to use InvalidOperationException, it should look like Assert.Throws(....), so we can assert both on exception type and exception message.


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

}
}