Issue 4120, add reasonable exception when user try to use OnnxSequenceType attribute without specify sequence type #4272

frank-dong-ms-zz · 2019-10-01T22:26:35Z

The issue is: when user use OnnxSequenceType attribute directly without specify sequence type like [OnnxSequenceType], user will hit run time exception when try to use it.

The ideal way to fix this issue is to remove the default constructor of OnnxSequenceTypeAttribute and user will get compiler error when he/she try to use [OnnxSequenceType].
But this can be a break change in Public API. After discuss with @eerhardt and @ericstj , we decide to obsolete the default constructor for now. Then we will remove the default constructor in next major release.

…uenceType attribute without specify sequence type

dnfclas · 2019-10-01T22:26:48Z

All CLA requirements met.

codecov · 2019-10-01T23:19:06Z

Codecov Report

❗ No coverage uploaded for pull request base (master@e19369b). Click here to learn what that means.
The diff coverage is 94.44%.

@@            Coverage Diff            @@
##             master    #4272   +/-   ##
=========================================
  Coverage          ?   74.55%           
=========================================
  Files             ?      878           
  Lines             ?   154042           
  Branches          ?    16854           
=========================================
  Hits              ?   114851           
  Misses            ?    34453           
  Partials          ?     4738

Flag	Coverage Δ
#Debug	`74.55% <94.44%> (?)`
#production	`70.14% <100%> (?)`
#test	`89.52% <93.75%> (?)`

Impacted Files	Coverage Δ
...c/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs	`89.74% <100%> (ø)`
test/Microsoft.ML.Tests/OnnxSequenceTypeTest.cs	`97.5% <93.75%> (ø)`

codemzs · 2019-10-02T01:43:20Z

src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs

+            if (_elemType == null)
+            {
+                throw new Exception("Please specify sequence type when use OnnxSequenceType Attribute.");
+            }


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

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)

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)

codemzs · 2019-10-02T01:44:40Z

test/Microsoft.ML.Tests/OnnxSequenceTypeTest.cs

 using System.Linq;
 using System.IO;
 using Microsoft.ML.TestFramework.Attributes;
+using System;


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

yes, will do

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

codemzs

harishsk · 2019-10-02T16:30:11Z

src/Microsoft.ML.OnnxTransformer/OnnxSequenceType.cs

+            if (_elemType == null)
+            {
+                throw new Exception("Please specify sequence type when use OnnxSequenceType Attribute.");
+            }


Perhaps it is better to throw ArgumentNullException here? #Resolved

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

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

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

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

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)

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

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

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.

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

Sounds good to me, will do

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

harishsk · 2019-10-02T16:31:31Z

test/Microsoft.ML.Tests/OnnxSequenceTypeTest.cs

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


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

A small typo here with Problematic #Resolved

yes, will do, thanks

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

harishsk · 2019-10-02T16:34:23Z

test/Microsoft.ML.Tests/OnnxSequenceTypeTest.cs

+        {
+            Exception ex = Assert.Throws<Exception>(() => CreatePredictorWithPronlematicOutputObj());
+            Assert.Equal("Please specify sequence type when use OnnxSequenceType Attribute.", ex.Message);
+        }


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

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)

harishsk

🕐

…w exception

frank-dong-ms added 2 commits October 1, 2019 15:16

fix issue 4120, add reasonable exception when user try to use OnnxSeq…

ec26518

…uenceType attribute without specify sequence type

rename function for better readibility

7958922

frank-dong-ms-zz requested a review from a team as a code owner October 1, 2019 22:26

codemzs reviewed Oct 2, 2019

View reviewed changes

codemzs approved these changes Oct 2, 2019

View reviewed changes

harishsk reviewed Oct 2, 2019

View reviewed changes

harishsk suggested changes Oct 2, 2019

View reviewed changes

frank-dong-ms added 6 commits October 2, 2019 11:11

take comments and modify accordingly

6c90cb2

take eric's comments, comment out default constructor instead of thro…

b201e17

…w exception

refine comments

1885987

remove unnecessary code change

18eff2d

Merge https://github.com/dotnet/machinelearning into issue-4120

2067f7d

Make default constructor of OnnxSequenceType attribute obsolete

1c71e99

harishsk approved these changes Oct 4, 2019

View reviewed changes

frank-dong-ms-zz merged commit 24f1cca into dotnet:master Oct 4, 2019

eerhardt mentioned this pull request Oct 9, 2019

Release notes for 1.4.0-preview2 and 0.16.0-preview2. #4315

Merged

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

Issue 4120, add reasonable exception when user try to use OnnxSequenceType attribute without specify sequence type #4272

Issue 4120, add reasonable exception when user try to use OnnxSequenceType attribute without specify sequence type #4272

Uh oh!

Conversation

frank-dong-ms-zz commented Oct 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfclas commented Oct 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codemzs Oct 2, 2019 • edited by frank-dong-ms-zz 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!

codemzs Oct 2, 2019 • edited by frank-dong-ms-zz 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!

codemzs left a comment

Choose a reason for hiding this comment

Uh oh!

harishsk Oct 2, 2019 • edited by frank-dong-ms-zz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Oct 2, 2019 • edited by frank-dong-ms-zz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Oct 2, 2019 • edited by harishsk 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!

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!

harishsk Oct 2, 2019 • edited by frank-dong-ms-zz 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!

harishsk Oct 2, 2019 • edited by frank-dong-ms-zz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frank-dong-ms-zz Oct 2, 2019 • 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!

harishsk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

frank-dong-ms-zz commented Oct 1, 2019 •

edited

Loading

dnfclas commented Oct 1, 2019 •

edited

Loading

codecov bot commented Oct 1, 2019 •

edited

Loading

codemzs Oct 2, 2019 •

edited by frank-dong-ms-zz

Loading

codemzs Oct 2, 2019 •

edited by frank-dong-ms-zz

Loading

harishsk Oct 2, 2019 •

edited by frank-dong-ms-zz

Loading

eerhardt Oct 2, 2019 •

edited by frank-dong-ms-zz

Loading

eerhardt Oct 2, 2019 •

edited by harishsk

Loading

harishsk Oct 2, 2019 •

edited by frank-dong-ms-zz

Loading

harishsk Oct 2, 2019 •

edited by frank-dong-ms-zz

Loading

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

edited

Loading