Skip to content
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 43 additions & 2 deletions src/Microsoft.ML/Models/OnnxConverter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,50 @@ namespace Microsoft.ML.Models
public sealed partial class OnnxConverter
{
/// <summary>
/// Converts the model to ONNX format.
///

@TomFinley TomFinley Jun 26, 2018

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.

/// [](start = 8, length = 3)

Would a link to ONNX project itself be appropriate, to motivate why someone might care? #Resolved

@GalOshri GalOshri Jun 26, 2018

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.

It might be worth providing some context on ONNX (at least linking to the ONNX website) and explaining why this is valuable. Similarly for Windows ML. #Resolved

/// This API converts the model to ONNX format by inspecting the transform pipeline

@TomFinley TomFinley Jun 26, 2018

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.

Is there a purpose to this blank line here? #Resolved

@GalOshri GalOshri Jun 26, 2018

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.

Might be worth clarifying that it converts an ML.NET model #Resolved

/// from the end, checking for components that know how to save themselves as ONNX.
/// The first data view in the transform pipeline that does not know how to save itself

@TomFinley TomFinley Jun 26, 2018

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.

data view [](start = 22, length = 9)

While data view is a central concept in the underlying implementation, I don't think that the current API actually exposes this concept in any meaningful fashion, does it? Which means that this is an undefined concept, from the point of view of an API user. #Closed

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.

Maybe just "item" for now, since from context it is clear that you're talking about transforms.


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

/// as ONNX, is considered the "input" to the ONNX pipeline. (Ideally this would be the
/// original loader itself, but this may not be possible if the user used unsavable
/// transforms in defining the pipe.) All the columns in the source that are a type the
/// ONNX knows what to deal with will be tracked. Intermediate transformations of the

@TomFinley TomFinley Jun 26, 2018

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.

what to deal [](start = 23, length = 12)

"what to deal" => "how to deal" #Closed

/// data appearing as new columns will appear in the output block of the ONNX, with names
/// derived from the corresponding column names. The ONNX json will be serialized to a

@GalOshri GalOshri Jun 26, 2018

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.

json -> JSON #Resolved

/// path defined through the Json option.

@GalOshri GalOshri Jun 26, 2018

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.

Json -> JSON #WontFix

@codemzs codemzs Jun 26, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Json is the name of the command line option. #Resolved

///
/// This API supports the following arguments:
/// <see cref="Onnx"/> indicates the file to write the ONNX protocol buffer file to.
/// <see cref="Json"/> indicates the file to write the JSON representation of the ONNX model.

@TomFinley TomFinley Jun 26, 2018

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.

indicates the file to write the JSON representation of the ONNX model. [](start = 31, length = 70)

Is that optional? #Closed

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes.


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

/// <see cref="Name"/> indicates the name property in the ONNX model. If left unspecified, it will
/// be the extension-less name of the file specified in the onnx indicates the protocol buffer file
/// to write the ONNX representation to.
/// <see cref="Domain"/> indicates the domain name of the model. We use reverse domain name space indicators.

@TomFinley TomFinley Jun 26, 2018

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.

We [](start = 73, length = 2)

ONNX, not us, right? #Resolved

/// For example com.microsoft.cognitiveservices. This is a required field.
/// <see cref="InputsToDrop"/> is a string array of input column names to omit from the input mapping.
/// A common scenario might be to drop the label column, for instance, since it may not be practically
/// useful for the pipeline. Note that any columns depending on these naturally cannot be saved.
/// <see cref="OutputsToDrop"/> is similar, except for the output schema. Note that the pipeline handler

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.

even though you can guess from the context, I'd say "similar to InputsToDrop"

/// is currently not intelligent enough to drop intermediate calculations that produce this value: this will
/// merely omit that value from the actual output.
///
/// Transforms that can be exported to ONNX
/// 1. Concat
/// 2. KeyToVector
/// 3. NAReplace
/// 4. Normalize
/// 5. Term
/// 6. Categorical
///
/// Learners that can be exported to ONNX

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.

Is it worth referring to what it would take to expand this list?

@codemzs codemzs Jun 26, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not sure if it is worth revealing anything that we are not sure about or isn't public.


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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

But we can say these are the most used learners.


In reply to: 198318122 [](ancestors = 198318122,198317926)

/// 1. FastTree
/// 2. LightGBM
/// 3. LibSVM

@TomFinley TomFinley Jun 26, 2018

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.

LibSVM [](start = 15, length = 6)

This has not yet been migrated, so should probably not be included. #Resolved

/// 4. Multi Class Logistic Regression
/// 5. Logistic Regression

@TomFinley TomFinley Jun 26, 2018

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.

This is somewhat peculiar, that we call out both LR and multi-class LR, when we haven't done the same for all the tasks FastTree can accomplish, for example. #Resolved

///
/// See <a href="https://github.com/dotnet/machinelearning/blob/master/test/Microsoft.ML.Tests/OnnxTests.cs"/>
/// for an example.
/// for an example on how to train a model and then convert that model to ONNX.
/// </summary>
/// <param name="model">Model that needs to be converted to ONNX format.</param>
public void Convert(PredictionModel model)
Expand Down