dotnet / TorchSharp

A .NET library that provides access to the library that powers PyTorch.
MIT License
1.41k stars 183 forks source link

Resolves #1387 and backfill tests for ThreadDisposeScopeStatistics #1389

Closed mvphelps closed 1 month ago

mvphelps commented 1 month ago

Added test to verify that attaching an unscoped Tensor or PackedSequence no longer decrements DetachedFromScopeCount, making it possibly become negative. Note also removed the scope counting tests from TestDisposeScopesPackedSequence, which now only tests disposal behavior. Metrics are now all tested in a dedicated set of test fixtures.

Added additional tests to document behavior and improve test coverage for both Tensor and PackedSequence objects, in scenarios for both scoped and unscoped instances of these objects (total of 4 new fixtures). This should help future changes as one can see what the system does to count the statistics.

Tests reflect the expected behavior based on the doc comments for ThreadDisposeScopeStatistics. Note that the ThreadTotalLiveCount property is still "approximate" and can go negative, not changes to this behavior were made.

Let me know if I've added too much here. I really wasn't sure what was supposed to be counted when, so I just went though it incrementally and added the tests to document it.

NiklasGustafsson commented 1 month ago

Since this resolves an issue, that fact must go in the RELEASENOTES.md file.

mvphelps commented 1 month ago

Release notes updated!

NiklasGustafsson commented 4 weeks ago

@mvphelps, I think there's a problem handling the statistics for this situation (in Parameter.cs):

        public class Parameter : Tensor
        {
            /// <summary>
            /// Constructor
            /// </summary>
            /// <param name="data">A tensor, which will become empty.</param>
            /// <param name="requires_grad"></param>
            public Parameter(Tensor data, bool requires_grad = true) :
                base(data.with_requires_grad(requires_grad).MoveHandle())
            {
                var scope = data.OwningDisposeScope;
                if (scope is not null) {
                    this.OwningDisposeScope = scope;
                    scope.Attach(this);
                    scope.Detach(data);
                }
            }

            /// <summary>
            /// Constructor
            /// </summary>
            /// <param name="handle">A tensor handle.</param>
            internal Parameter(System.IntPtr handle) : base(handle)
            {
            }
        };

I don't have a small repo, but I don't think the stats are collected properly when a parameter takes over the native handle from a non-Parameter tensor and registers itself with its dispose scope.

There are two tests that fail when I implement Linear in .NET rather than native code.