feat: Implement Research-Accurate and Standardized Embedding Models - #714
Conversation
|
Important Review skippedToo many files! 6 files out of 156 files are above the max files limit of 150. You can disable this status message by setting the WalkthroughThis PR introduces comprehensive embedding model support across multiple architectures—Word2Vec, FastText, GloVe, and transformer-based variants—alongside ONNX integration for CLIP and local transformers, HTTP-based async embedding APIs, and consistent regex timeout protections throughout the codebase. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In @src/Helpers/LayerHelper.cs:
- Around line 7996-8012: The call to ValidateLayerParameters in
CreateDefaultWord2VecLayers incorrectly validates against
architecture.OutputSize which can be unset; change the validation to use
vocabSize (i.e., ValidateLayerParameters(1, embeddingDimension, vocabSize)) and
optionally add an assertion that if architecture.OutputSize > 0 it must equal
vocabSize to preserve backward compatibility; update any related comments and
keep references to CreateDefaultWord2VecLayers, ValidateLayerParameters,
architecture.OutputSize and vocabSize so reviewers can locate the change.
In @src/NeuralNetworks/ClipNeuralNetwork.cs:
- Around line 371-397: The Dispose(bool disposing) method in ClipNeuralNetwork
has corrupted spacing: remove the spurious blank lines so the method is compact
and follows C# conventions; ensure the logic remains intact by keeping the
null-conditional disposals (_imageSession?.Dispose(); _textSession?.Dispose();),
setting _disposed = true, and calling base.Dispose(disposing) with normal
brace/indentation and no extra empty lines.
- Around line 90-94: SessionOptions and the first InferenceSession can leak if
creating _textSession throws; wrap SessionOptions in a using (or dispose it in
finally) and ensure _imageSession is disposed if constructing _textSession
fails. Specifically, create a SessionOptions instance, instantiate
_imageSession, then attempt to instantiate _textSession in a try block; on
exception dispose/close _imageSession and rethrow, and ensure SessionOptions is
disposed after both sessions are created; only call InitializeLayers() after
both _imageSession and _textSession are successfully constructed.
In @src/NeuralNetworks/FastText.cs:
- Around line 246-256: Implement DeserializeNetworkSpecificData to mirror
SerializeNetworkSpecificData: read _vocabSize, _bucketSize, _embeddingDimension,
and _maxTokens from the BinaryReader in the same order they were written, and
validate values (e.g., non-negative and within expected ranges) to avoid stream
corruption; update any dependent state or allocations that rely on these fields
after reading. Ensure the method in class FastText
(DeserializeNetworkSpecificData) uses the same field names and ordering as
SerializeNetworkSpecificData and throws a descriptive exception if validation
fails.
- Around line 214-226: CreateNewInstance currently passes the existing
_optimizer into the new FastText<T> instance which can leak old model/layer
state; instead instantiate a fresh optimizer (or clone the optimizer state if a
proper Clone/Copy method exists) and pass that new optimizer into the
FastText<T> constructor. Locate CreateNewInstance and replace usage of the field
_optimizer with either a call to a factory/new optimizer constructor (e.g., new
Optimizer(...) or OptimizerFactory.Create(...)) or call _optimizer.Clone() /
_optimizer.Copy() if such a method exists, ensuring the new optimizer has no
bindings to the old model.
In @src/NeuralNetworks/GloVe.cs:
- Around line 247-249: DeserializeNetworkSpecificData is currently empty while
SerializeNetworkSpecificData writes _vocabSize, _embeddingDimension, and
_maxTokens, so deserialization leaves those fields uninitialized; update
DeserializeNetworkSpecificData to read the same values (read in same
order/types) and assign them to the class state (either by converting the
readonly fields to private mutable backing fields, adding a deserialization
constructor that sets the readonly fields, or exposing an internal setter used
only during deserialization), ensuring the read order and types match
SerializeNetworkSpecificData and that any dependent initialization (e.g.,
buffers built from those sizes) is performed after assignment.
In @src/NeuralNetworks/Word2Vec.cs:
- Around line 161-195: Embed() wrongly assumes Layers[0] is an embedding lookup
and that passing token IDs as T via Tensor<T>.FromVector is valid; instead
validate and use the actual embedding layer API: check that Layers[0] implements
a known embedding contract (e.g., ITokenEmbedding or exposes a
GetTokenEmbeddings(IEnumerable<int> tokenIds) / GetEmbeddingsByIds(int[] ids)
method) and call that to obtain a [seqLen, dim] tensor or vector list, or throw
a clear exception if the layer does not support integer-index lookups; do not
convert token IDs to T for indexing into an embedding layer, and ensure the
returned shape matches _embeddingDimension before computing mean and
Normalize().
- Around line 214-227: CreateNewInstance currently passes the existing
_optimizer into the new Word2Vec, which will share internal state/moment buffers
and corrupt training; instead instantiate a fresh optimizer for the new model:
either call a proper cloning method on the optimizer (e.g., _optimizer.Clone()
or similar) if available, or construct a new optimizer instance from the same
hyperparameters or a provided optimizer factory and pass that into the Word2Vec
constructor; ensure no optimizer internals or parameter references are shared
between the original and the returned model.
In @src/RetrievalAugmentedGeneration/Embeddings/CohereEmbeddingModel.cs:
- Around line 49-55: The CohereEmbeddingModel constructor currently skips
setting Authorization when an HttpClient is injected and will always dispose the
client; update it to track ownership and always apply auth if an apiKey is
provided: add a private bool like _ownsHttpClient set to true when you create a
new HttpClient and false when one is injected, apply
_httpClient.DefaultRequestHeaders.Authorization and Accept headers whenever
_apiKey is non-empty (regardless of whether httpClient was passed), and change
the Dispose/DisposeAsync logic to only dispose/_httpClient when _ownsHttpClient
is true so injected clients are not disposed.
In @src/RetrievalAugmentedGeneration/Embeddings/GooglePalmEmbeddingModel.cs:
- Around line 58-63: The constructor currently assigns _httpClient = httpClient
?? new HttpClient() but only sets Authorization when httpClient is null and
later likely disposes the injected client; change the pattern to track ownership
and always apply auth: add a private bool _ownsHttpClient = (httpClient ==
null); after assigning _httpClient set
_httpClient.DefaultRequestHeaders.Authorization = new
AuthenticationHeaderValue("Bearer", _apiKey") so injected clients receive auth
too; update Dispose/DisposeAsync in GooglePalmEmbeddingModel to only
dispose/cleanup _httpClient when _ownsHttpClient is true to avoid disposing
injected instances.
In @src/RetrievalAugmentedGeneration/Embeddings/HuggingFaceEmbeddingModel.cs:
- Around line 87-98: The Dispose(bool) currently always disposes the injected
_httpClient which can break callers; add an ownership flag (e.g., private
readonly bool _ownsHttpClient) set to true when the class constructs its own
HttpClient and false when an HttpClient is passed in, then change Dispose(bool)
to only call _httpClient.Dispose() if (_ownsHttpClient); also update the
constructors/factory methods that create the HttpClient to set _ownsHttpClient =
true and constructors that accept an HttpClient to set _ownsHttpClient = false
so ownership is tracked correctly.
In @src/RetrievalAugmentedGeneration/Embeddings/ONNXSentenceTransformer.cs:
- Around line 141-167: The Dispose override in ONNXSentenceTransformer is
malformed with extra blank lines and extra closing braces causing syntax errors;
fix by collapsing the method into a properly formatted override Dispose(bool
disposing) that checks the _disposed flag, disposes _session (use
_session?.Dispose() to be safe) only when disposing is true, sets _disposed =
true, and then calls base.Dispose(disposing), and remove the stray closing
braces so the class and namespace braces remain balanced (look for the Dispose
method, _disposed field, and _session member to locate the code).
In @src/RetrievalAugmentedGeneration/Embeddings/OpenAIEmbeddingModel.cs:
- Around line 51-54: The EmbedCore method uses a sync-over-async pattern
(Task.Run(() => EmbedAsync(text)).GetAwaiter().GetResult()) which can deadlock;
change the implementation to either (a) expose and call an asynchronous API
(e.g., add EmbedCoreAsync and await EmbedAsync there) or (b) if you must keep a
synchronous wrapper, call
EmbedAsync(text).ConfigureAwait(false).GetAwaiter().GetResult() and propagate
ConfigureAwait(false) through the async chain so the continuation won't capture
the calling synchronization context; update references to EmbedCore and
EmbedAsync accordingly.
- Around line 103-114: The Dispose method always disposes _httpClient even when
an external client was injected; add an ownership flag (e.g., private readonly
bool _ownsHttpClient) set to true when the class creates its own HttpClient and
false when one is passed into the constructor, then update Dispose(bool
disposing) to call _httpClient.Dispose() only if disposing && _ownsHttpClient &&
_httpClient != null; ensure constructors set the flag appropriately (in
constructors that allocate new HttpClient set _ownsHttpClient = true, in
constructors that accept an HttpClient set it to false).
🟡 Minor comments (6)
src/NeuralNetworks/ClipNeuralNetwork.cs-104-108 (1)
104-108: Null check occurs afterinputis used.
TryForwardGpuOptimized(input, ...)is called at line 104 before the null check at line 107. Ifinputis null, this could cause unexpected behavior in the GPU optimization path.Proposed fix
public override AiDotNet.Tensors.LinearAlgebra.Tensor<T> Predict(AiDotNet.Tensors.LinearAlgebra.Tensor<T> input) { + if (input == null) + throw new ArgumentNullException(nameof(input)); + if (TryForwardGpuOptimized(input, out var gpuResult)) return gpuResult; - if (input == null) - throw new ArgumentNullException(nameof(input)); - var imageData = new double[input.Length];src/RetrievalAugmentedGeneration/Embeddings/HuggingFaceEmbeddingModel.cs-41-46 (1)
41-46: Authorization header not set when HttpClient is injected.When a user provides their own
HttpClient, the Bearer authorization header is not configured (line 43 checkshttpClient == null). The user must pre-configure auth on their client, but this isn't documented and could cause silent auth failures.Suggested documentation or code fix
Option 1: Document the expectation:
+ /// <param name="httpClient">Optional pre-configured HttpClient. If provided, it must include the Authorization header.</param>Option 2: Always set auth if apiKey is provided:
- if (httpClient == null && !string.IsNullOrEmpty(_apiKey)) + if (!string.IsNullOrEmpty(_apiKey) && _httpClient.DefaultRequestHeaders.Authorization == null) { _httpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", _apiKey); }src/NeuralNetworks/GloVe.cs-155-166 (1)
155-166: Tokenizer created on everyEmbedcall when not provided.When
_tokenizeris null, a new tokenizer is created on eachEmbed()call viaLanguageModelTokenizerFactory.CreateForBackbone(). For batch processing or repeated calls, this adds unnecessary overhead.🔧 Suggested fix - cache the fallback tokenizer
+ private ITokenizer? _fallbackTokenizer; public Vector<T> Embed(string text) { if (string.IsNullOrWhiteSpace(text)) return new Vector<T>(_embeddingDimension); - var tokenizer = _tokenizer ?? Tokenization.LanguageModelTokenizerFactory.CreateForBackbone(LanguageModelBackbone.OPT); + var tokenizer = _tokenizer ?? (_fallbackTokenizer ??= Tokenization.LanguageModelTokenizerFactory.CreateForBackbone(LanguageModelBackbone.OPT));src/RetrievalAugmentedGeneration/Embeddings/StaticWordEmbeddingModel.cs-103-110 (1)
103-110: Silent parse failures leave zero values in embedding.When
double.TryParsefails,values[i]remains atdefault(T)(likely zero), silently corrupting the vector. Consider logging a warning or skipping malformed lines entirely.🔧 Suggested validation
var values = new T[dimension]; + bool validLine = true; for (int i = 0; i < dimension; i++) { if (double.TryParse(parts[i + 1], out double val)) { values[i] = numOps.FromDouble(val); } + else + { + validLine = false; + break; + } } + if (!validLine) continue; + wordVectors[word] = new Vector<T>(values);src/NeuralNetworks/TransformerEmbeddingNetwork.cs-256-275 (1)
256-275: Minor: deserialization only validates vocab size; validate the rest (or document why mismatch is OK).
Since fields are readonly, “read + validate” is the safest behavior to avoid silently loading incompatible weights.src/NeuralNetworks/TransformerEmbeddingNetwork.cs-178-195 (1)
178-195: Minor:Train()computes loss but doesn’t store it (LastLoss) and the locallossis unused.
This makes training metrics inconsistent vs other models.Proposed fix
var loss = LossFunction.CalculateLoss(outputVec, expectedVec); +LastLoss = loss; var gradVec = LossFunction.CalculateDerivative(outputVec, expectedVec);
🧹 Nitpick comments (11)
src/NeuralNetworks/ClipNeuralNetwork.cs (1)
305-317: Silent dimension mismatch may produce incorrect embeddings.If the ONNX model's output dimension is smaller than
_embeddingDimension, the remaining values default to zero. Consider logging a warning whendim < _embeddingDimensionto help diagnose configuration mismatches.tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/TransformerEmbeddingNetworkTests.cs (3)
45-74: Consider disposing the model after use.
TransformerEmbeddingNetwork<T>likely inherits fromNeuralNetworkBasewhich may hold disposable resources. Wrapping in ausingstatement ensures proper cleanup.Suggested improvement
- var model = new TransformerEmbeddingNetwork<double>( + using var model = new TransformerEmbeddingNetwork<double>( architecture, tokenizer, vocabSize, embeddingDimension: embeddingDim, numLayers: 2, numHeads: 4, feedForwardDim: 128);
76-106: Same disposal recommendation applies to this test.Suggested improvement
- var model = new TransformerEmbeddingNetwork<double>( + using var model = new TransformerEmbeddingNetwork<double>(
108-136: Same disposal recommendation applies to the ClsToken test.Suggested improvement
- var model = new TransformerEmbeddingNetwork<double>( + using var model = new TransformerEmbeddingNetwork<double>(src/RetrievalAugmentedGeneration/Embeddings/HuggingFaceEmbeddingModel.cs (1)
49-52: Sync-over-async pattern can cause deadlocks.Using
Task.Run(() => ...).GetAwaiter().GetResult()can deadlock in contexts with aSynchronizationContext(e.g., UI apps, some ASP.NET scenarios). Consider exposing an async API or documenting this limitation.src/RetrievalAugmentedGeneration/Embeddings/CohereEmbeddingModel.cs (1)
58-61: Sync-over-async pattern - same concern as other embedding models.src/RetrievalAugmentedGeneration/Embeddings/GooglePalmEmbeddingModel.cs (1)
69-72: Sync-over-async pattern - consistent with other models.Consider a codebase-wide approach to either expose async APIs or document the threading limitations.
src/RetrievalAugmentedGeneration/Embeddings/OpenAIEmbeddingModel.cs (1)
45-48: Consider documenting external HttpClient auth requirements.When an external
HttpClientis provided, the authorization header is not set automatically. Callers must configure theAuthorizationheader themselves, which may not be obvious. Consider adding a note in the XML documentation or validating that the injected client has proper headers.src/RetrievalAugmentedGeneration/Embeddings/VoyageAIEmbeddingModel.cs (1)
28-45: UnusedapiKeyparameter may confuse users.The
apiKeyparameter is stored but never used since this model uses local ONNX inference. While the comment on line 34 explains this, consider one of:
- Removing the parameter if API compatibility isn't critical
- Marking with
[Obsolete]attribute to signal deprecation- Using
#pragma warning disableif intentionally unusedsrc/RetrievalAugmentedGeneration/Embeddings/StaticWordEmbeddingModel.cs (1)
140-144: Adding zero vector dilutes the average without semantic contribution.When
_ignoreUnknownis false, adding the zero_unknownVectortosumVectordoesn't change the sum, but incrementingvalidWordsreduces the final average magnitude. This penalizes sentences with unknown words. Consider either not incrementingvalidWordsfor unknown tokens, or using a learned UNK embedding.💡 Alternative approaches
Option 1 - Don't count unknown tokens in average:
else if (!_ignoreUnknown) { sumVector = sumVector.Add(_unknownVector); - validWords++; + // Don't increment validWords - unknown tokens don't contribute semantically }Option 2 - Current behavior may be intentional if you want to penalize unknown-heavy text. If so, add a comment explaining the rationale.
src/RetrievalAugmentedGeneration/Embeddings/LocalTransformerEmbedding.cs (1)
28-35: Missing_disposedguard for consistency.Unlike
OpenAIEmbeddingModelandVoyageAIEmbeddingModel, this class lacks a_disposedflag to prevent double-disposal warnings. While double-disposal is generally safe in .NET, adding the guard maintains consistency across the codebase.♻️ Suggested consistency fix
+ private bool _disposed; + protected override void Dispose(bool disposing) { - if (disposing) + if (!_disposed) { - _onnxTransformer.Dispose(); + if (disposing) + { + _onnxTransformer.Dispose(); + } + _disposed = true; } base.Dispose(disposing); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/Enums/Word2VecType.cssrc/Helpers/LayerHelper.cssrc/NeuralNetworks/Blip2NeuralNetwork.cssrc/NeuralNetworks/ClipNeuralNetwork.cssrc/NeuralNetworks/FastText.cssrc/NeuralNetworks/GloVe.cssrc/NeuralNetworks/TransformerEmbeddingNetwork.cssrc/NeuralNetworks/Word2Vec.cssrc/RetrievalAugmentedGeneration/Embeddings/CohereEmbeddingModel.cssrc/RetrievalAugmentedGeneration/Embeddings/EmbeddingModelBase.cssrc/RetrievalAugmentedGeneration/Embeddings/GooglePalmEmbeddingModel.cssrc/RetrievalAugmentedGeneration/Embeddings/HuggingFaceEmbeddingModel.cssrc/RetrievalAugmentedGeneration/Embeddings/LocalTransformerEmbedding.cssrc/RetrievalAugmentedGeneration/Embeddings/ONNXSentenceTransformer.cssrc/RetrievalAugmentedGeneration/Embeddings/OpenAIEmbeddingModel.cssrc/RetrievalAugmentedGeneration/Embeddings/StaticWordEmbeddingModel.cssrc/RetrievalAugmentedGeneration/Embeddings/VoyageAIEmbeddingModel.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/TransformerEmbeddingNetworkTests.cs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/Enums/Word2VecType.cssrc/RetrievalAugmentedGeneration/Embeddings/EmbeddingModelBase.cssrc/RetrievalAugmentedGeneration/Embeddings/CohereEmbeddingModel.cssrc/NeuralNetworks/Word2Vec.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/TransformerEmbeddingNetworkTests.cssrc/RetrievalAugmentedGeneration/Embeddings/OpenAIEmbeddingModel.cssrc/RetrievalAugmentedGeneration/Embeddings/GooglePalmEmbeddingModel.cssrc/RetrievalAugmentedGeneration/Embeddings/HuggingFaceEmbeddingModel.cssrc/NeuralNetworks/GloVe.cssrc/RetrievalAugmentedGeneration/Embeddings/LocalTransformerEmbedding.cssrc/RetrievalAugmentedGeneration/Embeddings/StaticWordEmbeddingModel.cssrc/NeuralNetworks/TransformerEmbeddingNetwork.cssrc/RetrievalAugmentedGeneration/Embeddings/ONNXSentenceTransformer.cssrc/NeuralNetworks/FastText.cssrc/Helpers/LayerHelper.cssrc/NeuralNetworks/ClipNeuralNetwork.cssrc/RetrievalAugmentedGeneration/Embeddings/VoyageAIEmbeddingModel.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/Enums/Word2VecType.cssrc/RetrievalAugmentedGeneration/Embeddings/EmbeddingModelBase.cssrc/RetrievalAugmentedGeneration/Embeddings/CohereEmbeddingModel.cssrc/NeuralNetworks/Word2Vec.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/TransformerEmbeddingNetworkTests.cssrc/RetrievalAugmentedGeneration/Embeddings/OpenAIEmbeddingModel.cssrc/RetrievalAugmentedGeneration/Embeddings/GooglePalmEmbeddingModel.cssrc/RetrievalAugmentedGeneration/Embeddings/HuggingFaceEmbeddingModel.cssrc/NeuralNetworks/GloVe.cssrc/RetrievalAugmentedGeneration/Embeddings/LocalTransformerEmbedding.cssrc/RetrievalAugmentedGeneration/Embeddings/StaticWordEmbeddingModel.cssrc/NeuralNetworks/TransformerEmbeddingNetwork.cssrc/RetrievalAugmentedGeneration/Embeddings/ONNXSentenceTransformer.cssrc/NeuralNetworks/FastText.cssrc/Helpers/LayerHelper.cssrc/NeuralNetworks/ClipNeuralNetwork.cssrc/RetrievalAugmentedGeneration/Embeddings/VoyageAIEmbeddingModel.cs
📚 Learning: 2025-12-19T19:05:13.598Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 445
File: src/Interfaces/IPredictionModelBuilder.cs:7-8
Timestamp: 2025-12-19T19:05:13.598Z
Learning: In ooples/AiDotNet, IAggregationStrategy<TModel> is declared in AiDotNet.Interfaces (src/Interfaces/IAggregationStrategy.cs). For src/Interfaces/IPredictionModelBuilder.cs (also in AiDotNet.Interfaces), referencing IAggregationStrategy does not require an additional using. Avoid flagging missing using directives for this case in future reviews.
Applied to files:
src/RetrievalAugmentedGeneration/Embeddings/CohereEmbeddingModel.cssrc/NeuralNetworks/ClipNeuralNetwork.cssrc/RetrievalAugmentedGeneration/Embeddings/VoyageAIEmbeddingModel.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: CodeQL analysis (csharp)
- GitHub Check: Build (Windows)
- GitHub Check: CodeQL Analysis
🔇 Additional comments (21)
src/NeuralNetworks/ClipNeuralNetwork.cs (3)
165-168: Serialization/deserialization asymmetry.
SerializeNetworkSpecificDatawrites model paths and configuration, butDeserializeNetworkSpecificDatathrowsNotSupportedException. This means serialized data cannot be used to reconstruct the network. Consider either:
- Removing serialization support entirely, or
- Implementing deserialization that accepts the required paths/tokenizer through a factory method.
183-199: LGTM!The attention mask logic correctly handles both padding (positions beyond original tokens get 0) and truncation (all positions get 1 when tokens exceed max length).
259-285: LGTM!The zero-shot classification follows the standard CLIP approach with prompt templates and softmax normalization over cosine similarities.
tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/TransformerEmbeddingNetworkTests.cs (1)
16-43: MockTokenizer implementation looks adequate for test purposes.The mock provides deterministic tokenization suitable for unit testing. Minor observations:
VocabularythrowsNotImplementedExceptionwhich is fine if tests don't access it.- The char-to-id mapping
(text[i] % 1000)ensures IDs stay withinVocabularySize.src/RetrievalAugmentedGeneration/Embeddings/CohereEmbeddingModel.cs (1)
99-103: LGTM on the response model.The private
CohereEmbeddingResponseclass cleanly maps the JSON structure with appropriate defaults.src/Enums/Word2VecType.cs (1)
1-19: LGTM!Clean enum definition with accurate and helpful XML documentation describing CBOW and Skip-Gram architectures.
src/RetrievalAugmentedGeneration/Embeddings/EmbeddingModelBase.cs (1)
175-192: LGTM!Correct implementation of the dispose pattern. The base class properly sets up the infrastructure for derived classes to override
Dispose(bool)for resource cleanup.src/RetrievalAugmentedGeneration/Embeddings/ONNXSentenceTransformer.cs (3)
50-100: Core embedding logic is well-implemented.The tokenization, ONNX inference pipeline, and mean pooling implementation follow standard sentence-transformer patterns. Good handling of optional
token_type_idsbased on model metadata.
102-129: Mean pooling implementation is correct.The pooling correctly skips masked positions (
attentionMask[i] == 0) and handles the edge case wheresumMaskcould be zero.
41-48: Verify ifAutoTokenizerandITokenizerimplementIDisposable.
_tokenizeris initialized viaAutoTokenizer.FromPretrainedbut is never disposed. While ONNX Runtime C# best practices require disposal of resource-intensive objects, you need to verify whether your specific tokenizer implementation implementsIDisposableand, if so, whether_tokenizershould be disposed (likely in a finalizer or classDisposemethod ifONNXSentenceTransformeris alsoIDisposable).src/RetrievalAugmentedGeneration/Embeddings/GooglePalmEmbeddingModel.cs (2)
113-129: LGTM on the Vertex AI response models.The nested classes correctly map the Vertex AI prediction response structure with appropriate JSON property attributes and defaults.
74-111: API integration logic is correct.The endpoint URL construction, request payload structure, and response handling follow Vertex AI conventions properly.
src/RetrievalAugmentedGeneration/Embeddings/VoyageAIEmbeddingModel.cs (1)
59-70: LGTM!The disposal pattern correctly disposes the owned
_onnxTransformerand follows the standardDispose(bool)pattern with proper guard and base class call.src/RetrievalAugmentedGeneration/Embeddings/StaticWordEmbeddingModel.cs (1)
156-161: LGTM!The simple whitespace/punctuation tokenizer is appropriate for static word embeddings where exact word matches are needed.
src/NeuralNetworks/GloVe.cs (1)
91-113: LGTM!The
ForwardandBackwardimplementations correctly propagate through layers sequentially and in reverse order respectively.src/RetrievalAugmentedGeneration/Embeddings/LocalTransformerEmbedding.cs (1)
11-26: LGTM!Clean delegation pattern to
ONNXSentenceTransformer. The class properly delegates embedding dimension, max tokens, and embedding logic to the underlying ONNX transformer.src/Helpers/LayerHelper.cs (2)
8060-8080: I don't see a review comment provided in your message. Please share the review comment that needs to be rewritten (within<review_comment>tags or as quoted text), and I will process it according to the specified format and output the final rewritten comment with the appropriate classification tag.
8084-8135: BLIP-2 factory is missing required cross-attention wiring between vision tokens and Q-Former queries.BLIP-2's Q-Former architecture requires:
- Learnable query vectors that apply cross-attention to frozen vision encoder outputs
- Self-attention over queries interleaved with cross-attention from queries → vision tokens
The current implementation shows:
- No
CrossAttentionLayerbetween vision embeddings and Q-Former- Two
TransformerEncoderLayerper Q-Former iteration (typically self-attention only; BLIP-2 requires self-attention + cross-attention per layer)EmbeddingLayerplaced after the Q-Former loop, treating text input as a separate, unrelated stream rather than integrating it into the Q-Former's cross-modal reasoningRecommendation: Expose a dedicated BLIP-2 builder with explicit cross-attention routing, learnable queries initialization, and structured multi-branch graph (vision encoder → Q-Former cross-attention → projection → LM decoder) rather than a flat
IEnumerable<ILayer<T>>that obscures the model's true dataflow.src/NeuralNetworks/Word2Vec.cs (3)
136-159: Gradient clipping/optimizer wiring needs verification—gradientslist may not be passed to update logic.The code builds a
gradientslist of backpropagated activation gradients (dL/dActivation from the output layer backward), passes it toClipGradients(gradients), then calls_optimizer.UpdateParameters(Layers)without passing the gradient list. If the intended design is for layers to store their own parameter gradients duringBackward(), clarify:
- What does
ClipGradients(...)operate on (layer parameter gradients or activation gradients)?- What does
AdamOptimizer.UpdateParameters(...)read fromLayers?- Is the
gradientslist used anywhere, or should it be removed?If parameter gradients are stored in layers, consider renaming
gradientsfor clarity and ensuring gradient clipping targets the correct gradient tensors.
43-86: Verify:InitializeLayers()initialization order and potential double-initialization risk.The concern is that if
NeuralNetworkBase<T>callsInitializeLayers()in its constructor before derived class fields (_vocabSize,_embeddingDimension, etc.) are assigned, andWord2Vecthen callsInitializeLayers()again after field assignment, this creates a risk of uninitialized field access or duplicated layers.To confirm this risk, verify the implementation of
NeuralNetworkBase<T>and its constructor—specifically whether it invokesInitializeLayers()and when relative to derived field initialization. If confirmed, the proposed solution of making the baseInitializeLayers()a no-op and using a separateInitializeCustomLayers()called after field assignment would resolve the issue.
136-159: Critical: training path can't be correct ifTryForwardGpuOptimized()short-circuits the layer graph.
Train()callsPredict()→Forward()which may return a GPU result without establishing per-layer forward caches, but then you backprop throughLayers. Ensure training uses the same execution path that supports backward (or make GPU path record needed state).If there's already a base helper (e.g.,
ForwardWithMemory) meant for training, consider using that instead (and removing the manual gradient list entirely).
There was a problem hiding this comment.
Pull request overview
This PR implements a major refactoring of embedding models and neural network architectures to align with research-accurate implementations and a standardized NeuralNetworkBase architecture. The changes introduce proper ONNX Runtime integration, IDisposable patterns for resource management, and new embedding model implementations (Word2Vec, GloVe, FastText, TransformerEmbeddingNetwork).
Changes:
- Added IDisposable pattern to embedding models for proper resource management
- Implemented ONNX Runtime integration for production-ready sentence transformers and multimodal models
- Created new neural network implementations for Word2Vec, GloVe, FastText, and TransformerEmbeddingNetwork following standardized architecture
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Word2VecType.cs | New enum defining CBOW and SkipGram architectures |
| EmbeddingModelBase.cs | Added IDisposable interface implementation |
| StaticWordEmbeddingModel.cs | New static word embedding model supporting GloVe/Word2Vec/FastText formats |
| ONNXSentenceTransformer.cs | Refactored to use actual ONNX Runtime instead of placeholder implementation |
| LocalTransformerEmbedding.cs | Simplified to wrapper around ONNXSentenceTransformer |
| VoyageAIEmbeddingModel.cs | Added instance reuse and disposal pattern |
| OpenAIEmbeddingModel.cs | Implemented actual OpenAI API calls with HttpClient |
| HuggingFaceEmbeddingModel.cs | Implemented actual HuggingFace API integration |
| GooglePalmEmbeddingModel.cs | Implemented Vertex AI API integration |
| CohereEmbeddingModel.cs | Implemented actual Cohere API calls |
| Word2Vec.cs | New Word2Vec implementation with NeuralNetworkBase |
| GloVe.cs | New GloVe implementation with dual-matrix structure |
| FastText.cs | New FastText implementation with subword support |
| TransformerEmbeddingNetwork.cs | New transformer-based embedding network |
| ClipNeuralNetwork.cs | Refactored to use ONNX Runtime sessions |
| Blip2NeuralNetwork.cs | Major refactor with simplified architecture |
| LayerHelper.cs | Added factory methods for new embedding models |
| TransformerEmbeddingNetworkTests.cs | Integration tests for transformer embeddings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In @src/NeuralNetworks/SiameseNeuralNetwork.cs:
- Around line 208-217: The SerializeNetworkSpecificData writes _vocabSize,
_embeddingDimension and _maxSequenceLength but DeserializeNetworkSpecificData is
empty and the corresponding fields are readonly, so implement
DeserializeNetworkSpecificData to read the values in the same order
(reader.ReadInt32/appropriate types) and restore them, and make the class able
to accept those restored values by removing readonly from
_vocabSize/_embeddingDimension/_maxSequenceLength or by introducing mutable
backing fields or a deserialization constructor that sets them; ensure
read/write order and types exactly match SerializeNetworkSpecificData and update
any constructors or field declarations (e.g., change readonly declarations) so
deserialization can assign the values.
In @src/NeuralNetworks/TransformerEmbeddingNetwork.cs:
- Around line 260-273: SerializeNetworkSpecificData writes seven network
parameters but DeserializeNetworkSpecificData is empty and the fields
(_vocabSize, _embeddingDimension, _maxSequenceLength, _numLayers, _numHeads,
_feedForwardDim, _poolingStrategy) are readonly so they cannot be restored;
implement DeserializeNetworkSpecificData to read those seven values in the same
order (use reader.ReadInt32 for ints and cast to the pooling enum for
_poolingStrategy) and then either (A) change the design so deserialization
constructs a new TransformerEmbeddingNetwork via a constructor/factory that
accepts those parameters and sets the readonly fields at construction, or (B)
remove readonly from the fields so the DeserializeNetworkSpecificData method can
assign them directly after reading; pick one approach and update
DeserializeNetworkSpecificData accordingly.
- Around line 198-209: The Train method is applying gradients as if they were
parameters and never uses the _optimizer; replace the incorrect
UpdateParameters(GetParameterGradients()) call so the optimizer updates model
parameters (mirror SiameseNeuralNetwork): ensure Backpropagate() computes
per-layer gradients, then call _optimizer.UpdateParameters(Layers) (or if your
optimizer API expects gradients, call
_optimizer.UpdateParameters(GetParameterGradients()) accordingly) and guard for
a null _optimizer. Also remove or rename the direct UpdateParameters call that
treats gradients as parameters so parameter vs gradient contract is respected.
🟠 Major comments (25)
src/NeuralNetworks/MatryoshkaEmbedding.cs-53-71 (1)
53-71: Constructor parameters ignored inInitializeLayers- same issue as BGE.
vocabSize,numLayers,numHeads, andfeedForwardDimparameters are not stored and the method hardcodes default values instead.src/NeuralNetworks/BGE.cs-69-84 (1)
69-84:CreateNewInstancealso hardcodes values, breaking clone fidelity.The cloned instance won't preserve the original configuration if a user specified custom
vocabSize,numLayers,numHeads, orfeedForwardDimvalues.src/NeuralNetworks/SimCSE.cs-56-74 (1)
56-74: Constructor parameters ignored and_dropoutRateunused in layer creation.Same hardcoding issue as other models. Additionally,
_dropoutRateis stored but never passed toCreateDefaultSimCSELayers, so dropout configuration won't affect the actual layers.src/NeuralNetworks/SGPT.cs-45-63 (1)
45-63: Same hardcoding issue - constructor parameters ignored.
vocabSize,numLayers,numHeads, andfeedForwardDimparameters are not stored, andInitializeLayersuses hardcoded defaults instead of the passed values.src/NeuralNetworks/BGE.cs-45-63 (1)
45-63: Constructor parameters are ignored in favor of hardcoded values.The
InitializeLayersmethod hardcodes30522,12,12,3072instead of using the constructor parametersvocabSize,numLayers,numHeads, andfeedForwardDim. This means any custom values passed to the constructor will be ignored.Consider storing these parameters as fields and using them in
InitializeLayers:Proposed fix
public class BGE<T> : TransformerEmbeddingNetwork<T> { + private readonly int _vocabSize; + private readonly int _numLayers; + private readonly int _numHeads; + private readonly int _feedForwardDim; + public BGE( NeuralNetworkArchitecture<T> architecture, ... int vocabSize = 30522, ... int numLayers = 12, int numHeads = 12, int feedForwardDim = 3072, ...) : base(...) { + _vocabSize = vocabSize; + _numLayers = numLayers; + _numHeads = numHeads; + _feedForwardDim = feedForwardDim; } protected override void InitializeLayers() { ... else { Layers.AddRange(LayerHelper<T>.CreateDefaultBGELayers( Architecture, - 30522, + _vocabSize, EmbeddingDimension, MaxTokens, - 12, - 12, - 3072)); + _numLayers, + _numHeads, + _feedForwardDim)); } }src/NeuralNetworks/InstructorEmbedding.cs-78-98 (1)
78-98: Validate/normalizeSetDefaultInstruction()+ null input handling.
SetDefaultInstruction(null)will later throw inEmbedWithInstructionvia string concatenation; also consider rejecting empty/whitespace instructions.Proposed fix
public void SetDefaultInstruction(string instruction) { - _defaultInstruction = instruction; + if (string.IsNullOrWhiteSpace(instruction)) + throw new ArgumentException("Instruction must be non-empty.", nameof(instruction)); + _defaultInstruction = instruction; } @@ public Vector<T> EmbedWithInstruction(string text, string? instruction = null) { + if (text is null) throw new ArgumentNullException(nameof(text)); string fullText = (instruction ?? _defaultInstruction) + text; return base.Embed(fullText); }src/NeuralNetworks/InstructorEmbedding.cs-100-115 (1)
100-115:CreateNewInstance()should preserve configured hyperparameters (and default instruction).
It currently reverts to hard-coded defaults and also drops_defaultInstruction.Proposed fix
protected override IFullModel<T, Tensor<T>, Tensor<T>> CreateNewInstance() { - return new InstructorEmbedding<T>( + var model = new InstructorEmbedding<T>( Architecture, null, null, - 30522, + _vocabSize, EmbeddingDimension, MaxTokens, - 12, - 12, - 3072, - PoolingStrategy.Mean, + _numLayers, + _numHeads, + _feedForwardDim, + _poolingStrategy, LossFunction, Convert.ToDouble(MaxGradNorm)); + model._defaultInstruction = _defaultInstruction; + return model; }src/NeuralNetworks/SPLADE.cs-117-131 (1)
117-131:CreateNewInstance()should preserve ctor-configured hyperparameters.
It currently recreates the model with hard-coded 12/12/3072.Proposed fix
protected override IFullModel<T, Tensor<T>, Tensor<T>> CreateNewInstance() { return new SPLADE<T>( Architecture, null, null, _vocabSize, EmbeddingDimension, MaxTokens, - 12, - 12, - 3072, + _numLayers, + _numHeads, + _feedForwardDim, LossFunction, Convert.ToDouble(MaxGradNorm)); }src/NeuralNetworks/ColBERT.cs-51-69 (1)
51-69: Stop hard-coding vocab/layer hyperparameters inInitializeLayers()(use ctor values).
Non-defaultvocabSize/numLayers/numHeads/feedForwardDimare currently ignored for default layer creation.Proposed fix (store ctor args; reuse)
public class ColBERT<T> : TransformerEmbeddingNetwork<T> { #region Fields private readonly int _outputDim; // Late interaction dimension (typically 128) + private readonly int _vocabSize; + private readonly int _numLayers; + private readonly int _numHeads; + private readonly int _feedForwardDim; #endregion @@ public ColBERT( @@ int vocabSize = 30522, int outputDimension = 128, @@ int numLayers = 12, int numHeads = 12, int feedForwardDim = 3072, @@ { _outputDim = outputDimension; + _vocabSize = vocabSize; + _numLayers = numLayers; + _numHeads = numHeads; + _feedForwardDim = feedForwardDim; } @@ else { Layers.AddRange(LayerHelper<T>.CreateDefaultColBERTLayers( Architecture, - 30522, + _vocabSize, _outputDim, MaxTokens, - 12, - 12, - 3072)); + _numLayers, + _numHeads, + _feedForwardDim)); } }src/NeuralNetworks/SPLADE.cs-79-115 (1)
79-115:Embed()should not ignore the injected tokenizer (and avoid per-call tokenizer creation).
This bypasses caller configuration and is likely a perf hotspot.Proposed fix (cache a tokenizer instance)
public class SPLADE<T> : TransformerEmbeddingNetwork<T> { + private readonly ITokenizer _tokenizer; @@ public SPLADE( NeuralNetworkArchitecture<T> architecture, ITokenizer? tokenizer = null, @@ { _vocabSize = vocabSize; + _tokenizer = tokenizer ?? Tokenization.LanguageModelTokenizerFactory.CreateForBackbone(LanguageModelBackbone.OPT); } @@ public override Vector<T> Embed(string text) { @@ - var tokenizer = Tokenization.LanguageModelTokenizerFactory.CreateForBackbone(LanguageModelBackbone.OPT); - var tokenResult = tokenizer.Encode(text); + var tokenResult = _tokenizer.Encode(text);Also consider adding a defensive shape check before indexing
tokenExpansions[0, s, v](to fail fast if the head isn’t actually projecting to_vocabSize).src/NeuralNetworks/SPLADE.cs-51-69 (1)
51-69: Stop hard-coding layer hyperparameters inInitializeLayers()(use ctor values).
As written, passing non-defaultnumLayers/numHeads/feedForwardDimwon’t affect default layer creation.Proposed fix
public class SPLADE<T> : TransformerEmbeddingNetwork<T> { #region Fields private readonly int _vocabSize; + private readonly int _numLayers; + private readonly int _numHeads; + private readonly int _feedForwardDim; #endregion @@ public SPLADE( @@ int numLayers = 12, int numHeads = 12, int feedForwardDim = 3072, @@ : base(architecture, tokenizer, optimizer, vocabSize, embeddingDimension, maxSequenceLength, numLayers, numHeads, feedForwardDim, PoolingStrategy.Max, lossFunction, maxGradNorm) { _vocabSize = vocabSize; + _numLayers = numLayers; + _numHeads = numHeads; + _feedForwardDim = feedForwardDim; } @@ else { Layers.AddRange(LayerHelper<T>.CreateDefaultSPLADELayers( Architecture, _vocabSize, EmbeddingDimension, MaxTokens, - 12, - 12, - 3072)); + _numLayers, + _numHeads, + _feedForwardDim)); } }src/NeuralNetworks/ColBERT.cs-141-165 (1)
141-165: FixLateInteractionScore()for empty docs (currently returns ~double.MinValue).
IfdocEmbeddings.Rows == 0, the inner loop never runs,maxSimstaysdouble.MinValue, and the score becomes a huge negative number.Proposed fix (empty + dimension checks)
public T LateInteractionScore(Matrix<T> queryEmbeddings, Matrix<T> docEmbeddings) { + if (queryEmbeddings is null) throw new ArgumentNullException(nameof(queryEmbeddings)); + if (docEmbeddings is null) throw new ArgumentNullException(nameof(docEmbeddings)); + if (queryEmbeddings.Rows == 0 || docEmbeddings.Rows == 0) return NumOps.Zero; + if (queryEmbeddings.Columns != docEmbeddings.Columns) + throw new ArgumentException("Query/doc embedding dimensions must match."); + T totalScore = NumOps.Zero; @@ - T maxSim = NumOps.FromDouble(double.MinValue); + T maxSim = NumOps.FromDouble(double.MinValue); // safe now because docEmbeddings.Rows > 0(If you want cosine similarity for non-normalized inputs, you’ll need to divide by norms; right now it’s a dot product.)
src/NeuralNetworks/ColBERT.cs-166-180 (1)
166-180:CreateNewInstance()should preserve ctor-configured hyperparameters.
It currently hard-codes 30522/12/12/3072.Proposed fix
protected override IFullModel<T, Tensor<T>, Tensor<T>> CreateNewInstance() { return new ColBERT<T>( Architecture, null, null, - 30522, + _vocabSize, _outputDim, MaxTokens, - 12, - 12, - 3072, + _numLayers, + _numHeads, + _feedForwardDim, LossFunction, Convert.ToDouble(MaxGradNorm)); }src/NeuralNetworks/ColBERT.cs-80-114 (1)
80-114: Avoid per-call tokenizer creation + validatePredict()output shape before indexing.
Today it always creates an OPT tokenizer and assumesoutput.Shape[2] >= _outputDim.Proposed fix (cache tokenizer + guard output shape)
public class ColBERT<T> : TransformerEmbeddingNetwork<T> { + private readonly ITokenizer _tokenizer; @@ public ColBERT( NeuralNetworkArchitecture<T> architecture, ITokenizer? tokenizer = null, @@ { _outputDim = outputDimension; + _tokenizer = tokenizer ?? Tokenization.LanguageModelTokenizerFactory.CreateForBackbone(LanguageModelBackbone.OPT); } @@ public Matrix<T> EmbedLateInteraction(string text) { @@ - var tokenizer = Tokenization.LanguageModelTokenizerFactory.CreateForBackbone(LanguageModelBackbone.OPT); - var tokenResult = tokenizer.Encode(text); + var tokenResult = _tokenizer.Encode(text); @@ var output = Predict(inputTensor); // [1, seqLen, outputDim] + if (output.Rank < 3 || output.Shape[2] < _outputDim) + throw new InvalidOperationException($"Expected Predict() to return [1, seqLen, >={_outputDim}] but got shape [{string.Join(", ", output.Shape)}].");src/NeuralNetworks/InstructorEmbedding.cs-51-69 (1)
51-69: Stop hard-coding model hyperparameters inInitializeLayers()(breaks non-default ctor args).
Right now the defaults (30522/12/12/3072) are used even if the caller passes different values to the constructor.Proposed fix (store ctor args; reuse in InitializeLayers/CreateNewInstance)
public class InstructorEmbedding<T> : TransformerEmbeddingNetwork<T> { #region Fields private string _defaultInstruction = "Represent this text for retrieval: "; + private readonly int _vocabSize; + private readonly int _numLayers; + private readonly int _numHeads; + private readonly int _feedForwardDim; + private readonly PoolingStrategy _poolingStrategy; #endregion @@ public InstructorEmbedding( NeuralNetworkArchitecture<T> architecture, ITokenizer? tokenizer = null, IGradientBasedOptimizer<T, Tensor<T>, Tensor<T>>? optimizer = null, int vocabSize = 30522, int embeddingDimension = 768, int maxSequenceLength = 512, int numLayers = 12, int numHeads = 12, int feedForwardDim = 3072, PoolingStrategy poolingStrategy = PoolingStrategy.Mean, ILossFunction<T>? lossFunction = null, double maxGradNorm = 1.0) : base(architecture, tokenizer, optimizer, vocabSize, embeddingDimension, maxSequenceLength, numLayers, numHeads, feedForwardDim, poolingStrategy, lossFunction, maxGradNorm) { + _vocabSize = vocabSize; + _numLayers = numLayers; + _numHeads = numHeads; + _feedForwardDim = feedForwardDim; + _poolingStrategy = poolingStrategy; } @@ else { Layers.AddRange(LayerHelper<T>.CreateDefaultInstructorLayers( Architecture, - 30522, + _vocabSize, EmbeddingDimension, MaxTokens, - 12, - 12, - 3072)); + _numLayers, + _numHeads, + _feedForwardDim)); } }src/Helpers/LayerHelper.cs-8081-8135 (1)
8081-8135: BLIP-2 factory is not shape-consistent and can’t be represented as a single sequential layer list without explicit routing.Concrete issues in this stack:
PatchEmbeddingLayer<T>(..., visionHiddenDim)outputsvisionHiddenDim, but subsequent Q-FormerTransformerEncoderLayer<T>(qformerHiddenDim, ...)expectsqformerHiddenDim—there’s no projection/adapter layer between them.- The loop yields
TransformerEncoderLayertwice per iteration (Line 8106-8107) plus an extraDenseLayer(Line 8108); this looks like an accidental duplication unless it’s meant to stand in for distinct blocks (e.g., self-attn vs cross-attn), but then it needs explicit wiring and likely different layer types.- Mixing “vision encoder”, “text embedding”, and “LM decoder” sequentially implies all are applied to the same tensor stream, which is incorrect for BLIP-2.
Recommendation: follow the pattern you already use for multi-branch models (e.g., SAM2 / SlowFast) and split this into separate factory methods or return a tuple (VisionEncoderLayers, QFormerLayers, TextEmbeddingLayers, LmDecoderLayers, Heads) so the model can wire branches explicitly.
src/Helpers/LayerHelper.cs-7983-8012 (1)
7983-8012: Word2Vec default layers aren’t “dual-matrix” and output-size validation is inconsistent.
ValidateLayerParameters(..., architecture.OutputSize)can fail even whenvocabSizeis valid, and can silently allowarchitecture.OutputSize != vocabSizeeven though the stack outputs[vocabSize].- PR objectives mention research-accurate dual-matrix (main + context); this factory currently builds a single embedding + dense-to-vocab + softmax, which doesn’t match that description.
Proposed fix (make validation consistent + guard output size mismatch)
public static IEnumerable<ILayer<T>> CreateDefaultWord2VecLayers( NeuralNetworkArchitecture<T> architecture, int vocabSize, int embeddingDimension) { - ValidateLayerParameters(1, embeddingDimension, architecture.OutputSize); + if (architecture is null) throw new ArgumentNullException(nameof(architecture)); + if (vocabSize < 1) throw new ArgumentOutOfRangeException(nameof(vocabSize)); + if (embeddingDimension < 1) throw new ArgumentOutOfRangeException(nameof(embeddingDimension)); + if (architecture.OutputSize > 0 && architecture.OutputSize != vocabSize) + throw new ArgumentException("architecture.OutputSize must match vocabSize for Word2Vec softmax output.", nameof(architecture)); // 1. Target word embeddings (U matrix) yield return new EmbeddingLayer<T>(vocabSize, embeddingDimension); // 2. Context word projection (V matrix) // Maps embedding space back to vocabulary for prediction yield return new DenseLayer<T>(embeddingDimension, vocabSize, (IActivationFunction<T>?)null); // 3. Output activation yield return new ActivationLayer<T>([vocabSize], new SoftmaxActivation<T>() as IVectorActivationFunction<T>); }src/Helpers/LayerHelper.cs-8046-8080 (1)
8046-8080: FastText factory is missing the “sum word + n-gram embeddings” composition and has the same output-size validation mismatch as Word2Vec.Right now it yields two embedding layers back-to-back with no layer that combines them before projecting to vocab; that’s not a functional sequential model unless the network runtime has special-casing for “FastText mode”.
Suggested direction (minimum: fix validation + document composition requirement)
public static IEnumerable<ILayer<T>> CreateDefaultFastTextLayers( NeuralNetworkArchitecture<T> architecture, int vocabSize, int bucketSize, int embeddingDimension) { - ValidateLayerParameters(1, embeddingDimension, architecture.OutputSize); + if (architecture is null) throw new ArgumentNullException(nameof(architecture)); + if (vocabSize < 1) throw new ArgumentOutOfRangeException(nameof(vocabSize)); + if (bucketSize < 1) throw new ArgumentOutOfRangeException(nameof(bucketSize)); + if (embeddingDimension < 1) throw new ArgumentOutOfRangeException(nameof(embeddingDimension)); + if (architecture.OutputSize > 0 && architecture.OutputSize != vocabSize) + throw new ArgumentException("architecture.OutputSize must match vocabSize for FastText softmax output.", nameof(architecture)); // 1. Word Embeddings yield return new EmbeddingLayer<T>(vocabSize, embeddingDimension); // 2. N-gram Embeddings yield return new EmbeddingLayer<T>(bucketSize, embeddingDimension); + // NOTE: FastText requires combining word + n-gram embeddings (e.g., sum/mean) before projection. + // If the framework expects the model to do this in its forward pass, keep as-is but document it here. + // 3. Context word projection (similar to Word2Vec) yield return new DenseLayer<T>(embeddingDimension, vocabSize, (IActivationFunction<T>?)null); // 4. Output activation yield return new ActivationLayer<T>([vocabSize], new SoftmaxActivation<T>() as IVectorActivationFunction<T>); }src/NeuralNetworks/TransformerEmbeddingNetwork.cs-112-127 (1)
112-127: OOV token ids can crash embedding lookup; also CLS pooling requires CLS-aware tokenization.
tokensare taken directly from the tokenizer (Line 119) and fed intoEmbeddingLayer<T>without bounds checking (Line 123). IfPoolingStrategy.ClsTokenis used (Line 152), you likely need to ensure the encoded sequence includes a CLS token at position 0—otherwise you’re just taking the first real token.src/NeuralNetworks/SiameseNeuralNetwork.cs-67-84 (1)
67-84: “Siamese” default architecture isn’t siamese; also hard-coded heads/FF dim.
Default stack is a single encoder (Line 76-82) and uses hard-coded12, 3072(Line 81). If this is intended to be a dual-encoder wrapper, you likely need two towers or explicit pair-handling in Train/inference.src/NeuralNetworks/SiameseNeuralNetwork.cs-42-61 (1)
42-61: Same tokenizer/vocab default mismatch risk as TransformerEmbeddingNetwork.
Defaulting tokenizer toOPT(Line 95) with defaultvocabSize=30522(Line 46) can easily generate OOV ids forEmbeddingLayer<T>(Line 79).src/NeuralNetworks/TransformerEmbeddingNetwork.cs-225-240 (1)
225-240: CreateNewInstance reuses the same optimizer instance (likely bound to the old network).
Passing_optimizerinto the new instance (Line 230) risks updating the wrong model / sharing mutable optimizer state across models.Proposed fix: don’t pass the optimizer instance into the clone
return new TransformerEmbeddingNetwork<T>( Architecture, _tokenizer, - _optimizer, + optimizer: null, _vocabSize, _embeddingDimension, _maxSequenceLength, _numLayers, _numHeads, _feedForwardDim, _poolingStrategy, _lossFunction, Convert.ToDouble(MaxGradNorm));src/NeuralNetworks/SiameseNeuralNetwork.cs-90-116 (1)
90-116: Embed indexes with _embeddingDimension, but output last-dim may differ (custom layers).
Loopingd < _embeddingDimension(Line 105) assumesoutput.Shape[2] == _embeddingDimension. With custom architectures (Architecture.Layerspath), that might not hold and could index out of bounds atoutput[0, s, d](Line 110).Proposed fix: use the tensor’s actual last dimension
- var result = new Vector<T>(_embeddingDimension); - for (int d = 0; d < _embeddingDimension; d++) + int dim = output.Shape[2]; + var result = new Vector<T>(dim); + for (int d = 0; d < dim; d++)src/NeuralNetworks/TransformerEmbeddingNetwork.cs-52-79 (1)
52-79: Default tokenizer/vocab mismatch + optimizer instance bound to “this”.
Defaulting toLanguageModelBackbone.OPT(Line 117) whilevocabSizedefaults to30522(Line 56) is likely to produce out-of-range token ids forEmbeddingLayer<T>(Line 97). Also,new AdamOptimizer(..., this)(Line 76) makes the optimizer instance model-bound; passing it around (seeCreateNewInstance) is risky.Proposed direction (keep defaults consistent + avoid sharing model-bound optimizer)
- var tokenizer = _tokenizer ?? Tokenization.LanguageModelTokenizerFactory.CreateForBackbone(LanguageModelBackbone.OPT); + var tokenizer = _tokenizer ?? Tokenization.LanguageModelTokenizerFactory.CreateForBackbone(LanguageModelBackbone.BERT); // or align defaults to OPT vocab - _optimizer = optimizer ?? new AdamOptimizer<T, Tensor<T>, Tensor<T>>(this); + _optimizer = optimizer ?? new AdamOptimizer<T, Tensor<T>, Tensor<T>>(this); // ok, but do NOT reuse across clones/loadsAdditionally: validate
tokenId < _vocabSizebefore creating the input tensor (or map OOV to an[UNK]id).src/NeuralNetworks/SiameseNeuralNetwork.cs-179-190 (1)
179-190: CreateNewInstance reuses the same optimizer instance (likely wrong).
Passing_optimizer(Line 184) into a new model risks shared mutable state / optimizer referencing the original network.Proposed fix: don’t pass optimizer through
return new SiameseNeuralNetwork<T>( Architecture, _tokenizer, - _optimizer, + optimizer: null, _vocabSize, _embeddingDimension, _maxSequenceLength, _lossFunction, Convert.ToDouble(MaxGradNorm));
🟡 Minor comments (3)
src/NeuralNetworks/SimCSE.cs-80-94 (1)
80-94: Unsupervised mode training appears incomplete.The comment indicates SimCSE unsupervised mode requires specialized contrastive training with dropout-based positive pairs, but the implementation just calls
base.Trainfor both modes. Consider either implementing the specialized training loop or adding a TODO/documentation note about this limitation.Would you like me to help design the contrastive training loop for SimCSE unsupervised mode?
src/Helpers/LayerHelper.cs-8136-8310 (1)
8136-8310: SimCSE / ColBERT / SPLADE / MRL / Instructor / BGE / SGPT factories: clarify “encoder vs head” and ensure the outputs match the model’s expected representation.Across these methods:
NeuralNetworkArchitecture<T> architectureis passed but not used (except implicitly by signature); consider removing it or using it to drive dims/lengths consistently.- Several factories end in vocab-projection heads (
DenseLayer(..., vocabSize, ...)) which is a language-model head, not an embedding output; if the goal is embedding models, consider either:
- returning only encoder layers (and let the embedding model handle pooling/norm), or
- adding an explicit pooling + normalization/projection head so the returned stack actually produces embeddings.
src/Helpers/LayerHelper.cs-8014-8044 (1)
8014-8044: GloVe factory: validation uses architecture.OutputSize but the method never uses it (and doesn’t build a chainable training graph).Given the comments (“tricky for a strictly sequential ILayer stack”), this seems intended as parameter containers for W/W̃ and biases. In that case, validating against
architecture.OutputSizeis likely accidental and can make this unusable unless callers set OutputSize.Proposed fix (validate only what this method actually needs)
public static IEnumerable<ILayer<T>> CreateDefaultGloVeLayers( NeuralNetworkArchitecture<T> architecture, int vocabSize, int embeddingDimension) { - ValidateLayerParameters(1, embeddingDimension, architecture.OutputSize); + if (architecture is null) throw new ArgumentNullException(nameof(architecture)); + if (vocabSize < 1) throw new ArgumentOutOfRangeException(nameof(vocabSize)); + if (embeddingDimension < 1) throw new ArgumentOutOfRangeException(nameof(embeddingDimension)); // ... yield return new EmbeddingLayer<T>(vocabSize, embeddingDimension); // W yield return new EmbeddingLayer<T>(vocabSize, embeddingDimension); // W_tilde yield return new EmbeddingLayer<T>(vocabSize, 1); // b yield return new EmbeddingLayer<T>(vocabSize, 1); // b_tilde }
🧹 Nitpick comments (3)
src/NeuralNetworks/TransformerEmbeddingNetwork.cs (3)
129-144: Null/streaming input handling.
texts.ToList()(Line 131) throws iftextsis null, and forces full materialization (could be fine, but it’s a choice). Consider guarding null and/or supporting streaming without eager ToList when possible.
146-183: Normalization can produce NaNs for all-zero vectors.
return result.Normalize();(Line 182) can blow up ifresultis the zero vector (possible early in training or with degenerate layers). Consider a safe-normalize path (return zeros when norm==0).
210-223: Parameter vector length is not validated.
Ifparameters.Lengthdoesn’t equal the sum oflayer.ParameterCount, this silently does a partial update (or slices beyond bounds depending onSlicebehavior). Consider asserting at the end thatindex == parameters.Length(and/or that you consumed exactlyParameterCount).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
docs/design/IAuxiliaryLossLayer-Comprehensive-Analysis.mdsrc/GlobalUsings.cssrc/Helpers/LayerHelper.cssrc/NeuralNetworks/BGE.cssrc/NeuralNetworks/ColBERT.cssrc/NeuralNetworks/InstructorEmbedding.cssrc/NeuralNetworks/MatryoshkaEmbedding.cssrc/NeuralNetworks/SGPT.cssrc/NeuralNetworks/SPLADE.cssrc/NeuralNetworks/SiameseNetwork.cssrc/NeuralNetworks/SiameseNeuralNetwork.cssrc/NeuralNetworks/SimCSE.cssrc/NeuralNetworks/TransformerEmbeddingNetwork.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/AdvancedNeuralNetworkModelsIntegrationTests.cs
💤 Files with no reviewable changes (1)
- src/NeuralNetworks/SiameseNetwork.cs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/GlobalUsings.cssrc/NeuralNetworks/SPLADE.cssrc/NeuralNetworks/BGE.cssrc/NeuralNetworks/SimCSE.cssrc/NeuralNetworks/SGPT.cssrc/NeuralNetworks/ColBERT.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/AdvancedNeuralNetworkModelsIntegrationTests.cssrc/NeuralNetworks/MatryoshkaEmbedding.cssrc/Helpers/LayerHelper.cssrc/NeuralNetworks/SiameseNeuralNetwork.cssrc/NeuralNetworks/TransformerEmbeddingNetwork.cssrc/NeuralNetworks/InstructorEmbedding.cs
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/GlobalUsings.cssrc/NeuralNetworks/SPLADE.cssrc/NeuralNetworks/BGE.cssrc/NeuralNetworks/SimCSE.cssrc/NeuralNetworks/SGPT.cssrc/NeuralNetworks/ColBERT.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/AdvancedNeuralNetworkModelsIntegrationTests.cssrc/NeuralNetworks/MatryoshkaEmbedding.cssrc/Helpers/LayerHelper.cssrc/NeuralNetworks/SiameseNeuralNetwork.cssrc/NeuralNetworks/TransformerEmbeddingNetwork.cssrc/NeuralNetworks/InstructorEmbedding.cs
📚 Learning: 2025-12-19T19:05:13.598Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 445
File: src/Interfaces/IPredictionModelBuilder.cs:7-8
Timestamp: 2025-12-19T19:05:13.598Z
Learning: In ooples/AiDotNet, IAggregationStrategy<TModel> is declared in AiDotNet.Interfaces (src/Interfaces/IAggregationStrategy.cs). For src/Interfaces/IPredictionModelBuilder.cs (also in AiDotNet.Interfaces), referencing IAggregationStrategy does not require an additional using. Avoid flagging missing using directives for this case in future reviews.
Applied to files:
src/GlobalUsings.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build (Windows)
- GitHub Check: CodeQL Analysis
🔇 Additional comments (9)
src/GlobalUsings.cs (1)
4-4: LGTM!Adding
AiDotNet.NeuralNetworks.Layersto global usings is appropriate given the PR introduces multiple neural network models that depend on layer types. This reduces boilerplate across the new model files.docs/design/IAuxiliaryLossLayer-Comprehensive-Analysis.md (1)
268-276: LGTM!Documentation updates correctly reflect the
SiameseNetwork→SiameseNeuralNetworkrename, including the updated file path and implementation details describing the dual-encoder architecture.tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/AdvancedNeuralNetworkModelsIntegrationTests.cs (2)
576-591: LGTM!Test correctly updated to use
SiameseNeuralNetwork<float>with the new Transformer-based architecture. The input shape[1, 32]and expected output shape[1, 32, 768]align with the embedding-oriented implementation.
597-609: LGTM!Metadata test properly validates the renamed model with
metadata.Nameassertion for"SiameseNeuralNetwork".src/NeuralNetworks/MatryoshkaEmbedding.cs (1)
83-96: LGTM!
EmbedResizedcorrectly validates the dimension boundary, truncates the embedding, and normalizes the result. This aligns with the Matryoshka Representation Learning pattern where prefixes are valid representations.src/NeuralNetworks/SGPT.cs (1)
86-92: LGTM!Model metadata is correctly configured with appropriate name and description for the SGPT decoder-based embedding model.
src/NeuralNetworks/InstructorEmbedding.cs (1)
117-124: Add null check or ensureAdditionalInfois always initialized.
Ifbase.GetModelMetadata()returns an instance whereAdditionalInfo == null, accessingmetadata.AdditionalInfo["DefaultInstruction"]will throw aNullReferenceException. Either verify that the base class always initializesAdditionalInfo, or add a defensive null check:if (metadata.AdditionalInfo == null) metadata.AdditionalInfo = new Dictionary<string, object>(); metadata.AdditionalInfo["DefaultInstruction"] = _defaultInstruction;src/Helpers/LayerHelper.cs (1)
7983-8311: Verify there are no duplicate method definitions.If any of these factory methods already exist elsewhere in the file/repo with the same signature, the build will fail.
src/NeuralNetworks/SiameseNeuralNetwork.cs (1)
148-163: Unable to verify due to repository access failure. However, based on standard Siamese network and ContrastiveLoss patterns:Standard Siamese training requires: (1) splitting paired inputs, (2) computing embeddings for each side independently, (3) computing distance/similarity, and (4) passing both distances and similarity labels to loss. The web search confirms ContrastiveLoss expects 1-D labels (shape [batch], indicating similar/dissimilar) paired with either 1-D distance tensors or dual embedding tensors.
The concern that
Predict(input)produces a single output that is then converted to a vector and fed to loss may be valid, but definitive verification requires access to:
- The actual
Predict()implementation inSiameseNeuralNetwork- The actual
ContrastiveLossimplementation and its expected input signature- How input data is structured when passed to
Train()If
Predict()internally handles pair splitting and embeddings, the approach may be correct; otherwise, the review comment is accurate.
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In @src/NeuralNetworks/ColBERT.cs:
- Around line 267-269: DeserializeNetworkSpecificData currently does nothing
while SerializeNetworkSpecificData writes _outputDim, which leaves the stream 4
bytes out of sync; fix by reading an Int32 from the BinaryReader in
DeserializeNetworkSpecificData and assigning it to the _outputDim field (i.e.,
call reader.ReadInt32() and set _outputDim) so the stream position and internal
state match what SerializeNetworkSpecificData wrote.
In @src/NeuralNetworks/SiameseNeuralNetwork.cs:
- Around line 300-310: DeserializeNetworkSpecificData must mirror
SerializeNetworkSpecificData by reading the three ints written (in the same
order) and restoring _vocabSize, _embeddingDimension, and _maxSequenceLength;
since those fields are currently readonly, either make them non-readonly with
private setters and assign the values in DeserializeNetworkSpecificData, or
implement a static factory method (e.g., FromSerialized(BinaryReader)) that
reads the three ints and constructs the SiameseNeuralNetwork with those values
and any other required ctor params so the reader position is consumed and state
is fully restored.
In @src/NeuralNetworks/TransformerEmbeddingNetwork.cs:
- Around line 399-402: DeserializeNetworkSpecificData is currently empty while
SerializeNetworkSpecificData writes seven values (28 bytes), which will corrupt
the stream; implement DeserializeNetworkSpecificData to read the same seven
values in the exact order and types that SerializeNetworkSpecificData writes and
assign them back to the corresponding fields/properties used by
TransformerEmbeddingNetwork (and subclasses like ColBERT), ensuring the stream
position is advanced correctly and that any versioning/compatibility checks
mirror those in SerializeNetworkSpecificData.
🟠 Major comments (19)
src/NeuralNetworks/SPLADE.cs-99-117 (1)
99-117: Partially hardcoded parameters inInitializeLayers.Good:
_vocabSizeis correctly stored and used. However,numLayers,numHeads, andfeedForwardDimare still hardcoded (12,12,3072).src/NeuralNetworks/SGPT.cs-82-100 (1)
82-100: Hardcoded parameters inInitializeLayersignore constructor arguments.Same pattern:
vocabSize(50257),numLayers,numHeads,feedForwardDimare hardcoded instead of using constructor values.src/NeuralNetworks/SPLADE.cs-212-215 (1)
212-215: Deserialization is incomplete —_vocabSizewon't be restored.src/NeuralNetworks/SGPT.cs-107-122 (1)
107-122:CreateNewInstancedoesn't preserve custom configuration.Hardcoded values will not clone a customized instance correctly.
src/NeuralNetworks/InstructorEmbedding.cs-95-113 (1)
95-113: Hardcoded parameters inInitializeLayersignore constructor arguments.Same pattern:
vocabSize,numLayers,numHeads,feedForwardDimare hardcoded.src/NeuralNetworks/MatryoshkaEmbedding.cs-152-168 (1)
152-168:CreateNewInstancealso uses hardcoded values instead of preserving configuration.The same hardcoding issue affects cloning. A cloned instance won't preserve custom
vocabSize,numLayers,numHeads, orfeedForwardDimvalues from the original.src/NeuralNetworks/BGE.cs-107-122 (1)
107-122:CreateNewInstancedoesn't preserve custom configuration.Hardcoded values prevent proper cloning of customized instances.
src/NeuralNetworks/InstructorEmbedding.cs-192-195 (1)
192-195: Deserialization is incomplete —_defaultInstructionwon't be restored.🔧 Suggested fix
protected override void DeserializeNetworkSpecificData(BinaryReader reader) { + base.DeserializeNetworkSpecificData(reader); + _defaultInstruction = reader.ReadString(); }src/NeuralNetworks/MatryoshkaEmbedding.cs-194-197 (1)
194-197: Deserialization is incomplete — serialized nested dimensions won't be restored.
SerializeNetworkSpecificDatawrites_nestedDimensions, butDeserializeNetworkSpecificDatais empty. This breaks model restoration from serialized state.🔧 Suggested fix
protected override void DeserializeNetworkSpecificData(BinaryReader reader) { + base.DeserializeNetworkSpecificData(reader); + int count = reader.ReadInt32(); + var dimensions = new int[count]; + for (int i = 0; i < count; i++) + { + dimensions[i] = reader.ReadInt32(); + } + // Note: _nestedDimensions is readonly, so you may need to make it non-readonly + // or use a different initialization pattern to support deserialization }Note: Since
_nestedDimensionsisreadonly, you'll need to either remove thereadonlymodifier or restructure initialization to support deserialization.src/NeuralNetworks/SimCSE.cs-205-208 (1)
205-208: Deserialization is incomplete —_simCseTypeand_dropoutRatewon't be restored.Serialization writes these values, but deserialization doesn't read them back.
🔧 Suggested fix
protected override void DeserializeNetworkSpecificData(BinaryReader reader) { + base.DeserializeNetworkSpecificData(reader); + // Note: fields are readonly, need to restructure for proper deserialization + // _simCseType = (SimCSEType)reader.ReadInt32(); + // _dropoutRate = reader.ReadDouble(); }src/NeuralNetworks/BGE.cs-82-100 (1)
82-100: Hardcoded parameters inInitializeLayersignore constructor arguments.Same pattern as other models:
vocabSize,numLayers,numHeads, andfeedForwardDimare hardcoded (30522,12,12,3072) instead of using constructor-provided values.🔧 Suggested fix: store and use constructor parameters
+ private readonly int _vocabSize; + private readonly int _numLayers; + private readonly int _numHeads; + private readonly int _feedForwardDim; public BGE(...) : base(...) { + _vocabSize = vocabSize; + _numLayers = numLayers; + _numHeads = numHeads; + _feedForwardDim = feedForwardDim; InitializeLayers(); } protected override void InitializeLayers() { ... else { Layers.AddRange(LayerHelper<T>.CreateDefaultBGELayers( Architecture, - 30522, + _vocabSize, EmbeddingDimension, MaxTokens, - 12, - 12, - 3072)); + _numLayers, + _numHeads, + _feedForwardDim)); } }Apply the same pattern to
CreateNewInstance.src/NeuralNetworks/InstructorEmbedding.cs-155-170 (1)
155-170:CreateNewInstancedoesn't preserve_defaultInstruction.If
SetDefaultInstructionwas called to customize the instruction, the cloned instance won't inherit it.🔧 Suggested fix
protected override IFullModel<T, Tensor<T>, Tensor<T>> CreateNewInstance() { - return new InstructorEmbedding<T>( + var instance = new InstructorEmbedding<T>( Architecture, null, null, 30522, EmbeddingDimension, MaxTokens, 12, 12, 3072, PoolingStrategy.Mean, LossFunction, Convert.ToDouble(MaxGradNorm)); + instance.SetDefaultInstruction(_defaultInstruction); + return instance; }src/NeuralNetworks/SimCSE.cs-114-132 (1)
114-132: Hardcoded parameters and unused_dropoutRate.
- Same hardcoded value issue:
30522,12,12,3072ignore constructor parameters._dropoutRateis stored but not passed toCreateDefaultSimCSELayers— the dropout configuration won't affect layer construction.🔧 Suggested fix
Store constructor params and pass
_dropoutRateto layer creation if supported:Layers.AddRange(LayerHelper<T>.CreateDefaultSimCSELayers( Architecture, - 30522, + _vocabSize, EmbeddingDimension, MaxTokens, - 12, - 12, - 3072)); + _numLayers, + _numHeads, + _feedForwardDim, + _dropoutRate)); // if LayerHelper supports thissrc/NeuralNetworks/MatryoshkaEmbedding.cs-101-119 (1)
101-119: Hardcoded parameters ignore constructor arguments.
InitializeLayersuses hardcoded values (30522,12,12,3072) instead of the constructor parameters. If a caller provides customvocabSize,numLayers,numHeads, orfeedForwardDim, these will be ignored when building default layers.🔧 Suggested fix: use instance fields or store constructor params
Store the parameters passed to the constructor and use them in
InitializeLayers:+ private readonly int _vocabSize; + private readonly int _numLayers; + private readonly int _numHeads; + private readonly int _feedForwardDim; public MatryoshkaEmbedding( ... int vocabSize = 30522, ... int numLayers = 12, int numHeads = 12, int feedForwardDim = 3072, ...) : base(...) { + _vocabSize = vocabSize; + _numLayers = numLayers; + _numHeads = numHeads; + _feedForwardDim = feedForwardDim; _nestedDimensions = nestedDimensions ?? new[] { 64, 128, 256, 512, 768, 1024, 1536 }; InitializeLayers(); } protected override void InitializeLayers() { ... else { Layers.AddRange(LayerHelper<T>.CreateDefaultMRLLayers( Architecture, - 30522, + _vocabSize, EmbeddingDimension, MaxTokens, - 12, - 12, - 3072)); + _numLayers, + _numHeads, + _feedForwardDim)); } }src/NeuralNetworks/ColBERT.cs-230-244 (1)
230-244: Hardcoded values inCreateNewInstanceprevent proper cloning.Same issue as
InitializeLayers: hardcoded values30522,12,12,3072are used instead of stored configuration. After fixing the field storage issue above, update this method to use the stored fields.🐛 Proposed fix (after adding fields)
protected override IFullModel<T, Tensor<T>, Tensor<T>> CreateNewInstance() { return new ColBERT<T>( Architecture, null, null, - 30522, + _vocabSize, _outputDim, MaxTokens, - 12, - 12, - 3072, + _numLayers, + _numHeads, + _feedForwardDim, LossFunction, Convert.ToDouble(MaxGradNorm)); }src/NeuralNetworks/ColBERT.cs-106-113 (1)
106-113: Constructor parameters are ignored – hardcoded values used instead.
InitializeLayersuses hardcoded30522,12,12,3072instead of the constructor parameters (vocabSize,numLayers,numHeads,feedForwardDim). This means customizing these values via the constructor has no effect on the default layer configuration.The class doesn't store the constructor parameters as fields, so they're lost after construction.
🐛 Proposed fix – store and use constructor parameters
Add private fields to store the constructor parameters:
private readonly int _outputDim; +private readonly int _vocabSize; +private readonly int _numLayers; +private readonly int _numHeads; +private readonly int _feedForwardDim;Update the constructor to store them:
_outputDim = outputDimension; +_vocabSize = vocabSize; +_numLayers = numLayers; +_numHeads = numHeads; +_feedForwardDim = feedForwardDim;Then use the stored fields:
Layers.AddRange(LayerHelper<T>.CreateDefaultColBERTLayers( Architecture, - 30522, + _vocabSize, _outputDim, MaxTokens, - 12, - 12, - 3072)); + _numLayers, + _numHeads, + _feedForwardDim));src/NeuralNetworks/SiameseNeuralNetwork.cs-101-110 (1)
101-110: Duplicate loss function instantiation and prematurethisreference.Two issues in constructor initialization:
Line 101 vs 107: When
lossFunctionis null, two separateContrastiveLoss<T>instances are created—one passed to the base class and another stored in_lossFunction. This wastes memory and could cause subtle bugs if the loss function maintains state.Line 108: Passing
thistoAdamOptimizerbeforeInitializeLayers()completes means the optimizer receives a reference to a partially initialized object.Proposed fix
- : base(architecture, lossFunction ?? new ContrastiveLoss<T>(), maxGradNorm) + : base(architecture, lossFunction ??= new ContrastiveLoss<T>(), maxGradNorm) { _tokenizer = tokenizer; _vocabSize = vocabSize; _embeddingDimension = embeddingDimension; _maxSequenceLength = maxSequenceLength; - _lossFunction = lossFunction ?? new ContrastiveLoss<T>(); - _optimizer = optimizer ?? new AdamOptimizer<T, Tensor<T>, Tensor<T>>(this); + _lossFunction = lossFunction; InitializeLayers(); + _optimizer = optimizer ?? new AdamOptimizer<T, Tensor<T>, Tensor<T>>(this); }Note: This requires making
_optimizernon-readonly or using a different initialization pattern.src/Helpers/LayerHelper.cs-7983-8010 (1)
7983-8010: Fix parameter validation + head divisibility inCreateDefaultSiameseLayers.
ValidateLayerParameters(1, embeddingDimension, architecture.OutputSize)will throw whenarchitecture.OutputSizeis unset/0 (common for embedding-only use) and is unrelated to the layers produced here.TransformerEncoderLayer<T>(embeddingDimension, 12, 3072)should validateembeddingDimension % 12 == 0(or derivenumHeads).Proposed fix
public static IEnumerable<ILayer<T>> CreateDefaultSiameseLayers( NeuralNetworkArchitecture<T> architecture, int vocabSize = 30522, int embeddingDimension = 768, int maxSequenceLength = 512) { - ValidateLayerParameters(1, embeddingDimension, architecture.OutputSize); + if (vocabSize <= 0) throw new ArgumentOutOfRangeException(nameof(vocabSize)); + if (embeddingDimension <= 0) throw new ArgumentOutOfRangeException(nameof(embeddingDimension)); + if (maxSequenceLength <= 0) throw new ArgumentOutOfRangeException(nameof(maxSequenceLength)); + const int numHeads = 12; + if (embeddingDimension % numHeads != 0) + throw new ArgumentException($"embeddingDimension must be divisible by {numHeads}.", nameof(embeddingDimension)); yield return new EmbeddingLayer<T>(vocabSize, embeddingDimension); yield return new PositionalEncodingLayer<T>(maxSequenceLength, embeddingDimension); - yield return new TransformerEncoderLayer<T>(embeddingDimension, 12, 3072); + yield return new TransformerEncoderLayer<T>(embeddingDimension, numHeads, 3072); }src/Helpers/LayerHelper.cs-8110-8164 (1)
8110-8164:CreateDefaultBlip2Layersappears structurally inconsistent (and may be misleading as a “default”).
- The Q-Former loop yields two
TransformerEncoderLayer<T>per iteration plus aDenseLayer—this looks accidental or at least undocumented.- BLIP-2 relies on explicit cross-attention between query tokens and vision features; a flat sequential list can’t express the routing unless your
TransformerEncoderLayer/TransformerDecoderLayerencapsulates it internally. If it doesn’t, this factory won’t be “research-accurate”.#!/bin/bash set -euo pipefail # Ensure there isn't another CreateDefaultBlip2Layers causing duplicates rg -n --type=cs "CreateDefaultBlip2Layers\s*\(" src/Helpers/LayerHelper.cs # Inspect transformer layer implementations to see whether they support cross-attention / multi-input routing rg -n --type=cs "class\s+TransformerEncoderLayer|class\s+TransformerDecoderLayer" src -S rg -n --type=cs "(cross[- ]attention|encoderOutput|keyValue|qformer|query)" src -S
🟡 Minor comments (2)
src/NeuralNetworks/SimCSE.cs-149-161 (1)
149-161: Train method override is effectively a no-op.Both branches of the
if/elsecallbase.Train(input, expectedOutput)identically. The documentation mentions unsupervised SimCSE should process inputs twice with different dropout masks, but this isn't implemented. Consider either implementing the differentiated training logic or removing the override.💡 If this is a placeholder, consider adding a TODO
public override void Train(Tensor<T> input, Tensor<T> expectedOutput) { if (_simCseType == SimCSEType.Unsupervised) { - // Unsupervised training typically involves processing the same input batch twice - // within the same contrastive loss calculation. - base.Train(input, expectedOutput); + // TODO: Implement unsupervised SimCSE contrastive training + // - Process input twice with different dropout masks + // - Compute contrastive loss between the two embeddings + throw new NotImplementedException("Unsupervised SimCSE training not yet implemented"); } else { base.Train(input, expectedOutput); } }src/NeuralNetworks/SiameseNeuralNetwork.cs-164-175 (1)
164-175: Potential division by zero in mean pooling.If
output.Shape[1]is zero after layer processing, line 172 will cause a division by zero. Additionally, the code assumes a 3D tensor shape[batch, seq_len, embedding_dim]without validation.Proposed defensive fix
// Standard mean pooling to get a single vector from token representations var result = new Vector<T>(_embeddingDimension); + int seqLen = output.Shape[1]; + if (seqLen == 0) + return result.Normalize(); + for (int d = 0; d < _embeddingDimension; d++) { T sum = NumOps.Zero; - for (int s = 0; s < output.Shape[1]; s++) + for (int s = 0; s < seqLen; s++) { sum = NumOps.Add(sum, output[0, s, d]); } - result[d] = NumOps.Divide(sum, NumOps.FromDouble(output.Shape[1])); + result[d] = NumOps.Divide(sum, NumOps.FromDouble(seqLen)); }
🧹 Nitpick comments (16)
src/NeuralNetworks/MatryoshkaEmbedding.cs (1)
68-87: Consider validating nested dimensions against max embedding dimension.The constructor accepts
nestedDimensionswithout validating that all values are ≤maxEmbeddingDimension. WhileEmbedResizeddoes check at call time, invalid configuration could be caught earlier.💡 Optional validation in constructor
_nestedDimensions = nestedDimensions ?? new[] { 64, 128, 256, 512, 768, 1024, 1536 }; + +if (_nestedDimensions.Any(d => d > maxEmbeddingDimension)) + throw new ArgumentException($"All nested dimensions must be <= {maxEmbeddingDimension}"); InitializeLayers();src/NeuralNetworks/SGPT.cs (1)
49-49: Documentation mentions last-token pooling, but default is Mean.The param doc says "research often uses last token" but the default is
PoolingStrategy.Mean. This isn't a bug, but you may want to either change the default to match research practice or clarify the discrepancy in documentation.src/NeuralNetworks/SPLADE.cs (1)
140-173: Tokenizer is recreated on everyEmbedcall.
LanguageModelTokenizerFactory.CreateForBackbone(LanguageModelBackbone.OPT)is called on each invocation. Consider caching the tokenizer or using the instanceTokenizerproperty (if set) for better performance.💡 Suggested optimization
public override Vector<T> Embed(string text) { if (string.IsNullOrWhiteSpace(text)) return new Vector<T>(_vocabSize); - var tokenizer = Tokenization.LanguageModelTokenizerFactory.CreateForBackbone(LanguageModelBackbone.OPT); + var tokenizer = Tokenizer ?? Tokenization.LanguageModelTokenizerFactory.CreateForBackbone(LanguageModelBackbone.OPT); var tokenResult = tokenizer.Encode(text); ... }Or cache the tokenizer as a field initialized once.
src/NeuralNetworks/ColBERT.cs (1)
136-137: Tokenizer created on every call – consider reusing.A new tokenizer is instantiated for each
EmbedLateInteractioncall. This is inefficient for repeated embeddings. Consider using the_tokenizerpattern from the base class or caching the fallback tokenizer.♻️ Suggested approach
-var tokenizer = Tokenization.LanguageModelTokenizerFactory.CreateForBackbone(LanguageModelBackbone.OPT); +var tokenizer = Tokenizer ?? Tokenization.LanguageModelTokenizerFactory.CreateForBackbone(LanguageModelBackbone.OPT);Or cache the fallback tokenizer as a lazy-initialized field.
src/NeuralNetworks/FastText.cs (1)
273-278: Subword n-gram lookup is incomplete.The comment indicates that character n-gram hashing and lookup in Layer 1 is not implemented. Without this, FastText behaves essentially like Word2Vec and loses its key differentiator: handling out-of-vocabulary words via subword information.
Would you like me to help implement the character n-gram extraction and bucket hashing logic, or open an issue to track this as follow-up work?
src/NeuralNetworks/TransformerEmbeddingNetwork.cs (1)
101-117: Consider movingPoolingStrategyenum to namespace level.The enum is nested inside the class but could be useful for other embedding models or configuration code. Moving it to the
AiDotNet.Enumsnamespace would improve discoverability and reusability.src/NeuralNetworks/SiameseNeuralNetwork.cs (2)
267-278: Shared optimizer instance in cloned model.Passing
_optimizerdirectly means the cloned instance shares optimizer state (momentum buffers, step counts, etc.) with the original. This could cause unexpected behavior during independent training of the clone.Consider passing
nullto let the new instance create its own optimizer:Proposed fix
protected override IFullModel<T, Tensor<T>, Tensor<T>> CreateNewInstance() { return new SiameseNeuralNetwork<T>( Architecture, _tokenizer, - _optimizer, + null, // Let new instance create fresh optimizer _vocabSize, _embeddingDimension, _maxSequenceLength, _lossFunction, Convert.ToDouble(MaxGradNorm)); }
291-296: Consider addingMaxSequenceLengthto metadata.
MaxSequenceLengthis serialized but not included inAdditionalInfo. For consistency and completeness:AdditionalInfo = new Dictionary<string, object> { { "EmbeddingDimension", _embeddingDimension }, - { "VocabSize", _vocabSize } + { "VocabSize", _vocabSize }, + { "MaxSequenceLength", _maxSequenceLength } }src/Helpers/LayerHelper.cs (8)
8042-8073:CreateDefaultGloVeLayersvalidation and “sequential stack” semantics need tightening.
ValidateLayerParameters(..., architecture.OutputSize)is disconnected from the produced layers (and may throw for embedding-only usage).- You’re returning 4 embedding tables (W/W̃/b/b̃) but there’s no indication to callers which layer is which; if downstream code depends on ordering, document it explicitly in XML docs to avoid silent misuse.
8164-8191:CreateDefaultSimCSELayers: consider renaming/clarifying the “MLM head” and add head-dimension validation.
- SimCSE training typically uses a pooling/projection head for sentence embeddings; an MLM head may be fine as an optional auxiliary head, but the factory should clarify whether downstream models actually consume that
Dense(embedding -> vocab)output.- Same
numHeadsdivisibility risk as other transformer stacks.
8192-8215:CreateDefaultColBERTLayers: missing normalization step used in ColBERT-style late interaction.ColBERT commonly L2-normalizes token embeddings post-projection; if your model does it elsewhere that’s fine, but the factory should either include it (if a layer exists) or document “normalization handled by model”.
8216-8239:CreateDefaultSPLADELayers: ReLU head alone may not match SPLADE pooling semantics.SPLADE usually applies a vocabulary projection and then a pooling across tokens (e.g., max over sequence) to get a sparse vector. This factory returns only
Dense(... -> vocab), so the caller still needs an aggregation step—worth encoding or documenting.
8240-8263:CreateDefaultMRLLayers: “Matryoshka” typically implies multi-slice outputs; factory only emits max-dim projection.If MRL is implemented by slicing the final embedding during training/inference, please document that here (since the layer list alone suggests only one fixed embedding size).
8264-8287:CreateDefaultInstructorLayers: architecture param unused; consider consumingarchitecturefor defaults or dropping it in future.Right now
architectureis unused, which makes the signature misleading and encourages inconsistent configuration sources.
8288-8310:CreateDefaultBGELayers: missing pooling and L2 norm (common in retrieval embedding models).If the model class handles pooling/norm, document it here; otherwise the factory produces token embeddings without a sentence embedding strategy.
8311-8337:CreateDefaultSGPTLayers: encoder-layer “proxy” needs explicit rationale + causal masking caveat.The comment hints at this, but since this is a public default factory: consider making the choice explicit in the method name/docs (e.g., “SGPTEncoderProxy”) or switching to an actual decoder layer if the framework supports causal masking.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/Enums/SimCSEType.cssrc/Helpers/LayerHelper.cssrc/NeuralNetworks/BGE.cssrc/NeuralNetworks/Blip2NeuralNetwork.cssrc/NeuralNetworks/ColBERT.cssrc/NeuralNetworks/FastText.cssrc/NeuralNetworks/GloVe.cssrc/NeuralNetworks/InstructorEmbedding.cssrc/NeuralNetworks/MatryoshkaEmbedding.cssrc/NeuralNetworks/SGPT.cssrc/NeuralNetworks/SPLADE.cssrc/NeuralNetworks/SiameseNeuralNetwork.cssrc/NeuralNetworks/SimCSE.cssrc/NeuralNetworks/TransformerEmbeddingNetwork.cssrc/NeuralNetworks/Word2Vec.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/NeuralNetworks/SimCSE.cssrc/NeuralNetworks/Word2Vec.cssrc/NeuralNetworks/SPLADE.cssrc/NeuralNetworks/ColBERT.cssrc/NeuralNetworks/FastText.cssrc/NeuralNetworks/SiameseNeuralNetwork.cssrc/NeuralNetworks/SGPT.cssrc/NeuralNetworks/BGE.cssrc/NeuralNetworks/GloVe.cssrc/Enums/SimCSEType.cssrc/NeuralNetworks/InstructorEmbedding.cssrc/NeuralNetworks/MatryoshkaEmbedding.cssrc/NeuralNetworks/TransformerEmbeddingNetwork.cssrc/Helpers/LayerHelper.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/NeuralNetworks/SimCSE.cssrc/NeuralNetworks/Word2Vec.cssrc/NeuralNetworks/SPLADE.cssrc/NeuralNetworks/ColBERT.cssrc/NeuralNetworks/FastText.cssrc/NeuralNetworks/SiameseNeuralNetwork.cssrc/NeuralNetworks/SGPT.cssrc/NeuralNetworks/BGE.cssrc/NeuralNetworks/GloVe.cssrc/Enums/SimCSEType.cssrc/NeuralNetworks/InstructorEmbedding.cssrc/NeuralNetworks/MatryoshkaEmbedding.cssrc/NeuralNetworks/TransformerEmbeddingNetwork.cssrc/Helpers/LayerHelper.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL Analysis
- GitHub Check: Build (Windows)
🔇 Additional comments (16)
src/Enums/SimCSEType.cs (1)
1-31: LGTM!The enum is well-documented with clear XML documentation explaining both the technical concepts and beginner-friendly analogies. The two training paradigms (Unsupervised and Supervised) are properly defined.
src/NeuralNetworks/MatryoshkaEmbedding.cs (1)
136-149: LGTM!The
EmbedResizedmethod correctly validates the dimension boundary and returns a properly truncated and normalized embedding vector.src/NeuralNetworks/Word2Vec.cs (2)
192-206: LGTM!The layer initialization correctly uses stored fields and delegates to
LayerHelperfor default layer creation, maintaining consistency with the PR's standardization goals.
455-464: Verify that the deserialization factory correctly initializes fields before advancing the stream.The pattern in
DeserializeNetworkSpecificData(lines 455-464) relies on a factory to initialize_vocabSize,_embeddingDimension,_windowSize,_maxTokens, and_typebefore calling this method. If the factory reads these values from the stream and passes them to the constructor, this approach works—but it's fragile. A mismatch between what the factory reads and what the stream contains would silently leave the model with incorrect parameters.Confirm that:
- The factory method reads the five int32 values from the stream in the correct order
- These values are passed directly to the constructor
- The constructor initializes all fields from these parameters before deserialize is called
src/NeuralNetworks/FastText.cs (1)
146-161: LGTM!Layer initialization properly uses stored configuration fields and follows the established pattern for custom vs. default layer setup.
src/NeuralNetworks/GloVe.cs (2)
330-342: Research-accurate dual-matrix implementation.Using
W + W_tildefor the final embedding aligns with the GloVe paper's recommendation and the PR objective of research-accurate implementations.
421-427: Same deserialization pattern as other models.Follows the same factory-based deserialization approach as Word2Vec and FastText. Ensure consistency in how all embedding model factories handle reconstruction.
src/NeuralNetworks/TransformerEmbeddingNetwork.cs (2)
180-203: LGTM!The layer initialization correctly builds a standard transformer encoder stack with embedding, positional encoding, and configurable encoder layers. The pattern properly handles both custom and default layer configurations.
264-301: Well-structured pooling implementation.The
PoolOutputmethod cleanly handles all three pooling strategies (ClsToken, Mean, Max) with proper normalization. The implementation is correct and readable.src/NeuralNetworks/SiameseNeuralNetwork.cs (5)
125-141: LGTM!The initialization logic correctly prioritizes user-provided layers with validation and falls back to sensible defaults via
LayerHelper.
181-191: LGTM!The batch embedding implementation correctly materializes the input and processes each text through
Embed.
200-225: LGTM!Standard forward and backward pass implementations with proper GPU optimization support.
230-243: LGTM!Correct parameter slicing and distribution across layers.
254-264: LGTM!Standard training loop with loss computation, gradient calculation, backpropagation, and optimizer update.
src/Helpers/LayerHelper.cs (2)
8011-8041: Manual verification of this review comment is required but cannot be completed due to inaccessibility to the codebase. The review raises three specific technical concerns aboutCreateDefaultWord2VecLayers:
- Validation parameter mismatch (
architecture.OutputSizevsvocabSize)- Dense+Softmax performance vs research-standard negative sampling/hierarchical softmax
- Potential missing dual-matrix exposure for Word2Vec model
To verify these claims, inspect:
- The
Word2Vecmodel implementation and its embedding architecture- The
ValidateLayerParametersmethod definition and intent- Call sites of
CreateDefaultWord2VecLayersand how the returned layers integrate with the model
8074-8109: Unable to verify this review comment due to repository access failure. Manual verification by the developer is required to confirm:
Whether n-gram aggregation components exist: Check if
AggregationLayer,SumLayer, or similar subword aggregation utilities exist elsewhere in the codebase (e.g., in a utilities or layer definitions file) that should be integrated intoCreateDefaultFastTextLayers.Whether n-gram hashing is implemented: Confirm if there are existing n-gram bucketing or hashing mechanisms that the method should wire together.
Word2Vec comparison: Compare the
CreateDefaultWord2VecLayersimplementation to assess whether the criticism about dense-to-vocab softmax is consistent across similar layer creation methods.Intended FastText architecture: Verify against any FastText documentation, papers, or design comments in the codebase to determine if the current implementation is incomplete or intentionally simplified.
Commit Messages Auto-FixedThe commitlint check failed because one or more commit messages did not follow Conventional Commits format. Action taken - All non-compliant commits have been fixed to follow the conventional commits format. Changes made:
The PR branch has been force-pushed with the fixed commits. If you had local changes, you may need to git pull --rebase. |
84ca3d8 to
db148a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/commitlint-fix.yml:
- Line 317: Replace the direct branch push that uses the variable "$BRANCH"
while in a potentially detached HEAD with a push that explicitly references the
current HEAD as the source; update the git push invocation (the line invoking
git push origin "$BRANCH" --force-with-lease) to push HEAD to the remote branch
(use HEAD:$BRANCH) so the action pushes the current commit even in detached HEAD
state.
- Line 211: Replace the failing push command that references a non-existent
local branch (git push origin "$BRANCH" --force-with-lease) with a push of the
current detached HEAD to the remote branch by using the explicit refspec: git
push origin "HEAD:$BRANCH" --force-with-lease; update the line where git push is
invoked to use "HEAD:$BRANCH" so the current commit is pushed even in detached
HEAD state.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
.github/workflows/commitlint-fix.ymldocs/design/IAuxiliaryLossLayer-Comprehensive-Analysis.mdsrc/AiDotNet.Tensors/LinearAlgebra/Vector.cssrc/Enums/SimCSEType.cssrc/Enums/Word2VecType.cssrc/GlobalUsings.cssrc/Helpers/LayerHelper.cssrc/Interfaces/IEmbeddingModel.cssrc/NeuralNetworks/AudioVisualCorrespondenceNetwork.cssrc/NeuralNetworks/BGE.cssrc/NeuralNetworks/Blip2NeuralNetwork.cssrc/NeuralNetworks/BlipNeuralNetwork.cssrc/NeuralNetworks/ClipNeuralNetwork.cssrc/NeuralNetworks/ColBERT.cssrc/NeuralNetworks/FastText.cssrc/NeuralNetworks/FeedForwardNeuralNetwork.cssrc/NeuralNetworks/GloVe.cssrc/NeuralNetworks/InstructorEmbedding.cssrc/NeuralNetworks/MatryoshkaEmbedding.cssrc/NeuralNetworks/SGPT.cssrc/NeuralNetworks/SPLADE.cssrc/NeuralNetworks/SiameseNetwork.cssrc/NeuralNetworks/SiameseNeuralNetwork.cssrc/NeuralNetworks/SimCSE.cssrc/NeuralNetworks/TransformerEmbeddingNetwork.cssrc/NeuralNetworks/Word2Vec.cssrc/RetrievalAugmentedGeneration/Embeddings/CohereEmbeddingModel.cssrc/RetrievalAugmentedGeneration/Embeddings/EmbeddingModelBase.cssrc/RetrievalAugmentedGeneration/Embeddings/GooglePalmEmbeddingModel.cssrc/RetrievalAugmentedGeneration/Embeddings/HuggingFaceEmbeddingModel.cssrc/RetrievalAugmentedGeneration/Embeddings/LocalTransformerEmbedding.cssrc/RetrievalAugmentedGeneration/Embeddings/ONNXSentenceTransformer.cssrc/RetrievalAugmentedGeneration/Embeddings/OpenAIEmbeddingModel.cssrc/RetrievalAugmentedGeneration/Embeddings/SentenceTransformersFineTuner.cssrc/RetrievalAugmentedGeneration/Embeddings/StaticWordEmbeddingModel.cssrc/RetrievalAugmentedGeneration/Embeddings/VoyageAIEmbeddingModel.cssrc/Tokenization/LanguageModelTokenizerFactory.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/AdvancedNeuralNetworkModelsIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/TransformerEmbeddingNetworkTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/design/IAuxiliaryLossLayer-Comprehensive-Analysis.md
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/AiDotNet.Tensors/LinearAlgebra/Vector.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/AiDotNet.Tensors/LinearAlgebra/Vector.cs
🔇 Additional comments (1)
src/AiDotNet.Tensors/LinearAlgebra/Vector.cs (1)
877-891: LGTM!The
SafeNormalize()method provides a clean, non-throwing alternative toNormalize()for handling zero vectors. The implementation is consistent with the existing normalization pattern and the method name clearly communicates its safe behavior. Returning a zero vector for zero-norm inputs is a sensible fallback commonly used in ML and graphics libraries.
Commit Messages Auto-FixedThe commitlint check failed because one or more commit messages did not follow Conventional Commits format. Action taken - All non-compliant commits have been fixed to follow the conventional commits format. Changes made:
The PR branch has been force-pushed with the fixed commits. If you had local changes, you may need to git pull --rebase. |
48cd3b7 to
f89e511
Compare
Commit Messages Auto-FixedThe commitlint check failed because one or more commit messages did not follow Conventional Commits format. Action taken - All non-compliant commits have been fixed to follow the conventional commits format. Changes made:
The PR branch has been force-pushed with the fixed commits. If you had local changes, you may need to git pull --rebase. |
0795acc to
838f7a4
Compare
Commit Messages Auto-FixedThe commitlint check failed because one or more commit messages did not follow Conventional Commits format. Action taken - All non-compliant commits have been fixed to follow the conventional commits format. Changes made:
The PR branch has been force-pushed with the fixed commits. If you had local changes, you may need to git pull --rebase. |
3217660 to
f551a19
Compare
Commit Messages Auto-FixedThe commitlint check failed because one or more commit messages did not follow Conventional Commits format. Action taken - All non-compliant commits have been fixed to follow the conventional commits format. Changes made:
The PR branch has been force-pushed with the fixed commits. If you had local changes, you may need to git pull --rebase. |
6573f03 to
2d43ae5
Compare
Commit Messages Auto-FixedThe commitlint check failed because one or more commit messages did not follow Conventional Commits format. Action taken - All non-compliant commits have been fixed to follow the conventional commits format. Changes made:
The PR branch has been force-pushed with the fixed commits. If you had local changes, you may need to git pull --rebase. |
f0bda21 to
464fe41
Compare
Commit Messages Auto-FixedThe commitlint check failed because one or more commit messages did not follow Conventional Commits format. Action taken - All non-compliant commits have been fixed to follow the conventional commits format. Changes made:
The PR branch has been force-pushed with the fixed commits. If you had local changes, you may need to git pull --rebase. |
/#1305) (#1749) These diffusion models exceed the 120s [Fact(Timeout)] training probe IN ISOLATION (verified solo, one model per process), so they're genuinely compute-bound — not parallel-starvation flakes. Tag them [Trait("Category","HeavyTimeout")] so the default PR shard gate (filtered &Category!=HeavyTimeout) skips them and the nightly HeavyTimeout lane runs them instead: PixArtDeltaLCM, PlaygroundV3, ARDiffusion, MoMask, SiDDiT Deliberately NOT tagged (verified to PASS solo in ~10-15s — they only timed out under parallel core-starvation / WeightRegistry OOM accumulation, which is the AiDotNet.Tensors #714 foundation-memory bug, not a per-model compute cost): DreamFusion, InstructVid2Vid, CogVideo, VideoCrafter2, FateZero, FlowVid Verified the exclusion: `FullyQualifiedName~PixArtDeltaLCMModelTests&Category!=HeavyTimeout` matches 0 tests after tagging. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
… robustness test - Shorten the class-level HeavyTimeout rationale to the durable summary (70 foundation-scale models trained one-per-test -> shard OOM) with links to #1754/#1706 and AiDotNet.Tensors #714, dropping the exact CI gate-filter string that would drift; the retention-source detail already lives in the class docstring. - Use [Trait("Category", "HeavyTimeout")] (the file already has 'using Xunit;') to match the surrounding convention. Comment/attribute-only; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…CV shard OOM is #714 (#1760) * test(cv): HeavyTimeout-tag SegmentationTrainingRobustnessTests (70-model OOM source) (#1754) This class trains 70 foundation-scale SAM/ViT/Swin-family seg models one per test (151 train calls — by far the heaviest single contributor to the Integration C - ComputerVision shard's OOM-hang). Route it to the HeavyTimeout nightly lane so the default PR gate (filtered &Category!=HeavyTimeout) sheds the largest memory load. NOTE: this alone does NOT fully green the shard. Verified locally that with this class excluded, the remaining CV shard STILL OOM-hangs (~20GB) — foundation seg models are instantiated (forward AND train) across ~10 CV classes and every instantiation registers weights in the process-global WeightRegistry, which is never released (AiDotNet.Tensors #714). That pervasive accumulation cannot be fixed by HeavyTimeout tagging without gutting the gate's CV coverage; the durable fix is #714 (weak-refs / pressure-evict) or a shard-wide WeightRegistry.Reset() between tests. This commit sheds the dominant load; #714 closes the rest. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(test): trim HeavyTimeout comment + use unqualified [Trait] in CV robustness test - Shorten the class-level HeavyTimeout rationale to the durable summary (70 foundation-scale models trained one-per-test -> shard OOM) with links to #1754/#1706 and AiDotNet.Tensors #714, dropping the exact CI gate-filter string that would drift; the retention-source detail already lives in the class docstring. - Use [Trait("Category", "HeavyTimeout")] (the file already has 'using Xunit;') to match the surrounding convention. Comment/attribute-only; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…htly lane After the Clone-construction fix, these models' Clone_ShouldProduceIdenticalOutput is logically correct but a single forward exceeds the 120s [Fact(Timeout)] gate — verified each times out SOLO (fresh process), so it is genuine compute-bound heaviness, not #714 parallel starvation. Tag [Trait("Category","HeavyTimeout")] so the default PR shard (which appends &Category!=HeavyTimeout) stays green and the nightly lane covers them. Tagged (verified solo timeout): ControlNetSD3 (MMDiT-X), ControlNetFlux, ControlNetPlusPlusFlux, FluxInpainting (FLUX ~12B), PixArt (DiT ~600M). MVDream was NOT tagged — it passes solo in ~73s (its MultiViewUNet.Clone fix works and it fits the envelope). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… to HeavyTimeout Systematic pass over the untagged heavy-predictor (Flux/DiT/MMDiT/VideoUNet) diffusion models in the failing shards. Each was verified to time out SOLO in a fresh process (rc=124/137 hard-kill or xunit 120s [Fact(Timeout)]) — genuine compute-bound heaviness, not #714 parallel starvation — so tagging is safe. Default PR shard appends &Category!=HeavyTimeout, so this greens those shards while the nightly lane covers them. Tagged (25, verified solo timeout): Allegro, CogVideoX15, Kling26, Kling, LTXVideo, Loong, LumaRay2, LumaRay3, MAGI1, Meissonic, MinimaxVideo, Mochi1, MovieGen, OpenSora2, Pika21, RecraftV3, RunwayGen4, Seedance1, SkyReelsV1, SnapVideo, Sora2, Sora, StableAudio, StableDiffusion35, StepVideo. NOT tagged — verified to PASS solo within the envelope (would have been mis-tagged by a by-architecture heuristic): AuraFlow, CogVideo, Latte, Lumiere, LuminaT2X, MakeAVideo, Mochi1Preview, ModelScopeT2V, OmniGen, OpenSora, PixArtDelta, PixArtSigma, PointE, RunwayGen, ShapE, Show1, SoundStorm, StableDiffusion3, StableVideoDiffusion, StreamingT2V. NOT tagged — fail solo for a NON-timeout reason (real bugs for the residual shard issues, not HeavyTimeout): Bark, Oasis, PyramidFlow. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…HeavyTimeout tags + reduced test configs (#1771) * fix(diffusion): clone resolved predictor/vae in 12 model clones (batch 1) Cross-shard Clone_ShouldProduceIdenticalOutput fix: these models' Clone() passed only conditioner/seed, so the clone rebuilt InitializeLayers' default-sized, lazily -unresolved UNet/VAE. Once the source resolved its lazy layers via a forward pass, TryShareParametersFrom no longer lined up 1:1 and the clone diverged (or SetParameters threw on the count mismatch). Now each clones the resolved predictor/VAE (+ same architecture/options/scheduler), mirroring InstaFlowModel/MultiDiffusionModel. Batch 1 (12): DMD2, FlowMap, MultistepLC, OSDS (FastGeneration); CCSR, PASD, SeeSR (SuperResolution); ControlAR, ControlNeXt, ControlNetLite, ControlNetTile, ReferenceOnly (Control). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(diffusion): clone resolved predictor/vae in 7 more model clones (batch 2) Same Clone_ShouldProduceIdenticalOutput root-cause fix as batch 1, extended to models with a ctor predictor/baseUNet param and configuration flags. Batch 2 (7): ControlNetModel, IPAdapterModel, IPAdapterPlusModel (Control); HyperSD, PCM, PeRFlow, TrainingEfficientLCM (FastGeneration). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(diffusion): clone resolved sub-models in 18 more model clones (batch 3) Completes the cross-shard Clone_ShouldProduceIdenticalOutput sweep. Same root-cause fix (clone the resolved predictor/VAE instead of rebuilding default-sized, lazily- unresolved sub-models) extended to the remaining genuinely-buggy models. Batch 3 (18): - ImageEditing: BrushNet, BrushNetX, CycleGANTurbo, FluxInpainting, ICEdit, PowerPaint, RAD, ReplaceAnything, TurboFill. - VirtualTryOn: CATDM, StableVITON. StyleTransfer: RBModulation, StyleStudio, TLoRA. - Control: ControlNetFlux, ControlNetPlusPlusFlux (Flux DiT — pass resolved predictor/ VAE, then existing control-encoder copy / COW share). - TextToImage: PixArt (added optional dit/vae ctor params so the clone gets resolved sub-models before ShareWeightsFrom, which previously threw on the lazy-vs-resolved layer mismatch). - ThreeD: MVDream (added MultiViewUNet.Clone() cloning its base U-Net + multi-view attention; Clone passes resolved multi-view UNet/VAE then shares the camera embedding). Models already correct (two-path COW clone, lazy-preserving clone, or sub-model .Clone() composition — SANASprint, FluxSchnell, CogVideo/RunwayGen/SVD/VideoCrafter, PointE, ShapE, SoundStorm, UpscaleAVideo) are intentionally untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(diffusion): move 5 compute-bound clone tests to HeavyTimeout nightly lane After the Clone-construction fix, these models' Clone_ShouldProduceIdenticalOutput is logically correct but a single forward exceeds the 120s [Fact(Timeout)] gate — verified each times out SOLO (fresh process), so it is genuine compute-bound heaviness, not #714 parallel starvation. Tag [Trait("Category","HeavyTimeout")] so the default PR shard (which appends &Category!=HeavyTimeout) stays green and the nightly lane covers them. Tagged (verified solo timeout): ControlNetSD3 (MMDiT-X), ControlNetFlux, ControlNetPlusPlusFlux, FluxInpainting (FLUX ~12B), PixArt (DiT ~600M). MVDream was NOT tagged — it passes solo in ~73s (its MultiViewUNet.Clone fix works and it fits the envelope). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(diffusion): move 25 more solo-verified compute-bound clone tests to HeavyTimeout Systematic pass over the untagged heavy-predictor (Flux/DiT/MMDiT/VideoUNet) diffusion models in the failing shards. Each was verified to time out SOLO in a fresh process (rc=124/137 hard-kill or xunit 120s [Fact(Timeout)]) — genuine compute-bound heaviness, not #714 parallel starvation — so tagging is safe. Default PR shard appends &Category!=HeavyTimeout, so this greens those shards while the nightly lane covers them. Tagged (25, verified solo timeout): Allegro, CogVideoX15, Kling26, Kling, LTXVideo, Loong, LumaRay2, LumaRay3, MAGI1, Meissonic, MinimaxVideo, Mochi1, MovieGen, OpenSora2, Pika21, RecraftV3, RunwayGen4, Seedance1, SkyReelsV1, SnapVideo, Sora2, Sora, StableAudio, StableDiffusion35, StepVideo. NOT tagged — verified to PASS solo within the envelope (would have been mis-tagged by a by-architecture heuristic): AuraFlow, CogVideo, Latte, Lumiere, LuminaT2X, MakeAVideo, Mochi1Preview, ModelScopeT2V, OmniGen, OpenSora, PixArtDelta, PixArtSigma, PointE, RunwayGen, ShapE, Show1, SoundStorm, StableDiffusion3, StableVideoDiffusion, StreamingT2V. NOT tagged — fail solo for a NON-timeout reason (real bugs for the residual shard issues, not HeavyTimeout): Bark, Oasis, PyramidFlow. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(diffusion): shrink LuminaT2X/LuminaImage2 test configs to fit CI (L shard) Both tests constructed their model at the DEFAULT foundation scale (Flag-DiT hiddenSize 4096 x 32 layers ~ tens of GB in float32), which OOMs the 16 GB CI runner and cancels the whole L shard. Pass a small Flag-DiT predictor + small VAE instead, preserving the shape-critical dims (inputChannels = LatentChannels, contextDim, latentSize) so the patchify/forward path is exercised identically. The tests stay exact (no precision loss) and now run all 26 tests in ~31s in the default PR gate, vs OOM before. Mirrors LatteModelTests/LGMModelTests/LatentConsistencyModelTests, which already do this. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(diffusion): shrink StableCascade test config to fit CI (Stable shard) The only remaining Stable-shard failure was StableCascadeModelTests.Training_ ShouldReducePredictionError timing out (the other 54 tests passed). The two-stage cascade at paper defaults (baseChannels 384/320, 64x64 latent) peaked ~47 GB and its multi-iteration train loop blew the 120s gate. Fixed by (1) building the prior/decoder U-Nets + VAE at a small width and (2) using a 16x16 latent so the prior's self-attention runs over 256 tokens instead of 4096. Shape-critical dims preserved (prior 24ch, decoder 4ch, contextDim 1280). Full StableCascade test set now passes 13/13 in ~13s (was timeout). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: preserve diffusion clone construction state * test(diffusion): shrink StyDiff test config to fit CI (Step-Sync shard) The only remaining Step-Sync failure was StyDiffModelTests.Training_ShouldReduce PredictionError timing out (87 other tests passed). SD1.5-scale U-Net (baseChannels 320) at a 64x64 latent peaked ~49 GB and blew the 120s Training gate. Fixed with a small U-Net + VAE and a 16x16 latent (self-attention over 256 tokens vs 4096); shape-critical dims preserved (4 latent channels, contextDim 768). StyDiff now passes 13/13 in ~14s. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: address diffusion clone review follow-ups * test(diffusion): shrink SeedEdit3 test config to fit CI (SE-SP shard) The only remaining SE-SP failure was SeedEdit3ModelTests.ForwardPass_ShouldBeFinite_ AfterTraining timing out (a small model, but its default SiT predictor is DiT-XL-class: hiddenSize 1152 x 28 layers, so the train-then-forward loop blew the 120s gate). Fixed with a small SiT (hiddenSize 64 x 2 layers) + small VAE, preserving inputChannels = 16, mirroring OmniGen2ModelTests. Also refine LuminaImage2's small config (latentSize 64 to match its 64x64 InputShape). Both full test sets now pass 26/26 in ~27s. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(diffusion): shrink MAR test config to fit CI (M A-Mi shard) Sole M A-Mi failure was MARModelTests.Training_ShouldReducePredictionError timing out (default SiT hiddenSize 1024 x 24 layers). Small SiT (64 x 2) + small VAE, inputChannels 16 preserved. Passes 13/13 in ~6s. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(diffusion): shrink MotionDiffuse/MotionDiffusion configs to fit CI (M Motion-MZ shard) Both used the default DiT-XL-class SiT (hiddenSize 1152 x 28 layers); the train-then- forward loop runs ~117s solo (just under the 120s gate) and tips over the timeout under shard load. Small SiT (64 x 2) + small VAE, inputChannels preserved (4 / 263). Both full test sets now pass 26/26 in ~17s. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(diffusion): shrink KLoRAStyle test config to fit CI (J-K shard) Sole J-K failure was KLoRAStyleModelTests.ForwardPass_ShouldBeFinite_AfterTraining timing out (SD1.5-scale U-Net at 64x64). Small U-Net + VAE + 16x16 latent, inputChannels 4 / contextDim 768 preserved. Passes 13/13 in ~11s. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(diffusion): shrink AutoRegressiveMaskedDiffusion config to fit CI (A-C shard) Sole A-C failure was AutoRegressiveMaskedDiffusionTests.ScaledInput_ShouldChangeOutput failing under shard load (passes solo in ~54s at the default DiT-XL-class SiT hiddenSize 1152 x 28 — the two forwards tip over the 120s gate under contention, not a real bug). Small SiT (64 x 2) + small VAE, inputChannels 16 preserved. Passes 13/13 in ~7s. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(diffusion): shrink PointE + Oasis configs to fit CI (N-R shard) N-R had two failures: PointE (ForwardPass_AfterTraining, borderline compute-bound at the default DiT 512x12 - passes solo, tips over under load) and Oasis (Clone + ForwardPass_ AfterTraining; the default DiT 1536x24 + baseChannels-128 temporal VAE peaked ~50 GB and stalled the shard - the Clone divergence was a scale/precision artifact, not a logic bug). Both shrunk to a small DiT (+ small temporal VAE for Oasis), shape-critical dims preserved (6/1024/32 and 16/4096/32). Both full test sets now pass 26/26 in ~13s. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(diffusion): shrink PyramidFlow config to fix Clone + fit CI (N-R shard) PyramidFlowModelTests.Clone_ShouldProduceIdenticalOutput failed reproducibly at the default foundation scale (DiT 1536x24 + baseChannels-128 temporal VAE, ~50 GB) — the same scale/precision Clone artifact as OasisModel, not a logic bug. Shrinking to a small DiT + temporal VAE (shape-critical dims preserved: 16 / 4096 / 32) makes Clone deterministic and exact; full test set now passes 13/13 in ~6s. No HeavyTimeout tag needed — it passes in the PR gate. BarkModel was re-verified and already passes (its earlier failure was flaky). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(diffusion): tag 88 foundation-scale-at-default models HeavyTimeout to fit 16GB CI The diffusion ModelFamily shards were still red on CI: models built at their full-scale DEFAULT config have a Training peak (weights + gradients + Adam state + activations ~ 4x the ~1 GB SD/DiT-scale weights) that OOMs the 16 GB GitHub runner. They passed locally only because this box has 64 GB. Confirmed via the CI logs (StableDiffusion15 -> testhost crash; A-C/SE-SP/M A-Mi -> runner OOM death / shutdown signal). The per-culprit shrinks (earlier commits) only caught the one model that failed on the 64 GB box per shard; the remaining full-scale-default models still OOM at 16 GB. Per maintainer decision, tag them HeavyTimeout so the default PR-gate shard (which appends &Category!=HeavyTimeout) excludes them and fits — the nightly HeavyTimeout lane covers them. Tagged 88 models across A-C, D-I, M A-Mi, M Motion-MZ, N-R, SE-SP, Stable, Step-Sync, T-Z (every untagged full-scale-default test in the failing shards). The already-shrunk culprits (ARMD, MAR, StyDiff, KLoRAStyle, SeedEdit3, Motion x2, StableCascade, PointE, Oasis, PyramidFlow, Lumina x2) stay as real in-gate tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(diffusion): assert clone weight-value equality + dedupe MultiViewUNet ctors Address PR #1771 review: - MultiViewUNetCloneTests now asserts GetParameters() value equality, not just ParameterCount, so a Clone() that re-initialized weights would fail (the exact bug class this PR fixes). - MultiViewUNet's public constructor now delegates to the private clone constructor (passing freshly-built base U-Net + attention), collapsing the duplicated readonly scalar-field assignments into a single initialization path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: franklinic <franklin@ivorycloud.com>
This PR refactors the embedding service and the multimodal neural network models (Word2Vec, GloVe, FastText, and Blip2NeuralNetwork) to align with the project's standardized NeuralNetworkBase architecture and research-paper accuracy. Key changes include standardized initialization via LayerHelper, dual-matrix structures for Word2Vec/GloVe, subword support for FastText, and native integration with GPU/JIT/AutoDiff.