Skip to content

Lockdown Microsoft.ML.Onnx public surface#2625

Merged
codemzs merged 8 commits intodotnet:masterfrom
codemzs:lockdownonnx
Feb 20, 2019
Merged

Lockdown Microsoft.ML.Onnx public surface#2625
codemzs merged 8 commits intodotnet:masterfrom
codemzs:lockdownonnx

Conversation

@codemzs
Copy link
Copy Markdown
Member

@codemzs codemzs commented Feb 19, 2019

fixes #2273

Reduces public API count from 423 to 1.

Before After
image image

Copy link
Copy Markdown
Contributor

@TomFinley TomFinley left a comment

Choose a reason for hiding this comment

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

Nice, straightforward and very clean, thank you @codemzs. The renaming is very nice too, helps us distinguish between ONNX conversion and transformer.

@codemzs codemzs self-assigned this Feb 19, 2019
@codemzs codemzs added the API Issues pertaining the friendly API label Feb 19, 2019
@codemzs codemzs added this to the 0219 milestone Feb 19, 2019

// Step 2: Convert ML.NET model to ONNX format and save it as a file.
var onnxModel = mlContext.Model.ConvertToOnnx(model, data);
var onnxModel = mlContext.Model.ConvertToOnnxProtobufModel(model, data);
Copy link
Copy Markdown
Contributor

@wschin wschin Feb 19, 2019

Choose a reason for hiding this comment

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

ConvertToOnnxProtobufModel [](start = 44, length = 26)

Seems ConvertToOnnxProtobuf is better. Model doesn't look like a part of a format's name. #Resolved

@wschin
Copy link
Copy Markdown
Contributor

wschin commented Feb 19, 2019

{

Is it auto-generated? Also this class looks more like a direct implemetation instead of a wrapper; I'd suggest using either OnnxProto or OnnxProtobuf. #Resolved


Refers to: src/Microsoft.ML.OnnxConverter/OnnxMl.cs:14 in 4cd1d45. [](commit_id = 4cd1d45, deletion_comment = False)

@codemzs
Copy link
Copy Markdown
Member Author

codemzs commented Feb 19, 2019

{

I added that piece of code.


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


Refers to: src/Microsoft.ML.OnnxConverter/OnnxMl.cs:14 in 4cd1d45. [](commit_id = 4cd1d45, deletion_comment = False)

@wschin
Copy link
Copy Markdown
Contributor

wschin commented Feb 19, 2019

{

I guess there is no way to have protobuf complier to do this for us, right? If so, please add a note to OnnxML.md so that future maintainers can see how this file got created.


In reply to: 465344411 [](ancestors = 465344411,465343652)


Refers to: src/Microsoft.ML.OnnxConverter/OnnxMl.cs:14 in 4cd1d45. [](commit_id = 4cd1d45, deletion_comment = False)

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 20, 2019

Codecov Report

Merging #2625 into master will decrease coverage by <.01%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master    #2625      +/-   ##
==========================================
- Coverage   71.53%   71.53%   -0.01%     
==========================================
  Files         800      800              
  Lines      141846   141847       +1     
  Branches    16119    16119              
==========================================
- Hits       101476   101475       -1     
- Misses      35918    35919       +1     
- Partials     4452     4453       +1
Flag Coverage Δ
#Debug 71.53% <91.66%> (-0.01%) ⬇️
#production 67.82% <75%> (-0.01%) ⬇️
#test 85.73% <100%> (ø) ⬆️

@codemzs codemzs merged commit 685b064 into dotnet:master Feb 20, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

API Issues pertaining the friendly API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Lockdown Microsoft.ML.Onnx public surface

4 participants