Merge pull request #1389 from mvphelps/fix1387

NiklasGustafsson · web-flow · commit 1a3b2c85b48e · 2024-10-23T09:11:05.000-07:00
Resolves #1387 and backfill tests for ThreadDisposeScopeStatistics
diff --git a/RELEASENOTES.md b/RELEASENOTES.md
@@ -11,6 +11,7 @@ __Bug Fixes__:
 
 #1383 `torch.linalg.vector_norm`: Make `ord`-argument optional, as specified in docs<br/>
 #1385 PackedSequence now participates in the DisposeScope system at the same level as Tensor objects.<br/>
+#1387 Attaching tensor to a DisposeScope no longer makes Statistics.DetachedFromScopeCount go negative.<br/>
 
 # NuGet Version 0.103.0
 
diff --git a/src/TorchSharp/DisposeScope.cs b/src/TorchSharp/DisposeScope.cs
@@ -237,17 +237,6 @@ public IReadOnlyList<IDisposable> Attach(IEnumerable<IDisposable> disposables)
 
             var result = new List<IDisposable>();
             foreach (var disposable in disposables) {
-                if (disposable is torch.Tensor tensor) {
-                    if (tensor.OwningDisposeScope == null && !tensor.IsInvalid) {
-                        _disposeScopeManager.StatisticsInstance.DetachedFromScopeCount--;
-                    }
-                }
-                else if (disposable is torch.nn.utils.rnn.PackedSequence sequence) {
-                    if (sequence.OwningDisposeScope == null && !sequence.IsInvalid) {
-                        _disposeScopeManager.StatisticsInstance.DetachedFromScopeCount--;
-                    }
-                }
-
                 AddToOther(this, disposable);
                 result.Add(disposable);
             }
diff --git a/src/TorchSharp/ThreadDisposeScopeStatistics.cs b/src/TorchSharp/ThreadDisposeScopeStatistics.cs
@@ -27,7 +27,7 @@ public class ThreadDisposeScopeStatistics
         public long DetachedFromScopeCount { get; internal set; }
 
         /// <summary>
-        /// The number of disposables that are currently live on the current thread. If a It's aproximate, see
+        /// The number of disposables that are currently live on the current thread. It's approximate, see
         /// Tensor.TotalCount. Disposables that weren't created within a DisposeScope, or detached from the dispose
         /// scope, will not be counted as alive.
         /// </summary>
diff --git a/test/TorchSharpTest/TestDisposeScopesPackedSequence.cs b/test/TorchSharpTest/TestDisposeScopesPackedSequence.cs
@@ -1,121 +1,82 @@
 using System.Reflection;
-using TorchSharp;
 using Xunit;
 
