Added IDisposable support for several classes #4939

harishsk · 2020-03-12T18:26:49Z

We have had a lot of instability in the Tensorflow tests. At least one of the issues has to do with memory leaks from ImageClassificationTrainer which holds references to the Tensorflow session and graphs. In order to fix this I have added IDisposable support for several of the classes involved in this scenario and fixed up the tests to call Dispose at the end of the tests.

sharwell · 2020-03-22T19:40:01Z

src/Microsoft.ML.Data/DataLoadSave/CompositeDataLoader.cs

+        #region IDisposable Support
+        private bool _disposed;
+
+        private void Dispose(bool disposing)


📝 This is the legacy IDisposable pattern. The new way to implement this is moving this implementation code directly into IDisposable.Dispose(). #Resolved

Weren't there two cases to be handled before? The one that calls Dispose(true) from from IDisposable.Dispose() and the other that calls Dispose(false) from the finalizer? (Or someplace else...I am not quite sure). How would that be handled?

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

the other that calls Dispose(false) from the finalizer

Finalizers are not allowed under the new dispose pattern. Since there are no finalizers, there are no calls to Dispose(false). Since there are no calls to dispose false, there is no need to add complexity by defining the method. #Resolved

sharwell · 2020-03-22T19:40:37Z

src/Microsoft.ML.Data/DataLoadSave/CompositeDataLoader.cs

@@ -15,7 +16,7 @@ namespace Microsoft.ML.Data
    /// This class represents a data loader that applies a transformer chain after loading.
    /// It also has methods to save itself to a repository.
    /// </summary>
-    public sealed class CompositeDataLoader<TSource, TLastTransformer> : IDataLoader<TSource>
+    public sealed class CompositeDataLoader<TSource, TLastTransformer> : IDataLoader<TSource>, IDisposable


📝 By making the class disposable when IDataLoader<TSource> is not disposable, it will be easy for a consumer to fail to dispose of the object. #Resolved

We can't change interfaces now that they are shipped. Right now, when customers discover they have a memory leak, we don't have a solution. With this method at least we can tell them to add a Dispose call.

Is there a better approach we can consider?

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

Every case that I reviewed locally required breaking API changes. Default interface methods might be an option for .NET Core targets, but those are completely unsupported on .NET Framework. #Resolved

Is the component GA? Or can we still make a breaking change? #Resolved

All public interfaces are frozen. This would be one of those.

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

All public interfaces are frozen. This would be one of those.

There may be a difference between interfaces released as GA and the preview releases. I'm not sure which one this falls under.

/cc @eerhardt

IDataLoader is a core interface. It is part of the GA set.

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

test/Microsoft.ML.Tests/ScenariosWithDirectInstantiation/TensorflowTests.cs

…ging test

frank-dong-ms-zz · 2020-03-23T04:06:13Z

test/Microsoft.ML.Tests/ScenariosWithDirectInstantiation/TensorflowTests.cs

@@ -463,6 +471,7 @@ public void TensorFlowTransformInputOutputTypesTest()
                }
                Assert.False(cursor.MoveNext());
            }
+            (tfModel as IDisposable).Dispose();


(tfModel as IDisposable).Dispose(); [](start = 12, length = 35)

(tfModel as IDisposable)?.Dispose();

frank-dong-ms-zz · 2020-03-23T04:10:47Z

