perf: lazy init for VGG + CapsuleNetwork to unblock NeuralNetworks ModelFamily shard - #1138
Conversation
…es (#1136) Applies the same lazy-init pattern used for diffusion noise predictors in PR #1137 to the two biggest OOM offenders in the NeuralNetworks ModelFamily shard (baseline run 24398739627): VGGNetwork: 14 per-test timeouts + 3 OOMs. VGG16BN at 224×224×3 production defaults allocates ~1.1 GB of weights per instance, with the three fully connected classifier layers alone accounting for ~1 GB (FC1 at 25088×4096 is ~822 MB). Every ModelFamilyTests.NeuralNetworks.VGGNetworkTests.* instantiation was eagerly renting these tensors in `new DenseLayer<T>(...)`. CapsuleNetwork: 8 per-test timeouts + 1 OOM. Same eager-rent pattern on the initial 9×9 convolutional layer at production spatial sizes. Threads `InitializationStrategies<T>.Lazy` into the ConvolutionalLayer and DenseLayer constructions in: - LayerHelper<T>.CreateDefaultVGGLayers (3×3 Conv blocks + FC1/FC2/FC3) - LayerHelper<T>.CreateDefaultCapsuleNetworkLayers (initial 9×9 Conv) Weight tensors stay at shape [0,…,0] until the first Forward() call, matching the lazy path DenseLayer and ConvolutionalLayer already support via `IInitializationStrategy<T>?.IsLazy`. Construction becomes O(1), and production-default VGG/Capsule instances no longer trigger the TensorAllocator.Rent OOM that was cancelling the NN shard at the 45-minute wall clock. Intentionally scoped to the two top offenders. FastText's 110 OOMs were caused by per-test accumulation rather than any single large alloc; that is addressed by the `IAsyncLifetime.DisposeAsync` GC hook on `NeuralNetworkModelTestBase` landing in PR #1137. VoxelCNN uses `Conv3DLayer` which doesn't yet support a lazy init path — deferred to the follow-up PR covering MHA / LayerNorm / Conv3D lazy support. Refs #1136
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughLayer construction now uses lazy initialization for selected convolutional and classifier dense layers (allocation deferred until first Forward()). ConvolutionalLayer parameter access, gradients, SetParameters and Deserialize were updated to respect and trigger deferred initialization when needed. Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as LayerHelper/ModelBuilder
participant Init as InitializationStrategies<T>
participant Layer as ConvolutionalLayer<T>
participant Runner as Runtime/Forward
Builder->>Layer: construct(..., initializationStrategy: Init.Lazy)
Note right of Layer: kernels & biases NOT allocated
Runner->>Layer: Forward(input)
Layer->>Layer: EnsureInitialized()
Layer->>Init: allocate kernels & biases
Init-->>Layer: return tensors
Layer->>Runner: perform convolution -> output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
No stubs, TODOs, or non-production placeholders were detected in the changed files; nothing flagged as BLOCKING. Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR applies the repository’s lazy-weight-initialization strategy to the largest allocation hot spots in the NeuralNetworks ModelFamily shard—VGG and CapsuleNetwork—so model construction doesn’t eagerly rent huge weight tensors and overwhelm CI memory/time budgets.
Changes:
- Thread
InitializationStrategies<T>.Lazyinto VGG’sConvolutionalLayer<T>and classifierDenseLayer<T>creation inLayerHelper<T>.CreateDefaultVGGLayers. - Thread
InitializationStrategies<T>.Lazyinto CapsuleNetwork’s initialConvolutionalLayer<T>creation inLayerHelper<T>.CreateDefaultCapsuleNetworkLayers. - Add the required
AiDotNet.Initializationimport inLayerHelper.cs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The `test-net10-sharded` job chain was pinned to windows-latest and gated
behind `build-windows`, so every Diffusion / NeuralNetworks / Unit shard on
every PR waited for a Windows build to finish before it could start. Since
these tests run net10.0 (fully cross-platform) and the only Windows-specific
concern is net471 cross-compilation — which the modern .NET SDK handles via
its auto-included reference assembly pack — there's no reason to keep them
on windows runners. Linux runners also start faster, cost less, and parallel
better.
Changes:
- `build` job (formerly `build-windows`): `runs-on: ubuntu-latest`. Keeps
multi-target net10.0+net471 `dotnet build`. Drops the Windows-only MSBuild
flags `-p:UseSharedCompilation=false -p:nodeReuse=false` (file-locking
workarounds that don't apply on Linux) and the `dotnet build-server
shutdown` step that existed for the same reason.
- `test-net10-sharded`: `runs-on: ubuntu-latest`, `needs: build`.
- `sonarcloud`: `runs-on: ubuntu-latest`, `needs: [build, test-net10-sharded]`.
Same `dotnet-sonarscanner` tool works cross-platform.
- `size-check`: `needs: build` (was `needs: build-windows`).
- All `shell: powershell` steps switched to `shell: pwsh`. PowerShell Core
is pre-installed on ubuntu-latest runners and runs the existing
`.github/scripts/report-slow-tests.ps1` and inline scripts without
modification.
- Windows-style `\scanner` paths converted to `/scanner` forward-slash paths;
the `& "${{ runner.temp }}/scanner/dotnet-sonarscanner" end` invocation is
wrapped in `&` to tolerate paths with spaces under pwsh.
- Stale top-level and in-line comments updated to reflect ubuntu-based CI.
Gains: cancelled test shards no longer queue behind a slow Windows build;
ubuntu runners are typically ~30% faster and free up the Windows queue for
the few jobs that actually need it. Combined with the lazy-init fixes in
PR #1137 and PR #1138, this should clear the 45-minute wall-clock
cancellations on the Diffusion / NeuralNetworks / Unit-03 / Unit-08e shards.
Refs #1136
…init support ConvolutionalLayer now has the same lazy-init infrastructure as FeedForwardLayer: ParameterCount: returns OutputDepth*InputDepth*KernelSize^2 + OutputDepth from constructor-time fields when uninitialized, so callers that read ParameterCount before the first Forward() get the correct count instead of reading _kernels.Length (which would be 0 for empty placeholders). GetParameters() / SetParameters() / GetParameterGradients(): call EnsureInitialized() up-front so weight loading, serialization, and manual gradient access trigger allocation before touching the tensors. Without this, SetParameters on an uninitialized conv layer would see kernelLen=0 and skip all weight data silently. The source generator's GetTrainableParameters() already calls EnsureInitialized() (added globally in the PR #1133 fix), so the tape collection path was already correct. This commit completes the picture for the non-tape API surface. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…port ConvolutionalLayer now has the same lazy-init infrastructure as FeedForwardLayer: ParameterCount: returns OutputDepth*InputDepth*KernelSize^2 + OutputDepth from constructor-time fields when uninitialized, so callers that read ParameterCount before the first Forward() get the correct count instead of reading _kernels.Length (which would be 0 for empty placeholders). GetParameters() / SetParameters() / GetParameterGradients(): call EnsureInitialized() up-front so weight loading, serialization, and manual gradient access trigger allocation before touching the tensors. Without this, SetParameters on an uninitialized conv layer would see kernelLen=0 and skip all weight data silently. The source generator's GetTrainableParameters() already calls EnsureInitialized() (added globally in the PR #1133 fix), so the tape collection path was already correct. This commit completes the picture for the non-tape API surface. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 `@src/NeuralNetworks/Layers/ConvolutionalLayer.cs`:
- Line 1325: The calls to EnsureInitialized() in the layer (e.g., at
EnsureInitialized() invocations) can re-randomize weights for layers that were
deserialized because Deserialize() doesn't mark the layer as initialized; update
the deserialization path to set the layer's initialized state instead of letting
EnsureInitialized() run later — either set the existing initialization flag
(e.g., _initialized or similar) to true inside Deserialize() or add a distinct
_isDeserialized flag and make EnsureInitialized() respect it (skip random init
if deserialized). Locate Deserialize(), EnsureInitialized(), and any
lazy-initialization fields in ConvolutionalLayer and ensure Deserialize() marks
the layer as initialized to prevent overwriting loaded parameters.
🪄 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: 38a4b580-af56-4e96-896a-c231c8f6c8b1
📒 Files selected for processing (1)
src/NeuralNetworks/Layers/ConvolutionalLayer.cs
…fter deserialize Deserialize() allocates real _kernels/_biases from the reader stream but never set _isInitialized = true. Without this flag, the next EnsureInitialized() call (triggered by Forward, GetParameters, or the tape collector) would re-randomize the just-deserialized weights — silently corrupting loaded models. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…erialize Deserialize() allocates real _kernels/_biases from the reader stream but never set _isInitialized = true. Without this flag, the next EnsureInitialized() call (triggered by Forward, GetParameters, or the tape collector) would re-randomize the just-deserialized weights — silently corrupting loaded models. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/NeuralNetworks/Layers/ConvolutionalLayer.cs (1)
1349-1354:⚠️ Potential issue | 🟠 MajorDon't allocate full weights just to return zero gradients.
Line 1349 eagerly initializes the layer even when no backward pass has run and this method immediately returns
new Vector<T>(ParameterCount). That defeats lazy init on any code path that probes gradients before training and can bring back the same cold-start pressure this PR is trying to remove.Proposed fix
public override Vector<T> GetParameterGradients() { - EnsureInitialized(); // If gradients haven't been computed yet, return zero gradients - if (_kernelsGradient == null || _biasesGradient == null) + if (!_isInitialized || _kernelsGradient == null || _biasesGradient == null) { return new Vector<T>(ParameterCount); } // Bulk copy from contiguous tensor storage — replaces 4-nested scalar loops🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NeuralNetworks/Layers/ConvolutionalLayer.cs` around lines 1349 - 1354, Don't call EnsureInitialized() before checking gradient fields; move the null-check for _kernelsGradient and _biasesGradient to the top of the method and return new Vector<T>(ParameterCount) immediately if either is null so you avoid forcing layer initialization. Only call EnsureInitialized() when you actually need to access or assemble gradients from the initialized fields (e.g., when aggregating _kernelsGradient/_biasesGradient into the returned Vector), preserving lazy init and avoiding the cold-start allocation.
♻️ Duplicate comments (1)
src/NeuralNetworks/Layers/ConvolutionalLayer.cs (1)
728-731:⚠️ Potential issue | 🔴 CriticalBlocking:
Deserialize()still leaves stale tensor state behind.Setting
_isInitialized = truefixes the re-randomization path, but Line 728 still swaps in new_kernels/_biaseswithout clearing or rebuilding the state that was derived from the old tensors. On a reused instance,_biasReshaped4D, optimizer state, and persistent registrations can keep pointing at pre-deserialize tensors, so the loaded model can run or update with stale data.Proposed fix
public override void Deserialize(BinaryReader reader) { base.Deserialize(reader); InputDepth = reader.ReadInt32(); OutputDepth = reader.ReadInt32(); KernelSize = reader.ReadInt32(); Stride = reader.ReadInt32(); Padding = reader.ReadInt32(); + Engine.InvalidatePersistentTensor(_kernels); + Engine.InvalidatePersistentTensor(_biases); + // Deserialize _kernels — flat span iteration replaces 4-nested indexing loops _kernels = TensorAllocator.RentUninitialized<T>([OutputDepth, InputDepth, KernelSize, KernelSize]); var kernelSpan = _kernels.Data.Span; for (int i = 0; i < kernelSpan.Length; i++) kernelSpan[i] = NumOps.FromDouble(reader.ReadDouble()); // Deserialize _biases — flat span iteration _biases = new Tensor<T>([OutputDepth]); var biasSpan = _biases.Data.Span; for (int i = 0; i < biasSpan.Length; i++) biasSpan[i] = NumOps.FromDouble(reader.ReadDouble()); // Reinitialize _lastInput and _lastOutput _lastInput = new Tensor<T>([OutputDepth, InputDepth, KernelSize, KernelSize]); _lastOutput = new Tensor<T>([OutputDepth, InputDepth, KernelSize, KernelSize]); + RegisterTrainableParameter(_kernels, PersistentTensorRole.Weights); + RegisterTrainableParameter(_biases, PersistentTensorRole.Biases); + _biasReshaped4D = null; + _preAllocatedOutput = null; + _kernelsGradient = null; + _biasesGradient = null; + _kernelsVelocity = null; + _biasesVelocity = null; // Mark as initialized so EnsureInitialized() doesn't re-randomize the // just-deserialized weights on the next Forward/GetParameters call. _isInitialized = true; }As per coding guidelines: “Production Readiness (CRITICAL)… Incomplete features: Half-implemented patterns where some code paths work but others silently do nothing.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NeuralNetworks/Layers/ConvolutionalLayer.cs` around lines 728 - 731, Deserialize currently replaces _kernels/_biases and sets _isInitialized but leaves derived state (e.g., _biasReshaped4D), optimizer state entries, and any persistent registrations pointing at old tensors, causing stale references; update Deserialize (the method that assigns _kernels and _biases) to: clear or rebuild derived fields like _biasReshaped4D, detach or reset optimizer state entries associated with the previous tensors, and re-register persistent resources so they point to the newly deserialized tensors (and ensure EnsureInitialized logic is consistent with these rebuilt state fields).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/NeuralNetworks/Layers/ConvolutionalLayer.cs`:
- Around line 1349-1354: Don't call EnsureInitialized() before checking gradient
fields; move the null-check for _kernelsGradient and _biasesGradient to the top
of the method and return new Vector<T>(ParameterCount) immediately if either is
null so you avoid forcing layer initialization. Only call EnsureInitialized()
when you actually need to access or assemble gradients from the initialized
fields (e.g., when aggregating _kernelsGradient/_biasesGradient into the
returned Vector), preserving lazy init and avoiding the cold-start allocation.
---
Duplicate comments:
In `@src/NeuralNetworks/Layers/ConvolutionalLayer.cs`:
- Around line 728-731: Deserialize currently replaces _kernels/_biases and sets
_isInitialized but leaves derived state (e.g., _biasReshaped4D), optimizer state
entries, and any persistent registrations pointing at old tensors, causing stale
references; update Deserialize (the method that assigns _kernels and _biases)
to: clear or rebuild derived fields like _biasReshaped4D, detach or reset
optimizer state entries associated with the previous tensors, and re-register
persistent resources so they point to the newly deserialized tensors (and ensure
EnsureInitialized logic is consistent with these rebuilt state fields).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 69cdb026-6b5f-496a-be8f-bcf82e9a6b83
📒 Files selected for processing (1)
src/NeuralNetworks/Layers/ConvolutionalLayer.cs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…, lazy gradients Deserialize re-registration: ClearRegisteredParameters + RegisterTrainableParameter after creating new _kernels/_biases so optimizers and tape target the live tensors, not the stale ones from a prior EnsureInitialized call. GetParameterGradients: return zero vector from ParameterCount (shape-based) when gradients are null, without forcing EnsureInitialized — avoids allocating and randomizing the full weight tensors just to return zeros. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…radients Deserialize re-registration: ClearRegisteredParameters + RegisterTrainableParameter after creating new _kernels/_biases so optimizers and tape target the live tensors, not the stale ones from a prior EnsureInitialized call. GetParameterGradients: return zero vector from ParameterCount (shape-based) when gradients are null, without forcing EnsureInitialized — avoids allocating and randomizing the full weight tensors just to return zeros. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/NeuralNetworks/Layers/ConvolutionalLayer.cs (2)
729-740:⚠️ Potential issue | 🔴 CriticalBlocking: clear all caches/state that still reference the pre-deserialize tensors.
Deserialize()swaps in new_kernelsand_biases, but_biasReshaped4D,_preAllocatedOutput, and the cached gradient/optimizer tensors still point at the old objects. On a reused instance,Forward()can keep adding the stale bias view, andUpdateParameters()can apply stale gradients/velocity to the newly loaded weights.Suggested fix
ClearRegisteredParameters(); RegisterTrainableParameter(_kernels, PersistentTensorRole.Weights); RegisterTrainableParameter(_biases, PersistentTensorRole.Biases); + _biasReshaped4D = null; + _preAllocatedOutput = null; + _kernelsGradient = null; + _biasesGradient = null; + _kernelsVelocity = null; + _biasesVelocity = null; // Mark as initialized so EnsureInitialized() doesn't re-randomize the // just-deserialized weights on the next Forward/GetParameters call. _isInitialized = true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NeuralNetworks/Layers/ConvolutionalLayer.cs` around lines 729 - 740, Deserialize() replaces _kernels and _biases but leaves other fields and caches pointing at the old tensors; clear or reset any cached state that references pre-deserialize tensors (e.g., _biasReshaped4D, _preAllocatedOutput, any cached gradient/velocity/optimizer tensors or views) immediately after swapping in the new tensors and before calling RegisterTrainableParameter/ClearRegisteredParameters, so Forward() and UpdateParameters() cannot use stale references; also ensure these caches are reallocated or nullified and that _isInitialized is set only after caches are refreshed.
704-740:⚠️ Potential issue | 🔴 CriticalBlocking: save/load is still broken for never-initialized lazy layers.
A lazy layer serialized before any
Forward(),SetParameters(), orGetParameters()call still writes zero kernel/bias values from the[0,…,0]placeholders. This method always reads a full parameter payload, so loading that file will fail withEndOfStreamException.Suggested fix
public override void Serialize(BinaryWriter writer) { + EnsureInitialized(); base.Serialize(writer); writer.Write(InputDepth); writer.Write(OutputDepth);
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/NeuralNetworks/Layers/ConvolutionalLayer.cs`:
- Around line 729-740: Deserialize() replaces _kernels and _biases but leaves
other fields and caches pointing at the old tensors; clear or reset any cached
state that references pre-deserialize tensors (e.g., _biasReshaped4D,
_preAllocatedOutput, any cached gradient/velocity/optimizer tensors or views)
immediately after swapping in the new tensors and before calling
RegisterTrainableParameter/ClearRegisteredParameters, so Forward() and
UpdateParameters() cannot use stale references; also ensure these caches are
reallocated or nullified and that _isInitialized is set only after caches are
refreshed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b775940e-c998-4154-bf18-fb6ba95efa39
📒 Files selected for processing (1)
src/NeuralNetworks/Layers/ConvolutionalLayer.cs
41342c5 to
b7d9e8a
Compare
|
Deployment failed with the following error: Learn More: https://vercel.com/franklins-projects-02a0b5a0?upgradeToPro=build-rate-limit |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/NeuralNetworks/Layers/ConvolutionalLayer.cs (1)
1399-1407:⚠️ Potential issue | 🟠 MajorAvoid eager random initialization in
SetParameters().Calling
EnsureInitialized()first allocates and fills the full tensor set before immediately overwriting it. For the large VGG/Capsule cases, that reintroduces the memory/CPU spike this PR is trying to remove, and even invalid-length inputs now pay that cost before throwing. Validate againstParameterCountfirst, then allocate backing storage withoutInitializeWeights()when the layer is still lazy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NeuralNetworks/Layers/ConvolutionalLayer.cs` around lines 1399 - 1407, Validate the incoming parameter length against the layer's ParameterCount before calling EnsureInitialized so you don't eagerly allocate; in SetParameters compare parameters.Length to ParameterCount (not kernelLen + biasLen) and throw if mismatched, and only after validation allocate or ensure backing storage for _kernels and _biases—but if allocation is needed do so without calling InitializeWeights (avoid calling EnsureInitialized/InitializeWeights before copying) so you don't pre-fill tensors unnecessarily; then copy values into _kernels and _biases and set any initialized flags.
♻️ Duplicate comments (2)
src/NeuralNetworks/Layers/ConvolutionalLayer.cs (2)
734-740:⚠️ Potential issue | 🔴 CriticalBlocking: reset cached bias views when deserializing new tensors.
Deserialize()replaces_biases, but_biasReshaped4Dis left intact. The fused and inference fast paths reuse that cache with??=, so a reused layer instance can keep adding the old bias tensor after load.Suggested fix
ClearRegisteredParameters(); RegisterTrainableParameter(_kernels, PersistentTensorRole.Weights); RegisterTrainableParameter(_biases, PersistentTensorRole.Biases); + _biasReshaped4D = null; + _preAllocatedOutput = null; // Mark as initialized so EnsureInitialized() doesn't re-randomize the // just-deserialized weights on the next Forward/GetParameters call. _isInitialized = true;As per coding guidelines: “Production Readiness (CRITICAL)… Incomplete features: Half-implemented patterns where some code paths work but others silently do nothing.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NeuralNetworks/Layers/ConvolutionalLayer.cs` around lines 734 - 740, When deserializing and replacing the layer's bias tensor in Deserialize(), reset any cached bias views so old views aren't reused: after assigning the new _biases set _biasReshaped4D to null (or its default/empty state) so the fused/inference fast paths that use the ??= cache will recreate the correct view; also ensure this happens before setting _isInitialized = true so EnsureInitialized()/Forward/GetParameters won't re-use stale cached bias views.
729-740:⚠️ Potential issue | 🔴 CriticalBlocking: lazy layers still cannot round-trip through serialization.
Deserialize()now always reads a full kernel/bias payload and marks the layer initialized, butSerialize()still writes zero scalars when the layer stayed lazy and_kernels/_biasesare still[0,…,0]. Saving a never-forwarded lazy layer will truncate the stream and fail on load or corrupt the next payload. Either materialize before writing or persist an explicit “uninitialized” state and read it symmetrically.Suggested fix
public override void Serialize(BinaryWriter writer) { + EnsureInitialized(); base.Serialize(writer); writer.Write(InputDepth); writer.Write(OutputDepth); writer.Write(KernelSize);As per coding guidelines: “Production Readiness (CRITICAL)… Incomplete features: Half-implemented patterns where some code paths work but others silently do nothing.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NeuralNetworks/Layers/ConvolutionalLayer.cs` around lines 729 - 740, Serialize() currently writes raw kernel/bias payloads even when the layer is lazy, causing truncated/corrupt loads because Deserialize() always expects full payloads and marks the layer initialized; fix this by making serialization symmetric: in Serialize(), write an explicit initialized flag (e.g., a boolean before weight data) and if _isInitialized == false write the flag and skip writing _kernels/_biases payloads, and in Deserialize() first read that flag and, if false, set _isInitialized = false and avoid reading/deserializing the kernel/bias raw payloads (do not mark initialized or re-register parameters); adjust places that call EnsureInitialized()/RegisterTrainableParameter/_kernels/_biases handling so the runtime behavior matches the new flag (or alternatively call EnsureInitialized() from Serialize() if you prefer auto-materialization), but implement the explicit flag approach in Serialize() and Deserialize() to guarantee symmetric, safe round-trip behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/NeuralNetworks/Layers/ConvolutionalLayer.cs`:
- Around line 1399-1407: Validate the incoming parameter length against the
layer's ParameterCount before calling EnsureInitialized so you don't eagerly
allocate; in SetParameters compare parameters.Length to ParameterCount (not
kernelLen + biasLen) and throw if mismatched, and only after validation allocate
or ensure backing storage for _kernels and _biases—but if allocation is needed
do so without calling InitializeWeights (avoid calling
EnsureInitialized/InitializeWeights before copying) so you don't pre-fill
tensors unnecessarily; then copy values into _kernels and _biases and set any
initialized flags.
---
Duplicate comments:
In `@src/NeuralNetworks/Layers/ConvolutionalLayer.cs`:
- Around line 734-740: When deserializing and replacing the layer's bias tensor
in Deserialize(), reset any cached bias views so old views aren't reused: after
assigning the new _biases set _biasReshaped4D to null (or its default/empty
state) so the fused/inference fast paths that use the ??= cache will recreate
the correct view; also ensure this happens before setting _isInitialized = true
so EnsureInitialized()/Forward/GetParameters won't re-use stale cached bias
views.
- Around line 729-740: Serialize() currently writes raw kernel/bias payloads
even when the layer is lazy, causing truncated/corrupt loads because
Deserialize() always expects full payloads and marks the layer initialized; fix
this by making serialization symmetric: in Serialize(), write an explicit
initialized flag (e.g., a boolean before weight data) and if _isInitialized ==
false write the flag and skip writing _kernels/_biases payloads, and in
Deserialize() first read that flag and, if false, set _isInitialized = false and
avoid reading/deserializing the kernel/bias raw payloads (do not mark
initialized or re-register parameters); adjust places that call
EnsureInitialized()/RegisterTrainableParameter/_kernels/_biases handling so the
runtime behavior matches the new flag (or alternatively call EnsureInitialized()
from Serialize() if you prefer auto-materialization), but implement the explicit
flag approach in Serialize() and Deserialize() to guarantee symmetric, safe
round-trip behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b0018e32-b218-4d63-a99d-95159b9e06ce
📒 Files selected for processing (2)
src/Helpers/LayerHelper.cssrc/NeuralNetworks/Layers/ConvolutionalLayer.cs
* ci: migrate sonarcloud.yml tests and build to ubuntu-latest
The `test-net10-sharded` job chain was pinned to windows-latest and gated
behind `build-windows`, so every Diffusion / NeuralNetworks / Unit shard on
every PR waited for a Windows build to finish before it could start. Since
these tests run net10.0 (fully cross-platform) and the only Windows-specific
concern is net471 cross-compilation — which the modern .NET SDK handles via
its auto-included reference assembly pack — there's no reason to keep them
on windows runners. Linux runners also start faster, cost less, and parallel
better.
Changes:
- `build` job (formerly `build-windows`): `runs-on: ubuntu-latest`. Keeps
multi-target net10.0+net471 `dotnet build`. Drops the Windows-only MSBuild
flags `-p:UseSharedCompilation=false -p:nodeReuse=false` (file-locking
workarounds that don't apply on Linux) and the `dotnet build-server
shutdown` step that existed for the same reason.
- `test-net10-sharded`: `runs-on: ubuntu-latest`, `needs: build`.
- `sonarcloud`: `runs-on: ubuntu-latest`, `needs: [build, test-net10-sharded]`.
Same `dotnet-sonarscanner` tool works cross-platform.
- `size-check`: `needs: build` (was `needs: build-windows`).
- All `shell: powershell` steps switched to `shell: pwsh`. PowerShell Core
is pre-installed on ubuntu-latest runners and runs the existing
`.github/scripts/report-slow-tests.ps1` and inline scripts without
modification.
- Windows-style `\scanner` paths converted to `/scanner` forward-slash paths;
the `& "${{ runner.temp }}/scanner/dotnet-sonarscanner" end` invocation is
wrapped in `&` to tolerate paths with spaces under pwsh.
- Stale top-level and in-line comments updated to reflect ubuntu-based CI.
Gains: cancelled test shards no longer queue behind a slow Windows build;
ubuntu runners are typically ~30% faster and free up the Windows queue for
the few jobs that actually need it. Combined with the lazy-init fixes in
PR #1137 and PR #1138, this should clear the 45-minute wall-clock
cancellations on the Diffusion / NeuralNetworks / Unit-03 / Unit-08e shards.
Refs #1136
* fix: PR #1139 review comments — sonar cache path + reference assembly comment
SonarCloud cache path: change ~/sonar/cache to ~/.sonar/cache to match the
default dotnet-sonarscanner cache location. Without this the cache action was
writing/reading the wrong directory and every analysis run re-downloaded the
SonarScanner cache from scratch.
Reference assembly comment: replace "auto-included reference assembly pack"
with the accurate mechanism — the Microsoft.NETFramework.ReferenceAssemblies
NuGet package that provides the .NET Framework targeting pack on Linux runners.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: franklinic <franklin@ivorycloud.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @ooples. * #1138 (comment) The following files were modified: * `src/NeuralNetworks/Layers/ConvolutionalLayer.cs`
Summary
Second PR in the #1136 series, targeting the cancelled
Tests (net10.0) - ModelFamily - NeuralNetworksshard on CI. Baseline run 24398739627 cancelled that shard at 45 min with 14 VGG timeouts, 8 Capsule timeouts, 110 FastText OOMs, and several others stacked up in a shared xunit process.What's in this PR
Applies the same
InitializationStrategies<T>.Lazypattern introduced in PR #1137 for diffusion noise predictors, but to the two biggest per-instance OOM offenders in the NeuralNetworks shard.VGGNetwork (14 timeouts + 3 OOMs on baseline): VGG16BN at 224×224×3 with 1000 classes allocates ~1.1 GB of weights per instance. The three fully connected classifier layers alone are ~1 GB (FC1 at 25088×4096 = ~822 MB). Each
ModelFamilyTests.NeuralNetworks.VGGNetworkTests.*was eagerly renting those tensors innew DenseLayer<T>(...).CapsuleNetwork (8 timeouts + 1 OOM): Same eager-rent pattern on the initial 9×9 convolutional layer at production spatial sizes.
Changes
LayerHelper<T>.CreateDefaultVGGLayersandLayerHelper<T>.CreateDefaultCapsuleNetworkLayersto threadInitializationStrategies<T>.Lazyinto the relevantConvolutionalLayerandDenseLayerconstructions. Weight tensors stay at shape[0,…,0]until the firstForward()call.Intentionally out of scope
IAsyncLifetime.DisposeAsynchook onNeuralNetworkModelTestBaseforces a full GC between test methods, which covers this.Conv3DLayer, which doesn't have a lazy-init branch yet. Deferred to the follow-up MHA/LayerNorm/Conv3D lazy-init PR tracked in perf: fix 5 cancelled CI jobs — Diffusion models OOM/timeout from eager weight allocation #1136.Verification
dotnet build src/AiDotNet.csproj --framework net10.0 -c Release— 0 errors.Tests (net10.0) - ModelFamily - NeuralNetworksshould transition CANCELLED → SUCCESS or FAILURE. Pre-existing test failures are acceptable at this stage — the goal is unblocking the CI signal.Depends on: none (branched off master). Does not conflict with PR #1137 (different files).
Refs #1136
Summary by CodeRabbit
Refactor
Bug Fixes