Skip to content

Raising of the recursions limit for onnx model loading. (Issue #5585)#5796

Merged
michaelgsharp merged 6 commits intodotnet:mainfrom
darth-vader-lg:hotfix-onnx-graph-recursion
May 27, 2021
Merged

Raising of the recursions limit for onnx model loading. (Issue #5585)#5796
michaelgsharp merged 6 commits intodotnet:mainfrom
darth-vader-lg:hotfix-onnx-graph-recursion

Conversation

@darth-vader-lg
Copy link
Contributor

Raised the limit of recursions (from 10 to 100) in the creation of the CodedInputStream in the OnnxTransformer (as the default value in the Google.Protobuf). Otherwise some models cannot be loaded (ex. TF2 Efficientdet).

Fixes #5585

I have already tested it on all TF2 ModelZoo models converted to onnx and on my custom object detection models. It's all right now.
Inference1
saved_model.onnx.zip

…m in the OnnxTransformer (as the default value in the Google.Protobuf). Otherwise some models cannot be loaded (ex. TF2 Efficentdet).
@codecov
Copy link

codecov bot commented May 17, 2021

Codecov Report

Merging #5796 (0cbc523) into main (7fafbf3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #5796   +/-   ##
=======================================
  Coverage   68.32%   68.32%           
=======================================
  Files        1131     1131           
  Lines      241291   241291           
  Branches    25053    25053           
=======================================
+ Hits       164863   164872    +9     
+ Misses      69923    69918    -5     
+ Partials     6505     6501    -4     
Flag Coverage Δ
Debug 68.32% <100.00%> (+<0.01%) ⬆️
production 62.94% <100.00%> (+<0.01%) ⬆️
test 89.24% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Microsoft.ML.OnnxTransformer/OnnxUtils.cs 86.63% <100.00%> (ø)
....ML.AutoML/PipelineSuggesters/PipelineSuggester.cs 78.74% <0.00%> (-3.15%) ⬇️
src/Microsoft.ML.Transforms/Text/LdaTransform.cs 88.29% <0.00%> (-0.15%) ⬇️
...StandardTrainers/Standard/LinearModelParameters.cs 66.32% <0.00%> (+0.25%) ⬆️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs 90.41% <0.00%> (+0.63%) ⬆️
...rosoft.ML.AutoML/ColumnInference/TextFileSample.cs 65.56% <0.00%> (+5.96%) ⬆️

michaelgsharp and others added 4 commits May 19, 2021 14:59
* updated arcade to the latest version

* updated eng/common correctly

* Fixed benchmark test.
* Use dotnet certificate

* Update 3.1 SDK

Co-authored-by: Prashanth Govindarajan <prgovi@microsoft.com>
Co-authored-by: Michael Sharp <51342856+michaelgsharp@users.noreply.github.com>
* arm testing

* initial commit with build working on arm64

* windows changes

* build fixes for arm/arm64 with cross compilation

* cross build instructions added

* renamed arm to Arm. Changed TargetArchitecture to default to OS architecture

* fixed some formatting

* fixed capitilization

* fixed Arm Capitilization

* Fix cross-compilation if statement

* building on apple silicon

* removed non build related files

* Changes from PR comments. Removal of FastTreeNative flag.

* Changes from pr comments.

* Fixes from PR comments.

* Changed how we are excluding files.
* fixed onnx temp model deleting

* random file path fixed

* updates from pr

* Changes from PR comments.

* Changed how auto ml caches.

* PR fixes.

* Update src/Microsoft.ML.AutoML/API/ExperimentSettings.cs

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

* Tensorflow fixes from PR comments

* fixed filepath issues

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
@darth-vader-lg
Copy link
Contributor Author

darth-vader-lg commented May 19, 2021

I've just merged the latest 4 official main branch's commits to resolve the conflict on OnnxUtils.cs and make more easy the integration.

@michaelgsharp
Copy link
Contributor

I just resolved the conflict. It looks good to me. I think eventually we will need to expose that value so it can be changed as needed, but setting our default to match google is a good idea. I'll go ahead and approve/merge when the tests pass.

Thanks so much for submitting this @darth-vader-lg

@darth-vader-lg
Copy link
Contributor Author

Hello @michaelgsharp
I think so. I also wanted to add a parameter to the OnnxTransformer options at the beginning but... it would be better that official ML.NET team will do this, with their standard style 😉.

Copy link
Contributor

@michaelgsharp michaelgsharp left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the submission!

@michaelgsharp michaelgsharp merged commit 3c3b298 into dotnet:main May 27, 2021
@darth-vader-lg darth-vader-lg deleted the hotfix-onnx-graph-recursion branch June 26, 2021 08:22
@ghost ghost locked as resolved and limited conversation to collaborators Mar 17, 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.

Why RecursionLimit = 10 in OnnxTransformer?

3 participants