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

Added IDisposable to OnnxTransformer and fixed memory leaks #5348

Merged
merged 4 commits into from
Aug 16, 2020
Merged

Added IDisposable to OnnxTransformer and fixed memory leaks #5348

merged 4 commits into from
Aug 16, 2020

Conversation

harishsk
Copy link
Contributor

Fixes #5342
A temp file was being created when the Onnx model was being loaded from within an ML.NET model. This file wasn't being deleted when the session exited because OnnxTransformer didn't support IDisposable. (Also added Dispose calls to OnnxTransformer objects in the tests)

Thanks to the reported bug, this also fixes memory leaks that we would see from OnnxRuntime.dll because the native memory associated with InferenceSession wasn't freed.

@codecov
Copy link

codecov bot commented Aug 15, 2020

Codecov Report

Merging #5348 into master will increase coverage by 0.00%.
The diff coverage is 68.83%.

@@           Coverage Diff           @@
##           master    #5348   +/-   ##
=======================================
  Coverage   74.04%   74.04%           
=======================================
  Files        1019     1019           
  Lines      189949   190005   +56     
  Branches    20429    20455   +26     
=======================================
+ Hits       140651   140696   +45     
+ Misses      43786    43779    -7     
- Partials     5512     5530   +18     
Flag Coverage Δ
#Debug 74.04% <68.83%> (+<0.01%) ⬆️
#production 69.85% <71.42%> (+<0.01%) ⬆️
#test 87.64% <68.57%> (-0.04%) ⬇️

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

Impacted Files Coverage Δ
test/Microsoft.ML.Tests/OnnxConversionTest.cs 96.17% <36.36%> (-0.52%) ⬇️
src/Microsoft.ML.OnnxTransformer/OnnxTransform.cs 91.60% <66.66%> (-0.38%) ⬇️
...osoft.ML.OnnxTransformerTest/OnnxTransformTests.cs 95.33% <74.13%> (-2.34%) ⬇️
src/Microsoft.ML.OnnxTransformer/OnnxUtils.cs 86.28% <100.00%> (-0.13%) ⬇️
...soft.ML.TestFramework/DataPipe/TestDataPipeBase.cs 73.88% <100.00%> (+0.03%) ⬆️
...L.AutoML/TrainerExtensions/TrainerExtensionUtil.cs 84.71% <0.00%> (-1.66%) ⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs 86.53% <0.00%> (-0.15%) ⬇️
...rosoft.ML.Data/Scorers/PredictedLabelScorerBase.cs 81.95% <0.00%> (+0.37%) ⬆️
...Microsoft.ML.Data/Scorers/PredictionTransformer.cs 91.00% <0.00%> (+0.50%) ⬆️
src/Microsoft.ML.Featurizers/TimeSeriesImputer.cs 88.37% <0.00%> (+1.22%) ⬆️
... and 2 more

@harishsk harishsk merged commit 2933216 into dotnet:master Aug 16, 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.

Load ONNX model creates temp file
4 participants