Onnx recursion limit

[Crabzmatic]

Fixes #5839

Optional parameter recursionLimit = 100 is added to methods that are loading ONNX models. This parameter is used in Google.Protobuf.CodedInputStream.CreateWithLimits(modelStream, Int32.MaxValue, recursionLimit) (see changes in file OnnxUtils.cs)

This parameter is saved in overriden SaveModel method in class OnnxTransformer.
Parameter is also read in method Create, if it is written, default is set otherwise.

[Crabzmatic]

@michaelgsharp Hi, could you please take a look at this PR? It fixes the linked issue. Build is failing because I changed the public API, I think. How is this change normally done?

[michaelgsharp]

Thanks for taking the time to submit this change.

The reason the builds are failing is because we have an API checker in place to make sure we don't change any public methods to help with backwards compatibility. You are changing the ApplyOnnxModel method, so the checker is failing which is failing the build.

The way to fix this is instead of modifying the three ApplyOnnxModel methods, you will have to add a 4th method that includes all the possible parameters, and only in that one will you include the new parameter you want. Then internally you can default the recursion limit to 100 if its not set. Does that make sense?

[codecov]

Codecov Report

Merging #5840 (2a42ab2) into main (d266c86) will decrease coverage by 0.11%.
The diff coverage is 87.23%.

@@            Coverage Diff             @@
##             main    #5840      +/-   ##
==========================================
- Coverage   68.32%   68.20%   -0.12%     
==========================================
  Files        1131     1134       +3     
  Lines      241291   241996     +705     
  Branches    25053    25303     +250     
==========================================
+ Hits       164863   165064     +201     
- Misses      69923    70276     +353     
- Partials     6505     6656     +151     

Flag	Coverage Δ
Debug	68.20% <87.23%> (-0.12%)	⬇️
production	62.86% <81.25%> (-0.07%)	⬇️
test	88.82% <100.00%> (-0.42%)	⬇️

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

Impacted Files	Coverage Δ
src/Microsoft.ML.OnnxTransformer/OnnxTransform.cs	90.82% <77.77%> (-0.79%)	⬇️
src/Microsoft.ML.OnnxTransformer/OnnxCatalog.cs	100.00% <100.00%> (ø)
src/Microsoft.ML.OnnxTransformer/OnnxUtils.cs	86.63% <100.00%> (ø)
...osoft.ML.OnnxTransformerTest/OnnxTransformTests.cs	95.44% <100.00%> (+0.11%)	⬆️
...ft.ML.Core.Tests/UnitTests/TestResourceDownload.cs	0.00% <0.00%> (-75.52%)	⬇️
...osoft.ML.Recommender/SafeTrainingAndModelBuffer.cs	61.97% <0.00%> (-16.91%)	⬇️
...osoft.ML.Recommender/MatrixFactorizationTrainer.cs	58.10% <0.00%> (-13.97%)	⬇️
...ests/TrainerEstimators/MatrixFactorizationTests.cs	83.59% <0.00%> (-13.48%)	⬇️
test/Microsoft.ML.FSharp.Tests/SmokeTests.fs	77.77% <0.00%> (-10.46%)	⬇️
src/Microsoft.ML.Data/Commands/TrainTestCommand.cs	82.64% <0.00%> (-9.10%)	⬇️
... and 87 more

[Crabzmatic]

The way to fix this is instead of modifying the three ApplyOnnxModel methods, you will have to add a 4th method that includes all the possible parameters, and only in that one will you include the new parameter you want. Then internally you can default the recursion limit to 100 if its not set. Does that make sense?

Yeah, it makes sense. I made these changes.

[michaelgsharp]

It looks really good. Only 2 things that I can see.

1 - Since how the save/load of the model is being changed, the model version needs to be updated. See here for number you need to increment. Only the verWrittenCur needs to change, the rest are fine. But do add a comment above saving what the change was.

2 - Can you add a unit test that uses a non-default value? Even if you just take an existing test and change the value that will work. The TestEstimatorCore method we have in the tests will test the saving/loading and make sure the model is the same before/after loading.

[michaelgsharp]

Hey @Crabzmatic I'll merge this today. Just trying to verify one more thing about backwards compatibility. I'm just testing to make sure these newer models can still be loaded fine on an older version of ML.NET.

[michaelgsharp]

@Crabzmatic Turns out we did need to update the verReadableCur. I went ahead and made that change and pushed it.

I also changed the model loading so instead of using the try/catch it only tries to load the recursion limit if the model was saved with the right version.

Once the tests are complete I will go ahead and merge this.