-namespace TorchSharpTest;
-
-public class TestDisposeScopesPackedSequence
+namespace TorchSharp
 {
-    [Fact]
-    public void MoveDisposeScope()
+    [Collection("Sequential")]
+    public class TestDisposeScopesPackedSequence
     {
-        var sequences = CreateTestSequences();
-        torch.nn.utils.rnn.PackedSequence packed_sequence;
-        var otherScope = torch.NewDisposeScope();
-        using (torch.NewDisposeScope())
+        [Fact]
+        public void MoveDisposeScope()
         {
-            using (torch.NewDisposeScope())
-            {
-                packed_sequence = torch.nn.utils.rnn.pack_sequence(sequences, enforce_sorted: false);
+            var sequences = CreateTestSequences();
+            torch.nn.utils.rnn.PackedSequence packed_sequence;
+            var otherScope = torch.NewDisposeScope();
+            using (torch.NewDisposeScope()) {
+                using (torch.NewDisposeScope()) {
+                    packed_sequence = torch.nn.utils.rnn.pack_sequence(sequences, enforce_sorted: false);
+                    AssertPackedSequenceValid(packed_sequence);
+
+                    packed_sequence.MoveToOuterDisposeScope();
+                }
+
                 AssertPackedSequenceValid(packed_sequence);
 
-                packed_sequence.MoveToOuterDisposeScope();
+                packed_sequence.MoveToOtherDisposeScope(otherScope);
             }
+
             AssertPackedSequenceValid(packed_sequence);
+            otherScope.Dispose();
 
-            packed_sequence.MoveToOtherDisposeScope(otherScope);
+            Assert.True(GetPackedSequenceIsInvalid(packed_sequence));
+            Assert.True(packed_sequence.data.IsInvalid);
         }
 
-        AssertPackedSequenceValid(packed_sequence);
-        otherScope.Dispose();
-
-        Assert.True(GetPackedSequenceIsInvalid(packed_sequence));
-        Assert.True(packed_sequence.data.IsInvalid);
-    }
-
-    [Fact]
-    public void DisposablesValidityWhenNotSorted()
-    {
-        var sequences = CreateTestSequences();
-        using var scope = torch.NewDisposeScope();
-        var packed = torch.nn.utils.rnn.pack_sequence(sequences, enforce_sorted: false);
-        Assert.Equal(1, scope.DisposablesCount);
-        AssertPackedSequenceValid(packed);
-    }
-
-    [Fact]
-    public void DisposablesValidityWhenSorted()
-    {
-        var sequences = CreateTestSequences();
-        using var scope = torch.NewDisposeScope();
-        var packed = torch.nn.utils.rnn.pack_sequence(sequences, enforce_sorted: true);
-        Assert.Equal(1, scope.DisposablesCount);
-        Assert.False(GetPackedSequenceIsInvalid(packed));
-        Assert.False(packed.batch_sizes.IsInvalid);
-        Assert.False(packed.data.IsInvalid);
-        Assert.True(packed.sorted_indices.IsInvalid);
-        Assert.True(packed.unsorted_indices.IsInvalid);
-    }
-
-    [Fact]
-    public void DisposeScopeStatistics()
-    {
-        DisposeScopeManager.Statistics.Reset();
-        AssertStatCounts(0, 0, 0, 0, 0);
-        var sequences = CreateTestSequences();
-        AssertStatCounts(0, 2, 0, 0, 0);
-        var outOfScope = torch.nn.utils.rnn.pack_sequence(sequences, enforce_sorted: true);
-        AssertStatCounts(0, 7, 0, 0, 0);
-        using var scope = torch.NewDisposeScope();
-        AssertStatCounts(0, 7, 0, 0, 0);
-
-        var inScope = torch.nn.utils.rnn.pack_sequence(sequences, enforce_sorted: true);
-        AssertStatCounts(5, 7, 4, 0, 1);
-
-        scope.Attach(outOfScope);
-        //Possible subtle bug. When attaching an object that isn't owned by any scope, the count subtracts.
-        AssertStatCounts( 5, 7, 3, 0, 2);
-
-        scope.Detach(inScope);
-        AssertStatCounts( 5, 7, 4, 0, 1);
-
-        outOfScope.Dispose();
-        AssertStatCounts( 5, 7, 4, 5, -4);
-
-    }
+        [Fact]
+        public void DisposablesValidityWhenNotSorted()
+        {
+            var sequences = CreateTestSequences();
+            using var scope = torch.NewDisposeScope();
+            var packed = torch.nn.utils.rnn.pack_sequence(sequences, enforce_sorted: false);
+            Assert.Equal(1, scope.DisposablesCount);
+            AssertPackedSequenceValid(packed);
+        }
 
-    private static void AssertStatCounts(long createdInScope, long createdOutsideScope, long detachedFrom, long disposedIn, long threadTotalLive)
-    {
-        var stats = DisposeScopeManager.Statistics;
-        Assert.Equal(createdInScope, stats.CreatedInScopeCount);
-        Assert.Equal(createdOutsideScope, stats.CreatedOutsideScopeCount);
-        Assert.Equal(detachedFrom, stats.DetachedFromScopeCount);
-        Assert.Equal(disposedIn, stats.DisposedInScopeCount);
-        Assert.Equal(threadTotalLive, stats.ThreadTotalLiveCount);
-    }
+        [Fact]
+        public void DisposablesValidityWhenSorted()
+        {
+            var sequences = CreateTestSequences();
+            using var scope = torch.NewDisposeScope();
+            var packed = torch.nn.utils.rnn.pack_sequence(sequences, enforce_sorted: true);
+            Assert.Equal(1, scope.DisposablesCount);
+            Assert.False(GetPackedSequenceIsInvalid(packed));
+            Assert.False(packed.batch_sizes.IsInvalid);
+            Assert.False(packed.data.IsInvalid);
+            Assert.True(packed.sorted_indices.IsInvalid);
+            Assert.True(packed.unsorted_indices.IsInvalid);
+        }
 
-    private static torch.Tensor[] CreateTestSequences()
-    {
-        return new[]
+        private static torch.Tensor[] CreateTestSequences()
         {
-            torch.tensor(new long[] { 1, 2, 3, 4 }),
-            torch.tensor(new long[] { 5, 6 }),
-        };
-    }
+            return new[] { torch.tensor(new long[] { 1, 2, 3, 4 }), torch.tensor(new long[] { 5, 6 }), };
+        }
 
-    private static void AssertPackedSequenceValid(torch.nn.utils.rnn.PackedSequence packed_sequence)
-    {
-        Assert.False(GetPackedSequenceIsInvalid(packed_sequence));
-        Assert.False(packed_sequence.batch_sizes.IsInvalid);
-        Assert.False(packed_sequence.data.IsInvalid);
-        Assert.False(packed_sequence.sorted_indices.IsInvalid);
-        Assert.False(packed_sequence.unsorted_indices.IsInvalid);
-    }
+        private static void AssertPackedSequenceValid(torch.nn.utils.rnn.PackedSequence packed_sequence)
+        {
+            Assert.False(GetPackedSequenceIsInvalid(packed_sequence));
+            Assert.False(packed_sequence.batch_sizes.IsInvalid);
+            Assert.False(packed_sequence.data.IsInvalid);
+            Assert.False(packed_sequence.sorted_indices.IsInvalid);
+            Assert.False(packed_sequence.unsorted_indices.IsInvalid);
+        }
 
-    private static bool GetPackedSequenceIsInvalid(torch.nn.utils.rnn.PackedSequence packed_sequence)
-    {
-        //HACK: reflection to avoid exposing internal method IsInvalid in API.
-        var getter = typeof(torch.nn.utils.rnn.PackedSequence).GetProperty("IsInvalid", BindingFlags.Instance | BindingFlags.NonPublic)!;
-        return (bool)getter.GetValue(packed_sequence)!;
+        private static bool GetPackedSequenceIsInvalid(torch.nn.utils.rnn.PackedSequence packed_sequence)
+        {
+            //HACK: reflection to avoid exposing internal method IsInvalid in API.
+            var getter =
+                typeof(torch.nn.utils.rnn.PackedSequence).GetProperty("IsInvalid",
+                    BindingFlags.Instance | BindingFlags.NonPublic)!;
+            return (bool)getter.GetValue(packed_sequence)!;
+        }
     }
 }
