fix(#1468): AiModelBuilder.BuildAsync on CNN/multi-dim-input NN models - #1477
Conversation
…x + GPU package bump)
AiModelBuilder.BuildAsync threw ArgumentOutOfRangeException ("Feature index N
exceeds the input dimension D") for any prebuilt NN whose first layer has a
non-flat input shape (CNN, GRU/LSTM, ViT, etc.). The optimizer's feature-selection
pass derives feature indices from the FLATTENED input size, but a multi-dimensional
NN validates/handles "features" against the first axis of its first layer's input
shape, so the counts diverge and any index >= that axis throws.
"Feature selection" (selecting a subset of input columns) is a tabular concept that
doesn't apply to spatial/sequence input, so these models train on the full input and
skip subsetting — generalizing the embedding-only #1113 fix:
- OptimizerBase: new HasNonFlatNeuralInput detector; multi-dim models use all features
and skip the data-subsetting block (flat indices don't map to a tensor's feature
axis, and GetColumnVectors/SelectFeatures would throw on rank>=3). SelectedFeature
indices stay the flat identity range, which AiModelResult treats as a no-op at
predict time.
- NeuralNetworkBase.SetActiveFeatureIndices: defensively no-ops for a multi-dim first
layer (rank > 1), mirroring the existing EmbeddingLayer guard.
Also bumps AiDotNet.Tensors 0.91.0 -> 0.91.1 to pick up Tensors#527, which fixes the
GPU MaxPool2D backward crash (float-vs-int indices + CUDA 9th-arg) that surfaced once
the feature-index fix let CNN training actually reach the GPU backward pass.
Tests: FacadeMultiDimInputBuildAsyncTests — the CNN BuildAsync repro from the issue
(train + predict; passes on real GPU and CPU) and a direct SetActiveFeatureIndices
no-op guard. Regression slice (AiModelBuilderPredictIntegrationTests +
RLTrainingIntegrationTests) stays green: flat-input models are unaffected.
Closes #1468
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughSkip feature-subsetting for models whose first layer expects multi-dimensional inputs, make layer validation and shape resolution tolerant of unresolved (-1) dims, fix reshape/split per-sample logic, add integration tests covering multi-dim/lazy-init cases, and bump AiDotNet packages to 0.91.1. ChangesNon-flat neural network input feature selection handling
AiDotNet ecosystem package updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Optimizers/OptimizerBase.cs`:
- Around line 581-607: The current logic sets selectedFeatures to an empty
List<Vector<T>> for hasNonFlatInput which collides with the sentinel check that
treats SelectedFeatures.Count == 0 as "uninitialized best step" (causing valid
multi-dimensional results to be overwritten); change the sentinel usage so it
does not rely on an empty list — either (A) initialize selectedFeatures to null
for multi-dimensional inputs and update all downstream checks to use null vs
non-null rather than Count==0, or (B) introduce a dedicated boolean flag (e.g.,
hasSelectedFeaturesInitialized) that you set true only when SelectFeatures was
actually run and use that flag everywhere the code currently checks
SelectedFeatures.Count; alternatively, update the sentinel check to use
bestStepData.Solution == null instead of SelectedFeatures.Count == 0; update
references to SelectedFeatures, hasNonFlatInput, selectedFeaturesIndices, and
bestStepData.Solution accordingly.
🪄 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: bb60b068-7690-4242-b11f-0bc81d760519
📒 Files selected for processing (4)
Directory.Packages.propssrc/NeuralNetworks/NeuralNetworkBase.cssrc/Optimizers/OptimizerBase.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/FacadeMultiDimInputBuildAsyncTests.cs
…al, split, reshape, GLU/LSTM) Follow-on to the CNN/multi-dim BuildAsync fix: a sweep of the prebuilt NN layers surfaced and fixed several construction/shape bugs in the same family, all of which block building or inspecting these models before training. Lazy-shape validation (same class as #1113 embedding / the CNN feature-index fix): - Autoencoder.ValidateCustomLayers: compared layer shapes with SequenceEqual, but DenseLayer's input shape is lazily [-1] until the first forward. A valid symmetric DenseLayer autoencoder threw "Input and output layer sizes must be the same" at construction. Now treats an unresolved (-1) dimension on either side as a wildcard (new ShapesCompatibleIgnoringUnresolved); resolved dims still compared exactly. - ResidualLayer.ValidateInnerLayer: same SequenceEqual-vs-lazy-[-1] issue — a residual block wrapping a lazy DenseLayer threw "Inner layer must have the same input and output shape". Same wildcard treatment. Batch-dimension handling in layer shape resolution: - SplitLayer.OnFirstForward: checked shape[0] (the BATCH size) for divisibility and returned a 1-D output shape, so input [2,32] split into 4 threw "Input size must be divisible by the number of splits" (2 % 4). Now splits the per-sample feature (last) dimension and resolves [numSplits, featureSize/numSplits], matching Forward. - ReshapeLayer.OnFirstForward: multiplied the whole input shape (including batch) and compared to the per-sample output, so [2,8,4] -> [32] threw on 64 != 32. Now counts per-sample elements (dims 1..) and resolves the per-sample input shape. Lazy parameter-count tests (layers are correctly lazy; tests under-specified): - GatedLinearUnitLayer / LSTMLayer ParameterCount tests asserted a resolved count without resolving the input dimension (constructors fix only the output/hidden size). They now run one forward to allocate the input-dependent weights before counting (GLU == 4160; LSTM > 0). Tests: FacadeMultiDimInputBuildAsyncTests gains autoencoder + residual-layer guards; the six AdvancedLayersIntegrationTests above pass; full AdvancedLayers suite green (211/211). Relates to #1468 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/AdvancedLayersIntegrationTests.cs`:
- Around line 817-822: The timeout-decorated async test methods in
AdvancedLayersIntegrationTests.cs are still effectively synchronous and need an
initial yield to enforce xUnit timeouts; add await Task.Yield(); as the very
first statement in each async test (including the test that currently does the
warmup call to layer.Forward(Tensor<float>.CreateRandom([2, 64])) and the other
timeout-decorated tests around the 1582–1586 region) before any warmup or test
logic so the test actually yields to the test runner and the [Fact(Timeout =
...)] will be enforced.
In
`@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/FacadeMultiDimInputBuildAsyncTests.cs`:
- Around line 139-142: Replace the initial await in the async test method
Autoencoder_CustomSymmetricLayers_ConstructsAndPredicts (and any other tests
that currently use await Task.CompletedTask, e.g., the one noted around lines
180-183) with await Task.Yield(); so the method yields to the caller immediately
and xUnit's [Fact(Timeout = ...)] can enforce timeouts; locate the method bodies
that start with await Task.CompletedTask and change them to await Task.Yield()
as the first statement.
🪄 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: ccdc94cc-7797-42cc-b637-1e09510ec46c
📒 Files selected for processing (6)
src/NeuralNetworks/Autoencoder.cssrc/NeuralNetworks/Layers/ReshapeLayer.cssrc/NeuralNetworks/Layers/ResidualLayer.cssrc/NeuralNetworks/Layers/SplitLayer.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/AdvancedLayersIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/FacadeMultiDimInputBuildAsyncTests.cs
…eout tests - OptimizerBase.UpdateBestSolution: the "uninitialized best step" sentinel keyed off SelectedFeatures.Count == 0 && FitnessScore == 0. Multi-dimensional-input models now produce an empty SelectedFeatures list (feature subsetting doesn't apply — #1468), so that sentinel treated every such step as uninitialized and could overwrite a real best without a fitness comparison whenever the score was 0. Switched to "Solution is null" (captured as a local and tested inline so the non-null narrowing flows to the bestResult build). (CodeRabbit) - AdvancedLayersIntegrationTests (GLU/LSTM ParameterCount) + FacadeMultiDimInputBuildAsyncTests: added `await Task.Yield()` as the first statement / replaced `await Task.CompletedTask` so [Fact(Timeout=...)] enforcement is actually active on these async tests. (CodeRabbit) Verified: 21/21 across FacadeMultiDimInputBuildAsyncTests, GLU/LSTM param-count, and AiModelBuilderPredictIntegrationTests (confirms the sentinel change doesn't regress flat-input best-solution tracking). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
TryFoldBatchNormIntoDense / TryFoldBatchNormIntoConv rewrite the layer's weight (and bias) tensors in place via Data.Span, but — unlike every other weight-mutation path (SetWeights, SetParameters, which call Engine.InvalidatePersistentTensor) — they never invalidated the engine's persistent-tensor cache. On a forward path that serves weights from that cache (native-BLAS pre-pack / GPU-resident buffer), the post-fold forward reused the STALE pre-fold weights, so the BatchNorm fold silently had no effect and the optimized model's output diverged sharply from the reference. This is the cause of ConvBatchNormFoldTests.FoldBatchNorm_IntoDense_PreservesOutput failing on the Linux native-BLAS CI runner (folded output diverged by ~0.18) while passing on the managed path, which happened to re-read the arrays. Invalidate the weight + bias persistent-tensor entries after folding, mirroring the other mutators. Co-Authored-By: Claude Opus 4.8 (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/NeuralNetworkBase.cs (1)
7998-8021:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the new no-op contract for non-flat inputs.
SetActiveFeatureIndicesnow silently ignores requests for embedding and multi-dimensional first-layer inputs, but the XML docs above still promise validation and explicit activation semantics, includingArgumentOutOfRangeExceptionfor invalid indices. Please update the remarks/exception contract to state that these models ignore feature selection rather than validating it.As per coding guidelines, methods on base classes that users call directly should be scrutinized.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 7998 - 8021, Update the XML documentation for NeuralNetworkBase.SetActiveFeatureIndices to reflect the new no-op behavior for models whose first layer is an EmbeddingLayer<T> or has a multi-dimensional input shape: change the remarks to explain that feature selection is ignored for sequence/vision/multi-dimensional inputs (no validation or activation occurs), and update the exception/returns contract to remove or qualify the guarantee of ArgumentOutOfRangeException for invalid indices (document that validation only applies for flat first-layer inputs and that for ignored cases the method clears any stale _explicitlySetActiveFeatures and returns silently). Ensure the doc text references SetActiveFeatureIndices, Layers.EmbeddingLayer<T>, GetInputShape(), and _explicitlySetActiveFeatures so callers understand the new behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/NeuralNetworks/NeuralNetworkBase.cs`:
- Around line 7998-8021: Update the XML documentation for
NeuralNetworkBase.SetActiveFeatureIndices to reflect the new no-op behavior for
models whose first layer is an EmbeddingLayer<T> or has a multi-dimensional
input shape: change the remarks to explain that feature selection is ignored for
sequence/vision/multi-dimensional inputs (no validation or activation occurs),
and update the exception/returns contract to remove or qualify the guarantee of
ArgumentOutOfRangeException for invalid indices (document that validation only
applies for flat first-layer inputs and that for ignored cases the method clears
any stale _explicitlySetActiveFeatures and returns silently). Ensure the doc
text references SetActiveFeatureIndices, Layers.EmbeddingLayer<T>,
GetInputShape(), and _explicitlySetActiveFeatures so callers understand the new
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e955d7bb-ba2d-4f0b-b675-745d770fa60e
📒 Files selected for processing (1)
src/NeuralNetworks/NeuralNetworkBase.cs
…#1454) + Tensors 0.91.2 (#528) (#1485) * fix(ci): clear per-shape tensor caches between heavy tests + shard diffusion by model The heavy ModelFamily / NN / Diffusion test shards were OOM-killed on the 16 GB CI runners ("The runner has received a shutdown signal"), with tests passing right up to the kill — a cumulative memory leak, not a hang or a single oversized model. Confirmed from the #1477 job logs (runner dies 2-6 min in, ~14 GB available at start, fail-fast:false so no cascade). Two independent contributors, fixed together: 1. Per-shape tensor caches were never cleared between tests. AutoTensorCache (and the TensorArena persistent pool) pool tensors keyed by SHAPE across ALL worker threads; over a shard of dozens-to-hundreds of distinct models they retain multiple GB of weight-sized tensors that GC.Collect cannot reclaim while the pool holds them. The existing teardowns did compacting Gen-2 + LOH GC and reset WeightRegistry, but never dropped these pools — so even MaxParallelThreads=1 + Server GC + GCConserveMemory=9 still accumulated ~160 MB/test net and exhausted RAM after ~80 models. Both NeuralNetworkModelTestBase and the two diffusion bases now call TensorCacheSettings.ClearCache() + TensorArena.ClearPersistentPool() + WeightRegistry.Reset() before their GC pass, returning to a clean baseline between tests. Clearing reusable caches is safe — it cannot change results (unlike the reverted weight-streaming attempts, which leaked pool handles). 2. The three ModelFamily Diffusion shards filtered by test-METHOD first letter (FullyQualifiedName~Tests.A …), which made EACH shard instantiate ALL ~244 diffusion models (just different subsets of each model's methods), so all three carried the full distinct-model cache footprint. Re-sharded by MODEL CLASS first letter (~Diffusion.A …) so each shard only loads its letter range (~1/3 the models) — same three runners, ~3x less cumulative memory. CI (the real 16 GB runners running the full model sets) is the faithful validator for the memory envelope; local repro is impractical because the diffusion training tests run for many minutes serially. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * deps: bump aidotnet.tensors 0.91.1 -> 0.91.2 (consume #528 conv/reduction perf) Pulls in AiDotNet.Tensors#528 (allocation-free axis reductions + small-batch channel-parallel conv-backward; ResNet50 fp64 train step 2553ms -> ~1150ms, bit-identical). This is the COMPUTE half of #1463 — together with the memory/OOM cache-clearing + diffusion re-shard already in this PR, #1485 now resolves both halves of the paper-scale CNN/diffusion CI-shard failures, so merging it closes #1463. Native packages bumped in lockstep to 0.91.2. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: apply CodeRabbit auto-fixes Fixed 2 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai> * fix: apply CodeRabbit auto-fixes Fixed 2 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai> * ci: drop ineffective tensor-cache clearing from heavy test teardowns A/B measurement (NN ModelFamily, MaxParallelThreads=1, in-process working-set trace after each teardown) shows the cache-clearing does NOT bound memory: teardown: 50 100 200 300 peak no clear: 3328MB 5879MB 10650MB 18067MB 55091MB with clear: 3885MB 10691MB 19294MB 26073MB 55709MB Both climb identically (~94 MB per test-method, linear) to ~55 GB. Clearing AutoTensorCache / TensorArena / WeightRegistry frees the per-shape pools but those are NOT the dominant grower — the leak is a rooted *managed* per-model- instance accumulation (GetTotalMemory climbs too) that survives Dispose + compacting Gen-2 GC + the cache clears. Removing the no-op clearing (and its misleading comments); the real per-instance leak needs a heap-dump root-cause hunt, tracked separately. The diffusion shard re-sharding (#1454) and the Tensors 0.91.2 bump in this PR are unaffected and stand on their own. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(ci): serialize heavy shards via xunit.runner.json to fix runner oom The `-- xunit.MaxParallelThreads=1` inline arg is silently ignored by the xunit.runner.visualstudio (VSTest) adapter; only xunit.runner.json in the build output is honored. Heavy shards therefore still ran test classes in parallel (maxParallelThreads = ProcessorCount), each holding a multi-GB model + Adam state for the whole class, peaking ~55 GB on a 16 GB runner and triggering the OOM shutdown signal. Rewrite the built xunit.runner.json to parallelizeTestCollections=false + maxParallelThreads=1 for the heavy shards only, immediately before dotnet test. Locally confirmed: serial trajectory peaks ~4.5 GB (one model in flight) and oscillates ~550 MB with no cumulative leak, vs ~55 GB under the parallel default. Every other shard keeps the parallel JSON default and runs full-speed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(ci): workstation gc + serial discovery for heavy shards; fail fast on missing config Serialization alone did not fix the heavy-shard runner OOM. CI logs show `parallel test collections = off` took effect, yet the shard still OOM'd ~70s into serial execution — so the remaining cause is footprint, not parallelism. Two contributors: the global Server GC (DOTNET_gcServer=1, chosen for parallel-collection throughput) reserves a heap segment per core and collects lazily, and the assembly discovers ~64k test cases. For the now-serial heavy shards: - override to Workstation GC (DOTNET_gcServer=0): smaller footprint, eager collection under memory pressure; - disable theory pre-enumeration (preEnumerateTheories=false) so discovery doesn't materialize every theory's data rows up front; - fail fast (exit 1) when the runner.json rewrite can't be applied instead of silently falling back to a parallel run that OOMs (addresses CodeRabbit). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Closes #1468
Summary
AiModelBuilder<T, Tensor<T>, Tensor<T>>.BuildAsync()threwArgumentOutOfRangeException("Feature index N exceeds the input dimension D") for any prebuilt NN whose first layer has a non-flat input shape (CNN, GRU/LSTM, Vision Transformer, ResNet, …), breaking the facade pattern for every CV/sequence model. This PR fixes that and the related prebuilt-layer shape/validation bugs surfaced in the same family (including the Autoencoder issue called out in #1468).1. Feature-index bug (the headline issue)
The optimizer's feature-selection pass derives indices from the flattened per-sample input size, but a multi-dimensional NN handles "features" against the first axis of its first layer's input shape — the counts diverge, so any index ≥ that axis throws (and the "use all features" path then breaks the tensor in
GetColumnVectors/SelectFeatures).OptimizerBase— newHasNonFlatNeuralInputdetector; multi-dim models use all features and skip the data-subsetting block (flat indices don't map to a spatial/sequence tensor's feature axis).SelectedFeatureIndicesstays the flat identity range → predict-time no-op. Generalizes the embedding-only AiModelBuilder: Feature selection crashes when training Transformer models #1113 fix.NeuralNetworkBase.SetActiveFeatureIndices— defensively no-ops for a multi-dimensional first layer (rank > 1).2. Lazy-shape validation (Autoencoder + ResidualLayer)
DenseLayer's input shape is lazily[-1]until the first forward, so structural checks usingSequenceEqualfailed at construction:Autoencoder.ValidateCustomLayers— a valid symmetric DenseLayer autoencoder threw "Input and output layer sizes must be the same". Now treats an unresolved-1dim as a wildcard (ShapesCompatibleIgnoringUnresolved); resolved dims still compared exactly. (The Autoencoder validation issue explicitly listed in AiModelBuilder.BuildAsync fails on CNN/multi-dim-input models · feature index exceeds input dimension #1468.)ResidualLayer.ValidateInnerLayer— a residual block wrapping a lazyDenseLayerthrew "Inner layer must have the same input and output shape". Same wildcard treatment.3. Batch-dimension handling in layer shape resolution
SplitLayer.OnFirstForward— checkedshape[0](the batch size) for divisibility and returned a 1-D shape, so input[2,32]split into 4 threw2 % 4. Now splits the per-sample feature (last) dimension →[numSplits, featureSize/numSplits], matchingForward.ReshapeLayer.OnFirstForward— multiplied the whole input shape (incl. batch) vs the per-sample output, so[2,8,4] → [32]threw on64 != 32. Now counts per-sample elements and resolves the per-sample input shape.4. Lazy parameter-count tests (GLU / LSTM)
GatedLinearUnitLayer/LSTMLayerare correctly lazy (constructors fix only the output/hidden size; input-dependent weights size on first forward). TheirParameterCounttests asserted a resolved count without resolving — now they run one forward to allocate the weights before counting (GLU== 4160, LSTM> 0).GPU dependency bump (0.91.0 → 0.91.1)
Once the feature-index fix let CNN training reach the GPU backward pass, it surfaced a crash in
AiDotNet.Tensors' GPU MaxPool2D backward (CUDA0xC0000005/ OpenCL-5): pooling indices uploaded asfloat[]but kernels readint*, and the CUDA kernel launched 8 args for a 9-param kernel. Fixed in AiDotNet.Tensors#527 (shipped in0.91.1); this PR bumps the pin (lockstep Native packages too) to consume it.Verification
FacadeMultiDimInputBuildAsyncTests(4 tests): the issue's CNNBuildAsyncrepro (train + predict, passes on a real CUDA GPU against the published0.91.1), theSetActiveFeatureIndicesno-op guard, the custom-layer Autoencoder construct+predict, and the ResidualLayer lazy-inner guard.AdvancedLayersIntegrationTests: the six Split/Reshape/GLU/LSTM failures fixed; full suite green (211/211).AiModelBuilderPredictIntegrationTests+RLTrainingIntegrationTests, 17 tests): green — flat-input models unaffected.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Chores