test/Microsoft.ML.Tests/ScenariosWithDirectInstantiation/TensorflowTests.cs

        public void TensorFlowImageClassification(ImageClassificationTrainer.Architecture arch)
        {
+            if (NotCentOS7FactAttribute.IsCentOS7())


IsCentOS7 [](start = 40, length = 9)

as discussed pleas also skip in Ubuntu as this also hangs on Ubuntu like below:

https://dev.azure.com/dnceng/public/_build/results?buildId=556713&view=logs&j=28859320-f5de-51e0-1fd2-7bea8c11cf7a&t=28348391-228f-5cbf-8fcd-3272a0755d32
https://dev.azure.com/dnceng/public/_build/results?buildId=550567&view=logs&j=5aa5c7df-492a-5eaf-973a-62b7b0f0ee3b&t=ffdbd604-f3e2-5332-cf61-c8dd00799b47
https://dev.azure.com/dnceng/public/_build/results?buildId=549005&view=logs&j=28859320-f5de-51e0-1fd2-7bea8c11cf7a&t=28348391-228f-5cbf-8fcd-3272a0755d32

frank-dong-ms-zz

eerhardt · 2020-03-23T17:41:21Z

src/Microsoft.ML.Data/DataLoadSave/CompositeDataLoader.cs

@@ -38,13 +39,15 @@ public CompositeDataLoader(IDataLoader<TSource> loader, TransformerChain<TLastTr

            Loader = loader;
            Transformer = transformerChain ?? new TransformerChain<TLastTransformer>();
+            _disposed = false;


bools are initialized to false by the runtime. There is no need to re-initialize them to false here.

eerhardt · 2020-03-23T17:43:06Z

src/Microsoft.ML.Data/DataLoadSave/CompositeDataLoader.cs

+            _disposed = true;
+        }
+
+        void IDisposable.Dispose()


Why are you explicitly implementing IDisposable? Instead, you can just have a public void Dispose() method here.

The reason to not explicitly implement an interface is so users of the class can just call Dispose() as necessary. They don't need to cast to IDisposable to call it.

eerhardt · 2020-03-23T17:46:47Z

src/Microsoft.ML.Data/Scorers/MulticlassClassificationScorer.cs

+                if (_disposed)
+                    return;
+
+                (_bindable as IDisposable)?.Dispose();


ISchemaBindableMapper is internal, so if you want you can make it inherit from IDisposable without a breaking change.

eerhardt · 2020-03-23T17:52:28Z

test/Microsoft.ML.Tests/ScenariosWithDirectInstantiation/TensorflowTests.cs

@@ -154,6 +154,8 @@ public void TensorFlowTransforCifarEndToEndTest2()
            Assert.Equal(0, prediction.PredictedScores[0], 2);
            Assert.Equal(0, prediction.PredictedScores[1], 2);
            Assert.Equal(1, prediction.PredictedScores[2], 2);
+
+            transformer.Dispose();


You can use the using statement in all these tests. It will make this cleaner.

https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/using-statement

justinormont · 2020-03-23T21:16:13Z

test/Microsoft.ML.Tests/ScenariosWithDirectInstantiation/TensorflowTests.cs

@@ -389,7 +385,7 @@ public void TensorFlowTransformInputOutputTypesTest()

            var inputs = new string[] { "f64", "f32", "i64", "i32", "i16", "i8", "u64", "u32", "u16", "u8", "b" };
            var outputs = new string[] { "o_f64", "o_f32", "o_i64", "o_i32", "o_i16", "o_i8", "o_u64", "o_u32", "o_u16", "o_u8", "o_b" };
-            var tfModel = mlContext.Model.LoadTensorFlowModel(modelLocation);
+            using var tfModel = mlContext.Model.LoadTensorFlowModel(modelLocation);


Consistency/user-expectations:
How will a user know which model types and prediction engines are disposable? Should we convert all models and prediction engines to be disposable?

Seems it should be an all-or-none for consistency.

I agree that it should be all or none for consistency. If we go for none, then we need to make the interfaces inherit from IDisposable and we can't do that now without a breaking change. If for "all" then the Dispose() calls for most would become no-ops and only Tensorflow (right now) needs a real Dispose() call.
This seems like one of those cases that we will need to fix whenever we do a v2 release and can take breaking changes. Until then, I am okay with documenting for the tensorflow related transformers.

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

justinormont · 2020-03-23T21:22:15Z

test/Microsoft.ML.Tests/ScenariosWithDirectInstantiation/TensorflowTests.cs

@@ -1185,10 +1189,10 @@ public void TensorFlowSentimentClassificationTest()
            // For explanation on how was the `sentiment_model` created 
            // c.f. https://github.com/dotnet/machinelearning-testdata/blob/master/Microsoft.ML.TensorFlow.TestModels/sentiment_model/README.md
            string modelLocation = @"sentiment_model";
-            var pipelineModel = mlContext.Model.LoadTensorFlowModel(modelLocation).ScoreTensorFlowModel(new[] { "Prediction/Softmax" }, new[] { "Features" })
+            using var pipelineModel = mlContext.Model.LoadTensorFlowModel(modelLocation).ScoreTensorFlowModel(new[] { "Prediction/Softmax" }, new[] { "Features" })


Similar changes may need to be made in the AutoML code, its use in Model Builder, and in CodeGen. Otherwise it will continue to leak in those components.

Todo: In a follow-up PR -- if needed add a using or call dispose() for TF models and TF prediction engines.

/cc @LittleLittleCloud

I have updated the docs and samples. I could not figure out easily where the corresponding fixes need to be bade in AutoML, Model Builder and CodeGen. @LittleLittleCloud, can you please help with that?

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

justinormont

LGTM from the AutoML side.

May want to file an issue for follow up work on the AutoML API & CodeGen fronts in this repo, an issue in the dotnet/machinelearning-samples repo for follow up work on the TF and the AutoML API samples; and another in the dotnet/machinelearning-modelbuilder.

(update) Should create an issue for updating docs to educate users when they have to dispose and when they do not.

This comment has been minimized.

Sign in to view

harishsk added 5 commits March 13, 2020 16:46

Draft PR to debug tensorflow issues

1b871b6

Added RunSpecific flags for tensorflow tests

e18ec7e

Added runSpecific:true

2d5a186

Added IDisposable support to try and fix memory leaks

98f5a10

Fixed build errors

c0fa246

This comment has been minimized.

Sign in to view

harishsk added 2 commits March 21, 2020 13:52

Disabling hanging test

07ef104

Changed disabled test

3c96cad

This comment has been minimized.

Sign in to view

harishsk marked this pull request as ready for review March 22, 2020 19:36

harishsk requested a review from a team as a code owner March 22, 2020 19:36

harishsk requested review from eerhardt, frank-dong-ms-zz, yaeldekel and ganik March 22, 2020 19:37

harishsk changed the title ~~Draft PR to debug tensorflow issues~~ Added IDisposable support for several classes Mar 22, 2020

sharwell reviewed Mar 22, 2020

View reviewed changes

test/Microsoft.ML.Tests/ScenariosWithDirectInstantiation/TensorflowTests.cs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

harishsk added 3 commits March 22, 2020 14:41

Added null checks for IDisposable conversion and disabled another han…

f12d5d3

…ging test

Fixed IDisposable implementation

60e8238

Skipped hanging tests only for CentOS

34fa8d5

frank-dong-ms-zz reviewed Mar 23, 2020

View reviewed changes

frank-dong-ms-zz approved these changes Mar 23, 2020

View reviewed changes

Enabled additional tests and addressed review comments

98647ca

harishsk requested a review from a team as a code owner March 23, 2020 05:48

eerhardt reviewed Mar 23, 2020

View reviewed changes

Addressed code review comments

65c3297

justinormont reviewed Mar 23, 2020

View reviewed changes

Updated docs, samples and additional tests

cb96929

justinormont approved these changes Mar 24, 2020

View reviewed changes

harishsk merged commit 5d531d3 into dotnet:master Mar 24, 2020

mstfbl mentioned this pull request Mar 27, 2020

Fixed & Reactivated AutoFitImageClassificationTrainTest hanging by freeing old Tensor objects #4978

Closed

ghost locked as resolved and limited conversation to collaborators Mar 18, 2022

Added IDisposable support for several classes #4939

Added IDisposable support for several classes #4939

Conversation

harishsk commented Mar 12, 2020 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

sharwell Mar 22, 2020 • edited by harishsk Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Mar 22, 2020 • edited by harishsk Loading

Choose a reason for hiding this comment

sharwell Mar 22, 2020 • edited by harishsk Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Mar 22, 2020 • edited by harishsk Loading

Choose a reason for hiding this comment

justinormont Mar 23, 2020 • edited by harishsk Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frank-dong-ms-zz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinormont Mar 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinormont left a comment • edited Loading

Choose a reason for hiding this comment

harishsk commented Mar 12, 2020 •

edited

Loading

sharwell Mar 22, 2020 •

edited by harishsk

Loading

sharwell Mar 22, 2020 •

edited by harishsk

Loading

sharwell Mar 22, 2020 •

edited by harishsk

Loading

sharwell Mar 22, 2020 •

edited by harishsk

Loading

justinormont Mar 23, 2020 •

edited by harishsk

Loading

justinormont Mar 23, 2020 •

edited

Loading

justinormont left a comment •

edited

Loading