diff --git a/test/TorchSharpTest/TestDisposeScopesStatisticsBase.cs b/test/TorchSharpTest/TestDisposeScopesStatisticsBase.cs
@@ -0,0 +1,23 @@
+using Xunit;
+
+namespace TorchSharp
+{
+    public class TestDisposeScopesStatisticsBase
+    {
+        protected static void ResetStats()
+        {
+            DisposeScopeManager.Statistics.Reset();
+            AssertStatCounts(0, 0, 0, 0, 0);
+        }
+
+        protected static void AssertStatCounts(long createdInScope, long createdOutsideScope, long detachedFrom, long disposedIn, long threadTotalLive)
+        {
+            var stats = DisposeScopeManager.Statistics;
+            Assert.True(createdInScope == stats.CreatedInScopeCount, $"CreatedInScope: Expected({createdInScope})!={stats.CreatedInScopeCount}");
+            Assert.True(createdOutsideScope == stats.CreatedOutsideScopeCount, $"CreatedOutsideScopeCount: Expected({createdOutsideScope})!={stats.CreatedOutsideScopeCount}");
+            Assert.True(detachedFrom == stats.DetachedFromScopeCount, $"DetachedFromScopeCount: Expected({detachedFrom})!={stats.DetachedFromScopeCount}");
+            Assert.True(disposedIn == stats.DisposedInScopeCount, $"DisposedInScopeCount: Expected({disposedIn})!={stats.DisposedInScopeCount}");
+            Assert.True(threadTotalLive == stats.ThreadTotalLiveCount, $"ThreadTotalLiveCount: Expected({threadTotalLive})!={stats.ThreadTotalLiveCount}");
+        }
+    }
+}
diff --git a/test/TorchSharpTest/TestDisposeScopesStatisticsPackedSequence.cs b/test/TorchSharpTest/TestDisposeScopesStatisticsPackedSequence.cs
@@ -0,0 +1,80 @@
+using TorchSharp;
+using Xunit;
+
+namespace TorchSharp
+{
+    [Collection("Sequential")]
+    public class TestDisposeScopesStatisticsPackedSequenceScoped : TestDisposeScopesStatisticsBase
+    {
+
+        //When reviewing these tests, refer first to TestDisposeScopesStatisticsTensor. The tests here are
+        //essentially identical, except that the numbers all look weird because a PackedSequence is constructed
+        //from Tensors, and contains Tensors that it detaches from any scope and manages itself.
+
+
+        [Fact]
+        public void CreatingIncrementsCreatedInScope()
+        {
+            using var scope = torch.NewDisposeScope();
+            ResetStats();
+            var ps = TestDisposeScopesStatisticsPackedSequenceUnscoped.Create();
+            //CreatedInScope = 2 tensors (source data) + 4 internal PackedSequence tensors, + the PackedSequence
+            //DetachedFromScope = the 4 internal PackedSequenceTensors
+            AssertStatCounts(7, 0, 4, 0, 3);
+        }
+
+        [Fact]
+        public void AttachingIncrementsNothing()
+        {
+            using var scope = torch.NewDisposeScope();
+            var ps = TestDisposeScopesStatisticsPackedSequenceUnscoped.Create();
+            ResetStats();
+            scope.Attach(ps);
+            AssertStatCounts(0, 0, 0, 0, 0);
+        }
+
+        [Fact]
+        public void DetachingIncrementsDetached()
+        {
+            using var scope = torch.NewDisposeScope();
+            var ps = TestDisposeScopesStatisticsPackedSequenceUnscoped.Create();
+            ResetStats();
+            scope.Detach(ps);
+            AssertStatCounts(0, 0, 1, 0, -1);
+        }
+
+        [Fact]
+        public void DisposingIncrementsDisposed()
+        {
+            using var scope = torch.NewDisposeScope();
+            var ps = TestDisposeScopesStatisticsPackedSequenceUnscoped.Create();
+            ResetStats();
+            ps.Dispose();
+            //DisposedInScope = The PackedSequence + the 4 internal tensors
+            AssertStatCounts(0, 0, 0, 5, -5);
+        }
+
+        [Fact]
+        public void DisposingScopeIncrementsDisposed()
+        {
+            var scope = torch.NewDisposeScope();
+            var ps = TestDisposeScopesStatisticsPackedSequenceUnscoped.Create();
+            ResetStats();
+            scope.Dispose();
+            //DisposedInScope = The PackedSequence and the 2 source data tensors. The internal tensors and not on the scope.
+            AssertStatCounts(0, 0, 0, 3, -3);
+        }
+
+        [Fact]
+        public void DisposingScopeAfterDetachingCountsOnlyTheSourceDataTensors()
+        {
+            var scope = torch.NewDisposeScope();
+            var ps = TestDisposeScopesStatisticsPackedSequenceUnscoped.Create();
+            scope.Detach(ps);
+            ResetStats();
+            scope.Dispose();
+            //DisposedInScope just the two source data tensors.
+            AssertStatCounts(0, 0, 0, 2, -2);
+        }
+    }
+}
diff --git a/test/TorchSharpTest/TestDisposeScopesStatisticsPackedSequenceUnscoped.cs b/test/TorchSharpTest/TestDisposeScopesStatisticsPackedSequenceUnscoped.cs
diff --git a/test/TorchSharpTest/TestDisposeScopesStatisticsTensor.cs b/test/TorchSharpTest/TestDisposeScopesStatisticsTensor.cs
diff --git a/test/TorchSharpTest/TestDisposeScopesStatisticsTensorUnscoped.cs b/test/TorchSharpTest/TestDisposeScopesStatisticsTensorUnscoped.cs
