feat: add clip multimodal embedding neural network - #583
Conversation
Add CLIP (Contrastive Language-Image Pre-training) implementation: - Create IMultimodalEmbedding<T> interface for multimodal models - Implement ClipNeuralNetwork<T> extending NeuralNetworkBase<T> - Add CreateDefaultClipLayers to LayerHelper for projection layers - Add ImageEmbeddingDim and TextEmbeddingDim to NeuralNetworkArchitecture - Add Clip enum value to ModelType The CLIP implementation uses ONNX Runtime for pre-trained model inference and follows the golden standard patterns from FeedForwardNeuralNetwork. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add ClipImagePreprocessor<T> that handles CLIP model image preprocessing: - Resize images to target size using bilinear interpolation - Normalize using ImageNet mean/std values - Convert between HWC and CHW formats - Handle grayscale expansion to RGB 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add CLIP-specific tokenization support: - Add SpecialTokens.Clip() for CLIP start/end tokens - Create ClipTokenizerFactory for loading pretrained vocabularies - Add helper methods for CLIP encoding options - Support loading from HuggingFace vocab.json and merges.txt files 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add unit tests for CLIP components: - ClipNeuralNetworkTests: Parameter validation tests - ClipImagePreprocessorTests: Image preprocessing tests for resize, normalize, format conversion - ClipTokenizerFactoryTests: Tokenizer factory and CLIP-specific encoding tests All tests pass (67 pass, 1 skipped for ONNX requirement). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Warning Rate limit exceeded@ooples has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 23 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (18)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds extensive multimodal support: new multimodal interfaces and enums, CLIP tokenizer/preprocessor/loader/helpers, many multimodal neural-network implementations (CLIP, BLIP, BLIP‑2, Flamingo, LLaVA, GPT‑4V, VideoCLIP, ImageBind, DallE3, audio‑visual nets), serving endpoints and repository multimodal APIs, and related tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Serving as EmbeddingsController
participant Repo as ModelRepository
participant Model as IServableMultimodalModel
participant Tokenizer
participant ImgPrep as ClipImagePreprocessor
participant Enc as Encoder (ONNX/native)
rect rgb(232,243,255)
Client->>Serving: POST /api/Embeddings/text/{model} {text}
Serving->>Repo: GetMultimodalModel(name)
Repo-->>Serving: Model
Serving->>Model: EncodeText(text)
Model->>Tokenizer: Tokenize(text)
Tokenizer->>Enc: Encode tokens
Enc-->>Model: text embedding
Model-->>Serving: embedding
Serving-->>Client: TextEmbeddingResponse
end
rect rgb(241,255,240)
Client->>Serving: POST /api/Embeddings/image/{model} {image}
Serving->>Repo: GetMultimodalModel(name)
Repo-->>Serving: Model
Serving->>ImgPrep: Preprocess(image)
ImgPrep-->>Model: image tensor
Model->>Enc: Encode image tensor
Enc-->>Model: image embedding
Model-->>Serving: embedding
Serving-->>Client: ImageEmbeddingResponse
end
rect rgb(255,248,230)
Client->>Serving: POST /api/Embeddings/classify/{model} {image, labels}
Serving->>Repo: GetMultimodalModel(name)
Repo-->>Serving: Model
Serving->>ImgPrep: Preprocess(image)
ImgPrep-->>Model: image tensor
Serving->>Tokenizer: Tokenize(labels)
Tokenizer->>Enc: Encode label tokens
Model->>Model: Compute similarities & softmax
Model-->>Serving: label probabilities
Serving-->>Client: ZeroShotClassifyResponse
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 4
🧹 Nitpick comments (9)
src/Helpers/LayerHelper.cs (1)
3744-3787: Consider failing fast instead of silently defaulting CLIP embedding dims
CreateDefaultClipLayersuses hardcoded fallbacks (768 for image, 512 for text) whenarchitecture.ImageEmbeddingDim/TextEmbeddingDimare not set. That’s convenient for ViT‑L/B defaults, but can silently mis-size the projection heads if an ONNX encoder with different embedding sizes is wired up without configuring the architecture fields.Consider either:
- Validating that
ImageEmbeddingDimandTextEmbeddingDimare > 0 and throwing if not, or- Making the defaults explicit in XML docs so callers know they must override for nonstandard CLIP variants.
src/Interfaces/IMultimodalEmbedding.cs (1)
35-232: Interface surface for multimodal embeddings looks solid; minor doc nuanceThe
IMultimodalEmbedding<T>API (embedding properties, text/image embedding, similarity, zero‑shot classification) is coherent and matches CLIP‑style usage, with good XML docs for consumers.One small improvement: the
GetImageEmbeddingremarks mention inputs in[-1, 1]or[0, 1], while the rest of the CLIP stack (e.g., ImageNet mean/std normalization) typically works on already‑normalized tensors. It may be worth tightening this description to clearly distinguish:
- Raw image range before preprocessing (e.g., 0–255), vs
- Normalized tensor range expected by the encoder after preprocessing.
Otherwise the interface itself looks ready to implement.
src/NeuralNetworks/NeuralNetworkArchitecture.cs (1)
181-231: Well-structured additions for multimodal support.The new
ImageEmbeddingDimandTextEmbeddingDimproperties are well-documented and follow the existing patterns in this class. The XML documentation is thorough and beginner-friendly.Consider adding validation in
ValidateInputDimensions()to reject negative embedding dimensions, similar to how other dimension properties are validated:+ if (ImageEmbeddingDim < 0) + throw new ArgumentException("ImageEmbeddingDim cannot be negative."); + if (TextEmbeddingDim < 0) + throw new ArgumentException("TextEmbeddingDim cannot be negative.");src/Tokenization/ClipTokenizerFactory.cs (1)
108-118: Consider wrapping JSON parsing in a more descriptive exception.If the vocab JSON is malformed,
JsonConvert.DeserializeObjectwill throw aJsonReaderExceptionwith a potentially cryptic message. Wrapping this in a more user-friendly exception would improve the developer experience.🔎 Suggested improvement
- var vocabJson = File.ReadAllText(vocabPath); - var vocabDict = JsonConvert.DeserializeObject<Dictionary<string, int>>(vocabJson); - if (vocabDict == null) - throw new InvalidOperationException("Failed to parse vocabulary file."); + Dictionary<string, int>? vocabDict; + try + { + var vocabJson = File.ReadAllText(vocabPath); + vocabDict = JsonConvert.DeserializeObject<Dictionary<string, int>>(vocabJson); + } + catch (JsonException ex) + { + throw new InvalidOperationException($"Failed to parse vocabulary file '{vocabPath}': {ex.Message}", ex); + } + if (vocabDict == null) + throw new InvalidOperationException($"Vocabulary file '{vocabPath}' is empty or invalid.");tests/AiDotNet.Tests/UnitTests/NeuralNetworks/ClipNeuralNetworkTests.cs (2)
86-136: Tests document validation order - consider robustness.These tests verify that path validation occurs before other parameter validation. While useful for documenting behavior, they're somewhat coupled to implementation details. If the validation order changes in the future, these tests would need updating.
Consider renaming to make the intent clearer:
- public void Constructor_WithZeroEmbeddingDimension_PathValidationComesFirst() + public void Constructor_WithNonExistentPath_ThrowsFileNotFoundEvenWithInvalidDimension()
142-147: Consider adding a TODO comment with expected implementation.The skipped integration test is appropriate, but an outline of what it would test once ONNX models are available would be helpful for future implementation.
🔎 Suggested improvement
public void Integration_WithValidModels_CreatesNetwork() { // This test would require actual ONNX models // Set CLIP_MODELS_PATH environment variable to enable + // TODO: When enabled, this test should: + // 1. Load image and text encoder ONNX models from CLIP_MODELS_PATH + // 2. Create ClipNeuralNetwork instance + // 3. Verify GetTextEmbedding returns expected dimension + // 4. Verify GetImageEmbedding returns expected dimension + // 5. Verify ComputeSimilarity works correctly }tests/AiDotNet.Tests/UnitTests/Preprocessing/ClipImagePreprocessorTests.cs (1)
335-357: Consider simplifying the consistency assertion.The nested loop comparison is thorough but verbose. Consider using a helper or LINQ-based comparison for readability.
🔎 Suggested alternative
- for (int c = 0; c < 3; c++) - { - for (int h = 0; h < 224; h++) - { - for (int w = 0; w < 224; w++) - { - Assert.Equal(result1[c, h, w], result2[c, h, w], 5); - } - } - } + Assert.Equal(result1.Data.Length, result2.Data.Length); + for (int i = 0; i < result1.Data.Length; i++) + { + Assert.Equal(result1.Data[i], result2.Data[i], 5); + }src/NeuralNetworks/ClipNeuralNetwork.cs (2)
290-328: Consider true batch inference for performance.The current implementation processes each text individually in a loop (lines 310-325), which doesn't leverage ONNX Runtime's batch inference capabilities. For large batches, this could be a performance bottleneck.
The tokenization correctly uses
EncodeBatch, but the ONNX inference could also be batched by constructing a single tensor with batch dimension > 1.
428-464: Consider making the prompt template configurable.The prompt template
"a photo of a {label}"at line 443 is hardcoded. Different use cases might benefit from different templates (e.g., "an image of a {label}", "a {label}", etc.). Consider adding an optional parameter or overload.🔎 Suggested enhancement
- public Dictionary<string, T> ZeroShotClassify(Tensor<T> image, IEnumerable<string> classLabels) + public Dictionary<string, T> ZeroShotClassify( + Tensor<T> image, + IEnumerable<string> classLabels, + string promptTemplate = "a photo of a {0}") { // ... - var promptedLabels = labels.Select(label => $"a photo of a {label}").ToList(); + var promptedLabels = labels.Select(label => string.Format(promptTemplate, label)).ToList();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/Enums/ModelType.cssrc/Helpers/LayerHelper.cssrc/Interfaces/IMultimodalEmbedding.cssrc/NeuralNetworks/ClipNeuralNetwork.cssrc/NeuralNetworks/NeuralNetworkArchitecture.cssrc/Preprocessing/Image/ClipImagePreprocessor.cssrc/Tokenization/ClipTokenizerFactory.cssrc/Tokenization/Models/SpecialTokens.cstests/AiDotNet.Tests/UnitTests/NeuralNetworks/ClipNeuralNetworkTests.cstests/AiDotNet.Tests/UnitTests/Preprocessing/ClipImagePreprocessorTests.cstests/AiDotNet.Tests/UnitTests/Tokenization/ClipTokenizerFactoryTests.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/Interfaces/IMultimodalEmbedding.cssrc/Tokenization/Models/SpecialTokens.cstests/AiDotNet.Tests/UnitTests/NeuralNetworks/ClipNeuralNetworkTests.cssrc/Tokenization/ClipTokenizerFactory.cssrc/Preprocessing/Image/ClipImagePreprocessor.cssrc/Helpers/LayerHelper.cssrc/Enums/ModelType.cssrc/NeuralNetworks/NeuralNetworkArchitecture.cstests/AiDotNet.Tests/UnitTests/Tokenization/ClipTokenizerFactoryTests.cssrc/NeuralNetworks/ClipNeuralNetwork.cstests/AiDotNet.Tests/UnitTests/Preprocessing/ClipImagePreprocessorTests.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/Interfaces/IMultimodalEmbedding.cssrc/Tokenization/Models/SpecialTokens.cstests/AiDotNet.Tests/UnitTests/NeuralNetworks/ClipNeuralNetworkTests.cssrc/Tokenization/ClipTokenizerFactory.cssrc/Preprocessing/Image/ClipImagePreprocessor.cssrc/Helpers/LayerHelper.cssrc/Enums/ModelType.cssrc/NeuralNetworks/NeuralNetworkArchitecture.cstests/AiDotNet.Tests/UnitTests/Tokenization/ClipTokenizerFactoryTests.cssrc/NeuralNetworks/ClipNeuralNetwork.cstests/AiDotNet.Tests/UnitTests/Preprocessing/ClipImagePreprocessorTests.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). (4)
- GitHub Check: CodeQL analysis (csharp)
- GitHub Check: Agent
- GitHub Check: CodeQL Analysis
- GitHub Check: Build (Windows)
🔇 Additional comments (12)
src/Enums/ModelType.cs (1)
1021-1027: Addition ofClipmodel type looks consistentThe new
ModelType.Clipentry is placed logically next to other neural network types and doesn’t affect existing enum values’ semantic meaning beyond the standard reindexing. Assuming factory/switch logic is updated elsewhere in this PR, this change is fine.src/Tokenization/Models/SpecialTokens.cs (1)
112-135: CLIP special tokens configuration is appropriate
SpecialTokens.Clip()correctly assigns CLIP-style BOS (<|startoftext|>), EOS/UNK/PAD (<|endoftext|>), and CLS (<|startoftext|>), leaving SEP/MASK empty. This aligns with typical CLIP tokenizer setups and should work cleanly with existingGetAllSpecialTokens()logic.src/NeuralNetworks/NeuralNetworkArchitecture.cs (1)
435-464: LGTM!The constructor correctly incorporates the new multimodal embedding dimension parameters with appropriate defaults. The
JsonConstructorattribute ensures proper deserialization support.src/Tokenization/ClipTokenizerFactory.cs (2)
164-187: LGTM!The
CreateSimplemethod provides a sensible default corpus for testing and clearly documents that it's not intended for production use.
228-245: LGTM!The compatibility check correctly validates CLIP-specific special tokens and vocabulary size bounds. The logic appropriately handles both
BosTokenandClsTokenfor the start token, accommodating different tokenizer configurations.tests/AiDotNet.Tests/UnitTests/NeuralNetworks/ClipNeuralNetworkTests.cs (1)
17-71: LGTM!Good coverage of null and empty path validation for both encoder paths. The tests properly verify that meaningful error messages are provided.
tests/AiDotNet.Tests/UnitTests/Preprocessing/ClipImagePreprocessorTests.cs (3)
22-78: LGTM!Comprehensive constructor validation tests covering default parameters, custom values, and invalid input scenarios with appropriate exception types.
80-270: LGTM!Excellent test coverage for various input formats and preprocessing scenarios. The tests properly verify shape transformations (grayscale expansion, HWC→CHW conversion, resize operations) and normalization behavior.
359-390: LGTM!Well-designed helper methods that enable deterministic testing with seeded random values. The 0-255 range for random values appropriately simulates typical image pixel data.
src/NeuralNetworks/ClipNeuralNetwork.cs (3)
253-283: LGTM!The text embedding method correctly tokenizes input, runs ONNX inference, and normalizes the output. The
usingstatement ensures proper disposal of inference results.
673-681: LGTM!Proper disposal of ONNX
InferenceSessionobjects with null-conditional operators and correct call to base disposal.
641-660: Verify CLS token extraction for 3D tensors.For 3D output tensors (shape
[batch, seq_len, hidden]), the current logic setsstartIdx = 0and iterates through the first_embeddingDimensionvalues. Confirm that the tensor is laid out contiguously with the CLS token first, and verify this matches your ONNX model's actual output format and memory layout to ensure correct embedding extraction.
There was a problem hiding this comment.
Pull request overview
This PR implements CLIP (Contrastive Language-Image Pre-training) multimodal embedding support for AiDotNet, enabling text-image similarity matching and zero-shot classification using pre-trained ONNX models.
Key Changes:
- Adds
IMultimodalEmbedding<T>interface defining contracts for text/image embedding generation - Implements
ClipNeuralNetwork<T>using ONNX Runtime for inference with vision and text encoders - Provides
ClipTokenizerFactoryfor CLIP-compatible BPE tokenization with proper special tokens - Includes comprehensive test coverage (67 tests) covering factory creation, preprocessing, and validation
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Interfaces/IMultimodalEmbedding.cs | New interface defining multimodal embedding operations for text/image processing |
| src/NeuralNetworks/ClipNeuralNetwork.cs | Core CLIP implementation with ONNX-based encoders and embedding generation |
| src/Tokenization/ClipTokenizerFactory.cs | Factory for creating CLIP-compatible BPE tokenizers with proper special tokens |
| src/Preprocessing/Image/ClipImagePreprocessor.cs | Image preprocessing pipeline with resize, normalization, and format conversion |
| src/Tokenization/Models/SpecialTokens.cs | Adds CLIP-specific special tokens (startoftext, endoftext) |
| src/NeuralNetworks/NeuralNetworkArchitecture.cs | Extends architecture with ImageEmbeddingDim and TextEmbeddingDim properties |
| src/Helpers/LayerHelper.cs | Adds CreateDefaultClipLayers for optional projection heads |
| src/Enums/ModelType.cs | Adds Clip enum value for model type identification |
| tests/.../ClipTokenizerFactoryTests.cs | Comprehensive tests for tokenizer factory (20+ tests) |
| tests/.../ClipImagePreprocessorTests.cs | Tests for image preprocessing pipeline (26+ tests) |
| tests/.../ClipNeuralNetworkTests.cs | Constructor validation and error handling tests (9 tests) |
💡 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}
- Fix tokenizer parameter documentation (required, not optional) - Add validation in DeserializeNetworkSpecificData - Fix ExtractFirstImage to support both NCHW and NHWC formats - Add isCompatible assertion in tokenizer test - Use Enumerable.Repeat instead of Select with unused param - Rename 'r' to descriptive names in tests - Remove unnecessary OrderBy in vocab loading - Simplify tokenizer null check - Add missing encoding options (PaddingSide, TruncationSide, etc.) - Clarify cosine similarity documentation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/NeuralNetworks/ClipNeuralNetwork.cs (5)
400-407: Batch image processing doesn't provide performance benefits.The implementation calls
GetImageEmbeddingindividually for each image, negating batch processing benefits. For true batching, stack images into a single ONNX tensor and run inference once.
332-347: Text batch processing runs inference individually.Similar to images, each text is processed separately in a loop. Consider using true batch inference with a properly batched ONNX tensor for better performance.
212-226: InferenceSession resources may leak if constructor fails partially.If
_textEncodercreation (line 214) fails after_imageEncoderis created (line 213), the image encoder session won't be disposed. Consider wrapping in try-catch with cleanup.🔎 Suggested fix
+ InferenceSession? imageEncoder = null; + try + { // Load ONNX models - _imageEncoder = new InferenceSession(imageEncoderPath); - _textEncoder = new InferenceSession(textEncoderPath); + imageEncoder = new InferenceSession(imageEncoderPath); + _imageEncoder = imageEncoder; + _textEncoder = new InferenceSession(textEncoderPath); + } + catch + { + imageEncoder?.Dispose(); + throw; + }
695-714: Flat tensor indexing may produce incorrect embeddings.
GetValue(startIdx + i)uses flat indexing, but ONNX output tensors have specific shapes like[batch, seq_len, hidden]. For text encoders, you need the CLS token at position[0, 0, :], which requires proper multi-dimensional access.🔎 Suggested fix with proper indexing
private Vector<T> ConvertToVector(OnnxTensors.Tensor<float> onnxTensor) { var result = new Vector<T>(_embeddingDimension); + int rank = onnxTensor.Dimensions.Length; + int vectorSize = Math.Min(_embeddingDimension, + rank > 0 ? onnxTensor.Dimensions[rank - 1] : onnxTensor.Length); - // Get the embedding from the last token (CLS token) for text or pooled output for images - // The output shape is typically [batch, seq_len, hidden] or [batch, hidden] - int startIdx = 0; - if (onnxTensor.Dimensions.Length > 2) - { - // For text encoder: use the embedding at position 0 (CLS token) - startIdx = 0; - } - - for (int i = 0; i < _embeddingDimension && i < onnxTensor.Length; i++) + for (int i = 0; i < vectorSize; i++) { - result[i] = NumOps.FromDouble(onnxTensor.GetValue(startIdx + i)); + float value = rank switch + { + 1 => onnxTensor.GetValue(i), + 2 => onnxTensor[0, i], + 3 => onnxTensor[0, 0, i], // CLS token at seq position 0 + _ => onnxTensor.GetValue(i) + }; + result[i] = NumOps.FromDouble(value); } return result; }
225-225: Virtual method call in constructor.
InitializeLayers()is a virtual method called from the constructor. If a derived class overrides this method, the override will be called before the derived class constructor completes, potentially accessing uninitialized fields.
🧹 Nitpick comments (6)
src/Tokenization/ClipTokenizerFactory.cs (1)
125-136: Consider using.Where()for filtering.The foreach loop filters lines with
StartsWith("#")andIsNullOrWhiteSpacechecks. While functional, using LINQ.Where()would be more idiomatic.🔎 Suggested refactor
- foreach (var line in mergesText) - { - // Skip header line if present - if (line.StartsWith("#") || string.IsNullOrWhiteSpace(line)) - continue; - - var parts = line.Split(' '); - if (parts.Length == 2) - { - merges[(parts[0], parts[1])] = rank++; - } - } + foreach (var line in mergesText.Where(l => !l.StartsWith("#") && !string.IsNullOrWhiteSpace(l))) + { + var parts = line.Split(' '); + if (parts.Length == 2) + { + merges[(parts[0], parts[1])] = rank++; + } + }src/AiDotNet.Serving/Controllers/EmbeddingsController.cs (3)
81-93: Consider using batch encoding for better performance.The loop processes each text individually, but
IServableMultimodalModel<T>providesEncodeTextBatch. Using the batch method would reduce overhead, especially for multiple texts.🔎 Suggested refactor
// Encode texts - var embeddings = new List<double[]>(); - foreach (var text in request.Texts) - { - var embedding = model.EncodeText(text); - embeddings.Add(ConvertToDoubleArray(embedding)); - } + var embeddingMatrix = model.EncodeTextBatch(request.Texts); + var embeddings = new double[request.Texts.Length][]; + for (int i = 0; i < request.Texts.Length; i++) + { + embeddings[i] = ConvertToDoubleArray(embeddingMatrix.GetRow(i)); + }
165-171: Consider using batch encoding for images.Same as text encoding - using
EncodeImageBatchwould improve performance for multiple images.
81-81: Hardcodedfloattype limits model flexibility.All endpoints use
GetMultimodalModel<float>, preventing use of models withdoubleordecimalnumeric types. Consider making this configurable or documenting this limitation.src/NeuralNetworks/ClipModelLoader.cs (2)
306-312: Silent exception swallowing hides configuration issues.The empty
catchblock silently ignores parsing errors, making it difficult to diagnose why auto-detection failed. Consider logging the exception or at least noting in the fallback that config parsing failed.🔎 Suggested improvement
catch { - // Fall through to defaults + // Config parsing failed - fall through to auto-detection defaults + // In production, consider logging this: logger.LogWarning(ex, "Failed to parse config.json") }
44-44: Static HttpClient lacks timeout configuration.The static
HttpClienthas no timeout set, defaulting to 100 seconds. For large ONNX model downloads, this may be insufficient. Consider configuring a longer timeout or making it configurable.🔎 Suggested improvement
- private static readonly HttpClient HttpClient = new HttpClient(); + private static readonly HttpClient HttpClient = new HttpClient + { + Timeout = TimeSpan.FromMinutes(30) // Allow longer timeout for large model downloads + };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/AiDotNet.Serving/Controllers/EmbeddingsController.cssrc/AiDotNet.Serving/Models/IServableMultimodalModel.cssrc/AiDotNet.Serving/Services/IModelRepository.cssrc/AiDotNet.Serving/Services/ModelRepository.cssrc/Interfaces/IMultimodalEmbedding.cssrc/NeuralNetworks/ClipModelLoader.cssrc/NeuralNetworks/ClipNeuralNetwork.cssrc/Preprocessing/Image/ClipImagePreprocessor.cssrc/Tokenization/ClipTokenizerFactory.cstests/AiDotNet.Tests/UnitTests/Preprocessing/ClipImagePreprocessorTests.cstests/AiDotNet.Tests/UnitTests/Tokenization/ClipTokenizerFactoryTests.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/AiDotNet.Tests/UnitTests/Tokenization/ClipTokenizerFactoryTests.cs
- tests/AiDotNet.Tests/UnitTests/Preprocessing/ClipImagePreprocessorTests.cs
- src/Preprocessing/Image/ClipImagePreprocessor.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/AiDotNet.Serving/Services/IModelRepository.cssrc/AiDotNet.Serving/Services/ModelRepository.cssrc/Tokenization/ClipTokenizerFactory.cssrc/Interfaces/IMultimodalEmbedding.cssrc/AiDotNet.Serving/Models/IServableMultimodalModel.cssrc/AiDotNet.Serving/Controllers/EmbeddingsController.cssrc/NeuralNetworks/ClipModelLoader.cssrc/NeuralNetworks/ClipNeuralNetwork.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.Serving/Services/IModelRepository.cssrc/AiDotNet.Serving/Services/ModelRepository.cssrc/Tokenization/ClipTokenizerFactory.cssrc/Interfaces/IMultimodalEmbedding.cssrc/AiDotNet.Serving/Models/IServableMultimodalModel.cssrc/AiDotNet.Serving/Controllers/EmbeddingsController.cssrc/NeuralNetworks/ClipModelLoader.cssrc/NeuralNetworks/ClipNeuralNetwork.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 (6)
src/Interfaces/IMultimodalEmbedding.cs (1)
1-236: Well-designed multimodal embedding interface.The interface provides a clean abstraction for CLIP-style models with comprehensive documentation. The similarity thresholds documentation appropriately notes these are "approximate guidelines" that "may vary by CLIP model variant and domain."
src/AiDotNet.Serving/Services/IModelRepository.cs (1)
71-88: LGTM!The multimodal model methods follow the established patterns from
LoadModel<T>andGetModel<T>, maintaining API consistency.src/AiDotNet.Serving/Services/ModelRepository.cs (1)
191-246: LGTM!The multimodal model implementation correctly mirrors the existing model loading/retrieval patterns with proper validation, type checking, and thread-safe storage.
src/AiDotNet.Serving/Models/IServableMultimodalModel.cs (1)
1-162: LGTM!The interface properly extends
IServableModel<T>for multimodal capabilities. Usingdouble[]for image data in the serving layer is appropriate for REST API serialization, while the coreIMultimodalEmbedding<T>usesTensor<T>for internal operations.src/NeuralNetworks/ClipNeuralNetwork.cs (1)
1-736: Overall implementation is well-structured.Despite the issues noted above (many from previous reviews), the CLIP implementation provides a solid foundation with:
- Proper ONNX session management with disposal
- Comprehensive validation of inputs
- L2 normalization of embeddings
- Zero-shot classification with softmax probabilities
- Detailed documentation
The deserialization validation was correctly addressed per previous feedback.
src/Tokenization/ClipTokenizerFactory.cs (1)
114-118: Verify vocabulary ID preservation during token loading.The vocabulary dictionary maps tokens to their specific IDs from the pretrained CLIP model, but the current code only passes the token string to
AddToken(kvp.Key). Confirm whetherVocabulary.AddTokenpreserves the ID fromvocabDictor auto-assigns sequential IDs. If IDs are auto-assigned, the tokenizer will produce different token IDs than the pretrained model expects, resulting in incorrect embeddings.
- Fix ONNX tensor extraction to handle multi-dimensional tensors correctly - Add proper InferenceSession disposal in constructor failure scenarios - Implement true batched inference for text encoding (batched tensor creation) - Implement true batched inference for image encoding (stacked tensor) - Add ExtractBatchEmbeddings helper for processing batched ONNX outputs - Use discards for binary format compatibility reads in deserialization - Use LINQ Where/Select for cleaner merge file parsing - Stream large file downloads to avoid memory issues with large ONNX models - Fix explicit floor semantics in bilinear interpolation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Convert if-else shape assignment to ternary expression - Combine nested if statements in model loader error handling 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The simple tokenizer with a small training corpus may not reach 1000 vocabulary tokens, which is required for CLIP compatibility. Updated test to handle both cases appropriately. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add ValidateAndCombinePath helper for path traversal attack prevention - Add SanitizeModelIdForPath helper to validate model IDs - Update all Path.Combine calls to use validation helpers - Replace generic catch clauses with specific exception types: - JsonException for invalid JSON parsing - IOException for file read errors - FileNotFoundException for missing files - Add IsMultimodal property to ModelEntry - Add IModelRepository multimodal interface methods to test fakes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Replace direct Path.Combine with SanitizeModelIdForPath + ValidateAndCombinePath - Add try-catch for InvalidOperationException and ArgumentException - Return false for invalid paths instead of throwing
- Convert foreach loop in GetTextEmbeddings to Select().ToList() - Use ternary operators in ValidateImageShape instead of if-else - Replace nested foreach with LINQ Any() in FindOnnxFile
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Tokenization/ClipTokenizerFactory.cs (1)
126-129: Consider making the filtering more explicit.The LINQ chain filters merges lines but uses implicit filtering. As noted in past reviews, explicitly using
.Where()would make the intent clearer.🔎 Proposed explicit filtering
-var validLines = mergesText - .Where(line => !line.StartsWith("#") && !string.IsNullOrWhiteSpace(line)) - .Select(line => line.Split(' ')) - .Where(parts => parts.Length == 2); +var validLines = mergesText + .Where(line => !line.StartsWith("#") && !string.IsNullOrWhiteSpace(line)) + .Select(line => line.Split(' ')) + .Where(parts => parts.Length == 2);Actually, the current code already uses
.Where()explicitly, so this past comment appears to have been addressed.
🧹 Nitpick comments (3)
src/Tokenization/ClipTokenizerFactory.cs (2)
109-118: Consider validating vocabulary entries before adding tokens.The vocabulary JSON is deserialized and tokens are added directly without validation. While the null check on line 111 is good, consider validating:
- Token string is not null or empty
- Token IDs are within expected range
- No duplicate tokens
This would make the factory more robust against malformed vocabulary files.
🔎 Proposed validation enhancement
var vocabDict = JsonConvert.DeserializeObject<Dictionary<string, int>>(vocabJson); if (vocabDict == null) throw new InvalidOperationException("Failed to parse vocabulary file."); +// Validate vocabulary entries +var seenIds = new HashSet<int>(); +foreach (var kvp in vocabDict) +{ + if (string.IsNullOrEmpty(kvp.Key)) + throw new InvalidOperationException("Vocabulary contains null or empty token."); + if (kvp.Value < 0) + throw new InvalidOperationException($"Vocabulary contains negative ID for token '{kvp.Key}'."); + if (!seenIds.Add(kvp.Value)) + throw new InvalidOperationException($"Vocabulary contains duplicate ID {kvp.Value}."); +} + var vocabulary = new Vocabulary.Vocabulary("<|endoftext|>"); foreach (var kvp in vocabDict) { vocabulary.AddToken(kvp.Key); }
238-240: Consider tightening the vocabulary size validation.The vocabulary size range of 1000-100000 is very broad and might accept tokenizers that aren't truly CLIP-compatible. Standard CLIP models use exactly 49408 tokens. Consider either:
- Requiring exact match (49408) for production compatibility
- Using a narrower range (e.g., 45000-52000) to allow minor variations while rejecting obviously incompatible tokenizers
The current range would accept test tokenizers created with
CreateSimple()but might also accept incompatible tokenizers.🔎 Proposed stricter validation
// Check vocabulary size is reasonable for CLIP -if (tokenizer.VocabularySize < 1000 || tokenizer.VocabularySize > 100000) +// Allow some tolerance around the standard 49408, but reject clearly incompatible sizes +if (tokenizer.VocabularySize < 45000 || tokenizer.VocabularySize > 52000) return false;src/NeuralNetworks/ClipNeuralNetwork.cs (1)
294-303: Consider using ClipTokenizerFactory.GetDefaultEncodingOptions for consistency.The encoding options are correctly configured with all required settings (MaxLength, Padding, PaddingSide, TruncationSide, AddSpecialTokens, ReturnAttentionMask). However, these duplicate the configuration in
ClipTokenizerFactory.GetDefaultEncodingOptions().Consider using the factory method for consistency and easier maintenance:
🔎 Proposed refactor
-var encodingOptions = new EncodingOptions -{ - MaxLength = _maxSequenceLength, - Truncation = true, - Padding = true, - PaddingSide = "right", - TruncationSide = "right", - AddSpecialTokens = true, - ReturnAttentionMask = true -}; +var encodingOptions = ClipTokenizerFactory.GetDefaultEncodingOptions(_maxSequenceLength);Also applies to: 345-354
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/AiDotNet.Serving/Services/ModelEntry.cssrc/NeuralNetworks/ClipModelLoader.cssrc/NeuralNetworks/ClipNeuralNetwork.cssrc/Preprocessing/Image/ClipImagePreprocessor.cssrc/Tokenization/ClipTokenizerFactory.cstests/AiDotNet.Serving.Tests/Controllers/InferenceControllerTests.cstests/AiDotNet.Serving.Tests/ModelStartupServiceHashVerificationTests.cstests/AiDotNet.Serving.Tests/TestModelRepository.cstests/AiDotNet.Tests/UnitTests/Tokenization/ClipTokenizerFactoryTests.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/AiDotNet.Tests/UnitTests/Tokenization/ClipTokenizerFactoryTests.cs
- src/NeuralNetworks/ClipModelLoader.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/Tokenization/ClipTokenizerFactory.cstests/AiDotNet.Serving.Tests/TestModelRepository.cssrc/AiDotNet.Serving/Services/ModelEntry.cstests/AiDotNet.Serving.Tests/ModelStartupServiceHashVerificationTests.cssrc/Preprocessing/Image/ClipImagePreprocessor.cssrc/NeuralNetworks/ClipNeuralNetwork.cstests/AiDotNet.Serving.Tests/Controllers/InferenceControllerTests.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/Tokenization/ClipTokenizerFactory.cstests/AiDotNet.Serving.Tests/TestModelRepository.cssrc/AiDotNet.Serving/Services/ModelEntry.cstests/AiDotNet.Serving.Tests/ModelStartupServiceHashVerificationTests.cssrc/Preprocessing/Image/ClipImagePreprocessor.cssrc/NeuralNetworks/ClipNeuralNetwork.cstests/AiDotNet.Serving.Tests/Controllers/InferenceControllerTests.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 (16)
src/AiDotNet.Serving/Services/ModelEntry.cs (1)
17-21: LGTM!The
IsMultimodalproperty is well-documented and cleanly integrates multimodal model tracking into the model entry metadata.tests/AiDotNet.Serving.Tests/ModelStartupServiceHashVerificationTests.cs (1)
73-76: LGTM!The multimodal method stubs are consistent with the existing test repository pattern—
LoadMultimodalModelreturns success andGetMultimodalModelreturns null, matching the behavior of their non-multimodal counterparts.tests/AiDotNet.Serving.Tests/TestModelRepository.cs (1)
46-54: LGTM!The multimodal method implementations are consistent with the existing pattern in
TestModelRepository, throwingNotSupportedExceptionwith appropriate messages that match their non-multimodal counterparts.src/Tokenization/ClipTokenizerFactory.cs (2)
162-185: LGTM - Clear testing-only implementation.The method provides a reasonable minimal tokenizer for testing purposes. The documentation appropriately warns users that this is not suitable for production use and recommends using
FromPretrainedinstead. The default corpus and vocabulary size (1000 vs. 49408 for production CLIP) are appropriate for quick testing scenarios.
200-212: LGTM - Correct CLIP encoding configuration.The encoding options correctly configure all necessary parameters for CLIP:
- Max length of 77 tokens (CLIP standard)
- Right-side padding and truncation
- Special tokens and attention mask included
This matches the expected CLIP tokenization behavior.
src/Preprocessing/Image/ClipImagePreprocessor.cs (5)
90-119: LGTM - Proper initialization and validation.The constructor correctly:
- Validates image size is positive
- Sets standard ImageNet normalization values used by CLIP
- Validates that mean and std arrays have exactly 3 values (RGB)
The default values (mean: [0.48145466, 0.4578275, 0.40821073], std: [0.26862954, 0.26130258, 0.27577711]) match OpenAI's CLIP preprocessing standards.
147-184: LGTM - Comprehensive preprocessing pipeline.The
Preprocessmethod correctly handles multiple input formats:
- 2D grayscale images (expanded to 3 channels)
- 3D images in either HWC or CHW format
- 4D batched images (extracts first image)
The pipeline (format normalization → resize → pixel normalization) follows standard CLIP preprocessing steps.
278-344: 4D batch handling properly addresses past review concerns.The updated implementation correctly detects and handles both NCHW and NHWC formats:
- Uses heuristic: if last dimension is 3 or 4, treats as NHWC (lines 297-298)
- Properly extracts first image in detected format
- Calls
NormalizeFormatto ensure channels-first outputThis addresses the past review comment about supporting both formats. The heuristic is pragmatic and works well for typical image data where the channel dimension is 3 (RGB) or 4 (RGBA).
Note: The heuristic could theoretically fail for NCHW images with 3 or 4 channels where height/width also equals 3 or 4 (e.g., 4×3×3 tensor), but such cases are extremely rare in practice.
428-463: LGTM - Explicit floor semantics address past review.The bilinear interpolation now uses explicit
Math.Floor(lines 435, 437) rather than implicit truncation, making the intent clear. The fractional part computation (lines 441-442) correctly subtracts the floored coordinates, and the bilinear interpolation formula is properly implemented.This addresses the past review comment about clarifying the floor semantics.
468-508: LGTM - Robust pixel normalization with adaptive scaling.The normalization correctly:
- Detects whether pixels are in 0-255 or 0-1 range
- Scales to 0-1 if needed
- Applies ImageNet normalization:
(pixel - mean) / stdThe adaptive scaling makes the preprocessor more flexible in handling different input formats.
src/NeuralNetworks/ClipNeuralNetwork.cs (6)
176-247: LGTM - Constructor properly addresses past review concerns.The constructor correctly implements:
- Comprehensive input validation for paths and dimensions
- Required tokenizer with null check (lines 221-222)
- Proper resource cleanup on failure using temp variables and try-catch (lines 213-246)
- Transfer of ownership only after all validation passes (lines 230-233)
The virtual call to
InitializeLayers(line 238) is acknowledged as intentional for the template method pattern in the comment (lines 235-237).All past review concerns about tokenizer validation and disposal have been addressed.
335-411: LGTM - True batched inference properly implemented.The method correctly implements batched inference as requested in past reviews:
- Checks if all tokenized sequences have uniform length (line 360)
- When uniform, creates a single
[batch_size, seq_length]tensor for true batched inference (lines 364-377)- Runs single ONNX call for entire batch (line 383)
- Falls back to sequential processing only when sequence lengths vary (lines 389-410)
This provides significant performance benefits for batched workloads while maintaining correctness.
458-500: LGTM - Efficient batch processing for images.The method implements true batched inference as requested in past reviews:
- Stacks all images into a single
[batch_size, channels, height, width]tensor (lines 473-490)- Performs single ONNX inference call for entire batch (line 496)
- Uses
ExtractBatchEmbeddingshelper to extract individual embeddings (line 499)This provides efficient GPU utilization and significant performance improvements over sequential processing.
689-719: LGTM - Deserialization properly validates and addresses past concerns.The method correctly addresses past review comments about unused variables:
- Uses explicit discard operator
_(lines 698-699) to show intentional reading- Includes clear comment explaining why values are read but not used (lines 695-697)
- Validates that deserialized values match current instance configuration (lines 702-718)
- Throws descriptive exceptions on mismatch
This ensures serialized models can only be loaded into compatible ClipNeuralNetwork instances.
838-879: LGTM - Robust tensor to vector conversion.The method correctly handles multiple ONNX output shapes:
- 2D
[batch, hidden]: extracts first batch item (lines 843-850)- 3D
[batch, seq_len, hidden]: extracts CLS token at position 0 (lines 852-859)- 1D
[hidden]: flat embedding vector (lines 861-867)- Fallback for unexpected shapes with flat indexing (lines 869-876)
The extraction is bounded by
_embeddingDimensionto handle mismatches between model output and configured dimension safely.
892-900: LGTM - Proper resource disposal.The
Disposemethod correctly:
- Uses null-conditional operators to safely dispose ONNX sessions
- Only disposes managed resources when
disposingis true- Calls base class
Disposeto ensure complete cleanupThis follows the standard IDisposable pattern for managed resources.
… path in getdefaultcachedir - Replace foreach loop with requiredFiles.All() in IsModelCached - Use ValidateAndCombinePath in GetDefaultCacheDir for consistency
- Replace foreach loop with LINQ chain ending in ToDictionary - Use Select with index parameter to track rank
- Fix ExtractBatchEmbeddings to properly handle 1D tensors - Throw InvalidOperationException for unexpected tensor shapes - Fix FakeModelRepository.GetMultimodalModel to throw NotSupportedException
| baseFullPath += Path.DirectorySeparatorChar; | ||
| } | ||
|
|
||
| string combinedPath = Path.Combine(baseDirectory, relativePath); |
Check notice
Code scanning / CodeQL
Call to 'System.IO.Path.Combine' may silently drop its earlier arguments Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 months ago
In general, to fix this issue, ensure that path combination in security‑sensitive code never depends on Path.Combine behavior when subsequent segments might be absolute. Either (a) reject absolute relativePath values before combining, or (b) use Path.Join, which concatenates segments and does not discard earlier ones.
For this specific method, the best fix is:
- Strengthen validation to reject any absolute
relativePathusingPath.IsPathRooted, which correctly handles Windows drive letters and UNC paths, rather than only checking for leading/or\. - Replace
Path.Combine(baseDirectory, relativePath)withPath.Join(baseDirectory, relativePath).Path.Joinhas the same basic signature and returns a path using the directory separator, but it will not throw awaybaseDirectoryifrelativePathis absolute (it simply concatenates the strings). Because we still callPath.GetFullPathand enforce theStartsWith(baseFullPath, ...)check, the path traversal protection continues to work as intended. - Keep the rest of the logic unchanged to avoid altering observable behavior apart from correctly handling malicious or unexpected absolute paths.
All changes are confined to ValidateAndCombinePath in src/NeuralNetworks/ClipModelLoader.cs (around lines 55–72). No new methods or imports are needed, since Path.IsPathRooted and Path.Join are in System.IO, which is already imported.
| @@ -55,8 +55,10 @@ | ||
| private static string ValidateAndCombinePath(string baseDirectory, string relativePath) | ||
| { | ||
| // Reject obviously malicious patterns early | ||
| if (relativePath.Contains("..") || relativePath.StartsWith("/", StringComparison.Ordinal) || | ||
| relativePath.StartsWith("\\", StringComparison.Ordinal)) | ||
| if (relativePath.Contains("..") || | ||
| relativePath.StartsWith("/", StringComparison.Ordinal) || | ||
| relativePath.StartsWith("\\", StringComparison.Ordinal) || | ||
| Path.IsPathRooted(relativePath)) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"Invalid path component detected: '{relativePath}'. Path traversal is not allowed."); | ||
| @@ -68,7 +70,7 @@ | ||
| baseFullPath += Path.DirectorySeparatorChar; | ||
| } | ||
|
|
||
| string combinedPath = Path.Combine(baseDirectory, relativePath); | ||
| string combinedPath = Path.Join(baseDirectory, relativePath); | ||
| string combinedFullPath = Path.GetFullPath(combinedPath); | ||
|
|
||
| if (!combinedFullPath.StartsWith(baseFullPath, StringComparison.OrdinalIgnoreCase)) |
| foreach (var layer in Layers) | ||
| { | ||
| var layerParams = layer.GetParameters(); | ||
| for (int i = 0; i < layerParams.Length; i++) | ||
| { | ||
| parameters[index++] = layerParams[i]; | ||
| } | ||
| } |
Check notice
Code scanning / CodeQL
Missed opportunity to use Select Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 months ago
To fix the issue, we should explicitly express that we are iterating over the sequence of parameter vectors derived from Layers, rather than over Layers themselves. This is done by using LINQ’s Select to map each layer to layer.GetParameters() in the foreach header, removing the intermediate layerParams declaration inside the loop.
Concretely, in ClipNeuralNetwork<T>.GetParameters() in src/NeuralNetworks/ClipNeuralNetwork.cs, replace:
foreach (var layer in Layers)and the first line in the bodyvar layerParams = layer.GetParameters();
with:
foreach (var layerParams in Layers.Select(layer => layer.GetParameters()))
leaving the inner for loop unchanged. We keep all existing logic, indices, and ordering of parameters. LINQ’s Select is available via System.Linq; if this file does not already have using System.Linq; (not shown in the snippet), we need to add that using directive at the top of the file. No other methods or definitions are required.
| @@ -1337,9 +1337,8 @@ | ||
| } | ||
|
|
||
| // Get layer parameters | ||
| foreach (var layer in Layers) | ||
| foreach (var layerParams in Layers.Select(layer => layer.GetParameters())) | ||
| { | ||
| var layerParams = layer.GetParameters(); | ||
| for (int i = 0; i < layerParams.Length; i++) | ||
| { | ||
| parameters[index++] = layerParams[i]; |
Adds BLIP (Bootstrapped Language-Image Pre-training) support extending CLIP: - Add IBlipModel interface for BLIP-specific operations (captioning, VQA, ITM) - Add BlipNeuralNetwork with native and ONNX mode support - Add Blip to ModelType enum - Add BlipNeuralNetworkTests for ONNX path validation BLIP extends CLIP with additional capabilities: - Image captioning (generate text descriptions) - Visual question answering (VQA) - Image-text matching (ITM) with cross-attention - Multiple caption generation with nucleus sampling 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
… networks - Add ILLaVAModel interface for visual instruction following - Add IFlamingoModel interface for few-shot visual learning - Add IVideoCLIPModel interface for video-text alignment - Add IImageBindModel interface for 7-modality embedding binding - Implement LLaVANeuralNetwork with vision encoder + LLM architecture - Implement FlamingoNeuralNetwork with gated cross-attention and perceiver - Implement VideoCLIPNeuralNetwork with temporal transformer aggregation - Implement ImageBindNeuralNetwork supporting image/text/audio/video/thermal/depth/IMU - Add ModelType enum values for all new model types - Include ONNX and native mode support for all implementations 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add comprehensive multimodal interfaces and implementations: Interfaces: - IGpt4VisionModel: GPT-4V-equivalent vision-language model - IDallE3Model: DALL-E 3 text-to-image generation - IAudioVisualModel: Audio-visual correspondence and event localization - IUnifiedMultimodalModel: Unified multimodal model interface Implementations: - Gpt4VisionNeuralNetwork: Vision-language understanding - DallE3Model: Text-to-image generation with diffusion - AudioVisualCorrespondenceNetwork: Sound-source localization - AudioVisualEventLocalizationNetwork: Temporal event detection - UnifiedMultimodalNetwork: Multi-modal encoding and generation Enhancements: - VideoCLIP: Autoregressive caption generation with nucleus sampling - VideoCLIP: Generative question answering with constrained decoding Closes #405 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (14)
src/NeuralNetworks/AudioVisualEventLocalizationNetwork.cs-105-119 (1)
105-119: Null-forgiving assignments followed byInitializeLayers()is fragile.If
InitializeLayers()throws or is overridden incorrectly, the fields remain null and will causeNullReferenceExceptionon any method call. Consider using a factory pattern or validating initialization.🔎 Suggested defensive approach
After
InitializeLayers(), add validation:InitializeLayers(); // Validate all layers were initialized if (_audioInputProjection == null || _audioEncoderLayers == null || _visualInputProjection == null || /* ... other fields */) { throw new InvalidOperationException("Layer initialization failed."); }src/NeuralNetworks/AudioVisualEventLocalizationNetwork.cs-1128-1153 (1)
1128-1153:Softmaxmay throw on empty input.Accessing
logits[0]on line 1131 will throwIndexOutOfRangeExceptioniflogits.Length == 0.🔎 Suggested fix
private Vector<T> Softmax(Vector<T> logits) { + if (logits.Length == 0) + { + return new Vector<T>(0); + } + var result = new Vector<T>(logits.Length); T maxVal = logits[0];src/NeuralNetworks/AudioVisualCorrespondenceNetwork.cs-710-711 (1)
710-711: Unused variablesheightandwidthinComputeSoundSourceAttention.The variables
heightandwidthare extracted fromoriginalShapebut never used in the subsequent logic. This suggests incomplete implementation or dead code.- var height = originalShape.Length >= 2 ? originalShape[^2] : 1; - var width = originalShape.Length >= 1 ? originalShape[^1] : numPositions; var patchH = (int)Math.Sqrt(numPositions); var patchW = patchH;src/NeuralNetworks/AudioVisualEventLocalizationNetwork.cs-1054-1055 (1)
1054-1055: Potential division by zero inComputeVisualActivity.If
frames[0].ToVector().Lengthis zero, the division will fail. Add a guard.🔎 Suggested fix
- return _numOps.Divide(totalDiff, _numOps.FromDouble(frames.Count * frames[0].ToVector().Length)); + int totalElements = frames.Count * frames[0].ToVector().Length; + return totalElements > 0 + ? _numOps.Divide(totalDiff, _numOps.FromDouble(totalElements)) + : _numOps.Zero;src/NeuralNetworks/AudioVisualEventLocalizationNetwork.cs-927-928 (1)
927-928: Hardcoded audio sample rate assumption.The
ExtractSegmentmethod assumesaudioSampleRate = 16000without using any configuration. This should use a class-level constant or configuration parameter for consistency.🔎 Suggested fix
Add a constant at the class level (there's no existing sample rate field):
private const int DEFAULT_EMBEDDING_DIM = 512; private const int SPECTROGRAM_BINS = 128; private const double DEFAULT_TEMPORAL_RESOLUTION = 0.1; private const int DEFAULT_NUM_CATEGORIES = 100; +private const int AUDIO_SAMPLE_RATE = 16000;Then use it in
ExtractSegment:- int audioSampleRate = 16000; // Assume 16kHz + int audioSampleRate = AUDIO_SAMPLE_RATE;src/NeuralNetworks/AudioVisualEventLocalizationNetwork.cs-267-268 (1)
267-268: Potential division by zero inComputeSpectrogram.If
end - startequals zero (whenend == start),_numOps.Divide(sum, _numOps.FromDouble(end - start))will produce division by zero or NaN.🔎 Suggested fix
- spectrogramData[i] = _numOps.Sqrt(_numOps.Divide(sum, _numOps.FromDouble(end - start))); + int count = end - start; + spectrogramData[i] = count > 0 + ? _numOps.Sqrt(_numOps.Divide(sum, _numOps.FromDouble(count))) + : _numOps.Zero;src/NeuralNetworks/AudioVisualCorrespondenceNetwork.cs-609-657 (1)
609-657:FlattenToPatcheshas potential index calculation issues.The patch dimension is hardcoded to 768 regardless of actual
patchDimcalculation on line 624. Also, ifnumPatchesHornumPatchesWis 0 (when image dimensions are smaller than patch size), the method returns an empty tensor which could cause issues downstream.🔎 Suggested bounds check
private Tensor<T> FlattenToPatches(Tensor<T> frame) { if (frame.Shape.Length < 3) { return frame; } var channels = frame.Shape[^3]; var height = frame.Shape[^2]; var width = frame.Shape[^1]; const int patchSize = 16; var numPatchesH = height / patchSize; var numPatchesW = width / patchSize; + + if (numPatchesH == 0 || numPatchesW == 0) + { + // Handle images smaller than patch size + return new Tensor<T>([1, 768]); + } + var numPatches = numPatchesH * numPatchesW;src/NeuralNetworks/AudioVisualCorrespondenceNetwork.cs-212-231 (1)
212-231: Sinusoidal embedding formula has incorrect alternation pattern.The standard sinusoidal positional encoding alternates sin/cos based on the dimension index where even indices use sin and odd use cos. However, the division term should use
i / 2pairs, not justi. This affects the frequency distribution.🔎 Standard positional encoding formula
private void InitializeSinusoidalEmbedding(Matrix<T> embedding, int maxLength) { for (int pos = 0; pos < maxLength; pos++) { for (int i = 0; i < _embeddingDimension; i++) { - var divTerm = Math.Exp(i * -Math.Log(10000.0) / _embeddingDimension); + var divTerm = Math.Exp((i / 2) * 2 * -Math.Log(10000.0) / _embeddingDimension); var angle = pos * divTerm; if (i % 2 == 0) { embedding[pos, i] = NumOps.FromDouble(Math.Sin(angle)); } else { embedding[pos, i] = NumOps.FromDouble(Math.Cos(angle)); } } } }src/NeuralNetworks/LLaVANeuralNetwork.cs-1162-1183 (1)
1162-1183: Minor: metadata key typo (NumLMayers)In
GetModelMetadata(Lines 1162–1183), the additional info dictionary uses key"NumLMayers"(Line 1178). This looks like a typo for"NumLmLayers"and may confuse downstream tooling or consumers expecting a consistent field name.Recommend correcting the key name before this surface is relied on externally.
src/Diffusion/Models/DallE3Model.cs-254-293 (1)
254-293: Helpers assume unbatched[C,H,W]layout but accept more general shapes
Upscale(Lines 255–293),BilinearUpscale(Lines 545–594), andCreateExpandedCanvas(Lines 596–624) all:
- Derive
channels,height,widthusingshape[^3],shape[^2],shape[^1].- Then index the underlying
Tensor<T>.Dataas if the tensor were laid out exactly as[C,H,W]with linear indexc * H * W + y * W + x.If a caller passes a 4D tensor in
[N,C,H,W]format (common in diffusion pipelines), these index calculations will ignore the batch dimension and produce incorrect results.Either:
- Restrict these helpers to strictly 3D images (e.g.,
if (shape.Length != 3) throw ...;), or- Update the indexing to account for an optional batch dimension.
Right now the API surface suggests support for any
shape.Length >= 3, but the implementation only really handles unbatched images.Also applies to: 545-594, 596-644
src/NeuralNetworks/BlipNeuralNetwork.cs-937-942 (1)
937-942: Weak random seed source.Using
DateTime.Now.Millisecondas a seed only provides 1000 possible values (0-999), leading to potential repetition when called multiple times within the same second. Consider using a more robust approach.🔎 Suggested improvement
- var random = Tensors.Helpers.RandomHelper.CreateSeededRandom(DateTime.Now.Millisecond); + var random = Tensors.Helpers.RandomHelper.CreateSeededRandom(Environment.TickCount ^ Guid.NewGuid().GetHashCode());Or use
Random.Sharedif thread-safety is handled appropriately:var random = Random.Shared;src/NeuralNetworks/Gpt4VisionNeuralNetwork.cs-1265-1280 (1)
1265-1280: New Random instance created on each call.Creating
new Random()on each invocation can produce identical sequences when called in rapid succession (same seed from system clock). UseRandom.Sharedor a class-level instance.🔎 Suggested fix
private int SampleFromDistribution(Vector<T> probs) { - double random = new Random().NextDouble(); + double random = Random.Shared.NextDouble(); double cumulative = 0; for (int i = 0; i < probs.Length; i++)src/NeuralNetworks/Gpt4VisionNeuralNetwork.cs-1425-1435 (1)
1425-1435: UpdateParameters validates but doesn't update.The method validates parameter count but the implementation comment (lines 1433-1434) indicates no actual parameter distribution occurs. This creates a silent no-op that could confuse callers.
🔎 Suggested fix
public override void UpdateParameters(Vector<T> parameters) { int expectedCount = ParameterCount; if (parameters.Length != expectedCount) { throw new ArgumentException($"Expected {expectedCount} parameters but got {parameters.Length}"); } - // Update parameters for all layers - // Implementation would distribute parameters to each layer + if (!_useNativeMode || expectedCount == 0) + { + return; // No parameters to update in ONNX mode + } + + // TODO: Distribute parameters to each native layer + throw new NotImplementedException("Parameter updates not yet implemented for native mode."); }Committable suggestion skipped: line range outside the PR's diff.
src/NeuralNetworks/BlipNeuralNetwork.cs-639-651 (1)
639-651: Clarify the terminology for the scaling parameter.The comment
// Temperatureon line 644 is misleading. The code applies100.0as a multiplier to scaled similarities before softmax, which is a logit_scale (inverse temperature), not temperature itself. CLIP uses similar scaling (logit_scale.exp() factor typically initialized around 14.3 for default 0.07 temperature). Consider renaming the comment to clarify this is a logit scale or inverse temperature parameter.
🧹 Nitpick comments (29)
src/Interfaces/IUnifiedMultimodalModel.cs (4)
33-41: Consider adding null validation for input parameters.The
FromTextmethod doesn't validate thattextis non-null. If a null value is passed, it will silently create an input withTextContent = null, which may cause issues downstream when processing. The same concern applies to theVector<T>parameters inFromImage,FromAudio, andFromVideo.🔎 Proposed fix
public static MultimodalInput<T> FromText(string text, int sequenceIndex = 0) { + ArgumentNullException.ThrowIfNull(text); return new MultimodalInput<T> { Modality = ModalityType.Text, TextContent = text, SequenceIndex = sequenceIndex }; }
79-80: Minor: Consider usingMath.Minfor consistency with other factory methods.While the tensor shape
[1, samples.Length]guarantees matching lengths here, usingMath.Minwould maintain consistency withFromImage(line 60) andFromVideo(line 104).
427-431: Consider using an enum forfusionStrategyinstead of a magic string.The
fusionStrategyparameter accepts string values ("early", "late", "attention", "hybrid"), which is less type-safe and discoverable than an enum. Typos would only be caught at runtime.🔎 Proposed approach
public enum FusionStrategy { Early, Late, Attention, Hybrid } // Then update the method signature: Vector<T> Fuse( IEnumerable<MultimodalInput<T>> inputs, FusionStrategy fusionStrategy = FusionStrategy.Attention);
207-232: Consider whether this interface could benefit from segregation.This interface defines 25+ members covering encoding, generation, retrieval, reasoning, safety checks, and more. While this represents a unified multimodal model design, the breadth may make it challenging to implement and test. Consider whether splitting into smaller, composable interfaces (e.g.,
IMultimodalEncoder<T>,IMultimodalGenerator<T>,IMultimodalRetriever<T>) would improve maintainability without sacrificing the unified model concept.This is a design consideration for the future—the current approach is valid if all implementations truly need all capabilities.
src/Interfaces/IAudioVisualModel.cs (3)
8-27: Consider using a record or enum forModality.The
Modalityproperty uses a magic string ("both") with no validation. Consider using an enum to prevent invalid values and improve type safety.🔎 Suggested enum-based approach
public enum EventModality { Audio, Visual, Both }Then change line 23:
- public string Modality { get; set; } = "both"; + public EventModality Modality { get; set; } = EventModality.Both;
107-109: Potential multiple enumeration ofIEnumerable<Tensor<T>> frames.Methods like
LocalizeSoundSourceandRetrieveAudioFromVisualsacceptIEnumerable<Tensor<T>>which may be enumerated multiple times in implementations. Consider documenting this expectation or usingIReadOnlyList<Tensor<T>>to ensure safe multiple enumeration.Also applies to: 140-143
236-239: Consider adding cancellation token support for long-running operations.Methods like
DetectEventsandDetectSpecificEventscould process large video streams. AddingCancellationTokenparameters would allow callers to cancel lengthy operations gracefully.Also applies to: 249-253
src/NeuralNetworks/AudioVisualCorrespondenceNetwork.cs (2)
512-535: Spectrogram computation is overly simplified and may produce unexpected results.The current implementation doesn't perform actual FFT-based spectrogram computation. It uses a simplified approximation that may not accurately represent audio features. The comment says "Simplified spectrogram computation" which is good for documentation, but consider adding a TODO or warning if this is intended to be replaced.
880-907:ReconstructAudiouses non-standard approximation of Griffin-Lim.The phase reconstruction using
Math.Sin(2.0 * Math.PI * i / SPECTROGRAM_HOP)is a placeholder, not actual Griffin-Lim iteration. The comment acknowledges this ("Griffin-Lim style reconstruction approximation"), but the output quality will be poor. Consider documenting this limitation prominently.src/Enums/ModelType.cs (1)
1025-1026: Add XML documentation forClipenum member.The
Clipenum member lacks XML documentation, unlike the other new multimodal model entries (Blip, Blip2, LLaVA, etc.) which have summary/remarks blocks. For consistency, consider adding documentation.🔎 Suggested documentation
+ /// <summary> + /// CLIP (Contrastive Language-Image Pre-training) for image-text embeddings. + /// </summary> + /// <remarks> + /// <para> + /// <b>For Beginners:</b> CLIP learns to match images with text descriptions. + /// It can perform zero-shot image classification and image-text similarity scoring. + /// </para> + /// </remarks> Clip,src/Interfaces/ILLaVAModel.cs (1)
1-2: Redundant using directive.Per project global usings,
AiDotNet.LinearAlgebratypes are already available. This import is redundant but harmless.Based on learnings, global using directives in
AiDotNet.csprojprovideTensor<T>andVector<T>types.src/Interfaces/IGpt4VisionModel.cs (1)
1-2: Redundant using directive.Same as other interface files -
AiDotNet.LinearAlgebrais covered by global usings.src/Interfaces/IBlipModel.cs (1)
1-2: Redundant using directive.Same as other interface files - covered by global usings.
src/Interfaces/IFlamingoModel.cs (1)
1-2: Redundant using directive.Same as other interface files - covered by global usings.
src/Interfaces/IBlip2Model.cs (1)
1-2: Redundant using directive.Same as other interface files - covered by global usings.
src/Interfaces/IImageBindModel.cs (2)
1-2: Redundant using directive.Same as other interface files - covered by global usings.
154-160: Type safety concern withobjectparameter.The
GetEmbeddingmethod usesobject datawhich loses compile-time type safety. While this provides flexibility for handling all 7 modalities uniformly, callers must ensure correct type matching at runtime. The same pattern appears inZeroShotClassify,FindBestMatch,GenerateDescriptions, andComputeAlignment.Consider documenting the expected types for each modality in the remarks, or accepting this as a reasonable tradeoff for API simplicity.
src/Interfaces/IVideoCLIPModel.cs (1)
1-2: Redundant using directive.Same as other interface files - covered by global usings.
src/Interfaces/IDallE3Model.cs (1)
188-247: Stringly-typed controls could be tightened to avoid runtime mistakes
textPlacement(Line 193) anddirection(Line 238) are documented as taking a small closed set of values, but are exposed as rawstring. That makes typos and unsupported values silent until runtime.If you plan to use these APIs broadly, consider switching them (and similar arguments like
useCase/artisticStyle) to enums or at least validating against known values in implementations to fail fast on invalid input.src/NeuralNetworks/Blip2NeuralNetwork.cs (1)
1058-1125: Custom cross-attention implementation ignoresCrossAttentionLayerparameters
ApplyCrossAttention(Lines 1058–1125) manually computes scaled dot‑product attention betweenqueriesandkeyValuesand never callscrossAttention.Forward(...)or uses any of that layer’s learned weights.As a result:
_qformerCrossAttentionLayerscontribute toParameterCountand are updated inUpdateParameters, but their parameters are never used in the forward pass.- The actual cross‑attention behavior is fully hard‑coded here, regardless of training.
If you intend to use the trainable
CrossAttentionLayer<T>implementation, you should either:
- Route the computation through
crossAttention.Forward(queries, keyValues)(or similar interface), or- Remove the unused layer instances and treat this function as the canonical, fixed attention mechanism.
Right now this is highly misleading for anyone trying to train or fine‑tune the model.
src/NeuralNetworks/FlamingoNeuralNetwork.cs (1)
1000-1048: ParameterCount / GetParameters surface is asymmetric
ParameterCount(Lines 1001–1021) includes:
- All
_visionEncoderLayers,_perceiverLayers,_gatedCrossAttentionLayers,_languageModelLayers_patchEmbedding,_textTokenEmbedding,_outputProjectionGetParameters(Lines 1025–1047) only aggregates parameters from_perceiverLayersand_gatedCrossAttentionLayers.This asymmetry means:
ParameterCountdoes not equalGetParameters().Length.- External callers cannot reliably use
ParameterCount+GetParametersto snapshot all trainable parameters.UpdateParameters(Lines 1102–1135) expects a vector long enough to update all perceiver and gated cross‑attention layers, but cannot reconstruct or update other components.If the intention is to only train perceiver + gated cross‑attention while keeping everything else frozen, it would be clearer to:
- Restrict
ParameterCountandGetParametersto just those “trainable” components, and- Potentially rename or document that these refer to “trainable parameter subset” rather than full network parameters.
Right now the API shape is misleading and easy to misuse.
Also applies to: 1101-1135
src/NeuralNetworks/BlipNeuralNetwork.cs (3)
552-590: Consider batch processing optimization for performance.Both
GetTextEmbeddingsandGetImageEmbeddingsprocess items sequentially. For production workloads with many items, consider implementing true batched inference to reduce overhead from repeated ONNX session calls or layer forward passes.
1260-1287: ONNX caption/QA methods return placeholders without throwing.These methods silently return placeholder strings instead of indicating the operation isn't supported. Consider throwing
NotSupportedExceptionor at minimum documenting this limitation in the XML docs.🔎 Suggested improvement
private string GenerateCaptionOnnx(Tensor<T> image, int maxLength, int numBeams) { - // Simplified implementation - actual ONNX captioning would need the decoder model - return "[Caption generation requires ONNX decoder model]"; + throw new NotSupportedException( + "Caption generation in ONNX mode requires a separate decoder model. " + + "Use native mode or provide an ONNX decoder model."); } private string AnswerQuestionOnnx(Tensor<T> image, string question, int maxLength) { - // Simplified implementation - return "[VQA requires ONNX decoder model]"; + throw new NotSupportedException( + "Visual question answering in ONNX mode requires a separate decoder model. " + + "Use native mode or provide an ONNX decoder model."); }
1498-1514: Redundant gradient computation.The loss derivative is computed twice: once on line 1500 and again on line 1510. Reuse the previously computed
lossGradientfor the vision encoder backward pass.🔎 Suggested fix
// Backward pass through text encoder foreach (var layer in _textEncoderLayers.AsEnumerable().Reverse()) { gradient = layer.Backward(gradient); } // Backward pass through vision encoder - var visionGradient = Tensor<T>.FromVector(LossFunction.CalculateDerivative(imageOutput.ToVector(), textOutput.ToVector())); + var visionGradient = Tensor<T>.FromVector(lossGradient); foreach (var layer in _visionEncoderLayers.AsEnumerable().Reverse()) { visionGradient = layer.Backward(visionGradient); }src/NeuralNetworks/Gpt4VisionNeuralNetwork.cs (5)
796-804: Fragile safety check response parsing.The detection logic (lines 799-800) checks if the category name AND "YES"/"FLAGGED" appear anywhere in the response. This can produce false positives if "YES" appears in an unrelated context or if the response discusses multiple categories together.
Consider parsing line-by-line to associate each category with its specific flag status, similar to the approach used in
AnalyzeChart.
568-605: Hardcoded confidence value and coordinate parsing mismatch.
- All detected objects receive a fixed confidence of
0.8(line 598), which doesn't reflect actual model certainty.- The prompt requests "percentages" but the parsing expects integers, which could fail for responses like "50.5, 30.2, 20, 15".
These are acceptable limitations for a text-based detection approach, but consider documenting them.
608-613: Fixed confidence value in VQA response.Similar to object detection,
AnswerVisualQuestionreturns a hardcoded confidence of0.85. This is an acceptable limitation of the text-generation approach but should be documented.
1485-1500: Deserialized values are discarded without validation.Unlike
BlipNeuralNetwork, this method reads serialized values but discards them without validating they match the current instance's configuration. This could lead to silent mismatches if a serialized model is loaded into an incompatible instance.🔎 Add validation like BlipNeuralNetwork
protected override void DeserializeNetworkSpecificData(BinaryReader reader) { - _ = reader.ReadInt32(); // embeddingDim - _ = reader.ReadInt32(); // visionEmbeddingDim + int embeddingDim = reader.ReadInt32(); + int visionEmbeddingDim = reader.ReadInt32(); _ = reader.ReadInt32(); // maxSeqLen // ... other reads ... + + if (embeddingDim != _embeddingDimension) + { + throw new InvalidOperationException( + $"Loaded embedding dimension ({embeddingDim}) doesn't match current ({_embeddingDimension})."); + } + + if (visionEmbeddingDim != _visionEmbeddingDim) + { + throw new InvalidOperationException( + $"Loaded vision embedding dimension ({visionEmbeddingDim}) doesn't match current ({_visionEmbeddingDim})."); + } }
910-916: Silent fallback to zero encoding when ONNX session missing.When
_visionEncoderis null, the method returns a zero-filled matrix without any indication of failure. This could mask configuration issues. Consider logging a warning or throwing an exception.🔎 Suggested improvement
private Matrix<T> EncodeImageOnnx(Tensor<T> image) { if (_visionEncoder is null) { - // Return dummy encoding - return Matrix<T>.CreateDefault((_imageSize / _patchSize) * (_imageSize / _patchSize) + 1, _visionEmbeddingDim, NumOps.Zero); + throw new InvalidOperationException( + "Vision encoder ONNX session not initialized. " + + "Ensure the ONNX model path is valid and the file exists."); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
src/Diffusion/Models/DallE3Model.cssrc/Enums/ModelType.cssrc/Interfaces/IAudioVisualModel.cssrc/Interfaces/IBlip2Model.cssrc/Interfaces/IBlipModel.cssrc/Interfaces/IDallE3Model.cssrc/Interfaces/IFlamingoModel.cssrc/Interfaces/IGpt4VisionModel.cssrc/Interfaces/IImageBindModel.cssrc/Interfaces/ILLaVAModel.cssrc/Interfaces/IUnifiedMultimodalModel.cssrc/Interfaces/IVideoCLIPModel.cssrc/NeuralNetworks/AudioVisualCorrespondenceNetwork.cssrc/NeuralNetworks/AudioVisualEventLocalizationNetwork.cssrc/NeuralNetworks/Blip2NeuralNetwork.cssrc/NeuralNetworks/BlipNeuralNetwork.cssrc/NeuralNetworks/FlamingoNeuralNetwork.cssrc/NeuralNetworks/Gpt4VisionNeuralNetwork.cssrc/NeuralNetworks/ImageBindNeuralNetwork.cssrc/NeuralNetworks/LLaVANeuralNetwork.cssrc/NeuralNetworks/UnifiedMultimodalNetwork.cssrc/NeuralNetworks/VideoCLIPNeuralNetwork.cstests/AiDotNet.Tests/UnitTests/NeuralNetworks/Blip2NeuralNetworkTests.cstests/AiDotNet.Tests/UnitTests/NeuralNetworks/BlipNeuralNetworkTests.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/Interfaces/IBlipModel.cssrc/Interfaces/ILLaVAModel.cssrc/Interfaces/IVideoCLIPModel.cssrc/Interfaces/IGpt4VisionModel.cssrc/Interfaces/IImageBindModel.cssrc/Interfaces/IBlip2Model.cssrc/NeuralNetworks/AudioVisualCorrespondenceNetwork.cssrc/NeuralNetworks/Blip2NeuralNetwork.cssrc/Enums/ModelType.cssrc/Diffusion/Models/DallE3Model.cssrc/NeuralNetworks/BlipNeuralNetwork.cssrc/Interfaces/IDallE3Model.cssrc/Interfaces/IFlamingoModel.cssrc/NeuralNetworks/FlamingoNeuralNetwork.cssrc/NeuralNetworks/ImageBindNeuralNetwork.cssrc/NeuralNetworks/AudioVisualEventLocalizationNetwork.cssrc/Interfaces/IAudioVisualModel.cssrc/NeuralNetworks/LLaVANeuralNetwork.cssrc/Interfaces/IUnifiedMultimodalModel.cssrc/NeuralNetworks/Gpt4VisionNeuralNetwork.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/Interfaces/IBlipModel.cssrc/Interfaces/ILLaVAModel.cssrc/Interfaces/IVideoCLIPModel.cssrc/Interfaces/IGpt4VisionModel.cssrc/Interfaces/IImageBindModel.cssrc/Interfaces/IBlip2Model.cssrc/NeuralNetworks/AudioVisualCorrespondenceNetwork.cssrc/NeuralNetworks/Blip2NeuralNetwork.cssrc/Enums/ModelType.cssrc/Diffusion/Models/DallE3Model.cssrc/NeuralNetworks/BlipNeuralNetwork.cssrc/Interfaces/IDallE3Model.cssrc/Interfaces/IFlamingoModel.cssrc/NeuralNetworks/FlamingoNeuralNetwork.cssrc/NeuralNetworks/ImageBindNeuralNetwork.cssrc/NeuralNetworks/AudioVisualEventLocalizationNetwork.cssrc/Interfaces/IAudioVisualModel.cssrc/NeuralNetworks/LLaVANeuralNetwork.cssrc/Interfaces/IUnifiedMultimodalModel.cssrc/NeuralNetworks/Gpt4VisionNeuralNetwork.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/Interfaces/IBlip2Model.cssrc/Interfaces/IFlamingoModel.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 (20)
src/Interfaces/IUnifiedMultimodalModel.cs (2)
113-179: LGTM!The
MultimodalOutput<T>class is well-designed with appropriate defensive copying viaClone()in getter methods and proper shape validation in dimension accessors.
491-543: LGTM!The
IAutoregressiveMultimodalModel<T>interface is a well-focused extension that adds appropriate token-level generation capabilities (vocabulary management, tokenization, next-token prediction) on top of the unified multimodal base.src/Interfaces/IAudioVisualModel.cs (2)
54-91: Interface design looks good with clear documentation.The interface exposes appropriate properties for embedding dimension and media characteristics. The use of
IEnumerable<T>for frame sequences is flexible, though implementers should be aware of potential multiple enumeration.
217-228: Interface properties are well-defined.The
TemporalResolutionandSupportedEventCategoriesproperties provide clear contracts for implementations. UsingIReadOnlyList<string>for categories is the right choice for immutability.src/NeuralNetworks/AudioVisualCorrespondenceNetwork.cs (1)
946-953:Trainmethod doesn't perform actual backpropagation.The method calculates loss but doesn't compute gradients or update parameters. The comment on line 501 in
LearnCorrespondenceacknowledges this ("Backward pass would go here in full implementation"). This is a significant limitation for training functionality.Is this network intended to be inference-only? If training is expected to work, backward pass implementation is needed. Consider adding a
NotImplementedExceptionor clear documentation that training is not yet supported.src/NeuralNetworks/AudioVisualEventLocalizationNetwork.cs (1)
136-139: VerifyMultiHeadAttentionLayerconstructor signature.The constructor is called with
(_embeddingDimension, 8, _embeddingDimension / 8, geluActivation)in this location (and at lines 147-149), but inAudioVisualCorrespondenceNetwork.cs(lines 141-145) it's called with(1, _embeddingDimension, NUM_ATTENTION_HEADS, geluActivation). This inconsistency suggests different overloads or potential incorrect parameter order that requires verification.src/Enums/ModelType.cs (1)
1027-1079: LGTM!The new multimodal model enum entries (Blip, Blip2, LLaVA, Flamingo, VideoCLIP, ImageBind) are well-documented with clear beginner-friendly explanations and appropriately grouped.
src/Interfaces/ILLaVAModel.cs (1)
35-189: LGTM!The
ILLaVAModel<T>interface is well-designed with comprehensive documentation. The method signatures for visual conversation, grounding, and multi-turn chat capabilities are appropriate for LLaVA-style models.src/Interfaces/IGpt4VisionModel.cs (1)
31-275: LGTM!The
IGpt4VisionModel<T>interface provides a comprehensive API for GPT-4V-style capabilities. The extensive method set (document analysis, code generation, OCR, chart analysis, safety checks) is well-documented and appropriately typed.src/Interfaces/IBlipModel.cs (1)
30-181: LGTM!The
IBlipModel<T>interface appropriately captures BLIP's multi-task capabilities. The distinction betweenGenerateCaption(beam search for quality) andGenerateCaptions(nucleus sampling for diversity) is well-designed. Documentation clearly explains the ITM vs embedding similarity tradeoffs.src/Interfaces/IFlamingoModel.cs (1)
38-200: LGTM!The
IFlamingoModel<T>interface effectively captures Flamingo's few-shot in-context learning paradigm. The method signatures appropriately use tuples for example pairs (image, text), (image, label), and (image, question, answer), enabling the few-shot pattern elegantly.src/Interfaces/IBlip2Model.cs (1)
36-313: LGTM!The
IBlip2Model<T>interface comprehensively models BLIP-2's Q-Former architecture. The distinction between ITC (fast embedding similarity) and ITM (slower cross-attention matching) methods is well-documented, and the two-stage retrieval pattern (ITC + optional ITM reranking) inRetrieveImagesreflects real-world BLIP-2 usage.src/Interfaces/IImageBindModel.cs (3)
8-24: LGTM!The
ModalityTypeenum is well-defined with clear documentation for each of the seven modalities supported by ImageBind.
60-61: Verify intentional omission ofIMultimodalEmbedding<T>inheritance.Unlike
IBlipModel<T>,IFlamingoModel<T>,IVideoCLIPModel<T>, and other multimodal interfaces in this PR,IImageBindModel<T>does not extendIMultimodalEmbedding<T>. If this is intentional (due to ImageBind's unique 6+ modality architecture), consider documenting this design decision.
272-281: LGTM on fusion API.The
FuseModalitiesmethod provides a clean API for multimodal fusion with configurable fusion methods. The dictionary-based approach elegantly handles variable modality combinations.src/Interfaces/IVideoCLIPModel.cs (1)
33-210: LGTM!The
IVideoCLIPModel<T>interface provides a comprehensive API for video-text understanding. Key highlights:
- Frame-based video representation with
IEnumerable<Tensor<T>>is appropriate.LocalizeMomentsfor temporal grounding returns useful (start, end, score) tuples.- Temporal similarity matrix enables video alignment use cases.
PredictNextActionextends the model to action anticipation tasks.src/NeuralNetworks/BlipNeuralNetwork.cs (4)
248-329: LGTM - ONNX constructor with proper resource cleanup.The try-catch pattern correctly disposes partially-created ONNX sessions on failure while preserving the original exception. The empty inner catch for disposal exceptions is acceptable to avoid masking the root cause.
355-393: LGTM - Native mode constructor.The constructor properly initializes all hyperparameters and provides sensible defaults for the tokenizer, optimizer, and loss function.
415-475: LGTM - Native layer initialization.The layer structure follows the BLIP architecture pattern correctly with vision encoder, text encoder, text decoder, cross-attention layers, and ITM head.
1598-1606: VerifyLayersproperty includes all layer collections.The
ParameterCountgetter iterates overLayers, but the class maintains separate lists (_visionEncoderLayers,_textEncoderLayers,_textDecoderLayers,_crossAttentionLayers). Confirm whether the base classLayersproperty aggregates all these collections or if the parameter count calculation is incomplete.
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
- fix virtual call in constructor for clipneuralnetwork - fix useless numpatches assignment in clipneuralnetwork - use linq sum for parameter count in clipneuralnetwork - fix dalle3model clone to properly copy unet and vae parameters - fix imageBindneuralnetwork findbestmatch empty candidates handling - fix imageBindneuralnetwork zeroshotclassify empty label handling - fix llavaneuralnetwork computesimilarity dimension mismatch 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 18
♻️ Duplicate comments (7)
src/NeuralNetworks/Gpt4VisionNeuralNetwork.cs (2)
1275-1290: Random instantiation in SampleFromDistribution creates poor randomness.Line 1277 creates a new
Random()instance on every call. When called in rapid succession, this can produce identical or correlated random values becauseRandom()seeds from the system clock. For multi-threaded scenarios, this is also not thread-safe.🔎 Proposed fix
Add a static thread-safe random generator at the class level:
+ private static readonly Random _random = new Random(); + private static readonly object _randomLock = new object(); + private int SampleFromDistribution(Vector<T> probs) { - double random = new Random().NextDouble(); + double random; + lock (_randomLock) + { + random = _random.NextDouble(); + } double cumulative = 0;Alternatively, for .NET 6+, use
Random.Shared:private int SampleFromDistribution(Vector<T> probs) { - double random = new Random().NextDouble(); + double random = Random.Shared.NextDouble(); double cumulative = 0;
1412-1433: Train method doesn't actually compute gradients from the loss.The Train method computes a loss and loss gradient (lines 1423-1426), but then calls
GetGpt4VParameterGradients()which retrieves existing gradients from layers without performing backpropagation through the loss. This means:
- The computed
lossGradient(line 1426) is never used- Layer gradients are not updated based on the current loss
- The network won't actually learn from the training data
To properly implement training, you need to:
- Implement backward pass that propagates
lossGradientthrough all layers- Or explicitly indicate training is not supported by throwing
NotSupportedException🔎 Suggested approach
If training is not yet implemented, make this explicit:
public override void Train(Tensor<T> input, Tensor<T> expectedOutput) { + if (!_useNativeMode) + { + throw new NotSupportedException("Training is not supported in ONNX mode. Use native mode for training."); + } + + throw new NotImplementedException( + "Training with backpropagation is not yet implemented for Gpt4VisionNeuralNetwork. " + + "This network currently supports inference only."); - SetTrainingMode(true); - - // Forward pass - var imageFeatures = EncodeImage(input); - var projected = ProjectVisionFeatures(imageFeatures); - - // Compute loss using the loss function - var predictedEmbedding = PoolFeatures(projected); - var targetEmbedding = TensorToVector(expectedOutput); - LastLoss = LossFunction.CalculateLoss(predictedEmbedding, targetEmbedding); - - // Compute gradient of loss w.r.t. output - var lossGradient = LossFunction.CalculateDerivative(predictedEmbedding, targetEmbedding); - - // Get parameter gradients and apply gradient descent update - var paramGradients = GetGpt4VParameterGradients(); - UpdateParameters(paramGradients); - - SetTrainingMode(false); }src/NeuralNetworks/AudioVisualCorrespondenceNetwork.cs (1)
431-467: Scene classification head dimension hardcoded to 256.Line 187 creates
_sceneClassificationHeadwith output dimension 256, butClassifySceneaccepts arbitrarysceneLabels. When the label count exceeds 256, labels beyond index 256 receive zero logits (line 452), producing incorrect probability distributions.Either:
- Make the classification head dynamic based on expected label count, or
- Document the 256-label limit and validate at runtime:
public Dictionary<string, T> ClassifyScene( Tensor<T> audioWaveform, IEnumerable<Tensor<T>> frames, IEnumerable<string> sceneLabels) { + var labelList = sceneLabels.ToList(); + if (labelList.Count > 256) + { + throw new ArgumentException( + $"Scene classification supports a maximum of 256 labels, but {labelList.Count} were provided.", + nameof(sceneLabels)); + } + var audioEmb = GetAudioEmbedding(audioWaveform, _audioSampleRate); var visualEmb = GetVisualEmbedding(frames);src/NeuralNetworks/LLaVANeuralNetwork.cs (1)
373-385: Similarity dimension mismatch fix prevents out-of-range errorsSwitching
ComputeSimilarityto uselength = Math.Min(textEmbedding.Length, imageEmbedding.Length)avoids the previous out-of-range access when text and image embeddings had different hidden sizes (e.g.,_lmHiddenDimvs_visionHiddenDim). This addresses the earlier critical bug and makes similarity computation robust to dimension mismatches.src/NeuralNetworks/Blip2NeuralNetwork.cs (1)
1176-1219: ONNX text embedding still assumes 2D tensor and will break on typical BLIP‑2 outputs
GetTextEmbeddingOnnxis still indexing the ONNX output as if it were 2D and usingoutput.Lengthto size the embedding:
embDim = Math.Min(_embeddingDimension, (int)output.Length);uses total element count, not the hidden dimension.output[0, i]assumes a[batch, hidden]tensor; for common BLIP‑2 exports with shape[batch, seq_len/num_queries, hidden], this will throw due to wrong rank/indexing and will not pick the CLS/token dimension correctly.You should instead:
- Derive the hidden size from the last dimension:
int hiddenDim = (int)output.Dimensions[output.Rank - 1];- Set
int embDim = Math.Min(_embeddingDimension, hiddenDim);- Handle 1D/2D/3D cases explicitly, e.g.:
[hidden]→output[i][batch, hidden]→output[0, i][batch, seq_len, hidden]→output[0, 0, i]or a pooled aggregation overseq_len.Without this, ONNX text embedding will be incorrect or crash for common BLIP‑2 text/Q‑Former ONNX models.
src/NeuralNetworks/FlamingoNeuralNetwork.cs (2)
108-172: ONNX mode remains non-functional.The ONNX constructor sets
_useNativeMode = falsebut does not initialize native-mode fields (_perceiverQueries,_patchEmbedding,_textTokenEmbedding, etc.). Methods likeExtractPerceiverFeaturesOnnx(line 792-795) delegate to native implementations that require these fields, andGenerateWithVisualContext(lines 879-935) directly uses_languageModelLayerswithout mode checks.Consider throwing
NotSupportedExceptionfrom ONNX-only methods until proper ONNX paths are implemented.
374-397: EmbedTextTokens silently returns zero embeddings in ONNX mode.When
_textTokenEmbeddingor_textPositionalEmbeddingsis null (ONNX mode), this returns a tensor of zeros rather than throwing. This leads to all text embeddings being zero vectors, causing degenerate similarity scores.🔎 Proposed fix
private Tensor<T> EmbedTextTokens(IReadOnlyList<int> tokenIds) { int seqLen = tokenIds.Count; var embeddings = Tensor<T>.CreateDefault([seqLen, _lmHiddenDim], NumOps.Zero); if (_textTokenEmbedding is null || _textPositionalEmbeddings is null) { - return embeddings; + throw new InvalidOperationException( + "Text embedding layers not initialized. ONNX mode text embedding is not yet supported."); }
🧹 Nitpick comments (15)
src/NeuralNetworks/AudioVisualEventLocalizationNetwork.cs (3)
143-143: Hardcoded visual input dimension may cause mismatch.The visual input projection assumes a fixed 768-dimensional input (ViT-style), but actual frame dimensions may vary. This could cause runtime errors if frames don't match this expected size.
Consider either:
- Making the visual input dimension configurable via constructor parameter
- Adding validation to reject frames with incorrect dimensions
- Documenting the expected frame format in the XML comments
928-928: Hardcoded audio sample rate assumption.The method assumes a fixed 16kHz sample rate, which may not match actual input audio. This could cause temporal misalignment if the actual sample rate differs.
Consider making the audio sample rate configurable, either as:
- A constructor parameter
- A property that can be set
- Metadata passed with the audio tensor
1346-1367: Excellent fix for gradient updates!The method now correctly applies gradient descent (
params = params - learning_rate * gradients) instead of replacing parameters with gradients. This resolves the previous critical issue.However, note that the learning rate is hardcoded to 0.001, while an
_optimizerfield exists (line 99). Consider using the optimizer for parameter updates instead of manual gradient descent.Based on learnings, this addresses the UpdateParameters concern from prior reviews.
🔎 Optional refactor to use the optimizer
public override void UpdateParameters(Vector<T> gradients) { if (gradients.Length != ParameterCount) { throw new ArgumentException( $"Gradient vector length ({gradients.Length}) must match parameter count ({ParameterCount}).", nameof(gradients)); } - // Get current parameters - var currentParams = GetParameters(); - - // Apply gradient descent update: params = params - learning_rate * gradients - T learningRate = NumOps.FromDouble(0.001); // Default learning rate - for (int i = 0; i < currentParams.Length; i++) - { - currentParams[i] = NumOps.Subtract(currentParams[i], NumOps.Multiply(learningRate, gradients[i])); - } - - // Set the updated parameters - SetParameters(currentParams); + // Use the optimizer to apply parameter updates + var gradientTensor = Tensor<T>.FromVector(gradients); + _optimizer.UpdateWeights(gradientTensor); }src/Diffusion/Models/DallE3Model.cs (2)
545-594: Consider performance optimization for bilinear upscaling.The implementation is correct but uses nested loops with individual element access. For large images or frequent upscaling operations, consider optimizing with:
- Parallel processing (
Parallel.Forfor the outer channel loop)- SIMD vectorization for inner loops if the platform supports it
- Leveraging existing optimized libraries (e.g., SixLabors.ImageSharp, OpenCV wrappers)
The current implementation is sufficient for correctness but may become a bottleneck for production workloads.
650-670: Consider usingArray.Copyfor parameter aggregation.The manual loop copying works correctly but could be more efficient. For potentially large parameter vectors, consider using
Array.CopyorBuffer.BlockCopyfor better performance.🔎 Alternative implementation
public override Vector<T> GetParameters() { var unetParams = _unet.GetParameters(); var vaeParams = _vae.GetParameters(); var totalLength = unetParams.Length + vaeParams.Length; var combined = new Vector<T>(totalLength); - for (int i = 0; i < unetParams.Length; i++) - { - combined[i] = unetParams[i]; - } - - for (int i = 0; i < vaeParams.Length; i++) - { - combined[unetParams.Length + i] = vaeParams[i]; - } + Array.Copy(unetParams.Data, 0, combined.Data, 0, unetParams.Length); + Array.Copy(vaeParams.Data, 0, combined.Data, unetParams.Length, vaeParams.Length); return combined; }Note: Adjust based on whether
Vector<T>exposes aDataproperty or similar for bulk operations.src/NeuralNetworks/Gpt4VisionNeuralNetwork.cs (4)
578-615: Fragile parsing and hardcoded confidence in DetectObjects.The object detection parsing relies on a specific text format (parentheses and commas) that the LLM may not always follow consistently. Additionally, all detected objects receive a hardcoded confidence of 0.8 (line 608), which doesn't reflect actual model confidence.
Consider:
- Adding more robust parsing with error handling for malformed responses
- Either computing actual confidence scores or documenting that 0.8 is a placeholder
- Handling cases where the LLM response doesn't match the expected format
784-816: SafetyCheck implementation is overly simplistic.The safety check uses basic string matching (lines 809-810) to detect safety concerns in the LLM's text response. This approach is prone to false positives and false negatives. The hardcoded confidence values (line 811) don't reflect actual detection confidence.
For production use, consider:
- Using dedicated safety/moderation models or APIs
- Implementing more robust text classification
- Documenting that this is a basic heuristic implementation
1292-1295: Consider making end token IDs configurable.The hardcoded end token IDs (2, 50256, 128001) are model-specific and may not be correct for all tokenizers. Consider making these configurable through constructor parameters or reading them from the tokenizer's special tokens.
1-1637: Consider documenting the limitations of LLM-based structured output parsing.Many methods throughout this file rely on parsing free-form text from the LLM to extract structured data (e.g.,
DetectObjects,AnalyzeChart,SafetyCheck,EvaluateImageQuality). This approach is inherently fragile and may produce inconsistent results.Consider adding XML documentation or a README section that:
- Explains these methods use prompt-based extraction
- Notes the reliability limitations
- Suggests alternatives for production use cases requiring high accuracy (dedicated models, APIs)
src/NeuralNetworks/AudioVisualCorrespondenceNetwork.cs (2)
71-71: Optimizer is instantiated but never used.The
_optimizerfield is created in the constructor but is never invoked. Line 969 inUpdateParametersuses a hardcoded learning rate (0.001) instead. This wastes resources and suggests incomplete integration.🔎 Suggested fix
If the optimizer isn't needed, remove it:
-private readonly IOptimizer<T, Tensor<T>, Tensor<T>> _optimizer;- _optimizer = optimizer ?? new Optimizers.AdamOptimizer<T, Tensor<T>, Tensor<T>>(this);And update the constructor signature:
public AudioVisualCorrespondenceNetwork( NeuralNetworkArchitecture<T> architecture, int embeddingDimension = DEFAULT_EMBEDDING_DIM, int audioSampleRate = DEFAULT_SAMPLE_RATE, double videoFrameRate = DEFAULT_FRAME_RATE, int numEncoderLayers = 6, - IOptimizer<T, Tensor<T>, Tensor<T>>? optimizer = null, ILossFunction<T>? lossFunction = null, int? seed = null)If the optimizer should be used, replace the hardcoded learning rate in
UpdateParameterswith optimizer invocation.Also applies to: 127-127
512-535: ComputeSpectrogram uses oversimplified approximation.The "simplified spectrogram" (line 520 comment) computes
log(|waveform[startSample + bin] * freq|)rather than a proper Short-Time Fourier Transform. Line 527's indexing treats frequency bins as sample offsets, which doesn't reflect actual spectral content.Consider either:
- Implementing a proper STFT using FFT (or calling an existing library), or
- Renaming to
ComputeSimplifiedFeaturesand documenting that this is a placeholder feature extraction rather than a true spectrogram.The current name may mislead users expecting standard audio spectrograms.
src/NeuralNetworks/BlipNeuralNetwork.cs (1)
420-426: Remove unusednumPatchesvariable.The variable
numPatchesis computed but never used in this method. This was flagged in a previous commit fix.🔎 Proposed fix
private void InitializeNativeLayers() { - int numPatches = (_imageSize / _patchSize) * (_imageSize / _patchSize); - // Vision encoder: Patch embedding + Transformer _patchEmbedding = new PatchEmbeddingLayer<T>(_imageSize, _imageSize, 3, _patchSize, _hiddenDim);Note: The
numPatchescalculation is correctly used later inInitializeParametersvia_visionPositionalEmbeddingsdimensions.src/NeuralNetworks/FlamingoNeuralNetwork.cs (3)
302-314: Consider using consistent random helper.This uses
new Random(42)directly, whileBlipNeuralNetworkusesTensors.Helpers.RandomHelper.CreateSeededRandom(42)for the same purpose. Consider using the helper for consistency.
965-976: AddTensors assumes 2D tensors without shape validation.This helper assumes both tensors are 2D with identical shapes. Consider adding a shape assertion for debugging.
🔎 Proposed defensive check
private Tensor<T> AddTensors(Tensor<T> a, Tensor<T> b) { + System.Diagnostics.Debug.Assert( + a.Shape.Length == 2 && b.Shape.Length == 2 && + a.Shape[0] == b.Shape[0] && a.Shape[1] == b.Shape[1], + "AddTensors requires matching 2D tensor shapes"); + var result = Tensor<T>.CreateDefault(a.Shape, NumOps.Zero);
1186-1203: Deserialization discards all values without validation.Unlike
BlipNeuralNetworkwhich validates that loaded values match the current instance, this method reads and discards all serialized values. This could lead to silently loading an incompatible model.🔎 Consider adding validation like BlipNeuralNetwork
protected override void DeserializeNetworkSpecificData(BinaryReader reader) { - _ = reader.ReadInt32(); // embeddingDimension - _ = reader.ReadInt32(); // maxSequenceLength - // ... etc + int embeddingDim = reader.ReadInt32(); + int maxSeqLen = reader.ReadInt32(); + // ... read other values + + if (embeddingDim != _embeddingDimension) + { + throw new InvalidOperationException( + $"Loaded embedding dimension ({embeddingDim}) doesn't match current ({_embeddingDimension})."); + } + // Add similar checks for other critical fields }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/Diffusion/Models/DallE3Model.cssrc/NeuralNetworks/AudioVisualCorrespondenceNetwork.cssrc/NeuralNetworks/AudioVisualEventLocalizationNetwork.cssrc/NeuralNetworks/Blip2NeuralNetwork.cssrc/NeuralNetworks/BlipNeuralNetwork.cssrc/NeuralNetworks/ClipNeuralNetwork.cssrc/NeuralNetworks/FlamingoNeuralNetwork.cssrc/NeuralNetworks/Gpt4VisionNeuralNetwork.cssrc/NeuralNetworks/ImageBindNeuralNetwork.cssrc/NeuralNetworks/LLaVANeuralNetwork.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/ClipNeuralNetwork.cssrc/NeuralNetworks/AudioVisualCorrespondenceNetwork.cssrc/NeuralNetworks/LLaVANeuralNetwork.cssrc/NeuralNetworks/Blip2NeuralNetwork.cssrc/NeuralNetworks/AudioVisualEventLocalizationNetwork.cssrc/Diffusion/Models/DallE3Model.cssrc/NeuralNetworks/Gpt4VisionNeuralNetwork.cssrc/NeuralNetworks/ImageBindNeuralNetwork.cssrc/NeuralNetworks/FlamingoNeuralNetwork.cssrc/NeuralNetworks/BlipNeuralNetwork.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/ClipNeuralNetwork.cssrc/NeuralNetworks/AudioVisualCorrespondenceNetwork.cssrc/NeuralNetworks/LLaVANeuralNetwork.cssrc/NeuralNetworks/Blip2NeuralNetwork.cssrc/NeuralNetworks/AudioVisualEventLocalizationNetwork.cssrc/Diffusion/Models/DallE3Model.cssrc/NeuralNetworks/Gpt4VisionNeuralNetwork.cssrc/NeuralNetworks/ImageBindNeuralNetwork.cssrc/NeuralNetworks/FlamingoNeuralNetwork.cssrc/NeuralNetworks/BlipNeuralNetwork.cs
🔇 Additional comments (16)
src/NeuralNetworks/AudioVisualEventLocalizationNetwork.cs (7)
82-120: LGTM!The constructor properly initializes all fields, provides sensible defaults for optional parameters, and safely handles nullable inputs. The defensive null-forgiving assignments are resolved by the
InitializeLayers()call.
195-351: LGTM!The encoding pipeline is well-structured with appropriate edge-case handling (empty frames, short audio, dimension mismatches). Division-by-zero is properly guarded in all averaging operations.
357-912: LGTM!All interface methods are well-implemented with proper edge-case handling (empty frames, empty candidates, out-of-bounds access). The SoftmaxTensor call now correctly delegates to
Engine.Softmax, addressing the previous review concern.
1057-1094: Excellent fix for deterministic hashing!The implementation now uses FNV-1a instead of
string.GetHashCode(), ensuring reproducible embeddings across runs and .NET versions. This resolves the previous critical issue.Based on learnings, this addresses the deterministic hash concern from prior reviews.
1178-1183: Excellent fix for axis-aware softmax!The method now properly delegates to
Engine.Softmax(tensor, axis), which correctly handles the axis parameter for multi-dimensional tensors. This resolves the previous major issue.Based on learnings, this addresses the axis parameter concern from prior reviews.
1396-1433: Excellent fix for deserialization validation!The method now properly validates that loaded configuration matches the current instance, throwing clear exceptions on mismatch. This resolves the previous major issue and ensures round-trip serialization integrity.
Based on learnings, this addresses the deserialization concern from prior reviews.
1447-1522: LGTM!The parameter management methods (
GetParametersandSetParameters) correctly collect and distribute parameters across all layers in consistent order, ensuring proper serialization and optimization.src/Diffusion/Models/DallE3Model.cs (2)
712-727: LGTM! Clone now correctly copies model weights.The Clone implementation properly addresses the issue from the previous review. The cloned model's UNet and VAE parameters are now explicitly copied via
SetParameters, ensuring the cloned model preserves trained weights rather than returning a randomly initialized model.
219-224: Verify thatImageToImagehandles empty prompts correctly.The method passes an empty string as the prompt to
ImageToImage. While this appears intentional for generating unconditioned variations, ensure that the base class implementation handles empty prompts gracefully (either as null conditioning or as a neutral prompt embedding).src/NeuralNetworks/ClipNeuralNetwork.cs (2)
272-351: Constructor/initialization pattern looks solid nowUsing a non‑virtual
InitializeLayersCorefrom both constructors, plus the try/catch with temporaryInferenceSessionvariables and disposal on failure, cleanly fixes the virtual‑call‑in‑constructor and resource‑leak risks. ONNX vs native mode configuration is clearly separated and validated.
1434-1472: Deserialization validation correctly aligns with readonly configuration
DeserializeNetworkSpecificDatanow:
- Reads the serialized embedding/sequence/image sizes and optimizer/loss type names,
- Discards the type strings only to advance the stream, and
- Validates the deserialized dimensions against the constructor‑initialized readonly fields.
This makes deserialization consistent and fails fast if metadata doesn’t match the instance, while remaining backward‑compatible with the existing binary format.
src/NeuralNetworks/LLaVANeuralNetwork.cs (1)
1087-1162: The review comment identifies a potential critical structural issue with parameter handling in the Train/UpdateParameters workflow. Without access to verify whether ParameterCount and GetParameters are overridden in LLaVANeuralNetwork, or how the private layer collections are registered, manual verification of the codebase is required to confirm whether the concern is valid or already addressed.src/NeuralNetworks/ImageBindNeuralNetwork.cs (1)
1395-1455: Train/UpdateParameters are inconsistent with ParameterCount and likely break native-mode trainingIn this class:
InitializeNativeLayersbuilds modality-specific encoder stacks in_imageEncoderLayers,_textEncoderLayers,_audioEncoderLayers, etc., but never registers them with the baseLayerscollection.- You don't override
ParameterCountorGetParameters, so they will come fromNeuralNetworkBase(typically summing overLayers).Traindoes:Backward(gradient); var currentParams = GetParameters(); UpdateParameters(currentParams);
UpdateParametersthen:
- Uses
expectedCount = ParameterCountfor a length check, and- Iterates over all *_EncoderLayers lists, slicing from
parameters[offset + i]and callinglayer.UpdateParameters(...).Unless
NeuralNetworkBasealready accounts for all of these private layer lists (which it has no visibility into),ParameterCount/GetParameterswill not include their parameters, so:
currentParams.Lengthwill be 0 while encoder layers have non‑zeroParameterCount, andUpdateLayerListParameterswill index into an undersizedparametersvector, leading to out‑of‑range access or effectively no weight updates.Given this PR's focus on inference, a safer interim behavior would be to:
- Either wire these encoder layers into
Layersand overrideParameterCount/GetParametersto aggregate all modality parameters consistently, or- Make
Train(and possiblyUpdateParameters) throwNotSupportedExceptionwhen training is not fully implemented for ImageBind.Right now, native‑mode training is very likely broken or unstable.
src/NeuralNetworks/BlipNeuralNetwork.cs (1)
1146-1193: LM head projection is now implemented correctly.The previous review flagged that
ForwardDecoderNativereturned hidden states instead of vocabulary logits. This has been addressed by adding the_lmHeadprojection (initialized at line 471) that correctly projects hidden states to vocabulary logits.src/NeuralNetworks/FlamingoNeuralNetwork.cs (2)
1206-1254: Optimizer sharing issue has been addressed.The code now creates a fresh
AdamOptimizerinstance (line 1209) instead of reusing_optimizer. While the optimizer is constructed withthis(the original instance) as the reference, it's a new instance with fresh state, which resolves the state-sharing concern from the previous review.
604-631: InContextClassify uses simple substring matching for scoring.The classification scoring relies on
generated.Contains(label)which is a basic heuristic. This works for simple cases but may produce unexpected results with partial matches or labels that are substrings of each other (e.g., "cat" matching "category").This is acceptable for an initial implementation, but consider documenting this limitation or using more robust token-based matching in the future.
- DallE3Model: use RandomHelper.ThreadSafeRandom for thread safety, add extensionPixels validation in Outpaint method - BlipNeuralNetwork: fix ArgMax and SampleWithTemperature to use last sequence position for autoregressive decoding, fix ParameterCount to iterate over all native layer lists, add empty labels validation - Gpt4VisionNeuralNetwork: add tokenizer null check in native constructor, add empty labels validation, fix deserialization to restore field values, fix CreateNewInstance to respect execution mode - LLaVANeuralNetwork: add empty class labels validation, fix NumLMayers typo in metadata to NumLMLayers 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (5)
src/Diffusion/Models/DallE3Model.cs (1)
405-419: Previously flagged: Weak prompt safety checking.This implementation still uses basic substring matching that can be easily bypassed, as noted in previous reviews. The concern about needing more robust moderation (API integration, regex, semantic analysis) remains valid but has already been documented.
src/NeuralNetworks/BlipNeuralNetwork.cs (2)
1881-1882: Metadata references baseLayerscollection which may be empty in native mode.In native mode, layers are stored in
_visionEncoderLayers,_textEncoderLayers,_textDecoderLayers, and_crossAttentionLayers, not in the base classLayersproperty. This causesLayers.CountandLayers.Select(...)to return 0 or incorrect information.🔎 Proposed fix
Compute layer count and types from the native layer lists when in native mode:
+ int totalLayers = _useNativeMode + ? _visionEncoderLayers.Count + _textEncoderLayers.Count + _textDecoderLayers.Count + _crossAttentionLayers.Count + : Layers.Count; + + var layerTypes = _useNativeMode + ? _visionEncoderLayers.Concat(_textEncoderLayers).Concat(_textDecoderLayers).Concat(_crossAttentionLayers) + .Select(l => l.GetType().Name).ToArray() + : Layers.Select(l => l.GetType().Name).ToArray(); + { "VocabularySize", _vocabularySize }, - { "LayerCount", Layers.Count }, - { "LayerTypes", Layers.Select(l => l.GetType().Name).ToArray() }, + { "LayerCount", totalLayers }, + { "LayerTypes", layerTypes },
1624-1860: _itmHead and _lmHead parameters missing from parameter management methods.The
_itmHead(line 468) and_lmHead(line 471) layers are initialized in native mode but are never included inParameterCount,GetParameters(), orUpdateParameters(). This causes their parameters to be excluded from training updates, serialization, and deserialization, breaking training and model persistence for these critical components.🔎 Proposed fix
Add
_itmHeadand_lmHeadto all three parameter management methods:ParameterCount (after line 1657):
foreach (var layer in _crossAttentionLayers) { count += layer.ParameterCount; } + + if (_itmHead is not null) + { + count += _itmHead.ParameterCount; + } + + if (_lmHead is not null) + { + count += _lmHead.ParameterCount; + } return count;GetParameters (after line 1751):
foreach (var layer in _crossAttentionLayers) { var layerParams = layer.GetParameters(); for (int i = 0; i < layerParams.Length; i++) { parameters[index++] = layerParams[i]; } } + + if (_itmHead is not null) + { + var layerParams = _itmHead.GetParameters(); + for (int i = 0; i < layerParams.Length; i++) + { + parameters[index++] = layerParams[i]; + } + } + + if (_lmHead is not null) + { + var layerParams = _lmHead.GetParameters(); + for (int i = 0; i < layerParams.Length; i++) + { + parameters[index++] = layerParams[i]; + } + } return parameters;UpdateParameters (after line 1859):
foreach (var layer in _crossAttentionLayers) { int layerParameterCount = layer.ParameterCount; if (layerParameterCount > 0) { var layerParameters = parameters.Slice(index, layerParameterCount); layer.UpdateParameters(layerParameters); index += layerParameterCount; } } + + if (_itmHead is not null) + { + int layerParameterCount = _itmHead.ParameterCount; + if (layerParameterCount > 0) + { + var layerParameters = parameters.Slice(index, layerParameterCount); + _itmHead.UpdateParameters(layerParameters); + index += layerParameterCount; + } + } + + if (_lmHead is not null) + { + int layerParameterCount = _lmHead.ParameterCount; + if (layerParameterCount > 0) + { + var layerParameters = parameters.Slice(index, layerParameterCount); + _lmHead.UpdateParameters(layerParameters); + index += layerParameterCount; + } + } }src/NeuralNetworks/Gpt4VisionNeuralNetwork.cs (1)
1422-1443: Training is non-functional: loss gradient is computed but never backpropagated.Line 1436 computes
lossGradientbut it's never used.GetGpt4VParameterGradients()(line 1439) retrieves existing layer gradients without propagating the loss backward. The model cannot learn because parameter updates are based on stale/zero gradients, not the actual loss.🔎 Suggested approach
Either implement proper backpropagation:
// Compute gradient of loss w.r.t. output var lossGradient = LossFunction.CalculateDerivative(predictedEmbedding, targetEmbedding); - // Get parameter gradients and apply gradient descent update - var paramGradients = GetGpt4VParameterGradients(); + // Backpropagate the loss gradient through the network + BackpropagateGradient(lossGradient); + + // Now get the computed gradients + var paramGradients = GetGpt4VParameterGradients(); UpdateParameters(paramGradients);Or throw
NotSupportedExceptionto clearly indicate training isn't implemented:public override void Train(Tensor<T> input, Tensor<T> expectedOutput) { + throw new NotSupportedException( + "Training is not implemented for Gpt4VisionNeuralNetwork. " + + "Use pretrained ONNX models for inference only."); + SetTrainingMode(true); // ...src/NeuralNetworks/LLaVANeuralNetwork.cs (1)
357-385: Image and text embeddings remain in different vector spaces.
GetImageEmbeddings(Lines 357–370) mean-pools the raw vision encoder output (dimension_visionHiddenDim= 1024), whileGetTextEmbeddings(Lines 325–348) mean-pools LM token embeddings (dimension_lmHiddenDim= 4096).ComputeSimilarity(Lines 373–385) usesMath.Minto avoid out-of-range exceptions, but comparing embeddings from different representational spaces yields semantically undefined similarity scores.For correct multimodal similarity, both pathways should project into a shared embedding dimension—typically by calling
ProjectToLanguageSpaceon image features before mean-pooling, or by defining a separate shared projection head for the IMultimodalEmbedding contract.🔎 Proposed fix
Update
GetImageEmbeddingsto project features before pooling:public IEnumerable<Vector<T>> GetImageEmbeddings(IEnumerable<Tensor<T>> images) { var results = new List<Vector<T>>(); foreach (var image in images) { var features = ExtractVisualFeatures(image); + var projected = ProjectToLanguageSpace(features); - var embedding = MeanPool(features); + var embedding = MeanPool(projected); var normalized = Normalize(embedding); results.Add(normalized); } return results; }This ensures both text and image embeddings reside in the same
_lmHiddenDim-dimensional space.
🧹 Nitpick comments (6)
src/Diffusion/Models/DallE3Model.cs (1)
43-43: Remove unused_userSeedfield.The
_userSeedfield is stored in the constructor but never referenced anywhere in the implementation. This dead code may confuse maintainers about whether seed handling is working correctly.If seed reproducibility via constructor parameter was intended, the field should be used; otherwise, remove it.
🔎 Proposed fix
- private readonly int? _userSeed; - // Safety patterns for content filtering private readonly HashSet<string> _unsafePatterns;public DallE3Model( DiffusionModelOptions<T>? options = null, INoiseScheduler<T>? scheduler = null, IConditioningModule<T>? conditioner = null, - int? seed = null) + int? seed = null) // Note: seed parameter currently unused : base(options, scheduler) { // Initialize components _vae = new StandardVAE<T>(LATENT_CHANNELS, VAE_SCALE_FACTOR); _unet = new UNetNoisePredictor<T>( inputChannels: LATENT_CHANNELS, outputChannels: LATENT_CHANNELS, baseChannels: 320, channelMultipliers: [1, 2, 4, 4], numResBlocks: 2, attentionResolutions: [4, 2, 1], contextDim: conditioner?.EmbeddingDimension ?? 768, numHeads: 8); _conditioner = conditioner; - _userSeed = seed;Also applies to: 91-111
src/NeuralNetworks/Gpt4VisionNeuralNetwork.cs (3)
255-258: Positional embeddings initialized to zero provide no positional information.
_visionClsTokenand_visionPositionalEmbeddingsare initialized with zeros, meaning the vision encoder has no positional awareness until weights are loaded or trained. Consider using sinusoidal positional encodings or random initialization.🔎 Suggested sinusoidal initialization pattern
// Vision CLS token and positional embeddings - _visionClsToken = Matrix<T>.CreateDefault(1, _visionEmbeddingDim, NumOps.Zero); - _visionPositionalEmbeddings = Matrix<T>.CreateDefault(numPatches + 1, _visionEmbeddingDim, NumOps.Zero); + _visionClsToken = Matrix<T>.CreateDefault(1, _visionEmbeddingDim, NumOps.Zero); + _visionPositionalEmbeddings = Matrix<T>.CreateDefault(numPatches + 1, _visionEmbeddingDim, NumOps.Zero); + + // Initialize positional embeddings with sinusoidal pattern + for (int pos = 0; pos < numPatches + 1; pos++) + { + for (int i = 0; i < _visionEmbeddingDim; i++) + { + double angle = pos / Math.Pow(10000, (2.0 * (i / 2)) / _visionEmbeddingDim); + _visionPositionalEmbeddings[pos, i] = NumOps.FromDouble( + i % 2 == 0 ? Math.Sin(angle) : Math.Cos(angle)); + } + }
1518-1523: Hardcoded learning rate limits training flexibility.The learning rate is hardcoded to
0.001(line 1519). This should be configurable, either via constructor parameter, property, or inherited from the base class.🔎 Suggested fix
+ private T _learningRate = NumOps.FromDouble(0.001); + + public T LearningRate + { + get => _learningRate; + set => _learningRate = value; + } public override void UpdateParameters(Vector<T> gradients) { // ... - T learningRate = NumOps.FromDouble(0.001); // Default learning rate + T learningRate = _learningRate; for (int i = 0; i < currentParams.Length; i++)
1302-1305: Document magic token IDs or make them configurable.The hardcoded end token IDs (
2,50256,128001) correspond to different tokenizer vocabularies but lack documentation. Consider adding comments or making these configurable based on the tokenizer.🔎 Suggested documentation
private bool IsEndToken(int token) { - return token == 2 || token == 50256 || token == 128001; + // Common end-of-sequence tokens: + // 2 - Generic EOS token + // 50256 - GPT-2 <|endoftext|> + // 128001 - LLaMA/GPT-4 style EOS + return token == 2 || token == 50256 || token == 128001; }src/NeuralNetworks/LLaVANeuralNetwork.cs (2)
451-455: Duplicate EOS token detection can be extracted to a helper.Lines 451–455 and 611–615 contain identical logic for detecting the EOS token. Extracting this into a helper method (e.g.,
private bool IsEosToken(int tokenId)) would reduce duplication and make updates easier.🔎 Proposed refactor
Add a helper method in the Helper Methods region:
+private bool IsEosToken(int tokenId) +{ + var specialTokens = _tokenizer.SpecialTokens; + var eosTokenStr = specialTokens?.EosToken ?? "[SEP]"; + var eosEncoded = _tokenizer.Encode(eosTokenStr); + return eosEncoded.TokenIds.Count > 0 && tokenId == eosEncoded.TokenIds[0]; +}Then replace duplicated checks:
-var specialTokens = _tokenizer.SpecialTokens; -var eosTokenStr = specialTokens?.EosToken ?? "[SEP]"; -var eosEncoded = _tokenizer.Encode(eosTokenStr); -if (eosEncoded.TokenIds.Count > 0 && nextToken == eosEncoded.TokenIds[0]) +if (IsEosToken(nextToken)) break;Also applies to: 611-615
942-967: Consider bilinear interpolation for better image quality.
ResizeImageuses nearest-neighbor sampling (Lines 959–960), which can produce blocky artifacts. For vision models that are sensitive to image quality, bilinear or bicubic interpolation may improve results, especially when downsampling. However, nearest-neighbor is simpler and faster, so this is an optional enhancement.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Diffusion/Models/DallE3Model.cssrc/NeuralNetworks/BlipNeuralNetwork.cssrc/NeuralNetworks/Gpt4VisionNeuralNetwork.cssrc/NeuralNetworks/LLaVANeuralNetwork.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/Diffusion/Models/DallE3Model.cssrc/NeuralNetworks/LLaVANeuralNetwork.cssrc/NeuralNetworks/BlipNeuralNetwork.cssrc/NeuralNetworks/Gpt4VisionNeuralNetwork.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/Diffusion/Models/DallE3Model.cssrc/NeuralNetworks/LLaVANeuralNetwork.cssrc/NeuralNetworks/BlipNeuralNetwork.cssrc/NeuralNetworks/Gpt4VisionNeuralNetwork.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 (13)
src/Diffusion/Models/DallE3Model.cs (3)
133-136: LGTM: Thread-safe random seed generation.The implementation correctly uses
RandomHelper.ThreadSafeRandom.Next()instead of a sharedRandominstance, resolving the race condition flagged in previous reviews. This ensures safe concurrent access across multiple generation methods.
308-314: LGTM: Proper parameter validation.The
extensionPixelsvalidation correctly checks for positive values and throws an appropriateArgumentOutOfRangeExceptionwith parameter name, actual value, and descriptive message. This addresses the validation concern from previous reviews.
726-741: LGTM: Clone correctly transfers model weights.The
Cloneimplementation now properly transfers weights by callingSetParameterson the cloned model's internal UNet and VAE instances (lines 735, 738). This ensures the cloned model preserves the trained behavior, addressing the issue flagged in previous reviews.src/NeuralNetworks/Gpt4VisionNeuralNetwork.cs (7)
131-175: LGTM - ONNX constructor validation is comprehensive.Path existence validation (lines 146-153) and tokenizer null check (line 158) properly guard against invalid inputs. This addresses previous review concerns.
180-216: LGTM - Native constructor properly validates tokenizer.The tokenizer null check on line 199 prevents NullReferenceException in downstream operations.
343-374: LGTM - ZeroShotClassify has proper input validation.Null and empty label checks (lines 345-354) prevent edge case issues as previously flagged.
1577-1592: LGTM - Deserialization now properly restores model configuration.The deserialized values are correctly assigned to instance fields, addressing the previous review concern.
1595-1635: LGTM - CreateNewInstance correctly handles both ONNX and native modes.The method now properly creates instances matching the current mode, with clear error messaging when ONNX paths are unavailable.
1249-1257: LGTM - Softmax implementation with numerical stability.The subtraction of max value before exponentiation (line 1253) prevents overflow, which is the correct numerical stability pattern.
1638-1647: Verify native layers are properly disposed.ONNX sessions are disposed, but native layer collections (
_visionEncoderLayers,_languageModelLayers,_crossAttentionLayers) are not. IfILayer<T>implementations hold unmanaged resources or implementIDisposable, they should be disposed.src/NeuralNetworks/LLaVANeuralNetwork.cs (3)
388-419: Empty label validation properly implemented.Lines 390–399 now correctly guard against both null and empty
classLabels, addressing the past review comment. The validation is clear and provides helpful error messages.
1257-1266: Resource disposal properly implemented.The
Disposemethod correctly disposes ONNX inference sessions and calls the base implementation, ensuring no resource leaks.
219-219: ****The technical explanation in the original review comment does not align with LLaVA's architecture. LLaVA does not tokenize visual patches—CLIP produces continuous patch embeddings that are projected through an adapter into LLaMA's embedding space. CLIP's tokenizer (byte-level BPE) is used only for text prompts, while LLaMA's tokenizer (SentencePiece) is used for text generation; they operate at different pipeline stages and do not cause token-ID incompatibilities.
However, the underlying concern—that defaulting to a CLIP tokenizer when a LLaMA-compatible tokenizer is expected—may still be valid depending on how the tokenizer is actually used in this C# implementation. Without access to the codebase, the actual impact cannot be determined.
…al network - Replace notimplementedexception with proper backpropagation in learnCorrespondence - Implement train method with proper gradient computation and parameter updates - Vectorize helper methods using IEngine (cosinesimilarity, dotproduct, gelu, sigmoid) - Change optimizer type to IGradientBasedOptimizer for proper training support - Fix deepcopy to not share optimizer/loss function references - Add backpropagate methods for audio and visual encoders - Update blip2 network with improved temporal coherence for video 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/NeuralNetworks/AudioVisualCorrespondenceNetwork.cs (1)
1078-1078: Consider making the learning rate configurable.The learning rate is hardcoded to 0.001. While this is a reasonable default, consider exposing it as a parameter or reading it from the architecture configuration for better flexibility.
💡 Possible enhancement
- T learningRate = NumOps.FromDouble(0.001); // Default learning rate + T learningRate = Architecture.LearningRate ?? NumOps.FromDouble(0.001);Or add a property:
public T LearningRate { get; set; } = NumOps.FromDouble(0.001);src/NeuralNetworks/Blip2NeuralNetwork.cs (5)
415-418: Tokenizer mismatch: BLIP-2 uses BERT-based tokenization, not CLIP.BLIP-2's Q-Former text encoder is BERT-based (hence
vocabularySize = 30522), but the default tokenizer here isClipTokenizerFactory.CreateSimple(), which uses CLIP's BPE tokenizer with a different vocabulary. This mismatch could cause incorrect tokenization and degraded performance.🔎 Suggested fix
Consider providing a BERT-compatible tokenizer as the default, or document that users must provide an appropriate tokenizer:
- _tokenizer = tokenizer ?? Tokenization.ClipTokenizerFactory.CreateSimple(); + _tokenizer = tokenizer ?? throw new ArgumentNullException(nameof(tokenizer), + "BLIP-2 requires a BERT-compatible tokenizer. Please provide one explicitly.");Alternatively, if the project has a BERT tokenizer factory:
- _tokenizer = tokenizer ?? Tokenization.ClipTokenizerFactory.CreateSimple(); + _tokenizer = tokenizer ?? Tokenization.BertTokenizerFactory.CreateSimple();
464-466: Cross-attention uses self-attention layer, limiting functionality.
TransformerEncoderLayerperforms self-attention, but Q-Former cross-attention requires queries to attend to separate key/values (vision features). The current implementation inApplyCrossAttention(lines 1060-1125) manually implements cross-attention, making these layers unused for their intended purpose.This is a minor issue since the manual implementation works, but it results in unused layer parameters being included in
ParameterCount.
877-882: ITM reranking is not implemented inRetrieveImages.The
useItmRerankingparameter is accepted but the actual reranking logic is not implemented. The method returns ITC results regardless of this flag. Consider either implementing the reranking or removing the parameter to avoid misleading users.
1493-1500: Potential indexing issue with ONNX output tensor.Using
outputIds.Length(line 1497) returns the total element count, but the loop indexes withoutputIds[0, i]which assumes a 2D tensor. While this happens to work for single-batch cases, it's fragile.🔎 Suggested fix
// Convert output IDs to list var generatedIds = new List<int>(); - for (int i = 0; i < outputIds.Length; i++) + int seqLength = (int)outputIds.Dimensions[^1]; // Last dimension is sequence length + for (int i = 0; i < seqLength; i++) { generatedIds.Add((int)outputIds[0, i]); }
2154-2172: Deserialization reads but discards data with no validation.
DeserializeNetworkSpecificDatareads serialized values into local variables but neither uses them nor validates consistency with the current instance. The comment mentions validation, but none is performed.This means serialized networks may silently load with mismatched configurations.
🔎 Suggested fix
Add actual validation:
protected override void DeserializeNetworkSpecificData(BinaryReader reader) { int embeddingDim = reader.ReadInt32(); int maxSeqLen = reader.ReadInt32(); int imageSize = reader.ReadInt32(); int qformerHiddenDim = reader.ReadInt32(); int numQformerLayers = reader.ReadInt32(); int numHeads = reader.ReadInt32(); int numQueryTokens = reader.ReadInt32(); int patchSize = reader.ReadInt32(); int vocabularySize = reader.ReadInt32(); int visionHiddenDim = reader.ReadInt32(); int lmHiddenDim = reader.ReadInt32(); string languageModelType = reader.ReadString(); bool useNativeMode = reader.ReadBoolean(); - // Note: Since fields are readonly, this just validates consistency - // In practice, you'd need to reconstruct the network if dimensions differ + // Validate consistency + if (embeddingDim != _embeddingDimension || + imageSize != _imageSize || + numQueryTokens != _numQueryTokens || + useNativeMode != _useNativeMode) + { + throw new InvalidOperationException( + "Serialized network configuration does not match current instance. " + + "Create a new instance with matching parameters to load this data."); + } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NeuralNetworks/AudioVisualCorrespondenceNetwork.cssrc/NeuralNetworks/Blip2NeuralNetwork.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/Blip2NeuralNetwork.cssrc/NeuralNetworks/AudioVisualCorrespondenceNetwork.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/Blip2NeuralNetwork.cssrc/NeuralNetworks/AudioVisualCorrespondenceNetwork.cs
🔇 Additional comments (8)
src/NeuralNetworks/AudioVisualCorrespondenceNetwork.cs (1)
675-681: Double GELU activation in encoder forward pass.Lines 678-679 apply GELU twice: once internally in the first FFN layer (constructed with
geluActivationat line 139), then again externally viaApplyGelu(ffn1)before passing to the second FFN. Standard transformer blocks apply GELU once between the two FFN layers.Option 1 (Recommended): Remove the external GELU call since the first FFN already has it built-in:
var current = projected; for (int i = 0; i < _audioEncoderLayers.Count; i += 3) { var attnOutput = _audioEncoderLayers[i].Forward(current); var ffn1 = _audioEncoderLayers[i + 1].Forward(attnOutput); - var ffn2 = _audioEncoderLayers[i + 2].Forward(ApplyGelu(ffn1)); + var ffn2 = _audioEncoderLayers[i + 2].Forward(ffn1); current = AddResidual(attnOutput, ffn2); }Option 2: If GELU should only be applied once externally, change the first FFN to use
nullActivationduring construction (line 139).src/NeuralNetworks/Blip2NeuralNetwork.cs (7)
13-44: LGTM!The class declaration and documentation are well-structured with clear explanations of the BLIP-2 architecture and its components.
46-235: LGTM!Field organization is clean with clear separation between ONNX mode, native mode, and shared configuration.
276-360: LGTM!The ONNX constructor implements robust validation and proper exception-safe resource cleanup.
543-623: LGTM!The
IMultimodalEmbeddingimplementation correctly handles input validation and provides proper dimension checks.
1215-1247: LGTM! ONNX tensor handling has been fixed.The implementation now correctly handles both 3D
[batch, num_query_tokens, hidden_size]and 2D[batch, hidden_size]Q-Former outputs with appropriate indexing and mean pooling.
1762-1798: LGTM! Parameter API consistency issue has been resolved.
ParameterCount,GetParameters, andSetParametersnow all operate on the same consistent set of parameters: query tokens, query positional embeddings, Q-Former layers, and projection heads.Also applies to: 1801-1886, 1889-1990
2198-2208: LGTM!The
Disposemethod correctly disposes all ONNX inference sessions.
- dalle3model: replace simple substring matching with compiled regex patterns for accurate prompt safety checking with word boundaries - blipneuralnetwork: use randomhelper.threadsaferandom for proper seed diversity, add bounds checking for skip/take operations, fix parametercount property name - gpt4visioneuralnetwork: improve safety check parsing to require category and yes/flagged on same line (prevents false positives), use thread-safe random - llavaneuralnetwork: cap temperature at 1.2 to prevent degenerate outputs, use thread-safe random, fix train method to use optimizer.updateparameters 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…eration - Add TransformerDecoderLayer-based LM decoder for text generation - Implement proper autoregressive generation with temperature sampling - Refactor query tokens and embeddings from Matrix<T> to Tensor<T> - Add gradient tracking for query tokens and positional embeddings - Add LM head for vocabulary logit projection 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Replace covariant return type with IFullModel in FlamingoNeuralNetwork - Replace Math.Clamp with Math.Max/Min for net471 compatibility - Fix String.Split signature to use char array overload for net471 - Fix null reference flow analysis in CreateNewInstance methods 🤖 Generated with [Claude Code](https://claude.ai/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Security fixes: - Replace 7 instances of new Random() with RandomHelper for thread-safe cryptographically seeded random number generation - Add RegexTimeout (1 second) to 12 Regex patterns to prevent ReDoS attacks - Add proper using statements for System.Text.RegularExpressions Files updated: - DallE3Model.cs: 5 compiled Regex patterns with timeout - Gpt4VisionNeuralNetwork.cs: 2 dynamic Regex patterns with timeout - StopWordRemovalQueryProcessor.cs: Regex.Split with timeout - LemmatizationQueryProcessor.cs: Regex.Split with timeout - KeywordExtractionQueryProcessor.cs: Regex.Split with timeout - SubQueryExpansion.cs: 2 Regex.Split calls with timeout - MultiQueryExpansion.cs: Regex.Split with timeout - Blip2/Flamingo/ImageBind/LLaVA/VideoCLIP NeuralNetwork.cs: RandomHelper 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Resolves merge conflicts between the CLIP multimodal embedding feature and master branch changes. Key resolutions: - EmbeddingsController.cs: Accepted master's more mature implementation with numeric type handling (Double, Float, Decimal switch statements) - IServableMultimodalModel.cs: Accepted master's version (trivial diff) - ModelEntry.cs: Kept feature branch's descriptive comments - IMultimodalEmbedding.cs: Kept feature branch's Tensor<T>-based API - ClipNeuralNetwork.cs: Kept feature branch's full native+ONNX implementation - ServableClipModel.cs: Updated adapter to bridge between IMultimodalEmbedding (Tensor-based) and IServableMultimodalModel (double[]-based) interfaces The ServableClipModel adapter converts between the interfaces by: - Converting double[] to Tensor<T> for image encoding - Converting IEnumerable<Vector<T>> to Matrix<T> for batch operations
Replace hardcoded double[] with generic Vector<T> in IServableMultimodalModel to maintain consistency with the library's fully generic design. Vector<T> provides span-based optimizations that double[] lacks. Changes: - IServableMultimodalModel: EncodeImage, EncodeImageBatch, ZeroShotClassify now accept Vector<T> instead of double[] - ServableClipModel: Simplified by removing ConvertDoubleArrayToImageTensor and unused _numOps field - EmbeddingsController: Updated to convert double[] from REST API to Vector<T> at the controller boundary before calling model methods
|
…1563) 0.92.5 is the first release carrying Tensors #583 (validate the prepacked-B GEMM cache against in-place weight mutation). The SgemmWithCachedB inference path reused a stale pre-packed B panel when a layer's weights were mutated in place without a MarkWeightDirty, so FusedLinear inference returned pre-update results — the root cause of the #1221 transformer production-scale convergence failure. Bumps the AiDotNet.Native.* packages (OneDNN/OpenBLAS/CLBlast) in lockstep, all published at 0.92.5. Restores clean (no NU1102); the 11 Issue1221 convergence/harness tests pass. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…1563) 0.92.5 is the first release carrying Tensors #583 (validate the prepacked-B GEMM cache against in-place weight mutation). The SgemmWithCachedB inference path reused a stale pre-packed B panel when a layer's weights were mutated in place without a MarkWeightDirty, so FusedLinear inference returned pre-update results — the root cause of the #1221 transformer production-scale convergence failure. Bumps the AiDotNet.Native.* packages (OneDNN/OpenBLAS/CLBlast) in lockstep, all published at 0.92.5. Restores clean (no NU1102); the 11 Issue1221 convergence/harness tests pass. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Implements Issue #272: Multimodal Embeddings - CLIP-Style Text/Image Encoders
This PR adds CLIP (Contrastive Language-Image Pre-training) support to AiDotNet:
<|startoftext|>,<|endoftext|>)Features
Usage
Known Limitations
Test plan
🤖 Generated with Claude Code