Skip to content
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

Update OnnxTransformer Docs #5296

Merged
merged 2 commits into from
Jul 10, 2020
Merged

Conversation

antoniovs1029
Copy link
Member

@antoniovs1029 antoniovs1029 commented Jul 9, 2020

  • Address Using OnnxTransformer throws TypeInitializationException #5262 (comment) : Users find it confusing not knowing where to find the info related to the dependencies of Onnx and how to make it work on the GPU. So I think it's best to keep this info in the docs of the OnnxScoringEstimator and add a note on every ApplyOnnxModel() overload and to OnnxTransformer pointing to the Estimator's docs.

  • Address Opaque disruptive handling of variable axes of ONNX-models #5293 : About the confusing nature of the shapeDictionary parameter. I've updated the docs for this parameter based on the explicit suggestion made by the user.

  • Added a couple of details about how the gpuDeviceId and fallbackToCpu interact with each other.

Note: As stated in #5262 (comment) for some reason the published docs for OnnxTransformer are outdated, and for some reason it includes info that was deleted on the ML.NET 1.5.0 release (or maybe before that). @natke thinks this is a bug in the tools that generate the published docs, and is seeking for help on this, as this tools are outside of the ML.NET repo itself. In the meantime, we'll try to see if updating this docs on this PR will force the tool to actually generate the correct updated docs.

@antoniovs1029 antoniovs1029 requested a review from natke July 9, 2020 08:08
@antoniovs1029 antoniovs1029 requested a review from a team as a code owner July 9, 2020 08:08
@codecov
Copy link

codecov bot commented Jul 9, 2020

Codecov Report

Merging #5296 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5296   +/-   ##
=======================================
  Coverage   73.68%   73.68%           
=======================================
  Files        1022     1022           
  Lines      190366   190366           
  Branches    20474    20474           
=======================================
+ Hits       140265   140279   +14     
+ Misses      44568    44560    -8     
+ Partials     5533     5527    -6     
Flag Coverage Δ
#Debug 73.68% <ø> (+<0.01%) ⬆️
#production 69.43% <ø> (+<0.01%) ⬆️
#test 87.62% <ø> (ø)
Impacted Files Coverage Δ
src/Microsoft.ML.OnnxTransformer/OnnxCatalog.cs 100.00% <ø> (ø)
src/Microsoft.ML.OnnxTransformer/OnnxTransform.cs 91.97% <ø> (ø)
...ML.Transforms/Text/StopWordsRemovingTransformer.cs 85.94% <0.00%> (+0.15%) ⬆️
src/Microsoft.ML.AutoML/Sweepers/Parameters.cs 85.16% <0.00%> (+0.84%) ⬆️
src/Microsoft.ML.Maml/MAML.cs 26.21% <0.00%> (+1.45%) ⬆️
....ML.AutoML/PipelineSuggesters/PipelineSuggester.cs 86.55% <0.00%> (+6.72%) ⬆️

/// </summary>
/// <remarks>
/// The name/type of input columns must exactly match name/type of the ONNX model inputs.
/// The name/type of the produced output columns will match name/type of the ONNX model outputs.
/// </remarks>
/// <param name="catalog">The transform's catalog.</param>
/// <param name="modelFile">The path of the file containing the ONNX model.</param>
/// <param name="shapeDictionary">ONNX shape should be used to over those loaded from <paramref name="modelFile"/>.</param>
/// <param name="shapeDictionary">ONNX shapes to be used over those loaded from <paramref name="modelFile"/>.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be useful to add an example here? Or do we already have one in the samples?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have samples for this.
I guess I can make one, but for that I'll need to find a model that requires it and upload it somewhere outside ML.NET repo so that it can be added to the sample?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the meantime, do you have any other suggestion on the docs, @natke ? maybe we can merge this PR just to see if this forces the staged docs to update... and add that sample on another PR

Copy link
Member Author

@antoniovs1029 antoniovs1029 Jul 10, 2020

Choose a reason for hiding this comment

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

Ok, I'm pushing this now to see if this unblocks the staging docs.

EDIT: Ok, I can't push it because the CI is stuck. I'll have to rerun the CI manually and after that I'll push.

Copy link
Contributor

@frank-dong-ms-zz frank-dong-ms-zz left a comment

Choose a reason for hiding this comment

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

:shipit:

@antoniovs1029
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@antoniovs1029
Copy link
Member Author

Closing and re-openning this PR as it seems the CI is stuck; hopefully doing this will fix that.

@antoniovs1029 antoniovs1029 merged commit 6415f7f into dotnet:master Jul 10, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Mar 18, 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.

3 participants