Skip to content

Fixing ONNX test #3253

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Apr 18, 2019
Merged

Fixing ONNX test #3253

merged 12 commits into from
Apr 18, 2019

Conversation

singlis
Copy link
Member

@singlis singlis commented Apr 9, 2019

The test pipeline for consuming an ONNX model would fail due to the Score column being named "Score0". The ONNX model will rename the output columns by design, therefore a different class with the ColumnName of "Score0" is needed. This fixes the test pipeline to address this issue.

Fixes #2981

Score column being named "Score0". The ONNX model will rename the output
columns by design, therefore a different class with the ColumnName of
"Score0" is needed. This fixes the test pipeline to address this issue.

Fixes dotnet#2981
@singlis singlis self-assigned this Apr 9, 2019
@singlis singlis requested a review from rogancarr April 9, 2019 00:55
@codecov
Copy link

codecov bot commented Apr 9, 2019

Codecov Report

Merging #3253 into master will increase coverage by <.01%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master    #3253      +/-   ##
==========================================
+ Coverage   72.69%   72.69%   +<.01%     
==========================================
  Files         807      807              
  Lines      145172   145171       -1     
  Branches    16225    16225              
==========================================
  Hits       105537   105537              
+ Misses      35220    35219       -1     
  Partials     4415     4415
Flag Coverage Δ
#Debug 72.69% <75%> (ø) ⬆️
#production 68.23% <0%> (ø) ⬆️
#test 88.97% <100%> (-0.01%) ⬇️
Impacted Files Coverage Δ
src/Microsoft.ML.OnnxTransformer/OnnxTransform.cs 86.24% <0%> (ø) ⬆️
test/Microsoft.ML.Functional.Tests/ONNX.cs 100% <100%> (ø) ⬆️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs 84.9% <0%> (+0.2%) ⬆️

Copy link
Contributor

@rogancarr rogancarr left a comment

Choose a reason for hiding this comment

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

Approved with comments.

singlis added 5 commits April 9, 2019 22:18
occur if the input type was not a variable vector or vector type. This
is not needed as we do support converting basic types to equivalent ONNX
tensor type. Therefore removing the check in GetOutputSchema fixes this
problem.
@singlis singlis requested a review from jignparm April 10, 2019 05:48
@@ -564,8 +564,6 @@ public override SchemaShape GetOutputSchema(SchemaShape inputSchema)
var input = Transformer.Inputs[i];
if (!inputSchema.TryFindColumn(input, out var col))
throw Host.ExceptSchemaMismatch(nameof(inputSchema), "input", input);
if (!(col.Kind == SchemaShape.Column.VectorKind.VariableVector || col.Kind == SchemaShape.Column.VectorKind.Vector))
throw Host.ExceptSchemaMismatch(nameof(inputSchema), "input", input, "vector", col.GetTypeString());

Copy link
Contributor

Choose a reason for hiding this comment

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

if input col.Kind == VariableVector, how is that case handled? If not handled, should throw exception.

Copy link
Member Author

Choose a reason for hiding this comment

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

so prior to this change, we allowed VariableVector in that it would not throw -- this change now allows for vector and simple types. I would prefer to have a test that uses VariableVector, but I have yet to find something that provides a VariableVector output AND can be saved to ONNX.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jignparm from our conversation, you mentioned that we should not have allowed VariableVector to begin with and should fix as part of this change. I have updated the code to reflect that, but we should have a test that confirms if this works correctly. I have created the following issue for tracking #3375

@rogancarr
Copy link
Contributor

LGTM @singlis

@singlis singlis merged commit 243ff02 into dotnet:master Apr 18, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot fit an Onnx Transform as part of a pipeline
4 participants