perf: DiffusionUnitTestBase forces GC between Unit-03 tests (#1136) - #1148
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Contributor
WalkthroughAdds a new abstract xUnit base class Changes
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly and specifically summarizes the main change: introducing DiffusionUnitTestBase to force garbage collection between diffusion unit tests for performance. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
fix/unit03-diffusion-test-lifecycle
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/AiDotNet.Tests/UnitTests/Diffusion/DiffusionUnitTestBase.cs`:
- Around line 52-57: The DisposeAsync teardown should explicitly compact the
Large Object Heap (LOH) before collecting to avoid LOH fragmentation; modify the
DisposeAsync method to set GCSettings.LargeObjectHeapCompactionMode =
GCLargeObjectHeapCompactionMode.CompactOnce (add using System.Runtime if
missing) immediately before the first GC.Collect(), then call GC.Collect(),
GC.WaitForPendingFinalizers(), and GC.Collect() as currently done, and return
Task.CompletedTask; keep the method name DisposeAsync unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bd73531c-37d4-43a8-be4d-832fd04d4e6c
📒 Files selected for processing (11)
tests/AiDotNet.Tests/UnitTests/Diffusion/AudioProcessingTests.cstests/AiDotNet.Tests/UnitTests/Diffusion/DiffusionUnitTestBase.cstests/AiDotNet.Tests/UnitTests/Diffusion/MemoryManagementTests.cstests/AiDotNet.Tests/UnitTests/Diffusion/Models/ControlModelContractTests.cstests/AiDotNet.Tests/UnitTests/Diffusion/Models/DDPMModelTests.cstests/AiDotNet.Tests/UnitTests/Diffusion/Models/DiffusionModelContractTests.cstests/AiDotNet.Tests/UnitTests/Diffusion/Models/EditingModelContractTests.cstests/AiDotNet.Tests/UnitTests/Diffusion/Models/FastGenContractTests.cstests/AiDotNet.Tests/UnitTests/Diffusion/Models/NewConditionerContractTests.cstests/AiDotNet.Tests/UnitTests/Diffusion/Models/PreprocessorContractTests.cstests/AiDotNet.Tests/UnitTests/Diffusion/SchedulerTests.cs
ooples
pushed a commit
that referenced
this pull request
Apr 15, 2026
Addresses CodeRabbit review comment on PR #1148: plain GC.Collect() sweeps but does NOT compact the Large Object Heap (LOH), which is critical here because diffusion model weight tensors are typically several hundred MB each — well above the 85KB LOH threshold. ## Why LOH compaction matters .NET's default GC.Collect sweeps the LOH but leaves freed regions fragmented (LargeObjectHeapCompactionMode.Default). Across ~200 sequential diffusion tests, fragmentation accumulates even when total free bytes is large — eventually the next test's weight allocation fails because no contiguous region is large enough, producing an OutOfMemoryException on a runner that APPEARS to have plenty of memory. Setting LargeObjectHeapCompactionMode.CompactOnce before a blocking Gen-2 collection forces the LOH to compact on that pass, eliminating fragmentation as an OOM vector. The mode auto-resets to Default after the compacting pass, so this is scoped per-teardown — no process-wide side-effect. ## Change ```csharp public Task DisposeAsync() { GCSettings.LargeObjectHeapCompactionMode = GCLargeObjectHeapCompactionMode.CompactOnce; GC.Collect(generation: 2, mode: GCCollectionMode.Forced, blocking: true, compacting: true); GC.WaitForPendingFinalizers(); // Second pass reclaims finalizer-freed memory and compacts anything // allocated by finalizers (e.g. GPU-pool return paths). GCSettings.LargeObjectHeapCompactionMode = GCLargeObjectHeapCompactionMode.CompactOnce; GC.Collect(generation: 2, mode: GCCollectionMode.Forced, blocking: true, compacting: true); return Task.CompletedTask; } ``` ## Tradeoff LOH compaction is expensive (.NET scans and relocates every LOH object), so per-test teardown adds ~tens to hundreds of ms depending on heap size. That's acceptable here: the alternative was CI cancellation at the 45-min wall clock from OOM. Expensive beats broken. Per .NET docs, the compaction overhead is amortized across subsequent allocations finding contiguous space — net throughput improves in allocation- heavy workloads like these tests. Build clean on net10.0. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Deployment failed with the following error: Learn More: https://vercel.com/franklins-projects-02a0b5a0?upgradeToPro=build-rate-limit |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/AiDotNet.Tests/UnitTests/Diffusion/DiffusionUnitTestBase.cs`:
- Line 53: The DisposeAsync() method in DiffusionUnitTestBase is mutating the
process-wide GCSettings.LargeObjectHeapCompactionMode twice and calling
GC.Collect() without synchronization; wrap the entire LOH compaction sequence
(both assignments to GCSettings.LargeObjectHeapCompactionMode and the
GC.Collect() calls) in a static lock object so all concurrent teardowns are
serialized, ensuring only one thread at a time toggles
LargeObjectHeapCompactionMode and performs the GC.Collect() calls to guarantee
deterministic LOH compaction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8073720e-9812-4f09-a319-ac8f9f81db0a
📒 Files selected for processing (1)
tests/AiDotNet.Tests/UnitTests/Diffusion/DiffusionUnitTestBase.cs
ooples
pushed a commit
that referenced
this pull request
Apr 15, 2026
Upgrades the IAsyncLifetime DisposeAsync hook added in the prior commit to use a blocking COMPACTING Gen-2 collection with explicit Large Object Heap compaction, matching the pattern applied on PR #1148's DiffusionUnitTestBase. Diffusion model weight tensors are typically several hundred MB each, well above the 85KB LOH threshold. Plain `GC.Collect` sweeps the LOH but does NOT compact it — across ~255 sequential diffusion tests, LOH fragmentation accumulates until the next allocation can't find a contiguous region even though total free bytes remain large, producing `OutOfMemoryException` on a runner that APPEARS to have plenty of memory. Setting `GCLargeObjectHeapCompactionMode.CompactOnce` on the next Gen-2 pass forces LOH compaction; the mode auto-resets to Default after each use, so this is scoped per-teardown — no process-wide side-effect. Same fix applied on PR #1148 (b4aa53f) per CodeRabbit review feedback. Applying here too for consistency — both test bases run diffusion models with similar LOH fragmentation risk. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ooples
pushed a commit
that referenced
this pull request
Apr 15, 2026
Addresses the round-2 CodeRabbit review comment on PR #1148: xunit parallelizes across test-classes by default (methods within a class stay sequential), so two test classes that both inherit from DiffusionUnitTestBase can hit DisposeAsync concurrently on different threads. GCSettings.LargeObjectHeapCompactionMode is process-global, so concurrent toggles race — thread A's set-to-CompactOnce can be observed by thread B's GC.Collect, or vice versa, producing non-deterministic LOH compaction between tests. Wraps the entire mode-set → collect → wait → mode-set → collect sequence in a static lock so concurrent teardowns are serialized. Each teardown's LOH compaction is now deterministic. Perf note: the lock only serializes teardowns, not the tests themselves. The GC.Collect calls are already blocking anyway, so the extra lock contention is negligible compared to the collection cost. Build clean on net10.0. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ooples
pushed a commit
that referenced
this pull request
Apr 15, 2026
Matches the lock added on PR #1148 (6794492). xunit parallelizes across test-classes by default — two derived test classes can hit DisposeAsync concurrently on different threads, racing on the process-global GCSettings.LargeObjectHeapCompactionMode flag. Wrapping the whole GC sequence in a static lock keeps LOH compaction deterministic per teardown. Perf note: the lock only serializes teardowns, not the tests themselves. The GC.Collect calls are already blocking anyway, so the extra lock contention is negligible compared to the collection cost. Build clean on net10.0. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Targets the `Tests (net10.0) - Unit - 03 Diffusion/Encoding` CI shard cancellation tracked in #1136. Adds a shared `DiffusionUnitTestBase` implementing `IAsyncLifetime` and wires all heavy Unit-03 Diffusion test classes to inherit from it. Each test method now triggers a blocking compacting Gen-2 GC with explicit Large Object Heap compaction after it returns, reclaiming undisposed model weight tensors AND defragmenting the LOH between sequential tests. ## Why forced gc + loh compaction matters Each test in `DiffusionModelContractTests`, `DDPMModelTests`, etc. constructs a production-default diffusion model via `new StableDiffusion15Model<double>()`, `new DDPMModel<double>()`, etc. These eagerly allocate ~300-4000 MB of weight tensors per instance, well above the 85KB LOH threshold. Two compounding problems without explicit teardown: 1. .NET's generational GC waits for memory pressure before running a gen-2 compacting pass — but ~200 sequential tests on a 16 GB Windows CI runner allocate fast enough to hit the 45-min wall clock before pressure triggers collection. 2. Plain `GC.Collect` sweeps the LOH but does NOT compact it. Over sequential tests, LOH fragmentation accumulates until the next allocation can't find a contiguous region even when total free bytes remain large, producing `OutOfMemoryException` on a runner that APPEARS to have plenty of memory. The teardown sets `GCSettings.LargeObjectHeapCompactionMode = CompactOnce` before a blocking compacting Gen-2 collection. `CompactOnce` auto-resets to `Default` after each use, so the side-effect is scoped per-teardown. The entire sequence runs under a static lock so concurrent teardowns from parallel test classes don't race on the process-global flag. ## Scope 9 test classes now inherit from `DiffusionUnitTestBase`: - `DiffusionModelContractTests` (1181 lines, 98+ model constructions) - `DDPMModelTests` (638 lines) - `EditingModelContractTests` (919 lines) - `FastGenContractTests` (776 lines) - `ControlModelContractTests` (267 lines) - `NewConditionerContractTests` (146 lines) - `PreprocessorContractTests` (177 lines) - `AudioProcessingTests` (574 lines) - `MemoryManagementTests` (411 lines) - `SchedulerTests` (395 lines, for consistency; schedulers are light) Not touched: `Conditioning/ConditioningModuleTests.cs` (no model allocations), `Schedulers/*Tests.cs` (lightweight scheduler-only tests), `UnitTests/Encoding/*.cs` (UTF-8 encoding, not diffusion). ## Tradeoff LOH compaction is expensive (.NET scans and relocates every LOH object), so per-test teardown adds tens to hundreds of ms depending on heap size. That's acceptable here: the alternative was CI cancellation at the 45-min wall clock from OOM. Expensive beats broken. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ooples
force-pushed
the
fix/unit03-diffusion-test-lifecycle
branch
from
April 15, 2026 19:36
6794492 to
af21e1a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/AiDotNet.Tests/UnitTests/Diffusion/DiffusionUnitTestBase.cs`:
- Around line 52-107: Add a non-parallel xUnit collection and annotate each
diffusion test class so they run serially: either reuse the existing
CpuOnlyCollection pattern or add a new [CollectionDefinition("DiffusionTests",
DisableParallelization = true)] class, then add [Collection("DiffusionTests")]
to each of the diffusion test classes (SchedulerTests,
PreprocessorContractTests, FastGenContractTests, DDPMModelTests,
ControlModelContractTests, EditingModelContractTests,
DiffusionModelContractTests, AudioProcessingTests, MemoryManagementTests,
NewConditionerContractTests); this ensures the DisposeAsync() logic (see
DiffusionUnitTestBase, _lohCompactionGate and DisposeAsync) is not defeated by
xUnit running those classes in parallel.
In `@tests/AiDotNet.Tests/UnitTests/Diffusion/Models/FastGenContractTests.cs`:
- Line 13: The FastGenContractTests class contains many trivial, low-signal
assertions (mostly Assert.NotNull and length/name checks) while inheriting
DiffusionUnitTestBase, which triggers an expensive full compacting GC; refactor
by either removing or upgrading these tests to assert meaningful behavior (e.g.,
verify expected property values, method outputs, metadata content and round-trip
fidelity) and stop inheriting the heavy DiffusionUnitTestBase for trivial
constructors—either switch FastGenContractTests to a lightweight base/test
fixture or create a minimal DiffusionLightweightTestBase to avoid the GC
teardown; update or replace tests that only call constructors or check
non-nullness to assert concrete contracts of the FastGen model (use symbols
FastGenContractTests, DiffusionUnitTestBase and any constructor/method names
used in the tests).
In
`@tests/AiDotNet.Tests/UnitTests/Diffusion/Models/PreprocessorContractTests.cs`:
- Line 10: Tests in PreprocessorContractTests currently use weak assertions
(Assert.NotNull and Assert.True(OutputChannels > 0)) which is insufficient for a
contract suite and causes unnecessary heavy teardown via DiffusionUnitTestBase;
update each constructor/initialization test in PreprocessorContractTests to
assert the exact expected channel count (e.g., Assert.Equal(expectedChannels,
preprocessor.OutputChannels)) and any other concrete behavior/state (shape,
sample rates, supported formats) instead of just NotNull, and only
inherit/instantiate DiffusionUnitTestBase when a test truly requires the full-GC
LOH-compacting teardown; replace generic OutputChannels > 0 checks with precise
equality checks against the known expected value and add one or two meaningful
behavioral assertions per test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73297219-59a2-48f4-816e-fe4fbb40df1b
📒 Files selected for processing (11)
tests/AiDotNet.Tests/UnitTests/Diffusion/AudioProcessingTests.cstests/AiDotNet.Tests/UnitTests/Diffusion/DiffusionUnitTestBase.cstests/AiDotNet.Tests/UnitTests/Diffusion/MemoryManagementTests.cstests/AiDotNet.Tests/UnitTests/Diffusion/Models/ControlModelContractTests.cstests/AiDotNet.Tests/UnitTests/Diffusion/Models/DDPMModelTests.cstests/AiDotNet.Tests/UnitTests/Diffusion/Models/DiffusionModelContractTests.cstests/AiDotNet.Tests/UnitTests/Diffusion/Models/EditingModelContractTests.cstests/AiDotNet.Tests/UnitTests/Diffusion/Models/FastGenContractTests.cstests/AiDotNet.Tests/UnitTests/Diffusion/Models/NewConditionerContractTests.cstests/AiDotNet.Tests/UnitTests/Diffusion/Models/PreprocessorContractTests.cstests/AiDotNet.Tests/UnitTests/Diffusion/SchedulerTests.cs
ooples
added a commit
that referenced
this pull request
Apr 16, 2026
…ffusion (#1143) * refactor: extract compile cache into composable CompiledModelHost<T> One attachment point for every future compilation feature — AOT plan serialization, CUDA Graph capture, symbolic shape plans, persistent autotune — rather than re-implementing the compile+cache+fallback dance per model family. New src/NeuralNetworks/CompiledModelHost.cs: - Sealed component, not a base class. Consumers own the layer list and Predict API; they compose one host instance. - Predict(input, structureVersion, eagerForward) — traces on miss, replays on hit, falls back to eager on disable/failure. - Automatic invalidation on structureVersion mismatch — callers bump the version when their layer graph mutates (lazy-init resize, layer add/remove, weight swap) and stale plans get dropped before the next compile. Previously the per-model code had to remember to call _compiledInferenceCache.Invalidate() on every mutation path; now it's centralized. NeuralNetworkBase: - Replaces the ad-hoc `_compiledInferenceCache` field and PredictCompiled body with `_compileHost.Predict(input, _layerStructureVersion, ...)`. PredictCompiled becomes a one-line delegation. - Dispose(bool) now cascades to every layer implementing IDisposable so pool-rented weight tensors return to the allocator before GC. Wrapped in try/catch(ObjectDisposedException) so a shared-layer graph doesn't abort the cascade when a previous owner already disposed the layer. - Compile host disposed first, then layers, then DisableMixedPrecision. Build clean on net10.0 + net471, 0 errors. This is the foundation commit. Follow-ups on the same branch or new PRs: - Wire NoisePredictorBase to the same host (closes diffusion compile gap) - Migrate 11 concrete predictor subclasses to register layers with compile-friendly hooks - DiffusionModelBase exposes NoisePredictor property so AiModelBuilder's BuildCompiledPredictFunction can reach through Part B.1 of the lucky-waddling-knuth plan. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: extract compile cache into composable CompiledModelHost<T> One attachment point for every future compilation feature — AOT plan serialization, CUDA Graph capture, symbolic shape plans, persistent autotune — rather than re-implementing the compile+cache+fallback dance per model family. New src/NeuralNetworks/CompiledModelHost.cs: - Sealed component, not a base class. Consumers own the layer list and Predict API; they compose one host instance. - Predict(input, structureVersion, eagerForward) — traces on miss, replays on hit, falls back to eager on disable/failure. - Automatic invalidation on structureVersion mismatch — callers bump the version when their layer graph mutates (lazy-init resize, layer add/remove, weight swap) and stale plans get dropped before the next compile. Previously the per-model code had to remember to call _compiledInferenceCache.Invalidate() on every mutation path; now it's centralized. NeuralNetworkBase: - Replaces the ad-hoc `_compiledInferenceCache` field and PredictCompiled body with `_compileHost.Predict(input, _layerStructureVersion, ...)`. PredictCompiled becomes a one-line delegation. - Dispose(bool) now cascades to every layer implementing IDisposable so pool-rented weight tensors return to the allocator before GC. Wrapped in try/catch(ObjectDisposedException) so a shared-layer graph doesn't abort the cascade when a previous owner already disposed the layer. - Compile host disposed first, then layers, then DisableMixedPrecision. Build clean on net10.0 + net471, 0 errors. This is the foundation commit. Follow-ups on the same branch or new PRs: - Wire NoisePredictorBase to the same host (closes diffusion compile gap) - Migrate 11 concrete predictor subclasses to register layers with compile-friendly hooks - DiffusionModelBase exposes NoisePredictor property so AiModelBuilder's BuildCompiledPredictFunction can reach through Part B.1 of the lucky-waddling-knuth plan. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: wire NoisePredictorBase through CompiledModelHost + Dispose Closes the diffusion-side half of the compile-plan gap. Previously DiffusionModelBase.NoisePredictor could not benefit from compiled-plan replay because the predictor base had no compile cache. Now every concrete predictor (DiT, MMDiT, UNet variants — 11 subclasses) can opt into compiled inference for its per-step Forward by calling the new protected PredictCompiled helper, and the 50-step denoising loop gets a near-zero-overhead replay after one trace instead of eager full-graph execution on every step. NoisePredictorBase changes: - Implements IDisposable. No existing predictor subclass declared its own Dispose, so this is a pure addition to the interface surface. - Owns a private CompiledModelHost<T> (the sealed component landed in the prior commit). One attachment point; everything else — AOT plan serialization, CUDA Graph capture, persistent autotune — attaches here too. - _layerStructureVersion + InvalidateCompiledPlans() hook for concrete predictors to signal "the layer graph just changed, drop stale plans." Called from lazy-init paths and SetParameters swaps. - protected virtual EnumerateLayers() — concrete predictors override to expose their private layer fields (_blocks, _timeEmbed1, _patchEmbed, etc.) for Dispose cascade. Default returns empty so this commit is non-breaking; migration of the 11 concrete predictors to populate EnumerateLayers lands in follow-up commits on this same branch. - PredictCompiled(input, eagerFallback) — one-line delegation to the host. Concrete predictors route their Forward through this once they're verified safe for trace+replay. - Dispose cascade: host first (so pool-captured tensors are freed), then every ILayer<T> from EnumerateLayers that implements IDisposable. Wrapped in try/catch(ObjectDisposedException) for shared-layer graphs. Build clean on net10.0 + net471, 0 errors. Behavior unchanged for all existing callers — the opt-in hooks are additive. Part B.2 of the lucky-waddling-knuth plan. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: wire NoisePredictorBase through CompiledModelHost + Dispose Closes the diffusion-side half of the compile-plan gap. Previously DiffusionModelBase.NoisePredictor could not benefit from compiled-plan replay because the predictor base had no compile cache. Now every concrete predictor (DiT, MMDiT, UNet variants — 11 subclasses) can opt into compiled inference for its per-step Forward by calling the new protected PredictCompiled helper, and the 50-step denoising loop gets a near-zero-overhead replay after one trace instead of eager full-graph execution on every step. NoisePredictorBase changes: - Implements IDisposable. No existing predictor subclass declared its own Dispose, so this is a pure addition to the interface surface. - Owns a private CompiledModelHost<T> (the sealed component landed in the prior commit). One attachment point; everything else — AOT plan serialization, CUDA Graph capture, persistent autotune — attaches here too. - _layerStructureVersion + InvalidateCompiledPlans() hook for concrete predictors to signal "the layer graph just changed, drop stale plans." Called from lazy-init paths and SetParameters swaps. - protected virtual EnumerateLayers() — concrete predictors override to expose their private layer fields (_blocks, _timeEmbed1, _patchEmbed, etc.) for Dispose cascade. Default returns empty so this commit is non-breaking; migration of the 11 concrete predictors to populate EnumerateLayers lands in follow-up commits on this same branch. - PredictCompiled(input, eagerFallback) — one-line delegation to the host. Concrete predictors route their Forward through this once they're verified safe for trace+replay. - Dispose cascade: host first (so pool-captured tensors are freed), then every ILayer<T> from EnumerateLayers that implements IDisposable. Wrapped in try/catch(ObjectDisposedException) for shared-layer graphs. Build clean on net10.0 + net471, 0 errors. Behavior unchanged for all existing callers — the opt-in hooks are additive. Part B.2 of the lucky-waddling-knuth plan. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: add opt-in dispose cascade to diffusion model base via enumerate-disposable-components Closes the diffusion-side Dispose gap. Previously DiffusionModelBase implemented neither IDisposable nor a cascade — concrete diffusion models (249 of them) couldn't release their composed predictor's pool-rented weight tensors back to the allocator without each implementing Dispose by hand. This caused the test-shard OOMs in the earlier perf work even after the layer-side Dispose plumbing (commit a9070a6 in PR #1140). DiffusionModelBase changes: - Implements IDisposable. No existing concrete diffusion subclass declared its own Dispose, so this is purely additive. - protected virtual EnumerateDisposableComponents() — concrete models override to yield the components they own (predictor, VAE, conditioner). Default returns empty so the 249 existing subclasses keep working unchanged until each opts in. Migration is per-model: protected override IEnumerable<IDisposable> EnumerateDisposableComponents() { yield return _unet; yield return _vae; } - Dispose(bool) walks the enumeration, catching ObjectDisposedException for shared-component graphs (a predictor reused across two diffusion wrappers for ensembling). Why a method-based opt-in, not a NoisePredictor property: ILatentDiffusionModel<T> already declares INoisePredictor<T> NoisePredictor { get; } as a non-nullable contract that LatentDiffusionModelBase implements. Adding a virtual nullable property on the base would either collide on the name (CS0114), require nullability gymnastics across every latent-diffusion subclass, or break the existing interface contract. The method-based pattern sidesteps the collision entirely and matches the EnumerateLayers hook that NoisePredictorBase introduced in the prior commit. Build clean on net10.0 + net471, 0 errors. Behavior unchanged for all existing concrete diffusion models. Part B.3 of the lucky-waddling-knuth plan. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: DiffusionModelBase opt-in Dispose cascade via EnumerateDisposableComponents Closes the diffusion-side Dispose gap. Previously DiffusionModelBase implemented neither IDisposable nor a cascade — concrete diffusion models (249 of them) couldn't release their composed predictor's pool-rented weight tensors back to the allocator without each implementing Dispose by hand. This caused the test-shard OOMs in the earlier perf work even after the layer-side Dispose plumbing (commit a9070a6 in PR #1140). DiffusionModelBase changes: - Implements IDisposable. No existing concrete diffusion subclass declared its own Dispose, so this is purely additive. - protected virtual EnumerateDisposableComponents() — concrete models override to yield the components they own (predictor, VAE, conditioner). Default returns empty so the 249 existing subclasses keep working unchanged until each opts in. Migration is per-model: protected override IEnumerable<IDisposable> EnumerateDisposableComponents() { yield return _unet; yield return _vae; } - Dispose(bool) walks the enumeration, catching ObjectDisposedException for shared-component graphs (a predictor reused across two diffusion wrappers for ensembling). Why a method-based opt-in, not a NoisePredictor property: ILatentDiffusionModel<T> already declares INoisePredictor<T> NoisePredictor { get; } as a non-nullable contract that LatentDiffusionModelBase implements. Adding a virtual nullable property on the base would either collide on the name (CS0114), require nullability gymnastics across every latent-diffusion subclass, or break the existing interface contract. The method-based pattern sidesteps the collision entirely and matches the EnumerateLayers hook that NoisePredictorBase introduced in the prior commit. Build clean on net10.0 + net471, 0 errors. Behavior unchanged for all existing concrete diffusion models. Part B.3 of the lucky-waddling-knuth plan. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #1143 review feedback — Dispose completeness + observability Critical (blocking): - DiffusionModelBase.EnumerateDisposableComponents default is now a reflection walk over instance fields that yields anything implementing IDisposable. Previously it returned empty so all 249 concrete diffusion subclasses kept their predictors/VAEs/conditioners alive after Dispose. Subclasses can still override for an explicit allow-list (preferred in perf-sensitive code). - NoisePredictorBase.EnumerateLayers default is the analogous reflection walk that finds ILayer<T> fields and ILayer<T> entries inside collection fields (catches _blocks, _timeEmbed1, _patchEmbed, etc.). Concrete predictors no longer leak their network graph by default. - NeuralNetworkBase.Dispose calls DisableMemoryManagement() before DisableMixedPrecision() so activation pool / gradient checkpoint state is freed instead of surviving model disposal. - All ObjectDisposedException catches in the cascade now Trace.TraceWarning with the layer/component name + exception message instead of fully swallowing — double-dispose bugs become diagnosable in production telemetry. Major: - DiffusionModelBase.Dispose now disposes _scheduler when it implements IDisposable. Previously schedulers with native handles or precomputed alpha/beta buffers leaked across model disposal. - NoisePredictorBase.Dispose releases the _timestepEmbeddingCache tensors — these are owned exclusively by the predictor and had no other Dispose path. - CompiledModelHost.Predict catch now invalidates the entire cache after a failure (replay path could leave a broken plan cached, causing every subsequent call to re-enter the same exception). Resets the version watermark so the next call recompiles fresh. Trace.TraceWarning logs the failure for observability. - CompiledModelHost is now `internal sealed` instead of `public sealed` — it's plumbing behind the model base classes, not a user-consumed type. Reflection walks use TensorReferenceComparer<object>.Instance for identity tracking (avoids double-yielding the same disposable reachable from two fields, e.g., a predictor stored as both interface and concrete alias). Works on net471 (no ReferenceEqualityComparer there). Skips primitives, strings, value types. Build clean on net10.0 + net471, 0 errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #1143 review feedback — Dispose completeness + observability Critical (blocking): - DiffusionModelBase.EnumerateDisposableComponents default is now a reflection walk over instance fields that yields anything implementing IDisposable. Previously it returned empty so all 249 concrete diffusion subclasses kept their predictors/VAEs/conditioners alive after Dispose. Subclasses can still override for an explicit allow-list (preferred in perf-sensitive code). - NoisePredictorBase.EnumerateLayers default is the analogous reflection walk that finds ILayer<T> fields and ILayer<T> entries inside collection fields (catches _blocks, _timeEmbed1, _patchEmbed, etc.). Concrete predictors no longer leak their network graph by default. - NeuralNetworkBase.Dispose calls DisableMemoryManagement() before DisableMixedPrecision() so activation pool / gradient checkpoint state is freed instead of surviving model disposal. - All ObjectDisposedException catches in the cascade now Trace.TraceWarning with the layer/component name + exception message instead of fully swallowing — double-dispose bugs become diagnosable in production telemetry. Major: - DiffusionModelBase.Dispose now disposes _scheduler when it implements IDisposable. Previously schedulers with native handles or precomputed alpha/beta buffers leaked across model disposal. - NoisePredictorBase.Dispose releases the _timestepEmbeddingCache tensors — these are owned exclusively by the predictor and had no other Dispose path. - CompiledModelHost.Predict catch now invalidates the entire cache after a failure (replay path could leave a broken plan cached, causing every subsequent call to re-enter the same exception). Resets the version watermark so the next call recompiles fresh. Trace.TraceWarning logs the failure for observability. - CompiledModelHost is now `internal sealed` instead of `public sealed` — it's plumbing behind the model base classes, not a user-consumed type. Reflection walks use TensorReferenceComparer<object>.Instance for identity tracking (avoids double-yielding the same disposable reachable from two fields, e.g., a predictor stored as both interface and concrete alias). Works on net471 (no ReferenceEqualityComparer there). Skips primitives, strings, value types. Build clean on net10.0 + net471, 0 errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #1143 round-2 review feedback — Dispose hardening + cache concurrency Major: - DiffusionModelBase.ReflectInstanceDisposables skips _scheduler (handled explicitly by Dispose) so it isn't double-disposed via the cascade. - DiffusionModelBase + NoisePredictorBase reflection helpers Trace.TraceWarning the field name + exception when GetValue throws, instead of silently skipping. Without the trace a private field whose getter throws would leak its disposable resource with no diagnostic at Dispose time. - NoisePredictorBase.InvalidateCompiledPlans now calls _compileHost.Invalidate() immediately rather than waiting for the next PredictCompiled to detect the version mismatch. Releases captured tensor buffers eagerly when the caller's intent is "drop everything, reclaim memory now." - CompiledModelHost adds _sync object and protects all _cache / _lastCompiledVersion / _disposed mutations under lock. Predict acquires the lock briefly to take the cache snapshot, then releases before the potentially-slow compile/replay (CompiledModelCache itself is thread-safe for distinct shapes). Concurrent Dispose/Invalidate/Predict on the same model instance (model serving in a request pool) no longer tears down the cache mid-call. - NeuralNetworkBase.SetParameters now invalidates _compileHost after redistributing parameters. Some ITrainableLayer implementations swap parameter tensors wholesale instead of mutating in place — without invalidation, compiled plans replay against captured tensor references that no longer hold the new weights. Build clean on net10.0 + net471, 0 errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #1143 round-2 review feedback — Dispose hardening + cache concurrency Major: - DiffusionModelBase.ReflectInstanceDisposables skips _scheduler (handled explicitly by Dispose) so it isn't double-disposed via the cascade. - DiffusionModelBase + NoisePredictorBase reflection helpers Trace.TraceWarning the field name + exception when GetValue throws, instead of silently skipping. Without the trace a private field whose getter throws would leak its disposable resource with no diagnostic at Dispose time. - NoisePredictorBase.InvalidateCompiledPlans now calls _compileHost.Invalidate() immediately rather than waiting for the next PredictCompiled to detect the version mismatch. Releases captured tensor buffers eagerly when the caller's intent is "drop everything, reclaim memory now." - CompiledModelHost adds _sync object and protects all _cache / _lastCompiledVersion / _disposed mutations under lock. Predict acquires the lock briefly to take the cache snapshot, then releases before the potentially-slow compile/replay (CompiledModelCache itself is thread-safe for distinct shapes). Concurrent Dispose/Invalidate/Predict on the same model instance (model serving in a request pool) no longer tears down the cache mid-call. - NeuralNetworkBase.SetParameters now invalidates _compileHost after redistributing parameters. Some ITrainableLayer implementations swap parameter tensors wholesale instead of mutating in place — without invalidation, compiled plans replay against captured tensor references that no longer hold the new weights. Build clean on net10.0 + net471, 0 errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #1143 round-3 — eager fallback out of lock + tape-cache invalidation - CompiledModelHost.Predict moves the eager fallback OUT of the _sync lock. Previously _disposed/!EnableCompilation branches called eagerForward() while holding _sync — blocking other threads on what may be a slow op. Now: lock decides which path to take, releases, and the caller-supplied delegate runs unblocked. - NeuralNetworkBase.SetParameters now invalidates BOTH the compile host AND TapeTrainingStep<T>'s collected-parameter cache. The tape cache also keys off captured tensor references — when a layer swaps parameter tensors wholesale, the next training step would replay against parameters that no longer exist. Build clean on net10.0 + net471, 0 errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #1143 round-3 — eager fallback out of lock + tape-cache invalidation - CompiledModelHost.Predict moves the eager fallback OUT of the _sync lock. Previously _disposed/!EnableCompilation branches called eagerForward() while holding _sync — blocking other threads on what may be a slow op. Now: lock decides which path to take, releases, and the caller-supplied delegate runs unblocked. - NeuralNetworkBase.SetParameters now invalidates BOTH the compile host AND TapeTrainingStep<T>'s collected-parameter cache. The tape cache also keys off captured tensor references — when a layer swaps parameter tensors wholesale, the next training step would replay against parameters that no longer exist. Build clean on net10.0 + net471, 0 errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #1143 round-4 — IEnumerable traversal + docs match reflection-walk default - ReflectInstanceDisposables now traverses IEnumerable fields for nested disposables (List<IDisposable>, Dictionary<K, IDisposable>, etc.), matching ReflectInstanceLayers' behavior. Without this, a predictor holding its sub-models in a List<IDisposable> would leak them even though the field itself implements IEnumerable. - Updated XML docs on DiffusionModelBase.Dispose and NoisePredictorBase.Dispose to accurately describe the current behavior: reflection-walk is the DEFAULT, subclasses override to CONSTRAIN (return an explicit allow-list) — the previous docs still said 'subclasses must override to opt in,' which no longer matches reality. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #1143 round-4 — IEnumerable traversal + docs match reflection-walk default - ReflectInstanceDisposables now traverses IEnumerable fields for nested disposables (List<IDisposable>, Dictionary<K, IDisposable>, etc.), matching ReflectInstanceLayers' behavior. Without this, a predictor holding its sub-models in a List<IDisposable> would leak them even though the field itself implements IEnumerable. - Updated XML docs on DiffusionModelBase.Dispose and NoisePredictorBase.Dispose to accurately describe the current behavior: reflection-walk is the DEFAULT, subclasses override to CONSTRAIN (return an explicit allow-list) — the previous docs still said 'subclasses must override to opt in,' which no longer matches reality. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #1143 round-3 — DisposeOnceGuard + trace lambda + reflector doc Five reviewer-flagged concerns on the composable compile-host foundation: 1. CompiledModelHost.Predict trace lambda `() => { eagerForward(); }` discarded the Tensor<T> return, which can select a different GetOrCompileInference overload and leave the output tensor ambiguous to the tracer. Changed to `() => eagerForward()` so the output threads through. 2. Introduce DisposeOnceGuard (new Helpers/DisposeOnceGuard.cs) — a process-wide ConditionalWeakTable-backed registry that guarantees each IDisposable instance is disposed at most once, regardless of how many owners cascade into it. Replaces the previous ObjectDisposedException-as-signal pattern, which was unsafe because many layers (e.g., DenseLayer returning pooled tensors to TensorAllocator) are not idempotent on second Dispose. 3. Wire DisposeOnceGuard through: - NeuralNetworkBase.Dispose layer cascade - DiffusionModelBase.Dispose scheduler + component cascade - NoisePredictorBase.Dispose layer cascade Shared-layer graphs (same layer in multiple networks, predictor reused across diffusion wrappers, VAE injected into several models) are now safe. 4. Clarify NoisePredictorBase.EnumerateLayers doc: the reflector walks fields + IEnumerable elements but does NOT recurse into container types whose elements aren't themselves ILayer<T>. Predictors storing layers in container types like `List<DiTBlock>` (where DiTBlock holds layer properties) must override EnumerateLayers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #1143 round-3 — DisposeOnceGuard + trace lambda + reflector doc Five reviewer-flagged concerns on the composable compile-host foundation: 1. CompiledModelHost.Predict trace lambda `() => { eagerForward(); }` discarded the Tensor<T> return, which can select a different GetOrCompileInference overload and leave the output tensor ambiguous to the tracer. Changed to `() => eagerForward()` so the output threads through. 2. Introduce DisposeOnceGuard (new Helpers/DisposeOnceGuard.cs) — a process-wide ConditionalWeakTable-backed registry that guarantees each IDisposable instance is disposed at most once, regardless of how many owners cascade into it. Replaces the previous ObjectDisposedException-as-signal pattern, which was unsafe because many layers (e.g., DenseLayer returning pooled tensors to TensorAllocator) are not idempotent on second Dispose. 3. Wire DisposeOnceGuard through: - NeuralNetworkBase.Dispose layer cascade - DiffusionModelBase.Dispose scheduler + component cascade - NoisePredictorBase.Dispose layer cascade Shared-layer graphs (same layer in multiple networks, predictor reused across diffusion wrappers, VAE injected into several models) are now safe. 4. Clarify NoisePredictorBase.EnumerateLayers doc: the reflector walks fields + IEnumerable elements but does NOT recurse into container types whose elements aren't themselves ILayer<T>. Predictors storing layers in container types like `List<DiTBlock>` (where DiTBlock holds layer properties) must override EnumerateLayers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #1143 round-4 — dictionary walk + atomic cache swap + narrow catch Three reviewer concerns flagged by CodeRabbit on the round-3 changes: 1. Dispose reflection walks in DiffusionModelBase and NoisePredictorBase handled IEnumerable generically but Dictionary<K, V>.GetEnumerator yields KeyValuePair<K,V>, not values — so disposables/layers stored as dictionary values were silently skipped. Add an explicit IDictionary branch (via DictionaryEntry) before the IEnumerable branch in both walks. 2. CompiledModelHost previously invalidated its CompiledModelCache<T> in place on structureVersion changes, but Predict releases _sync before GetOrCompileInference, so an older in-flight call could repopulate the same invalidated cache after a newer call had already bumped the version. Replace the cache instance instead — Dispose the old cache and allocate a fresh one while still holding _sync. The older call now holds a detached reference with no cross-version bleed. 3. Narrow CompiledModelHost.Predict's fallback catch with an exception filter so fatal CLR failures (OOM, AccessViolation, StackOverflow, BadImageFormat, InvalidProgram, ThreadAbort, AppDomainUnloaded, CannotUnloadAppDomain) propagate instead of being masked by the eager fallback. Allocating again under a poisoned process state would just crash a second way. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #1143 round-4 — dictionary walk + atomic cache swap + narrow catch Three reviewer concerns flagged by CodeRabbit on the round-3 changes: 1. Dispose reflection walks in DiffusionModelBase and NoisePredictorBase handled IEnumerable generically but Dictionary<K, V>.GetEnumerator yields KeyValuePair<K,V>, not values — so disposables/layers stored as dictionary values were silently skipped. Add an explicit IDictionary branch (via DictionaryEntry) before the IEnumerable branch in both walks. 2. CompiledModelHost previously invalidated its CompiledModelCache<T> in place on structureVersion changes, but Predict releases _sync before GetOrCompileInference, so an older in-flight call could repopulate the same invalidated cache after a newer call had already bumped the version. Replace the cache instance instead — Dispose the old cache and allocate a fresh one while still holding _sync. The older call now holds a detached reference with no cross-version bleed. 3. Narrow CompiledModelHost.Predict's fallback catch with an exception filter so fatal CLR failures (OOM, AccessViolation, StackOverflow, BadImageFormat, InvalidProgram, ThreadAbort, AppDomainUnloaded, CannotUnloadAppDomain) propagate instead of being masked by the eager fallback. Allocating again under a poisoned process state would just crash a second way. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address pr #1143 review round-3 — dict-cascade + per-version cache + narrow catch + tests Addresses 5 unresolved PR #1143 comments: 1. DiffusionModelBase.ReflectInstanceDisposables: dict-held disposables were silently leaked. Dictionary<TKey, IDisposable> yields KeyValuePair<,> structs (not IDisposable) when enumerated, so the generic IEnumerable branch never reached the payload. Added an IDictionary branch BEFORE the IEnumerable branch that walks DictionaryEntry and extracts .Value, so dictionary-keyed disposables cascade through Dispose. 2. NoisePredictorBase.ReflectInstanceLayers: identical bug for layer enumeration. Dictionary<TKey, ILayer<T>> was silently dropping layers because the enumerator yields KeyValuePair<,> structs. Same fix — IDictionary branch before IEnumerable branch, extracting Value and matching it against ILayer<T>. 3. CompiledModelHost.Predict (structureVersion invalidation): previous code invalidated the cache IN PLACE when structureVersion changed, then reused the same CompiledModelCache<T> instance. A racing Predict that had just released the lock with an older version could repopulate the same cache after the switch, leaking version-N plans into version-N+1 callers. Now we replace the cache reference with a fresh instance per structure version — every per-version local `cache` closes over a unique object, so once we leave the lock it's frozen to that version. 4. CompiledModelHost.Predict (narrow catch): the fallback-to-eager path previously used catch-all `catch (Exception)`, which would swallow fatal CLR exceptions (OutOfMemoryException, StackOverflowException, AccessViolationException, thread aborts) and then immediately invoke eagerForward() on a poisoned process. Replaced with an exception filter that propagates fatal exceptions and only recovers from compile/replay failures. Follows the same exclusion list the runtime team uses for "recoverable vs. non-recoverable" distinctions. 5. CompiledModelHostTests: 9 new unit tests covering the host's invalidation contract, Dispose idempotency, eager fallback paths (compilation disabled, post-Dispose), structureVersion-bump cache invalidation, explicit Invalidate(), null-guard on eagerForward, concurrent Predict+Invalidate, and concurrent Predict+Dispose. All tests passing locally on net10.0. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: PR #1143 review round-3 — dict-cascade + per-version cache + narrow catch + tests Addresses 5 unresolved PR #1143 comments: 1. DiffusionModelBase.ReflectInstanceDisposables: dict-held disposables were silently leaked. Dictionary<TKey, IDisposable> yields KeyValuePair<,> structs (not IDisposable) when enumerated, so the generic IEnumerable branch never reached the payload. Added an IDictionary branch BEFORE the IEnumerable branch that walks DictionaryEntry and extracts .Value, so dictionary-keyed disposables cascade through Dispose. 2. NoisePredictorBase.ReflectInstanceLayers: identical bug for layer enumeration. Dictionary<TKey, ILayer<T>> was silently dropping layers because the enumerator yields KeyValuePair<,> structs. Same fix — IDictionary branch before IEnumerable branch, extracting Value and matching it against ILayer<T>. 3. CompiledModelHost.Predict (structureVersion invalidation): previous code invalidated the cache IN PLACE when structureVersion changed, then reused the same CompiledModelCache<T> instance. A racing Predict that had just released the lock with an older version could repopulate the same cache after the switch, leaking version-N plans into version-N+1 callers. Now we replace the cache reference with a fresh instance per structure version — every per-version local `cache` closes over a unique object, so once we leave the lock it's frozen to that version. 4. CompiledModelHost.Predict (narrow catch): the fallback-to-eager path previously used catch-all `catch (Exception)`, which would swallow fatal CLR exceptions (OutOfMemoryException, StackOverflowException, AccessViolationException, thread aborts) and then immediately invoke eagerForward() on a poisoned process. Replaced with an exception filter that propagates fatal exceptions and only recovers from compile/replay failures. Follows the same exclusion list the runtime team uses for "recoverable vs. non-recoverable" distinctions. 5. CompiledModelHostTests: 9 new unit tests covering the host's invalidation contract, Dispose idempotency, eager fallback paths (compilation disabled, post-Dispose), structureVersion-bump cache invalidation, explicit Invalidate(), null-guard on eagerForward, concurrent Predict+Invalidate, and concurrent Predict+Dispose. All tests passing locally on net10.0. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #1143 round-5 — in-flight coordination + empty EnumerateLayers default Two reviewer concerns flagged on the round-4 changes: 1. CompiledModelHost's Dispose/Invalidate + version-swap path could tear down a cache that an in-flight Predict call was still using outside the lock. Add proper in-flight coordination: - _activeCalls counter (incremented inside lock right before release, decremented in Predict's finally block via DrainPendingDisposals). - On version swap, the detached old cache is parked on _pendingDisposeCaches instead of being Disposed inline — the last Predict to finish drains and disposes the queue. - Dispose sets _disposeRequested; if _activeCalls == 0 it immediately cleans up, otherwise defers to DrainPendingDisposals. - Invalidate() follows the same pattern: if in-flight callers hold the cache, park it on the queue instead of disposing inline. - TraceWarning now logs ex.ToString() so stack traces and inner exceptions survive for production diagnostics. 2. NoisePredictorBase.EnumerateLayers() default changes back to Enumerable.Empty<ILayer<T>>() to match the original PR contract. Reflecting by default would dispose layers the predictor doesn't own (injected cross-attention layers from a shared encoder, an injected VAE, etc.). Ownership should be expressed by what the predictor chooses to enumerate, not by what reflection happens to find. Make ReflectInstanceLayers protected so subclasses can opt in explicitly when they DO want the walk. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #1143 round-5 — in-flight coordination + empty EnumerateLayers default Two reviewer concerns flagged on the round-4 changes: 1. CompiledModelHost's Dispose/Invalidate + version-swap path could tear down a cache that an in-flight Predict call was still using outside the lock. Add proper in-flight coordination: - _activeCalls counter (incremented inside lock right before release, decremented in Predict's finally block via DrainPendingDisposals). - On version swap, the detached old cache is parked on _pendingDisposeCaches instead of being Disposed inline — the last Predict to finish drains and disposes the queue. - Dispose sets _disposeRequested; if _activeCalls == 0 it immediately cleans up, otherwise defers to DrainPendingDisposals. - Invalidate() follows the same pattern: if in-flight callers hold the cache, park it on the queue instead of disposing inline. - TraceWarning now logs ex.ToString() so stack traces and inner exceptions survive for production diagnostics. 2. NoisePredictorBase.EnumerateLayers() default changes back to Enumerable.Empty<ILayer<T>>() to match the original PR contract. Reflecting by default would dispose layers the predictor doesn't own (injected cross-attention layers from a shared encoder, an injected VAE, etc.). Ownership should be expressed by what the predictor chooses to enumerate, not by what reflection happens to find. Make ReflectInstanceLayers protected so subclasses can opt in explicitly when they DO want the walk. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: IDiffusionModel : IDisposable + using var in DiffusionModelTestBase (#1136) ## Summary Completes the Dispose cascade wired up in this PR by exposing `IDisposable` on the `IDiffusionModel<T>` interface itself and flipping the 13 `var model = CreateModel()` calls in `DiffusionModelTestBase` to `using var model`. Without this pair of changes the `DiffusionModelBase<T> : IDisposable` implementation was unreachable through the interface — consumers who held `IDiffusionModel<T>` references had no compile-time visibility of the Dispose contract, so the reflection-walk Dispose that this PR adds was effectively opt-in via an `IDisposable` cast. ## Why this matters for CI (issue #1136) The three cancelled Diffusion ModelFamily shards (A-I / J-R / S-Z) on master baseline [24398739627](https://github.com/ooples/AiDotNet/actions/runs/24398739627) were hitting the 45-minute wall-clock budget with ~950 `OutOfMemoryException`s across ~255 diffusion model tests. Every test was constructing a production-default model (DiT-XL: ~4 GB eagerly-allocated weights) and letting the reference fall out of scope without disposing. With no Dispose path, the rented weight tensors stayed live in the TensorAllocator pool — and the next test's allocation stacked on top. `using var model = CreateModel()` now calls Dispose() at the end of every test method, which walks the reflection-discovered child disposables added in this PR (noise predictors, schedulers) and releases their layer trees back to the pool. Memory now plateaus across tests instead of monotonically climbing to OOM. ## Scope - `IDiffusionModel<T>` inherits `System.IDisposable`. `DiffusionModelBase<T>` already implemented `IDisposable` in this PR; this commit just exposes it on the interface so the compiler can see it. - `DiffusionModelTestBase`: 13 call sites converted to `using var model`. `Clone()` call in `Clone_ShouldProduceIdenticalOutput` left unchanged — it returns `IFullModel<...>` which doesn't derive from `IDisposable`. The primary `model` is now disposed, which is the dominant memory path. ## Not in scope - `NeuralNetworkModelTestBase` — already handled on PR #1137 branch via the `INeuralNetwork<T> : IDisposable` change. - Other ModelFamily test bases (20+) use different model interfaces (IClassificationModel, ITimeSeriesModel, etc.). Adding `IDisposable` to each is the natural follow-up once their respective base classes implement Dispose — tracked separately, not this PR. Build clean on net10.0 + net471, 0 errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: IAsyncLifetime forced GC between DiffusionModelTestBase tests (#1136) ## Summary Completes Part 4 of the #1136 fix plan for DiffusionModelTestBase by implementing `IAsyncLifetime.DisposeAsync` with the standard two-pass GC pattern (`GC.Collect → WaitForPendingFinalizers → GC.Collect`) that runs AFTER each test disposes its model via `using var model = CreateModel()`. ## Why a forced GC is necessary The prior commit (<a2acab430>) added `using var model = CreateModel()` to all 13 Diffusion test methods, which calls Dispose at end-of-test — the Dispose walks child layers and returns their rented weight buffers to the TensorAllocator pool. But `Dispose()` only releases the REFERENCES; the managed weight arrays (300 MB per DiT-XL model instance) sit in gen-2 heap until GC runs a compacting pass. On 16 GB Windows CI runners, GC waits for memory pressure — but the ~255 sequential diffusion tests allocate fast enough to hit the 45-min wall clock before gen-2 pressure triggers a collection. The forced GC runs explicitly after each test disposes, reclaiming the pool-returned arrays immediately. Memory now plateaus across tests instead of monotonically climbing to OOM. ## Pattern Implements `IAsyncLifetime` (xunit's per-test lifecycle hook): - `InitializeAsync`: no-op — no ambient state to set up - `DisposeAsync`: two-pass GC (the standard .NET pattern for forcing finalizers to run AND reclaiming the collected memory) ## Scope DiffusionModelTestBase only. The same pattern should apply to every ModelFamily shard (NeuralNetworkModelTestBase, AudioDiffusionTestBase, VideoDiffusionTestBase, etc.), but those belong in separate commits as each family matures its own Dispose cascade. Build clean on net10.0, 0 errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * perf: compact LOH in DiffusionModelTestBase teardown (#1136) Upgrades the IAsyncLifetime DisposeAsync hook added in the prior commit to use a blocking COMPACTING Gen-2 collection with explicit Large Object Heap compaction, matching the pattern applied on PR #1148's DiffusionUnitTestBase. Diffusion model weight tensors are typically several hundred MB each, well above the 85KB LOH threshold. Plain `GC.Collect` sweeps the LOH but does NOT compact it — across ~255 sequential diffusion tests, LOH fragmentation accumulates until the next allocation can't find a contiguous region even though total free bytes remain large, producing `OutOfMemoryException` on a runner that APPEARS to have plenty of memory. Setting `GCLargeObjectHeapCompactionMode.CompactOnce` on the next Gen-2 pass forces LOH compaction; the mode auto-resets to Default after each use, so this is scoped per-teardown — no process-wide side-effect. Same fix applied on PR #1148 (b4aa53f) per CodeRabbit review feedback. Applying here too for consistency — both test bases run diffusion models with similar LOH fragmentation risk. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * perf: serialize LOH compaction with static lock (#1136) Matches the lock added on PR #1148 (6794492). xunit parallelizes across test-classes by default — two derived test classes can hit DisposeAsync concurrently on different threads, racing on the process-global GCSettings.LargeObjectHeapCompactionMode flag. Wrapping the whole GC sequence in a static lock keeps LOH compaction deterministic per teardown. Perf note: the lock only serializes teardowns, not the tests themselves. The GC.Collect calls are already blocking anyway, so the extra lock contention is negligible compared to the collection cost. Build clean on net10.0. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: franklinic <franklin@ivorycloud.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refs #1136. Targets the
Tests (net10.0) - Unit - 03 Diffusion/EncodingCI shard cancellation. AddsDiffusionUnitTestBase : IAsyncLifetimeand wires all heavy Unit-03 Diffusion test classes to inherit from it — each test method now triggers a full two-pass GC cycle (GC.Collect → WaitForPendingFinalizers → GC.Collect) after it returns, reclaiming undisposed model weight tensors between sequential tests.Why forced GC helps
Each test in
DiffusionModelContractTests,DDPMModelTests, etc. constructs a production-default diffusion model (new StableDiffusion15Model<double>(),new DDPMModel<double>(), etc.) with eagerly-allocated ~300-4000 MB of weight tensors per instance. The test holds the reference in a localvar, does a few assertions, and lets the reference fall out of scope..NET's generational GC waits for memory pressure before running a gen-2 compacting pass — but ~200 sequential tests on a 16 GB Windows CI runner allocate fast enough to hit the 45-min wall clock before pressure triggers collection. The forced GC between tests reclaims immediately.
Pattern lifted from
DiffusionModelTestBase(#1143 does the same for the ModelFamily Diffusion shards).Scope
9 test classes now inherit from
DiffusionUnitTestBase:DiffusionModelContractTests(1181 lines, 98+ model constructions)DDPMModelTests(638 lines)EditingModelContractTests(919 lines)FastGenContractTests(776 lines)ControlModelContractTests(267 lines)NewConditionerContractTests(146 lines)PreprocessorContractTests(177 lines)AudioProcessingTests(574 lines)MemoryManagementTests(411 lines)SchedulerTests(395 lines — for consistency; schedulers are light)Not touched:
Conditioning/ConditioningModuleTests.cs(no model allocations),Schedulers/*Tests.csin the subfolder (lightweight scheduler-only tests),UnitTests/Encoding/*.cs(UTF-8 encoding, not diffusion).Test plan
Tests (net10.0) - Unit - 03 Diffusion/Encodingshard finishes within 45-min wall clock (vs. cancelled on baseline run 24398739627)Not in scope
This PR is deliberately scoped to the Unit-03 Diffusion shard lifecycle only. The underlying eagerly-allocated weight tensor problem is addressed by the parent #1136 PR chain (#1137, #1138, #1140, #1141, #1143) which adds lazy init to noise predictors, MHA, and LayerNorm. This PR is a complementary lifecycle hygiene fix — even with full lazy init, sequential tests still allocate on first use, and GC between tests keeps memory flat instead of monotonically climbing.
🤖 Generated with Claude Code
Summary by CodeRabbit