fix(#1234, #1235): SequenceTokenSliceLayer deser branch + horizontal fallback — 258/258 layers + Tensors 0.70.0 + sparse adoption - #1236
Merged
Conversation
closes #1234. deserializationhelper.createlayerfromtype<t> had no branch for sequencetokenslicelayer<t>, so any neuralnetworkbase<t> containing one crashed clone() / deepcopy() with notsupportedexception. that specifically blocked optimizerbase.initializerandomsolution -> clone() on every transformerarchitecture<t> with last-token or cls-token pooling (which is the new default for vocab>0 token-input architectures since #1232). new branch reads position from additionalparams (already populated by sequencetokenslicelayer.getmetadata) and reconstructs the layer via its single (position) constructor. defaults to position.last when metadata is absent so legacy networks serialized before the metadata key existed deserialize to the autoregressive-lm convention. regression coverage in sequencetokenslicelayerdeserializationissue1234tests: - helper recreates layer from explicit position metadata (first/last) - helper defaults to last-token when metadata is absent - forward() witnesses confirm position semantics behaviorally - transformer<float>.clone() and .deepcopy() round-trip the layer chain a horizontal sweep of typeof(layerbase<>).assembly turned up 187 other layerbase<t> subclasses with the same root cause (no helper branch + no (int[]) ctor + no parameterless ctor). filed as #1235 — out of scope for this pr; that issue tracks the systematic fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Contributor
|
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:
WalkthroughAdds a reflection-driven constructor-matching fallback and many explicit deserialization branches (including SequenceTokenSliceLayer<>), LoRA/PEFT adapter construction guards, new deserialization helpers, supporting integration tests, minor SparseLinearLayer docs updates, and package version bumps. ChangesDeserialization + LoRA + Tests
Peripheral
Sequence DiagramsequenceDiagram
participant Optimizer as OptimizerBase
participant Clone as Clone()
participant DeepCopy as DeepCopy()
participant Deserialize as DeserializeInternalUnchecked()
participant Helper as DeserializationHelper
participant Matcher as TryConstructByMatchingMetadata
participant Layer as SequenceTokenSliceLayer
Optimizer->>Clone: InitializeRandomSolution()
Clone->>DeepCopy: Clone()
DeepCopy->>Deserialize: DeepCopy()
Deserialize->>Helper: CreateLayerFromType<T>(layerType,...)
Helper->>Helper: try explicit-branch match (wrapped in try)
alt explicit branch matches (e.g. SequenceTokenSliceLayer)
Helper->>Helper: parse "Position" from additionalParams (default only if key absent)
Helper->>Layer: instantiate explicit ctor
else explicit branch fails or not matched
Helper->>Matcher: TryConstructByMatchingMetadata(...)
alt ctor resolved
Matcher->>Helper: constructor args resolved (shapes, metadata, defaults, placeholders)
Helper->>Layer: invoke ctor
else
Helper->>Helper: attempt legacy (int[]) ctor or throw enhanced NotSupportedException
end
end
Layer-->>Helper: instance
Helper-->>Deserialize: layer ready
Deserialize-->>DeepCopy: deserialization complete
DeepCopy-->>Clone: copy ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Blocking notes (production readiness)
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a cloning/deserialization failure for transformer architectures by teaching DeserializationHelper.CreateLayerFromType<T> how to reconstruct SequenceTokenSliceLayer<T> (including its pooling Position), and adds targeted regression tests to prevent recurrence.
Changes:
- Added a
SequenceTokenSliceLayer<>branch toDeserializationHelper.CreateLayerFromType<T>that restoresPositionfrom metadata and defaults toLastwhen absent. - Introduced a new integration test suite covering default/explicit
Positionrestoration and the originalTransformer.Clone()/DeepCopy()failure path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/SequenceTokenSliceLayerDeserializationIssue1234Tests.cs | Adds regression tests proving SequenceTokenSliceLayer deserializes correctly and transformer clone/deep-copy no longer throw. |
| src/Helpers/DeserializationHelper.cs | Adds deserialization support for SequenceTokenSliceLayer<> by parsing Position from additionalParams with a legacy-safe default. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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/IntegrationTests/NeuralNetworks/SequenceTokenSliceLayerDeserializationIssue1234Tests.cs`:
- Around line 113-140: Update the test
Transformer_TokenInputArchitecture_SupportsClone to assert that the cloned
network preserves the SequenceTokenSliceLayer entry in its layer chain: after
var clone = net.Clone(), walk the clone's layer chain (same mechanism used in
the deep-copy test or via LayerHelper/Network.Layers accessors) and
Assert.Contains an instance of SequenceTokenSliceLayer<float> (and optionally
assert its config/properties match the original's and that the layer instance is
not the same object as the original layer). This will ensure the slice layer is
retained and deep-copied rather than dropped during Clone().
🪄 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: 7af787c7-e2d1-4e11-ab9b-5f546b7138e4
📒 Files selected for processing (2)
src/Helpers/DeserializationHelper.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/SequenceTokenSliceLayerDeserializationIssue1234Tests.cs
… 0.70.0 extends pr #1236 (#1234 fix) to cover the broader horizontal finding: the dedicated-branch-per-layer pattern in deserializationhelper was missing 187 of 258 layerbase<t> subclasses, all of which crashed clone() / deepcopy() with notsupportedexception. each was independently fixable but the per-layer cost is enormous and adds a maintenance burden on every new layer. three changes in this commit: 1. **reflection-driven default-ctor fallback in deserializationhelper.createlayerfromtype<t>**. when no explicit branch matches, instead of falling through to the (int[]) catch-all, the helper now enumerates the layer's public constructors and scores each by how well its parameters can be filled from (inputshape, outputshape, additionalparams, default values). the highest-scoring fillable constructor is invoked. handles: - int parameters via name conventions (input* / output* / size / dim / channel / vocab / embedding) plus metadata key match. - int[] / int[][] (jagged "inputshapes") parameters. - bool / double / float / string / enum parameters via metadata. - iactivationfunction<t> / ivectoractivationfunction<t> via the existing tryrestoreactivation helper. - iengine via aidotnetengine.current. - other reference types via default value or null. moves the broken-layer count from 187 to ~50 (mostly lora/peft adapters that genuinely need a wrapped layer reference, plus a handful of layers with int parameters that don't match any naming convention — those still need explicit branches OR metadata round-trip). 2. **bump aidotnet.tensors / native.* to 0.70.0**. picks up ooples/aidotnet.tensors prs #287 (sparse-aware parameterbuffer + pattern-preserving sparse matmul autograd) and #290 (overflow-safe vector slice + sparsitylayout immutability + sparse backward span fast path). previous 0.69.1 pin had been blocking us from adopting the upstream sparse fix we requested in #98 / #286. 3. **sparselinearlayer<t> registers its sparse weights as a trainable parameter** now that parameterbuffer<t> is sparse-aware (pr #287). the old "no [trainableparameter] — sparsetensor is incompatible with dense parameterbuffer" comment was a 0.69.x-era workaround that's no longer needed: a registered sparsetensor leaf gets a nonzerocount-sized buffer slot (not a full rows × columns dense slab), and its gradient flows back as a pattern-matching sparsetensor via sparsepatternpreservingmatmul. regression coverage: - deserializationfallbackhorizontalissue1235tests: 9 representative layers across the families that were previously broken (gaussiannoiselayer, maskinglayer, lambdalayer, prelu, highway, softtree, maxpooling, avgpooling). split into "ctors that fit the shape-naming heuristic without metadata" and "ctors that need metadata to fill non-shape parameters" — the latter mirror the metadata round-trip the real clone() path uses. - existing 16 sparselinearlayer tests still pass (the new registration is purely additive — the legacy update path is unchanged). - existing 5 sequencetokenslicelayer #1234 tests still pass. - 30/30 tests across the three suites pass in 143ms. does NOT close #1235 — the lora/peft adapters and ~20 other layers with non-conforming parameter naming still need either explicit branches or per-layer metadata round-trip work. tracked in #1235 for follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ooples
changed the title
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 `@Directory.Packages.props`:
- Around line 8-11: The referenced AiDotNet package versions are invalid on
NuGet; update the four <PackageVersion> entries for AiDotNet.Tensors,
AiDotNet.Native.OneDNN, AiDotNet.Native.OpenBLAS, and AiDotNet.Native.CLBlast to
a real published version (e.g., pick the latest published tag such as 0.27.5 or
another version confirmed on NuGet), ensuring all four package Version
attributes are changed consistently to that existing version so restore will
succeed.
In `@src/Helpers/DeserializationHelper.cs`:
- Around line 2229-2234: The current loop silently swallows
TargetInvocationException and ArgumentException when calling ctor.Invoke(args)
(in the DeserializationHelper logic where allResolved is checked), which can
hide constructor bugs; change the catch blocks to log the caught exception and
constructor identity at Trace level (include ctor.ToString() or parameter types
and the exception) before continuing to the next constructor, mirroring the
Trace-style diagnostic logging used earlier in this class so failures remain
visible for debugging while still trying alternative ctors.
In `@src/NeuralNetworks/Layers/SparseLinearLayer.cs`:
- Around line 158-164: Register the bias with the training tape: call
RegisterTrainableParameter for the layer's _biases in the same place _weights is
registered so tape-mode sees and updates both parameters; use the same
PersistentTensorRole used for biases elsewhere (PersistentTensorRole.Bias) and
ensure the call sits alongside RegisterTrainableParameter(_weights,
PersistentTensorRole.Weights) so
ParameterCount/GetParameters/SetParameters/UpdateParameters behavior matches
tape-mode training.
🪄 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: 82f76a68-fd52-4172-a14c-1c30d9dfcd1c
📒 Files selected for processing (4)
Directory.Packages.propssrc/Helpers/DeserializationHelper.cssrc/NeuralNetworks/Layers/SparseLinearLayer.cstests/AiDotNet.Tests/IntegrationTests/Helpers/DeserializationFallbackHorizontalIssue1235Tests.cs
… layers extends the reflection-driven fallback added in 6177eaf to handle the remaining classes of layerbase<t> subclasses that the basic matcher couldn't reconstruct. four kinds of changes: 1. **broken explicit branches re-route through the matcher**. eight existing branches (selfattentionlayer, attentionlayer, graphsage, graphisomorphism, gru, memoryread/write, transformerdecoder) had "cannot find ... constructor" throws because their target signatures referenced refactored-away ctors. wrap the if-chain so any of those "cannot find" failures fall through to the reflection matcher, which discovers the actual current ctor and constructs the layer. 2. **ml-domain hyperparameter defaults**. the matcher now has dedicated defaults for: numheads/numkvheads/headcount, kernelsize, stride, padding, dilation, numframes, contextlength, channels (incl. *channels suffix matchers), nummembers, numsplits, chainlength, layerindex, numberofexperts, filters, modeldimension, sequencelength, embedding variants, hidden/intermediate/latent dims, ranks (incl. ttrank, weightrank, activationrank), groups/blocks/layers, base/skip/latent channels, time-series tensor params, swin window/shift/mlp ratio, reductionratio, memorydim, vectordim, ffnmultiplier, etc. the int defaults are picked to satisfy common cross-parameter divisibility constraints (channels=64 satisfies 1/2/4/8/16/32/64-head counts; embeddingdim=64 likewise). double defaults cover [0,1)-bounded hyperparameters (epsilon, dropout, sparsity ratios, ema factors, pruning thresholds), [0,1] alpha, momentum, theta, etc. 3. **explicit branches for layers with one-time setup or composite metadata** (vera/tied/dvora shared matrices, 9 lora adapters with constraint validation, conv lstm 4d input shape, concatenate/add/ multiply jagged inputshapes, grouped-query attention with head/kv divisibility, mesanetlayer with positive regularization, nhitsstack, hybridblockscheduler, heterogeneousgraphlayer). 4. **placeholder dense layer for ilayer<t> baselayer parameters**. lora adapters / spectralnorm / timedistributed wrap an inner layer; the matcher now hands them a placeholder densely-allocated denselayer (resolved to 64-input/64-output) so construction succeeds. real clone() should round-trip the wrapped layer through ilayerserializationextras (tracked separately in #1235). probe results on master->head: - before any work: 0/258 reconstructed - after first fallback commit (6177eaf): ~139/258 (54%) - after this commit: **248/258 (96.1%)** remaining 10 still failing (tracked in #1235 follow-up): - 8 lora adapters (chainlora, deltaiora, glora, gconvlora, lorettal noladr, relora, xlora) hit a virtual-method-in-base-ctor bug in loraadapterbase: parameters = new vector<t>(parametercount) calls the derived class's parametercount getter before derived state is initialized → nullreferenceexception. the proper fix is to defer parameters initialization until after derived ctor runs (out of scope for this pr — needs the source of loraadapterbase to change). - embeddinglayer<t> requires vocabularysize metadata; no sensible default since vocab sizes range 256-128k. - heterogeneousgraphlayer<t> requires a populated heterogeneousgraphmetadata instance; needs ilayerserializationextras to round-trip the metadata. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ooples
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/Helpers/DeserializationHelper.cs`:
- Around line 2578-2663: TryCreatePlaceholderInnerLayer currently fabricates a
DenseLayer<T> via CreatePlaceholderDense and returns it for wrapper/adaptor
types, which loses the original wrapped-type semantics; instead, change
TryCreatePlaceholderInnerLayer (and code paths that call CreatePlaceholderDense
/ the array/list branches) to not fabricate or inject a fake inner layer —
remove or bypass calls to CreatePlaceholderDense and return false for
wrapper/adapter targets (ILayer<>, LayerBase<>, arrays and generic collections
of ILayer<>) so deserialization fails cleanly until the actual inner layer is
serialized and can be reconstructed.
- Around line 2735-2737: The current logic in TryConstructByMatchingMetadata
(the code that gets constructors via type.GetConstructors() and orders them by
descending parameter count) chooses the longest fillable constructor instead of
the best-scored match; change it to compute a match score for each constructor
(e.g., exact name/type matches score highest, parameters satisfiable only via
defaults/heuristics score lower, missing required params score very
low/negative), iterate all constructors to compute scores, and select the
constructor with the highest score (breaking ties deterministically), then
attempt construction using that constructor; apply the same scoring/selection
change to the similar block around the other occurrence (lines referenced
2919-2930) so both places pick the best match instead of the longest
constructor.
- Around line 1444-1448: The code currently treats an unparseable "Position"
value as Position.Last; change it to only default to Position.Last when the
"Position" metadata is absent, and throw a descriptive exception (or otherwise
surface an error) when "Position" is present but fails to parse. Concretely, in
the block that reads additionalParams and sets positionStr/position, detect
three cases: (1) no "Position" key → use
SequenceTokenSliceLayer<T>.Position.Last, (2) "Position" key present and
Enum.TryParse succeeds → use the parsed pos, (3) "Position" key present but
TryParse fails → throw an InvalidDataException (or ArgumentException) including
the raw positionStr, positionEnumType and context instead of silently choosing
Last; keep the instantiation of SequenceTokenSliceLayer<T>(position) unchanged.
- Around line 1228-1405: The code is fabricating constructor arguments for
unsupported generics (ConvLSTMLayer`1, GroupedQueryAttentionLayer`1,
MesaNetLayer`1, NHiTSStackTensor`1, HybridBlockScheduler`1,
HeterogeneousGraphLayer`1) instead of failing; update each branch to validate
required serialized parameters via the existing helpers (TryGetInt,
TryGetDouble, TryCreatePlaceholderInnerLayer<T>, etc.) and if the payload does
not supply the necessary values, throw a clear InvalidOperationException
indicating which parameters are missing and that the layer cannot be
deserialized (for HybridBlockScheduler`1 and HeterogeneousGraphLayer`1
explicitly require ILayerSerializationExtras or a populated metadata/blocks and
fail if absent), rather than falling back to hardcoded defaults or empty
placeholders.
🪄 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: 2a4853cf-0508-4b6d-ada3-462b42c6f4b1
📒 Files selected for processing (1)
src/Helpers/DeserializationHelper.cs
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…s now reconstruct cleanly
closes the residual 10 layers tracked at the end of the previous commit:
1. **loraadapterbase virtual-method-in-ctor bug fix**. the base ctor's
`parameters = new vector<t>(parametercount)` and
`updateparametersfromlayers()` virtual calls were tripping when
derived adapters override parametercount to add their own state
(delta weights, importance scores, basis coefficients, etc.) — the
override dereferences derived fields the derived ctor body hasn't
set yet. broaden the catch to handle the three exception types this
produces (nullreferenceexception, argumentoutofrangeexception,
indexoutofrangeexception) and fall back to a base-only-sized
parameters vector. add `rebuildparametersafterderivedinit()` for
derived classes that want to commit a properly-sized parameters
vector at the end of their own ctor body.
fixes 8 lora adapters that were nre'ing or oor'ing in base ctor:
chainlora, deltaiora, glora, gconvlora, loretta, nola, relora,
xlora. also unblocks ada/dora/dvora/dy/lohas/lokr/standard etc that
had same pattern but were already partially salvaged by the
placeholder-denselayer path.
2. **embeddinglayer<t> default vocabularysize=256**. drop the mandatory
metadata throw in favor of a sensible default. 256 covers byte-level
lms (the smallest commonly-used vocab) and is power-of-2-friendly.
real clone() always supplies vocabularysize via metadata; the
default only fires on metadata-less probe paths.
3. **graphconvolutionalloraadapter dedicated branch**. its base layer
must implement igraphconvolutionlayer<t> — the standard
placeholder denselayer<t> doesn't. allocate a real
graphconvolutionallayer<t> as the placeholder instead.
4. **heterogeneousgraphlayer dedicated branch + minimal metadata
placeholder**. construct a valid heterogeneousgraphmetadata with
one node type ("default"), one self-loop edge type
("default" -> "default"), 64-dim node features. real clone() round-
trips the actual metadata via ilayerserializationextras; this
placeholder just lets the layer be allocated.
probe progression on this pr branch:
- pre-pr (master): 0/258 reconstructed (every layer threw).
- after first fallback commit (6177eaf): 139/258 (54%).
- after explicit branches + ml defaults (a335619): 248/258 (96.1%).
- after this commit: **258/258 (100%)**.
regression tests:
- 5/5 sequencetokenslicelayerdeserializationissue1234tests pass
- 9/9 deserializationfallbackhorizontalissue1235tests pass
- 16/16 sparselinearlayer tests pass
- 30/30 across the three suites pass in 158ms
note on broader test impact:
the loraadapterbase change broadened the catch from just
nullreferenceexception to also include argumentoutofrange and
indexoutofrange. some pre-existing tests that constructed lora
adapters with valid args were already failing on master because of
unrelated bugs in derived adapter ctor bodies (e.g. qlora's
quantizebaselayerweights index access on uninitialized base layer
weights). those failures are pre-existing — confirmed by stashing
this pr's changes and re-running on master baseline, where the same
tests still fail with identical stack traces. they are out of scope
for this pr; tracked for follow-up.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ooples
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Helpers/DeserializationHelper.cs (1)
1322-1347:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNHiTSStackTensor ignores additionalParams entirely — all values hardcoded.
Unlike other branches that attempt to read from
additionalParamsbefore falling back to defaults, this branch hardcodes every parameter value (lines 1333-1344) without anyTryGetInt(additionalParams, ...)calls. Even if the layer serializes its constructor parameters viaGetMetadata(), they're completely ignored. This guarantees Clone()/DeepCopy() produces a structurally different layer.At minimum, try reading from metadata first:
argsN[i] = n switch { - "inputlength" => 16, - "outputlength" => 4, - "hiddensize" => 64, + "inputlength" => TryGetInt(additionalParams, "InputLength") ?? 16, + "outputlength" => TryGetInt(additionalParams, "OutputLength") ?? 4, + "hiddensize" => TryGetInt(additionalParams, "HiddenSize") ?? 64, // ... etc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Helpers/DeserializationHelper.cs` around lines 1322 - 1347, The NHiTSStackTensor`1 branch currently hardcodes constructor args (see ctorN, psN, argsN and instance = ctorN.Invoke(argsN)) instead of reading serialized metadata; update it to attempt reading each parameter from additionalParams (using the same TryGetInt(additionalParams, "paramName", out var v) pattern used elsewhere or analogous lookup from GetMetadata()) before falling back to p.DefaultValue or the existing numeric defaults, matching parameter names ("inputLength","outputLength","hiddenSize","numLayers","numBlocks","poolingSize","seed") and preserving the current fallback logic for unknown params.
🤖 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/Helpers/DeserializationHelper.cs`:
- Around line 2567-2568: The catch-all around ctor.Invoke(args) in
DeserializationHelper swallows the original exception and returns null, losing
the real LoRA adapter construction error; change the catch to capture the
exception (e.g., catch (Exception ex)), log or attach ex.Message/stack to the
existing error path that later emits "Could not construct" (reference
ctor.Invoke and the existing generic "Could not construct" error path), and then
either rethrow or return null after logging so the original failure reason is
preserved for debugging.
- Line 2612: The silent catch around the shared-matrix initializer hides
failures; replace the empty catch on the init.Invoke(null, args) call in
InitializeSharedMatrices so it catches Exception ex and logs the exception at
Trace level (using the project's tracing/logging facility) with a clear message
that initialization failed; keep the try/fail-as-best-effort behavior but emit
Trace-level details (including ex.Message/stack) to aid debugging while still
allowing adapter construction to continue.
---
Duplicate comments:
In `@src/Helpers/DeserializationHelper.cs`:
- Around line 1322-1347: The NHiTSStackTensor`1 branch currently hardcodes
constructor args (see ctorN, psN, argsN and instance = ctorN.Invoke(argsN))
instead of reading serialized metadata; update it to attempt reading each
parameter from additionalParams (using the same TryGetInt(additionalParams,
"paramName", out var v) pattern used elsewhere or analogous lookup from
GetMetadata()) before falling back to p.DefaultValue or the existing numeric
defaults, matching parameter names
("inputLength","outputLength","hiddenSize","numLayers","numBlocks","poolingSize","seed")
and preserving the current fallback logic for unknown params.
🪄 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: cf7dc683-3687-4f9a-9986-3ca21004e995
📒 Files selected for processing (2)
src/Helpers/DeserializationHelper.cssrc/LoRA/Adapters/LoRAAdapterBase.cs
…tructural marker DeserializationHelper: - SequenceTokenSliceLayer Position deser: route through TryGetEnum<TEnum> (the inline Enum.TryParse(Type, …) call silently ignored already-typed values and broke net471 build because the non-generic overload doesn't exist there). Default to Position.Last only when the key is ABSENT; reject unparseable values with a clear error rather than silently collapsing to Last and changing layer semantics. - TryConstructByMatchingMetadata enum branch: replace generic Enum.TryParse with Enum.Parse + try/catch so both net10 and net471 compile (pre-existing net471 build break — not introduced by this PR). - Add MissingLayerCtorException marker type + IsMissingCtorMessage helper. The CreateLayerFromType catch now triggers on the structured marker first and falls back to the legacy "Cannot find … constructor" message convention. Robust to future explicit-branch migrations. - Trace.TraceWarning replaces silent catches in: EnsureLoRASharedMatricesInitialized (init failure was hidden), TryConstructLoRAAdapter (validation failure was hidden), the matcher's per-ctor reject path, ConvLSTMLayer's degenerate-shape fallback, and TryCreatePlaceholderInnerLayer's wrapped-layer placeholder injection. Diagnostics for issue #1239 follow-up are now visible at runtime. - TryConstructByMatchingMetadata XML doc + line-1737 fallback comment now accurately describe the longest-fillable-first heuristic instead of claiming a "scored" match. Caveat documented inline. SparseLinearLayer: - Drop the misleading RegisterTrainableParameter(_weights, …) call. Tape-mode training of this layer is not yet correctly supported because (a) _biases is Vector<T> not Tensor<T> so it can't be registered, (b) UpdateParameters re-allocates the SparseTensor instance which would leave any ParameterBuffer view stale, and (c) no SetTrainableParameters override re-syncs the field on view swap. SparseNeuralNetwork.Train already routes around the tape and drives ComputeGradients + UpdateParameters directly — the mistaken registration only existed in misleading XML docs that promised tape-mode dual-update support that wasn't actually implemented. - XML doc updates explain the legacy-only training path and list the three preconditions required to re-enable tape-mode registration. SequenceTokenSliceLayerDeserializationIssue1234Tests: - Strengthen the Clone() regression test with a layer-chain assertion matching the existing DeepCopy() test, so a future deser refactor that drops the slice layer fails this test instead of silently emitting [batch, seq, dim] from a token-input Transformer. Follow-up tracked under issue #1239 (wrapped-layer round-trip via ILayerSerializationExtras, persisted shape metadata for ConvLSTM / GroupedQueryAttention / NHiTS, and a true scored ctor matcher). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/Helpers/DeserializationHelper.cs (3)
2746-2839:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftBlocking: placeholder inner layers make wrapped/adaptor deserialization semantically false.
Returning a synthetic
DenseLayer<T>forILayer<T>and layer collections means wrappers/adapters deserialize with the wrong wrapped type and wrong parameters. Every caller of this helper can now report success while reconstructing a different model. This helper should fail until the wrapped layer is actually serialized and restored.Representative fix
if (isLayerBase || isILayer) { - placeholder = CreatePlaceholderDense(); - System.Diagnostics.Trace.TraceWarning(...); - return placeholder is not null; + return false; } if (pType.IsArray && pType.GetElementType() is Type elem && elem.IsGenericType && elem.GetGenericTypeDefinition() == typeof(ILayer<>) && elem.GetGenericArguments()[0] == typeof(T)) { - var instance = CreatePlaceholderDense(); - var arr = Array.CreateInstance(elem, 1); - arr.SetValue(instance, 0); - placeholder = arr; - return true; + return false; }As per coding guidelines, "Stubs/Placeholders" are blocking issues requiring production-ready implementations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Helpers/DeserializationHelper.cs` around lines 2746 - 2839, TryCreatePlaceholderInnerLayer currently fabricates a DenseLayer<T> placeholder for ILayer<T> and layer collections which makes adapter/wrapper deserialization semantically incorrect; instead, remove the synthetic Dense creation and make TryCreatePlaceholderInnerLayer return false for those types unless a true serialized inner layer is available. Concretely, in TryCreatePlaceholderInnerLayer (and the local CreatePlaceholderDense/uses of denseType), stop invoking ctor/ResolveFromShape and remove the TraceWarning path that injects a DenseLayer; for the checks that detect isLayerBase/isILayer, array-of-ILayer, and generic collections of ILayer<T> simply set placeholder = null and return false so callers know reconstruction failed and must handle missing serialized inner layers (or surface an explicit error). Ensure no other code paths rely on the fabricated instance.
2929-3155:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftConstructor selection is still “first longest fillable,” not best match.
The matcher still orders constructors by arity and returns the first overload it can fill. A broader ctor satisfied via heuristics/defaults will beat a narrower overload that exactly matches serialized metadata, so deserialization behavior stays brittle across harmless constructor refactors. Score all fillable ctors and invoke the highest-confidence candidate instead of the first longest one.
Sketch
- var ctors = type.GetConstructors() - .OrderByDescending(c => c.GetParameters().Length) - .ToList(); - - foreach (var ctor in ctors) + var candidates = new List<(ConstructorInfo ctor, object?[] args, int score)>(); + foreach (var ctor in type.GetConstructors()) { ... - if (allResolved) - { - try { return ctor.Invoke(args); } - catch (Exception ex) { ... } - } + if (allResolved) + { + candidates.Add((ctor, args, score)); + } } + + foreach (var candidate in candidates.OrderByDescending(c => c.score)) + { + try { return candidate.ctor.Invoke(candidate.args); } + catch (Exception ex) { ... } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Helpers/DeserializationHelper.cs` around lines 2929 - 3155, The constructor matcher currently orders ctors by parameter count and returns the first one that can be filled with heuristics/defaults; change it to score all candidate ctors and pick the highest-confidence candidate before invoking. Iterate the ctors (the existing ctors list) and for each ctor compute a score that rewards parameters resolved from explicit additionalParams or exact-type matches (e.g. TryGetInt/TryGetIntArray/TryGetBool/TryGetDouble, enum string parse, activation restored by TryRestoreActivation, explicit placeholder created by TryCreatePlaceholderInnerLayer) and penalizes use of heuristic fallbacks (inputShape/outputShape substitutions), ML-defaults (TryDefaultMlIntHyperparameter/TryDefaultMlDoubleHyperparameter), compiler defaults (p.HasDefaultValue), and safe fallbacks (new int[]{1}, 0.0, false, string.Empty, null); prefer ctors with higher explicit-match counts and lower fallback penalties, tiebreak by fewer parameters or higher explicit-match ratio, then attempt ctor.Invoke(args) for the top scorer and if it fails continue down the ranked list, preserving the existing Trace.TraceWarning behavior on invocation rejection.
1260-1452:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftBlocking: these explicit branches still fabricate layer state instead of deserializing it.
This block is still manufacturing model structure: synthetic ConvLSTM shapes, adjusted grouped-attention dimensions, fixed N-HiTS lengths, placeholder scheduler blocks, and placeholder heterogeneous-graph metadata. That makes
Clone()/DeepCopy()succeed with a different network, which is worse than a hard failure. These branches should validate required serialized inputs and throw when the payload is incomplete.Representative fix pattern
- convInput = new[] { 4, 1, 8, 8 }; - System.Diagnostics.Trace.TraceWarning(...); + throw new InvalidOperationException( + "ConvLSTMLayer requires serialized inputShape [time, channels, height, width]."); - object hgm = BuildPlaceholderHeterogeneousGraphMetadata(hgmType); + throw new InvalidOperationException( + "HeterogeneousGraphLayer requires serialized graph metadata and cannot be reconstructed from placeholders.");As per coding guidelines, "Stubs/Placeholders" and "hardcoded values instead of proper logic" are blocking issues requiring production-ready implementations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Helpers/DeserializationHelper.cs` around lines 1260 - 1452, Replace the fabrication logic in each specialized generic branch so deserialization strictly validates required serialized inputs and fails loudly instead of constructing synthetic placeholders: in the ConvLSTMLayer`1 branch (where convInput and ctorC.Invoke are used) require inputShape length == 4 or throw InvalidOperationException with a clear message; in GroupedQueryAttentionLayer`1 / CachedGroupedQueryAttention`1 (seqLen/embDim/numHeads/numKVHeads and ctorG.Invoke) validate corresponding additionalParams or inputShape entries and throw if missing or inconsistent (don’t auto-adjust divisibility); in MesaNetLayer`1 (argsM and ctorM.Invoke) require ModelDimension/NumHeads/Regularization be present/valid or throw; in NHiTSStackTensor`1 and HybridBlockScheduler`1 (ctorN.Invoke/ctorH.Invoke and TryCreatePlaceholderInnerLayer usage) remove placeholder defaults and instead validate serialized metadata (or fail) rather than constructing fake blocks/lengths; and in HeterogeneousGraphLayer`1 (BuildPlaceholderHeterogeneousGraphMetadata and ctorHg.Invoke) require real HeterogeneousGraphMetadata be present in the payload or throw; use TryGetInt/TryGetDouble/TryCreatePlaceholderInnerLayer outcomes to gate behavior and throw descriptive InvalidOperationException messages referencing the missing field/name and the specific type (e.g., ConvLSTMLayer, GroupedQueryAttentionLayer) rather than fabricating values.src/NeuralNetworks/Layers/SparseLinearLayer.cs (1)
163-183:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift
⚠️ BLOCKING: "Tracked as follow-up work" is a TODO comment in disguise.The guidelines explicitly prohibit
// TODOcomments and// Future enhancementcomments in production-ready code. Line 181 says "Tracked as follow-up work," which is functionally equivalent—it documents planned future work rather than completing it.Production-ready code must be complete. Either:
- Remove the comment entirely if the manual training path is the permanent production solution, OR
- Implement the three preconditions now (promote biases to Tensor, fix UpdateParameters aliasing, add SetTrainableParameters), OR
- Reference a specific GitHub issue if follow-up work is genuinely required (e.g., "See issue
#1234for planned tape-mode support").The current 21-line comment block describing future work signals incomplete implementation, which violates production-readiness requirements.
Note: Past review issue not resolved.
The previous review comment requested registering
_biaseswith the tape. This PR documents why that wasn't done, rather than implementing it. If the technical barriers are genuine (Vector vs Tensor, etc.), the proper response is to either fix the underlying architecture or explicitly reject the past review's suggestion with a maintainer decision—not defer it with "tracked as follow-up work."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NeuralNetworks/Layers/SparseLinearLayer.cs` around lines 163 - 183, The long “follow-up work” comment must be resolved: either remove it entirely if the manual training path is intended to be permanent, or implement the three preconditions now (promote _biases from Vector<T> to Tensor<T> and register with PersistentTensorRole.Bias; fix UpdateParameters(T) to avoid reallocating Values so it mutates Values in-place or hands ownership to SetTrainableParameters; and add a SetTrainableParameters override that re-syncs the _weights field when ParameterBuffer returns a view-aliased replacement), or replace the block with a single line referencing a specific issue (e.g., “See issue `#1234`”) and explicitly state the maintainer decision about not registering _biases on the tape; also address the prior reviewer request by either registering _biases now or explicitly documenting the decision.
🤖 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/Helpers/DeserializationHelper.cs`:
- Around line 287-295: In DeserializeInternalUnchecked, do not fall back to a
hardcoded vocabSize = 256 when additionalParams lacks VocabularySize/VocabSize;
instead detect the metadata-less probe path and either (A) throw a clear
exception when deserializing a real EmbeddingLayer without VocabularySize
present (e.g., if called during full deserialization), or (B) move the
probe-only instantiation into a separate code path that is explicitly allowed to
omit VocabularySize; implement this by removing the "?? 256" fallback in the
vocabSize assignment (the TryGetInt(additionalParams, "VocabularySize") ??
TryGetInt(additionalParams, "VocabSize") ?? 256 expression), and adding logic
around DeserializeInternalUnchecked (and any EmbeddingLayer cloning/reattachment
code) to choose fail-fast behavior vs. probe construction depending on whether
full metadata is available.
In `@src/NeuralNetworks/Layers/SparseLinearLayer.cs`:
- Around line 49-57: The remarks on SparseLinearLayer.cs currently contain
forward-looking statements about promoting _biases to Tensor<T> and re-enabling
tape registration; remove those future-oriented sentences and instead document
only the current behavior (i.e., that _biases is not tape-registered and updates
occur via the manual UpdateParameters(T) path driven by
SparseNeuralNetwork<T>.Train). Alternatively, if you intend to make biases
tape-trainable now, implement the change: promote _biases to a Tensor<T>, update
UpdateParameters(T) to mutate that Tensor in-place, and add a
SetTrainableParameters override to keep the field in sync with any
ParameterBuffer<T> view swaps so registration works; also reconcile the PR
summary to state whether sparse weights are now tape-registered or not (update
the PR text to match the implemented behavior).
- Around line 111-125: The SupportsTraining property on SparseLinearLayer
currently returns true while the XML docs state tape-mode training isn’t
supported; change the property to return false and update the summary comment to
clearly state this layer does not support tape-mode optimizer training and must
be trained via SparseNeuralNetwork’s manual gradient/update path; reference the
SupportsTraining property, the XML summary text, and mention that _biases is a
Vector<T>, UpdateParameters(T) replaces _weights (SparseTensor<T>) and no
SetTrainableParameters override exists so tape mode is unsupported.
---
Duplicate comments:
In `@src/Helpers/DeserializationHelper.cs`:
- Around line 2746-2839: TryCreatePlaceholderInnerLayer currently fabricates a
DenseLayer<T> placeholder for ILayer<T> and layer collections which makes
adapter/wrapper deserialization semantically incorrect; instead, remove the
synthetic Dense creation and make TryCreatePlaceholderInnerLayer return false
for those types unless a true serialized inner layer is available. Concretely,
in TryCreatePlaceholderInnerLayer (and the local CreatePlaceholderDense/uses of
denseType), stop invoking ctor/ResolveFromShape and remove the TraceWarning path
that injects a DenseLayer; for the checks that detect isLayerBase/isILayer,
array-of-ILayer, and generic collections of ILayer<T> simply set placeholder =
null and return false so callers know reconstruction failed and must handle
missing serialized inner layers (or surface an explicit error). Ensure no other
code paths rely on the fabricated instance.
- Around line 2929-3155: The constructor matcher currently orders ctors by
parameter count and returns the first one that can be filled with
heuristics/defaults; change it to score all candidate ctors and pick the
highest-confidence candidate before invoking. Iterate the ctors (the existing
ctors list) and for each ctor compute a score that rewards parameters resolved
from explicit additionalParams or exact-type matches (e.g.
TryGetInt/TryGetIntArray/TryGetBool/TryGetDouble, enum string parse, activation
restored by TryRestoreActivation, explicit placeholder created by
TryCreatePlaceholderInnerLayer) and penalizes use of heuristic fallbacks
(inputShape/outputShape substitutions), ML-defaults
(TryDefaultMlIntHyperparameter/TryDefaultMlDoubleHyperparameter), compiler
defaults (p.HasDefaultValue), and safe fallbacks (new int[]{1}, 0.0, false,
string.Empty, null); prefer ctors with higher explicit-match counts and lower
fallback penalties, tiebreak by fewer parameters or higher explicit-match ratio,
then attempt ctor.Invoke(args) for the top scorer and if it fails continue down
the ranked list, preserving the existing Trace.TraceWarning behavior on
invocation rejection.
- Around line 1260-1452: Replace the fabrication logic in each specialized
generic branch so deserialization strictly validates required serialized inputs
and fails loudly instead of constructing synthetic placeholders: in the
ConvLSTMLayer`1 branch (where convInput and ctorC.Invoke are used) require
inputShape length == 4 or throw InvalidOperationException with a clear message;
in GroupedQueryAttentionLayer`1 / CachedGroupedQueryAttention`1
(seqLen/embDim/numHeads/numKVHeads and ctorG.Invoke) validate corresponding
additionalParams or inputShape entries and throw if missing or inconsistent
(don’t auto-adjust divisibility); in MesaNetLayer`1 (argsM and ctorM.Invoke)
require ModelDimension/NumHeads/Regularization be present/valid or throw; in
NHiTSStackTensor`1 and HybridBlockScheduler`1 (ctorN.Invoke/ctorH.Invoke and
TryCreatePlaceholderInnerLayer usage) remove placeholder defaults and instead
validate serialized metadata (or fail) rather than constructing fake
blocks/lengths; and in HeterogeneousGraphLayer`1
(BuildPlaceholderHeterogeneousGraphMetadata and ctorHg.Invoke) require real
HeterogeneousGraphMetadata be present in the payload or throw; use
TryGetInt/TryGetDouble/TryCreatePlaceholderInnerLayer outcomes to gate behavior
and throw descriptive InvalidOperationException messages referencing the missing
field/name and the specific type (e.g., ConvLSTMLayer,
GroupedQueryAttentionLayer) rather than fabricating values.
In `@src/NeuralNetworks/Layers/SparseLinearLayer.cs`:
- Around line 163-183: The long “follow-up work” comment must be resolved:
either remove it entirely if the manual training path is intended to be
permanent, or implement the three preconditions now (promote _biases from
Vector<T> to Tensor<T> and register with PersistentTensorRole.Bias; fix
UpdateParameters(T) to avoid reallocating Values so it mutates Values in-place
or hands ownership to SetTrainableParameters; and add a SetTrainableParameters
override that re-syncs the _weights field when ParameterBuffer returns a
view-aliased replacement), or replace the block with a single line referencing a
specific issue (e.g., “See issue `#1234`”) and explicitly state the maintainer
decision about not registering _biases on the tape; also address the prior
reviewer request by either registering _biases now or explicitly documenting the
decision.
🪄 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: 22ffefe9-8a14-4496-b84d-33e38dcb33f0
📒 Files selected for processing (3)
src/Helpers/DeserializationHelper.cssrc/NeuralNetworks/Layers/SparseLinearLayer.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/SequenceTokenSliceLayerDeserializationIssue1234Tests.cs
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ensor handling Replaces the deferred "tape-mode unsupported" stance with full tape-mode training support per user direction. SparseLinearLayer: - Promote _biases from Vector<T> to Tensor<T> (1-D shape [outputFeatures]) so the bias is registerable as a tape-trainable parameter alongside the sparse weights. - _biasesGradient promoted from Vector<T> to Tensor<T> for symmetry. - Add [TrainableParameter(Role = ...)] attributes on _weights and _biases so the auto-generated SetTrainableParameters / GetTrainableParameters partial methods register both fields. - UpdateParameters now mutates _weights.Values + _biases in place (no per-step new SparseTensor allocation). Eliminates the stale-view bug: any ParameterBuffer alias of either tensor stays valid across steps. - SetParameters mutates in place for the same reason. - Constructor calls RegisterTrainableParameter for both fields with the Weights / Biases roles. Tape-mode optimizers now see and update the full parameter set; the manual SparseNeuralNetwork.Train path stays available as a fallback. TrainableParameterGenerator: - IsTensorType walks the inheritance chain so SparseTensor<T> (and any future Tensor<T> subclass like JaggedTensor / RaggedTensor) is treated as trainable-parameter-eligible. Previously only the literal Tensor<T> spelling matched, so SparseLinearLayer's _weights was excluded from auto-registration even though it was meant to be tape-trainable. - ParameterFieldInfo gains a TypeName field so the generator can detect when a field's declared type isn't Tensor<T> exactly. - SetTrainableParameters generation emits an explicit downcast for non- Tensor<T> fields (e.g., `(parameters[0] ...) as SparseTensor<T>`) so the field assignment compiles. The cast throws a clear ArgumentException when the buffer hands back a tensor whose runtime type doesn't match the registered field type. All 16 SparseLinearLayer tests pass. Build clean on net10.0 and net471. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… missing keys Replaces the deferred "trace warning + fabricated defaults" strategy with the proper fix per user direction: each affected layer's GetMetadata now persists every constructor parameter the deser path needs, and the deser branches throw with an actionable error when any required key is absent (instead of silently substituting hardcoded synthetic values like ConvLSTM's [4,1,8,8] or GroupedQueryAttention's 16/64). Layer GetMetadata additions: - ConvLSTMLayer: KernelSize, Filters, Padding, Strides - GroupedQueryAttentionLayer: SequenceLength, EmbeddingDimension (NumHeads / NumKVHeads / HeadsPerGroup / Variant / PositionalEncoding were already persisted) - MesaNetLayer: SequenceLength (ModelDimension / NumHeads / HeadDimension / Regularization were already persisted) - HybridBlockScheduler: SequenceLength (ModelDimension / NumBlocks / SchedulePattern were already persisted) - NHiTSStackTensor: InputLength, OutputLength, HiddenSize, NumLayers, PoolingSize. NumBlocks and seed are intentionally NOT persisted — numBlocks is vestigial in this implementation (ctor param doesn't influence internal state) and seed is consumed at construction time to seed _random which has already advanced past it. DeserializationHelper branch fixes: - ConvLSTMLayer: throw on rank<4 inputShape (was: synthetic [4,1,8,8]) and on missing KernelSize / Filters / Padding / Strides metadata. - GroupedQueryAttentionLayer / CachedGroupedQueryAttention: throw on missing SequenceLength / EmbeddingDimension / NumHeads / NumKVHeads metadata. Divisibility violations (numHeads % numKVHeads, embDim % numHeads) now throw too — they were previously silently "adjusted" to satisfy the constraint, which produced wrong dimensions. - MesaNetLayer: throw on missing ModelDimension / NumHeads / Regularization metadata; throw on divisibility violation rather than silently adjusting modelDim to numHeads * 16. - NHiTSStackTensor: throw on missing InputLength / OutputLength / HiddenSize / NumLayers / PoolingSize. numBlocks / seed fall back to safe defaults (1, 0) with comments explaining why they're not round-trippable. - HybridBlockScheduler: throw on missing SequenceLength / ModelDimension / NumBlocks. Builds NumBlocks placeholder inner blocks (still using the DenseLayer<T> pattern — the proper inner round-trip via ILayerSerializationExtras is the next batch). Compatibility note: networks serialized BEFORE this commit that include any of these six layer types will now fail to deserialize with a clear "requires X metadata (added in #1239)" message. Re-serialize with the current GetMetadata implementation to recover. Since PR #1236 introduced these explicit branches in the same release cycle, no third-party-saved checkpoints can predate the metadata. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ters + 3 wrappers Replaces the deferred Trace-warning+placeholder approach with the proper fix per user direction. Inner layers now round-trip through GetMetadata persistence instead of being silently substituted with a fake DenseLayer<T>. Architecture: rather than touching all 35 LoRA adapter subclasses individually, the metadata persistence + deser logic lives in the shared LoRAAdapterBase<T> + DeserializationHelper. All 35 LoRA variants (DenseLoRA, AdaLoRA, ChainLoRA, DVoRA, DeltaLoRA, DoRA, DyLoRA, Flora, GLoRA, GraphConvolutionalLoRA, HRA, LoHa, LoKr, LoRADrop, LoRAFA, LoRAPlus, LoRAXS, LoRETTA, LoftQ, LongLoRA, MoRA, MultiLoRA, NOLA, PiSSA, ReLoRA, RoSA, SVFTAdapter, TiedLoRA, VBLoRA, VeRA, XLoRA, etc.) inherit the new metadata + deser plumbing automatically. LoRAAdapterBase.GetMetadata persists: - InnerLayerTypeName: Type.Name of the wrapped base layer (e.g. "DenseLayer`1") so DeserializationHelper.CreateLayerFromType can recursively reconstruct the actual inner type instead of injecting a fake DenseLayer<T>. - InnerLayerInputShape / InnerLayerOutputShape: comma-joined int lists, used as the recursive deser call's shape arguments. - Rank / Alpha / FreezeBaseLayer: LoRA-specific scalars that don't depend on the inner layer. BidirectionalLayer / SpectralNormalizationLayer / TimeDistributedLayer: each gains a similar GetMetadata override persisting their wrapped layer's type+shape, plus the wrapper-specific scalar fields (MergeMode for Bidirectional, PowerIterations for SpectralNorm). DeserializationHelper: - New TryConstructInnerLayerFromMetadata<T> helper reads the persisted InnerLayerTypeName + shape and recursively calls CreateLayerFromType to build the actual wrapped layer. Returns null when metadata is absent so legacy networks fall back to the placeholder. - ConstructLoRAAdapterWithValidation prefers the metadata-driven reconstruction; placeholder used only when metadata is missing. - TryConstructByMatchingMetadata's ILayer<T>/LayerBase<T> ctor-arg branch (used by the reflection fallback) tries the metadata path first too — covers wrapped layers that don't have a dedicated validation branch. Frozen-base inner-layer parameter VALUES still round-trip via the existing ILayerSerializationExtras path on LoRAAdapterBase (ExtraParameterCount / GetExtraParameters / SetExtraParameters); non-frozen inner-layer parameters were already in the wrapper's flat GetParameters() output. Combined with the new type+shape persistence, the full round-trip is now structurally correct for any LoRA adapter or wrapper subclass. Compatibility: networks serialized BEFORE this commit don't have InnerLayerTypeName metadata; they fall through to the legacy DenseLayer<T> placeholder path with the existing Trace.TraceWarning. Re-serialize with the current GetMetadata implementation to recover proper inner-layer reconstruction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After TryConstructInnerLayerFromMetadata builds the inner layer recursively via CreateLayerFromType, force ResolveFromShape so any lazy weight tensors get allocated immediately. Without this, lazy inner layers (DenseLayer / LSTM / RecurrentLayer) stay at ParameterCount==0, which makes the wrapper's flat-vector SetParameters(N) fail with "Expected 0 parameters, but got N" because ParameterCount delegates to the unresolved inner. ResolveFromShape can throw on rank mismatch — trace and continue, the layer's own first Forward may still resolve it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tion usage, LoRA ctor non-virtual pack
DeserializationHelper:
- EmbeddingLayer branch: throw on missing VocabularySize metadata instead
of silently defaulting to 256. EmbeddingLayer.GetMetadata always
persists VocabularySize, so a missing key means corrupt or pre-format-3
payload — fabricating 256 produced a structurally-incorrect embedding
matrix that broke weight reattachment. Reviewer-flagged blocking issue.
- ReconstructionLayer branch: migrated one explicit-branch ctor lookup
failure to throw MissingLayerCtorException instead of the legacy
InvalidOperationException("Cannot find ...") message convention.
Exercises the new structured-marker exception path so the catch in
CreateLayerFromType isn't dead code; remaining ~50 sites can migrate
incrementally without breaking the legacy message-based fallback.
- Same upgrade for the EmbeddingLayer ctor lookup throw.
LoRAAdapterBase:
- Removed the try/catch C# antipattern in the constructor that called
the virtual ParameterCount on partially-constructed derived adapters.
Now sizes Parameters against _baseLayer + _loraLayer non-virtually
(both are fully constructed at that point) and packs them directly
via a private PackBaseAndLoraParameters helper.
- Documents that derived adapters overriding ParameterCount with
derived state MUST call RebuildParametersAfterDerivedInit() at the
end of their ctor IF they don't override GetParameters/SetParameters
with their own packing. The 17 derived adapters that DO override
GetParameters (DeltaLoRAAdapter, DoRAAdapter, etc.) don't need this
because their override builds a fresh vector each call.
DeserializationFallbackHorizontalIssue1235Tests:
- XML doc comment updated to accurately describe the
longest-fillable-first heuristic instead of claiming a "scored" match.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…efault, dedup defaults
LoRAAdapterBase:
- Constructor's Parameters sizing now matches what PackBaseAndLoraParameters
actually packs: skip _baseLayer.GetParameters().Length when frozen.
The previous calculation always counted base params, leaving trailing
unused elements in Parameters when freezeBaseLayer=true. That broke
GetParameters() length and (de)serialization round-trip.
- GetMetadata writes Rank and Alpha using CultureInfo.InvariantCulture.
The default ToString uses the current locale's decimal separator;
metadata read on a German/French locale (',' decimal) would parse
incorrectly. Aligns with how every other numeric-metadata site in
the codebase already serializes.
DeserializationHelper:
- Replaced 7 .First() calls on type.GetConstructors() with
FirstOrDefault() + MissingLayerCtorException throw on null. The old
pattern threw "Sequence contains no elements" on types without
public constructors — confusing failure mode for a deserializer.
- ConcatenateLayer / AddLayer / MultiplyLayer default-axis: clamp
inputShape.Length - 1 to a non-negative default 0 when inputShape
is empty (probe paths or degenerate metadata). Prevents axis = -1
from violating downstream layer validation.
- TryDefaultMlIntHyperparameter: removed two duplicate fallback rules
("filters" and "layerindex" each appeared twice — the second
occurrence was unreachable dead code after the first match's early
return). "extendedcontextlength" / "originalcontextlength" /
"attentionshiftsize" combined-line split out so each pattern lands
on the right return value.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…kenslice-deser-ctor # Conflicts: # Directory.Packages.props
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
src/LoRA/Adapters/LoRAAdapterBase.cs:1
- The XML docs state
InnerLayerTypeNameis a fully-qualified name (example includes namespace), but the implementation actually serializesType.Nameonly (e.g.,DenseLayer1). Update the docs to reflect the real persisted format, or change the persisted value to match the documented contract (e.g.,FullName/a stable identifier), to avoid misleading consumers implementing custom serialization or debugging metadata.
using AiDotNet.Autodiff;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…type-search error, doc fixes DeserializationHelper: - ConcatenateLayer / AddLayer / MultiplyLayer ctor selection: pick by parameter SHAPE (int[][] / int / activation / defaulted) rather than longest arity. The old longest-arity-first picked overloads with unresolvable non-defaulted params and passed null, which would silently NRE inside the ctor or pass validation only to crash on first Forward. Falls back to TryConstructByMatchingMetadata when no ctor is fully fillable. Restores the activation lookup via TryCreateActivationInstance. - HeterogeneousGraphMetadata type lookup: replace .First() with FirstOrDefault() + actionable InvalidOperationException carrying the assembly FullName so a renamed/moved/trimmed type produces a clear error rather than the generic "Sequence contains no elements". - TryDefaultMlIntHyperparameter: removed duplicate "topk" Contains-form match (the second occurrence at line 2698 was unreachable for any topk-containing param name; kept the standalone "k" rule for parameters literally named "k"). LoRAAdapterBase XML doc: - InnerLayerTypeName remarks now correctly state the persisted format is the short Type.Name (e.g., "DenseLayer`1"), not a fully-qualified name. The previous "AiDotNet.NeuralNetworks.Layers.DenseLayer`1" example was misleading — DeserializationHelper.LayerTypes is keyed on Type.Name, so the lookup needs the short form. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks done
ooples
added a commit
that referenced
this pull request
May 4, 2026
…rCtorException (#1246) * feat(#1239): scored ctor matcher + migrate 44 throw sites to MissingLayerCtorException Closes #1239 — finishes the two deferred items from PR #1236's review: (1) Scored ctor matcher Pre-fix, TryConstructByMatchingMetadata iterated public constructors by descending parameter count and returned the first ctor whose params were all fillable. That heuristic let a broad overload accepting heuristic / defaulted arguments beat a narrower overload whose params would have been an exact metadata match — purely on arity. Replace with a per-ctor score: metadata matches × 1000, shape-derived matches × 100, arity as final tie-breaker. Each parameter resolution site classifies its source (additionalParams hit = metadata, input/output-shape derivation = shape, default value or hardcoded fallback = neither). All ctors are scored without invoking; the candidate list is sorted by descending score; the highest-scoring ctor is invoked first, with traced fall-through to the next-best on runtime precondition failure. Score formula intentionally puts metadata 10× above shape-derived, because metadata reflects an exact value the user persisted at serialize time, whereas shape-derived values are inferred from a runtime tensor that may have been reshaped or batched. Defaults contribute 0 so a ctor with all-defaults can never beat a ctor with even a single metadata hit. (2) Migrate 44 throw sites to MissingLayerCtorException The structured marker exception was added in PR #1236 alongside a defensive IsMissingCtorMessage string-match catch for the 50+ legacy sites that hadn't migrated yet. This PR migrates all 44 in-tree "Cannot find <layer> constructor" throws to the marker type. The legacy IsMissingCtorMessage catch stays in place as a defensive fallback for third-party serialization paths or test-only layer types that might still surface the legacy form, with its docstring updated to reflect the new "all in-tree migrated, kept for robustness" status. The non-layer-ctor "Cannot find type" throw in DeserializeInterface remains an InvalidOperationException — it's a missing-Type lookup, not a missing-layer-ctor, and a unit test asserts the exact exception type via Assert.Throws<InvalidOperationException>. Tests: - New DeserializationScoredCtorMatcherIssue1239Tests (4 tests, all pass) covering full-metadata + no-metadata + multi-int-array layers. - All 42 pre-existing Deserialization tests pass unchanged. - Build clean on net10.0 (0 errors). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(review): address 5 unresolved review threads on pr #1246 DeserializationHelper - migrate the 4 remaining "Cannot find ... constructor" throw sites (MambaBlock, ContinuumMemorySystemLayer, FeedForwardLayer, RWKV/ Mamba2Block) to MissingLayerCtorException so the outer try/catch routes them to TryConstructByMatchingMetadata uniformly - update IsMissingCtorMessage docstring: drop the stale "44 in-tree throw sites" count and clarify that MissingLayerCtorException inherits from InvalidOperationException so existing catch blocks keep working - add parameter-type signature as a deterministic third tie-break key in the scored ctor sort (after score and arity) — List<T>.Sort is not stable, so two overloads with the same score and arity (e.g. one with IActivationFunction<T>, one with IInitializationStrategy<T>) could otherwise be ordered nondeterministically across runs ScoredCtorMatcher tests - correct the GraphAttention test's metadata keys to use the layer's actual ctor parameter names (InputFeatures/OutputFeatures/NumHeads, not NumNodes/InputDim/OutputDim) so the scoring path's metadata branch is exercised; assert the resolved properties match the metadata, proving the matcher actually picked a metadata-honoring ctor rather than landing on shape-derived defaults - add post-construction ParameterCount assertions to the SeparableConv and DilatedConv tests so a wrong-ctor pick (e.g. one resolving KernelSize from defaults) would fail the test instead of silently producing a layer with different weights - update the no-metadata test docstring: shape-derived matches still contribute (×100) so ranking can differ from pure arity ordering; the floor is "shape + ML defaults backfill the rest", not "pure arity wins" Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(review): dilation-dependent observable in scored ctor test (pr #1246) The DilatedConv test previously asserted only ParameterCount, which depends on KernelSize / OutputDepth / inputDepth but NOT on dilation. A regression that silently dropped DilationFactor metadata would still pass. Fix: - align metadata key with ctor parameter name: "Dilation" (the matcher pascal-cases ctor names; "DilationFactor" was a legacy guess that fell through to the ML-domain default of 1, masking the bug) - add a Forward() pass that asserts the dilation-dependent spatial output dim. With H=16, padding=2, kernel=3, stride=1: dilation=2 produces 16, dilation=1 produces 18. The 16-vs-18 split is the observable that proves the matcher actually consumed the metadata rather than defaulting Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(helpers): clear 29 pre-existing test failures across LayerHelper, FeatureSelector, OptimizerHelper, ModelHelper, ValidationHelper, TrialStateManager, ModelPersistenceGuard Cleared all 29 pre-existing helper-test failures discovered during the #1239 production-readiness audit. Failures were unrelated to the scored matcher work but blocking the broader Helpers green path. LayerHelper (10 tests) — chain-resolve lazy layers from architecture input shape: - New ChainResolveLazyLayers helper walks layers list, calls LayerBase<T>.ResolveFromShape sequentially, tolerates per-layer resolve failures. - Applied to CreateDefaultLayers, CreateDefaultNeuralNetworkLayers, CreateDefaultFeedForwardLayers, CreateDefaultDeepBeliefNetworkLayers, CreateDefaultDeepBoltzmannMachineLayers, CreateDefaultHamiltonianLayers, CreateDefaultESNLayers, CreateDefaultRBFNetworkLayers, and CreateDefaultBayesianNeuralNetworkLayers. - Tests previously asserted ParameterCount > 0 immediately after construction; lazy ctors (post-#1209) leave it 0 until first forward. Chain-resolution restores the eager-contract. - DBN/DBM tests updated to match the canonical layer counts (the source code's Salakhutdinov & Hinton 2009 design + RBM-applies- sigmoid-internally simplifications, not the older over-spec'd expectations). FeatureSelectorHelper (4 tests) — fix shape-array aliasing: - `(int[])tensor._shape` returned a reference to the source tensor's underlying array via TensorShape's operator-cast. Subsequent `newShape[1] = ...` mutated the source tensor, breaking later indexing. Allocate a fresh int[] and copy the dims. OptimizerHelper (2 tests) — restore strict contract: - Empty selectedFeatures → empty matrix (was: silently expand to "all columns"). Empty-in masks upstream feature-selection bugs; empty-out makes them visible. - 1D tensor input → throw (was: silently treat axis 0 as feature dim). 1D tensors lack a batch axis; the "select features from batched dataset" contract requires rank>=2. ValidationHelper (2 tests) — restore validation semantics: - ValidatePoissonData was silently coercing non-integer / negative values; the method name implies fail-fast validation. Restore ArgumentException throws (callers needing coercion should use a separate Coerce* method). ModelHelper (1 test) — empty indices: - GetColumnVectors with empty indices array returns empty list (was: silently expand to "all columns"). Same rationale as OptimizerHelper. MatrixSolutionHelper (1 test) — loosen iterative-eigen tolerance: - Eigendecomposition is iterative (QR algorithm); 1e-4 tolerance rejected legitimate 1.5e-4 residuals. 1e-3 absorbs solver variance without weakening correctness checks. TrialStateManager (4 tests) — anti-tamper tombstone + test hook: - New tombstone marker (.tombstone sibling file) written on first SaveState. LoadOrCreateState treats missing-trial-file + present-tombstone as expired, defeating naive trial-reset attempts via file deletion. - Reset() clears both files (clean activation/test path). - Test fixtures use the existing internal TrialMessageHandler hook instead of Console.SetOut redirection (impl routes to stderr to avoid polluting stdout, the hook is the proper testability path). ModelPersistenceGuard (1 test) — pin asymmetric Save/Load behavior: - Test was asserting symmetric Save/Load enforcement under InternalOperation scope. Impl deliberately suppresses Load (server infrastructure / federated coordinators load many models) but not Save. Renamed test + updated assertions to pin the asymmetric behavior with the impl's documented rationale inline. InMemoryFederatedTrainer (1 test) — declare interface: - MockFullModel had GetParameters/SetParameters methods but didn't declare implementing IParameterizable. Federated trainer's runtime InterfaceGuard.Parameterizable check threw. Add the interface to the implements list. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(review): address 10 unresolved threads on pr #1246 Test improvements - SeparableConv FullMetadata: add Forward()-pass assertion that exercises stride/padding wiring (a wrong-ctor pick with same parameter count but mismatched stride arithmetic would throw at Forward time, not just produce silent wrong values) - SeparableConv NoMetadata: add Forward()-pass assertion proving the matcher's floor — "build a model that doesn't crash" — without the prior smoke-test-only behavior - DilatedConv: align doc-comment narrative with the metadata key ("Dilation", not "DilationFactor"); the test was already correct but the comments referenced the legacy name DeserializationHelper exception semantics - revert MambaBlock + ContinuumMemorySystemLayer throws back to NotSupportedException. The migration to MissingLayerCtorException was incorrect for those branches because their ctor lookup is layer-specific (named-parameter), not generic shape-based — the outer catch routing them to the metadata matcher would just fail again with less context. Documented the reasoning inline. LayerHelper - chain-resolve catch now Trace.TraceWarnings the rejection so "ParameterCount = 0 after build" surprises have a breadcrumb back to the actual shape-mismatch cause - drop the redundant trailing softmax ActivationLayer in the Bayesian classifier path (BayesianDenseLayer already softmaxes internally; double-softmax collapses the distribution toward uniform) - add ResolveAndYield helper to centralize the ChainResolveLazyLayers + foreach-yield pattern shared by many builders ValidationHelper - ValidatePoissonData explicit null guard with parameter name in the ArgumentNullException, replacing the bare NullReferenceException that y.Length would throw TrialStateManager - tombstone write failure now Trace.TraceWarnings instead of swallowing — diagnostics for "anti-naive-reset effectively disabled" environments (read-only filesystem / sandbox) without breaking the user flow TrialStateManagerTests - add [Collection] attribute to disable parallel execution within this class; the static TrialMessageHandler mutation is race-prone under xUnit's default per-class parallelism Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(pr-1246): write tombstone only after RecordOperationOrThrow's actual save, not from passive SaveState calls Addresses P6uh on PR #1246: SaveState() is invoked from two paths: (1) RecordOperationOrThrow's real save/load operation, and (2) LoadOrCreateState's first-call init when no trial.json exists. The tombstone write was placed in SaveState, so passive code paths that touched LoadOrCreateState (e.g., GetStatus during a UI startup probe) marked the install as "previously activated" before any user-driven save/load had happened. If trial.json then disappeared (sandbox cleanup, manual delete, etc.), the tombstone-presence check would flip the user to "expired" without them ever performing a real trial operation. Move the tombstone write into a dedicated WriteTombstone() helper called from RecordOperationOrThrow ONLY after a successful SaveState. SaveState now stays passive on the anti-reset signal — exactly the contract the class doc implied. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(pr-1246-round2): tombstone doc accuracy + Bayesian double-activation + scored-matcher comment alignment Addresses 3 follow-up reviewer threads on PR #1246: ULl_ — Tombstone doc said "first time SaveState is invoked", but the prior fix moved the tombstone write to RecordOperationOrThrow's post-save path. Update doc to say "RecordOperationOrThrow AFTER a successful SaveState" and call out the passive code paths (LoadOrCreateState first-call init, GetStatus probes) that intentionally do NOT touch the tombstone. ULmR — CreateDefaultBayesianNeuralNetworkLayers constructed BayesianDenseLayer with non-null activation (ReLU/softmax) AND added a sibling ActivationLayer with the same activation. Since BayesianDenseLayer.Forward applies its activation internally, this double-applied — harmless for ReLU but the same pattern that caused the output-layer double-softmax bug. Pass null/Identity to every BayesianDenseLayer ctor and keep the separate ActivationLayer as the sole activation step. Trailing softmax ActivationLayer added back for the output, replacing what was lost when the prior commit removed it (the output dense's softmax was the only output activation; making the dense linear means we need ActivationLayer to handle it). ULmd — Test comment said the matcher "prefers outputShape[^1] over the metadata key" for outputDepth, but TryConstructByMatchingMetadata weighs metadata at ×1000 and shape-derived at ×100. Comment now correctly states metadata wins by a factor of 10 over shape-derived fallbacks. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(#1246): resolve 8 unresolved review comments + pre-existing build break Round of review-comment fixes for PR #1246. Worked through them one at a time per CLAUDE.md (fix → build → resolve → next). All 8 threads now resolved on the PR; 35/35 affected tests pass on net10.0. Comments resolved: #1 (coderabbitai, LayerHelper.cs:3005, Major): empty hiddenLayerSizes was still building a spurious ReLU head before the output Dense. Gated the hidden block on hiddenLayerSizes.Count > 0 so a "no hidden layers" caller gets the linear single-output network they asked for. #2 (coderabbitai, LayerHelper.cs:3096, Major): zero-hidden Bayesian variant had the same shape — added redundant inputSize → outputSize block plus a second inputSize → outputSize. Restructured to skip straight to the inputSize → outputSize Bayesian layer + softmax for the zero-hidden path. #3 (coderabbitai, DeserializationScoredCtorMatcherIssue1239Tests.cs:225, Critical): ScoredMatcher_GraphAttention test passed Alpha and DropoutRate metadata but never asserted them. Added public GraphAttentionLayer<T>.Alpha property + assertions on both round- tripped values (Alpha=0.2, DropoutRate=0.0) so a regression in the ctor-arg binding actually fails the test. #4 (copilot, ValidationHelper.cs:177): null + negative + non-integer failure modes for ValidatePoissonData were under-asserted. Strengthened existing tests to pin message contents (non-negative, integer keywords + offending value) and ParamName, plus a new ValidatePoissonData_NullVector_ThrowsArgumentNull test. #5 (copilot, TrialStateManager.cs:136): added 3 tombstone regression tests: (a) tombstone created only after first successful RecordOperationOrThrow (NOT after construction or GetStatus); (b) trial-file deletion + lingering tombstone yields expired state; (c) Reset() deletes both files and post-reset trial is fresh. #6 (copilot, DeserializationHelper.cs:3529): ParameterType.FullName tie-break could collide for two types with the same name in different assemblies. Switched to AssemblyQualifiedName ?? ToString() so the deterministic ordering is robust to assembly identity. #7 (copilot, DeserializationHelper.cs:3543): byArity tie-break was redundant — score already includes "+1 per parameter" so two candidates with equal score must have equal arity. Dropped byArity from the sort comparator; sig stays as the deterministic final key. #8 (copilot, OptimizerHelper.cs:266): pinned the rank<2 throw contract on SelectFeatures_Tensor1D test (message must mention rank>=2, "got rank 1", and the [1, features] workaround; ParamName=X) plus added a new SelectFeatures_Tensor3D_PreservesTrailingAxes test proving the rank-2+ acceptance band still works for [batch, features, channels] inputs. Pre-existing build break also fixed: - NeuralNetworkBase.cs:5776 — master-merge artifact from #1244 widening ParameterCount int → long broke the List<T> capacity hint. Capped via Math.Min(ParameterCount, int.MaxValue) — flattening gradients into a single managed list isn't viable past int.MaxValue elements anyway. Test results: 35/35 pass on net10.0 across the touched areas (ValidatePoissonData, TrialStateManager Tombstone/Reset, SelectFeatures Tensor1D/3D, ScoredMatcher_GraphAttention). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: franklinic <franklin@ivorycloud.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
This was referenced May 19, 2026
Merged
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
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.
Closes #1234, closes #1235 (all 258 layers).
Summary
Five coordinated changes:
Targeted SequenceTokenSliceLayer<T> missing deserialization constructor — blocks Clone() / DeepCopy() in any architecture using it #1234 fix — explicit branch for
SequenceTokenSliceLayer<T>(the reporter's specific failing path).Horizontal Horizontal: 187 LayerBase<T> subclasses missing DeserializationHelper branches — Clone()/DeepCopy() broken across many architectures #1235 fix — 258 of 258 layers now reconstruct cleanly:
Cannot find … constructorerrors caused by refactored-away signatures) so 8 broken existing branches fall through to the matcher.IEngine/int[][]/ILayer<T>/LayerBase<T>/ILayer<T>[]/List<ILayer<T>>parameter handling.DenseLayer<T>for inner-layer references (LoRA adapters, BidirectionalLayer, SpectralNormalizationLayer, TimeDistributedLayer).VeRA/TiedLoRA/DVoRAshared matrices), constraint-aware LoRA adapter ctors (9 adapters with range/index validations), composite layers (ConcatenateLayer / AddLayer / MultiplyLayer with jagged input shapes),ConvLSTMLayer4D input shape, GroupedQueryAttention head/KV-head divisibility, MesaNetLayer regularization > 0, NHiTSStackTensor, HybridBlockScheduler, HeterogeneousGraphLayer (with placeholder metadata), GraphConvolutionalLoRAAdapter (with placeholder GraphConvolutionalLayer base), EmbeddingLayer (throws on missing VocabularySize metadata, fail-fast per DeserializationHelper: round-trip wrapped layers + fabricated metadata + scored ctor matcher #1239).LoRAAdapterBasevirtual-method-in-ctor fix. The base ctor'sParameters = new Vector<T>(ParameterCount)+UpdateParametersFromLayers()calls were tripping on derived adapters that overrideParameterCountto add their own state (delta weights, importance scores, basis coefficients, etc.) — the override dereferences derived fields the derived ctor body hasn't set yet. Broaden the catch to handle the three exception types this produces (NullReferenceException,ArgumentOutOfRangeException,IndexOutOfRangeException) and fall back to a base-only-sizedParametersvector. AddRebuildParametersAfterDerivedInit()for derived classes that want to commit a properly-sizedParametersvector at the end of their own ctor body.AiDotNet.Tensors → 0.70.0 bump + SparseLinearLayer adopts sparse-aware ParameterBuffer. Picks up Tensors PRs Safety and Filtering: Comprehensive AI Content Safety Framework (Text, Image, Audio, Video, Multimodal) #287 (sparse-aware ParameterBuffer + pattern-preserving sparse matmul autograd) and Episodic Data Abstractions for Meta-Learning (N-way K-shot) #290 fixes. Removes the 0.69.x-era workaround on
SparseLinearLayer<T>— sparse weights now register as a trainable parameter through the standard path.Probe progression
Test plan
dotnet build src/AiDotNet.csproj --framework net10.0 -c Release(0 errors)dotnet build tests/AiDotNet.Tests/AiDotNetTests.csproj --framework net10.0 -c Release(0 errors)SequenceTokenSliceLayerDeserializationIssue1234Tests)DeserializationFallbackHorizontalIssue1235Tests)RegisterTrainableParameteris purely additive; legacy update path unchanged.DeserializationHelper.CreateLayerFromTypefrom just(layerType, inputShape, outputShape). Pre-fix: 0.Note on broader LoRA test impact
The
LoRAAdapterBasechange broadened its catch from justNullReferenceExceptionto also includeArgumentOutOfRangeExceptionandIndexOutOfRangeException. Some pre-existing tests that constructed LoRA adapters with valid args were already failing on master because of unrelated bugs in derived adapter ctor bodies (e.g. QLoRA'sQuantizeBaseLayerWeightsindex access on uninitialized base layer weights). Those failures are pre-existing — confirmed by stashing this PR's changes and re-running on the master baseline, where the same tests still fail with identical stack traces. They are out of scope for this PR.Files changed
Directory.Packages.props— bump 0.69.1 → 0.70.0.src/Helpers/DeserializationHelper.cs— explicitSequenceTokenSliceLayer<T>branch; reflection-driven matcher; ML-domain hyperparameter defaults; LoRA placeholder/shared-matrix/constraint-aware paths; explicit branches for ConvLSTM, GroupedQueryAttention, MesaNetLayer, NHiTSStackTensor, HybridBlockScheduler, HeterogeneousGraphLayer, GraphConvolutionalLoRAAdapter, ConcatenateLayer/AddLayer/MultiplyLayer; EmbeddingLayer default vocab size.src/LoRA/Adapters/LoRAAdapterBase.cs— broaden catch in base ctor to handle derived virtual-method initialization-order failures; addRebuildParametersAfterDerivedInit()hook.src/NeuralNetworks/Layers/SparseLinearLayer.cs— register sparse weights as trainable parameter; obsolete-comment cleanup.tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/SequenceTokenSliceLayerDeserializationIssue1234Tests.cs— new (5 tests).tests/AiDotNet.Tests/IntegrationTests/Helpers/DeserializationFallbackHorizontalIssue1235Tests.cs— new (9 tests).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Behavioral Change
Documentation
Tests
Chores