chore: enable JIT compilation for remaining unsupported layers

[ooples]

PR Title (Auto-Fixed)

Note: PR titles are automatically fixed to follow Conventional Commits format for automated releases.

The workflow will intelligently detect the appropriate type based on:

• Title keywords (fix/add/implement/update/etc.)
• Files changed (docs/tests/ci/source files)
• Default to chore: if unsure

If the auto-detected type is incorrect, simply edit the PR title manually.

User Story / Context

• Reference: [US-XXX] (if applicable)
• Base branch: merge-dev2-to-master

Summary

• What changed and why (scoped strictly to the user story / PR intent)

Verification

• Builds succeed (scoped to changed projects)
• Unit tests pass locally
• Code coverage >= 90% for touched code
• Codecov upload succeeded (if token configured)
• TFM verification (net46, net6.0, net8.0) passes (if packaging)
• No unresolved Copilot comments on HEAD

Copilot Review Loop (Outcome-Based)

Record counts before/after your last push:

• Comments on HEAD BEFORE: [N]
• Comments on HEAD AFTER (60s): [M]
• Final HEAD SHA: [sha]

Files Modified

• List files changed (must align with scope)

Notes

• Any follow-ups, caveats, or migration details

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

src/AiDotNet.Tensors/LinearAlgebra/TensorBase.cs (1)

262-294: Flat index accessors are correct and consistent; only micro‑refactors are optional

The implementations of GetFlat and SetFlat look correct and align with the rest of TensorBase<T>:

• Bounds checks match the documented range (0 to Length - 1).
• Exceptions (ArgumentOutOfRangeException with flatIndex param name) are appropriate and explicit.
• Semantics are consistent with the existing row‑major layout and with how GetFlatIndex/GetIndices treat flat indices.

If you care to micro‑tune later, two optional nits:

• You could rely on _data[flatIndex]’s own bounds checks and skip the explicit if to avoid duplicating range logic, at the cost of a slightly less descriptive exception.
• If more flat helpers appear, consider a small internal helper like EnsureValidFlatIndex(int flatIndex) to centralize the guard.

Nothing blocking here; this is ready to use as‑is.

docs/JIT_ROADMAP.md (1)

277-277: Use markdown headings instead of emphasis for batch labels.

Lines use bold emphasis (**Batch N**) instead of proper markdown headings, triggering linter warnings (MD036). Replace with #### headings for consistency with documentation standards.

Apply this diff to fix the formatting:

- **Batch 1: Simple Utility Layers (Week 1)**
+ #### Batch 1: Simple Utility Layers (Week 1)

- **Batch 2: Core Vision Layers (Week 2)**
+ #### Batch 2: Core Vision Layers (Week 2)

- **Batch 3: Normalization & Regularization (Week 3)**
+ #### Batch 3: Normalization & Regularization (Week 3)

- **Batch 4: Recurrent Layers (Weeks 4-5)**
+ #### Batch 4: Recurrent Layers (Weeks 4-5)

- **Batch 5: Attention Layers (Weeks 6-7)**
+ #### Batch 5: Attention Layers (Weeks 6-7)

Also applies to: 281-281, 285-285, 289-289, 293-293

docs/JIT_COMPILATION_PATTERN_GUIDE.md (1)

570-587: Minor style suggestion: consider rephrasing "Very large" in troubleshooting.

LanguageTool flags "Very large or complex graphs" as weak intensification. Consider rephrasing to "Large or complex graphs" or "Graphs that are large and complex" for stronger writing.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1fd7cf1 and 62c8d73.

📒 Files selected for processing (5)

• docs/JIT_ACTIVATION_MAPPING.md (1 hunks)
• docs/JIT_COMPILATION_PATTERN_GUIDE.md (1 hunks)
• docs/JIT_ROADMAP.md (1 hunks)
• src/AiDotNet.Tensors/LinearAlgebra/Tensor.cs (5 hunks)
• src/AiDotNet.Tensors/LinearAlgebra/TensorBase.cs (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/AiDotNet.Tensors/LinearAlgebra/Tensor.cs (2)

src/AiDotNet.Tensors/Helpers/RandomHelper.cs (1)

• RandomHelper (19-108)

src/Extensions/EnumerableExtensions.cs (1)

• T (35-39)

🪛 LanguageTool

docs/JIT_ROADMAP.md

[grammar] ~328-~328: Ensure spelling is correct
Context: ...ons ### Integration Requirements - [ ] IEngine operations used (for GPU acceleration) ...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

docs/JIT_COMPILATION_PATTERN_GUIDE.md

[style] ~570-~570: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ... Compilation takes too long Cause: Very large or complex graphs can take time to comp...

(EN_WEAK_ADJECTIVE)

🪛 markdownlint-cli2 (0.18.1)

docs/JIT_ROADMAP.md

277-277: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

281-281: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

285-285: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

289-289: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

293-293: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

⏰ 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). (1)

• GitHub Check: Build All Frameworks

🔇 Additional comments (14)

docs/JIT_ACTIVATION_MAPPING.md (2)

7-9: Verify activation count total.

The header states 37 total activations (10 production-ready + 27 pending), but the detailed sections list 10 + 27 + 6 = 43 distinct activations. Reconcile the count or clarify the categorization.

---

169-169: Clarify LeakyReLU integration status.

LeakyReLU is listed in "Available Activations - Pending Integration" (line 169) but the production-ready CanActivationBeJitted() example (line 183-184) includes it. Verify the intended status: is it production-ready or pending?

Also applies to: 183-184

docs/JIT_ROADMAP.md (1)

301-333: Acceptance criteria are well-defined and actionable.

The checklist-style criteria clearly specify code, documentation, testing, and integration requirements for each layer. This provides a solid reference implementation template for Phase 3 rollout.

docs/JIT_COMPILATION_PATTERN_GUIDE.md (6)

1-54: Comprehensive and well-structured JIT implementation guide.

The guide provides clear value with overview, current status, prerequisites, and strategic organization. Positioning JIT as inference-focused (forward pass only) sets correct expectations early.

---

83-169: ExportComputationGraph walkthrough is thorough with good inline guidance.

Step 1 includes numbered substeps, validation checks, symbolic batch dimension explanation, and parameter handling. Code comments explain the "why" behind each operation (e.g., weight transposition for efficient computation).

---

178-242: ApplyActivationToGraph example demonstrates parameterized activation handling well.

The activation mapping pattern correctly distinguishes scalar vs. vector activations, extracts parameters from types (e.g., elu.Alpha), and provides clear error messaging. Matches the DenseLayer production reference mentioned in Phase 2.

---

251-290: CanActivationBeJitted whitelist is clean and extensible.

Simple boolean logic with no activation = identity fallback. Easy pattern for developers to extend as more activations are added in Phase 3.

---

505-601: Troubleshooting section provides practical solutions for common issues.

Each issue includes cause, solution, and code examples. Addresses realistic pain points: unsupported activations, uninitialized weights, shape mismatches, backward pass placeholders, and performance considerations.

---

604-707: ConvolutionalLayer complete example ties all patterns together effectively.

Shows how the five-step pattern applies to a different layer type, including Conv2D-specific concerns (kernel shapes, stride/padding/dilation) and the same activation mapping strategy.

src/AiDotNet.Tensors/LinearAlgebra/Tensor.cs (5)

375-375: Security improvement: cryptographically secure RNG.

Good change switching from new Random() to RandomHelper.CreateSecureRandom(). This uses a cryptographically secure seed, making the random initialization less predictable and more secure for ML applications where initialization randomness can affect training outcomes.

---

467-509: LGTM: Consistent broadcasting semantics for 2D and 3D tensors.

The expansion to support both 2D and 3D tensors is well-implemented with consistent broadcasting semantics. In both cases, the vector is broadcast across all dimensions except the last, with appropriate validation to ensure the vector length matches the last dimension.

---

2287-2316: LGTM: Transpose documentation and implementation properly updated.

The dimension-aware transpose behavior is now correctly implemented and documented:

• 1D tensors return a copy (no-op for vectors)
• 2D tensors perform standard matrix transpose (swap rows/columns)
• N-D tensors reverse all dimensions by default

This addresses the past review comment requesting documentation updates for the different tensor ranks.

---

2329-2342: LGTM: Useful addition for batched matrix operations.

The TransposeLast2D method is a well-designed API for batch-aware transpose operations. It correctly swaps only the last two dimensions while preserving batch dimensions, which is essential for batched matrix operations like A @ B.TransposeLast2D().

---

1384-1509: Verify the batched matrix multiplication implementation with comprehensive tests.

The batched matrix multiplication implementation is complex, involving NumPy-style batch broadcasting, stride calculations, and offset computations. While the code structure appears sound, it currently lacks dedicated test coverage.

Add tests to verify:

• Different batch shapes requiring broadcasting (e.g., [1,2,3,4] @ [2,1,4,5])
• Edge cases with single-element batch dimensions
• Large batch sizes to check for integer overflow in offset calculations
• Correctness against known results for various tensor ranks and shapes

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

src/AiDotNet.csproj (1)

103-125: Assess global usings for namespace clarity and potential conflicts.

Global usings improve developer ergonomics but reduce explicit dependency visibility. Verify:

1. No naming conflicts between AiDotNet.Tensors.* and existing AiDotNet.* namespaces.
2. Internal-only namespaces (AiDotNet.Autodiff, AiDotNet.Helpers) are appropriate for global scope.
3. All global usings are documented in project or architecture guidelines.

Consider documenting the rationale in an inline comment or ADR to clarify why these specific namespaces warrant global import.

Add an inline comment explaining the global usings strategy:

  <!-- Global usings for AiDotNet.Tensors namespaces -->
+ <!-- These are fundamental to the new Tensors infrastructure and reduce boilerplate in downstream files -->
  <ItemGroup>
    <Using Include="AiDotNet.Tensors.LinearAlgebra" />
    <Using Include="AiDotNet.Tensors.Engines" />
    <Using Include="AiDotNet.Tensors.Interfaces" />
    <Using Include="AiDotNet.Tensors.NumericOperations" />
    <Using Include="AiDotNet.Tensors.Helpers" />
  </ItemGroup>

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 62c8d73 and e223be7.

📒 Files selected for processing (1)

• src/AiDotNet.csproj (2 hunks)

⏰ 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). (1)

• GitHub Check: Build All Frameworks

🔇 Additional comments (1)

src/AiDotNet.csproj (1)

93-96: Polyfill exclusion is correct and well-justified.

LanguageFeaturePolyfills.cs defines Index and Range structs that are identical to those in AiDotNet.Tensors/IndexPolyfill.cs. Since AiDotNet references AiDotNet.Tensors as a project dependency and includes global usings for its namespaces, excluding the duplicate file prevents compilation conflicts while maintaining all required polyfill functionality for .NET Framework support.

[coderabbitai]

Actionable comments posted: 6

🧹 Nitpick comments (15)

src/Configuration/InferenceOptimizationConfig.cs (1)

43-48: Consider caching static preset instances.

The static properties Default and HighPerformance create new instances on every access. If these presets are accessed frequently, consider caching them to avoid unnecessary allocations.

Apply this diff to cache the instances:

-    public static InferenceOptimizationConfig Default => new()
+    private static readonly InferenceOptimizationConfig _default = new()
     {
         EnableKVCache = true,
         EnableBatching = true,
         EnableSpeculativeDecoding = false
     };
+    
+    public static InferenceOptimizationConfig Default => _default;

-    public static InferenceOptimizationConfig HighPerformance => new()
+    private static readonly InferenceOptimizationConfig _highPerformance = new()
     {
         EnableKVCache = true,
         KVCacheMaxSizeMB = 2048,
         EnableBatching = true,
         MaxBatchSize = 64,
         EnableSpeculativeDecoding = true,
         SpeculationDepth = 5
     };
+    
+    public static InferenceOptimizationConfig HighPerformance => _highPerformance;

Also applies to: 59-67

src/AiDotNet.Serving/Batching/BatchingStrategyBase.cs (2)

135-150: Nested lock acquisitions within GetStatistics are redundant.

GetStatistics acquires SyncLock, then calls GetAverageLatency() and GetLatencyPercentile() which each acquire SyncLock again. While C# Monitor locks are reentrant so this won't deadlock, it adds unnecessary overhead and obscures the locking intent.

Consider extracting lock-free internal helpers:

 public virtual Dictionary<string, object> GetStatistics()
 {
     lock (SyncLock)
     {
         return new Dictionary<string, object>
         {
             ["name"] = Name,
             ["totalBatchesProcessed"] = TotalBatchesProcessed,
-            ["averageLatencyMs"] = GetAverageLatency(),
-            ["p50LatencyMs"] = GetLatencyPercentile(50),
-            ["p95LatencyMs"] = GetLatencyPercentile(95),
-            ["p99LatencyMs"] = GetLatencyPercentile(99),
+            ["averageLatencyMs"] = GetAverageLatencyUnsafe(),
+            ["p50LatencyMs"] = GetLatencyPercentileUnsafe(50),
+            ["p95LatencyMs"] = GetLatencyPercentileUnsafe(95),
+            ["p99LatencyMs"] = GetLatencyPercentileUnsafe(99),
             ["sampleCount"] = LatencyHistory.Count
         };
     }
 }
+
+private double GetAverageLatencyUnsafe() =>
+    LatencyHistory.Count == 0 ? 0 : TotalLatencyMs / LatencyHistory.Count;

---

117-129: GetLatencyPercentile allocates and sorts on every call.

For high-frequency metrics collection, creating a sorted list via OrderBy(...).ToList() on each invocation may introduce GC pressure and latency spikes.

If percentiles are queried frequently, consider maintaining a sorted data structure (e.g., SortedList or reservoir sampling) or caching sorted snapshots periodically.

src/AiDotNet.Serving/Controllers/InferenceController.cs (1)

386-417: Consider early 501 return for unimplemented LoRA endpoint.

The endpoint validates potentially large TrainingFeatures and TrainingLabels arrays before returning 501 Not Implemented. For large payloads, this validation is wasted work.

Consider returning 501 immediately after model existence check, or adding a feature flag to skip validation entirely:

+            // Feature not implemented - return early before validating large payloads
+            sw.Stop();
+            return StatusCode(501, new LoRAFineTuneResponse
+            {
+                Success = false,
+                Error = "LoRA fine-tuning is not yet implemented for REST API serving...",
+                ...
+            });
+
             if (request.TrainingFeatures == null || request.TrainingFeatures.Length == 0)
             ...

However, keeping validation provides better feedback if users test the API contract. This is a trade-off depending on expected usage patterns.

src/AiDotNet.Serving/Services/RequestBatcherBase.cs (2)

1-1: Unused import.

System.Collections.Concurrent is imported but ConcurrentQueue or other concurrent collections are not used in this base class. Derived classes have their own imports.

-using System.Collections.Concurrent;

---

54-75: Mixed synchronization for statistics counters.

TotalRequests uses Interlocked.Increment (line 147), while TotalBatches, TotalBatchSize, and TotalLatencyMs are updated under StatsLock. This is functionally correct but creates an inconsistency. Consider using Interlocked for all counters or locking for all.

src/AiDotNet.Serving/Services/ContinuousBatchingRequestBatcher.cs (4)

237-242: Fire-and-forget pattern discards task without observation.

The _ = Task.WhenAll(tasks) discards the aggregate task. If any task in tasks faults after being started but before individual exception handling, the exception may go unobserved. Since ProcessRequestAsync has its own try-catch, this is likely safe, but consider adding .ContinueWith(t => { /* log unobserved */ }, TaskContinuationOptions.OnlyOnFaulted) for defensive logging.

---

304-330: Synchronous model prediction may block thread pool threads.

model.Predict(input) is called synchronously within an async context. If prediction is CPU-intensive, this blocks a thread pool thread. Consider wrapping in Task.Run if predictions are heavy, or document that the model's Predict should be non-blocking.

 private Task ProcessTypedRequest<T>(ContinuousRequest request, CancellationToken cancellationToken)
 {
     var model = ModelRepository.GetModel<T>(request.ModelName);
     if (model == null)
     {
         SetRequestException(request, new InvalidOperationException(
             $"Model '{request.ModelName}' not found or wrong numeric type"));
         return Task.CompletedTask;
     }

     try
     {
         var input = (Vector<T>)request.Input;
-        var result = model.Predict(input);
+        // Consider Task.Run for CPU-intensive predictions
+        var result = model.Predict(input);

         if (request.CompletionSource is TaskCompletionSource<Vector<T>> tcs)
         {
             tcs.TrySetResult(result);
         }
     }

---

335-340: Reflection-based exception setting is fragile.

Using reflection to invoke TrySetException is less type-safe and slower than alternatives. Consider storing typed TaskCompletionSource<Vector<T>> references or using a type-erased wrapper that exposes a SetException(Exception) method directly.

---

413-422: Consider using non-nullable init properties.

Input and CompletionSource are initialized to null! which suppresses nullability warnings but doesn't guarantee initialization. Since this is a private class with controlled construction, it's acceptable, but required properties (C# 11+) would be cleaner if targeting a recent framework.

src/AiDotNet.Serving/Batching/ContinuousBatchingStrategy.cs (2)

72-87: Race condition on _lastProcessTime access.

_lastProcessTime is read and written without synchronization (lines 79, 85), but GetStatistics doesn't expose it and this field isn't critical for correctness. However, concurrent calls to ShouldProcessBatch could see stale values. Consider using volatile or moving the check under SyncLock if precise throttling is important.

---

154-165: Double-locking but no deadlock risk.

base.GetStatistics() acquires SyncLock, then this method acquires it again (line 157). Since it's the same reentrant-capable lock object (C# lock is reentrant), this is safe but slightly inefficient. Consider calling base.GetStatistics() inside the lock or refactoring to avoid double acquisition.

 public override Dictionary<string, object> GetStatistics()
 {
-    var stats = base.GetStatistics();
     lock (SyncLock)
     {
+        var stats = base.GetStatistics();
         stats["currentConcurrency"] = _currentOptimalConcurrency;
         stats["maxConcurrency"] = _maxConcurrency;
         stats["targetLatencyMs"] = _targetLatencyMs;
         stats["adaptiveConcurrency"] = _adaptiveConcurrency;
+        return stats;
     }
-    return stats;
 }

src/AiDotNet.Serving/Models/PredictionRequest.cs (1)

226-236: Add validation for SaveModel/SavePath invariant when LoRA fine-tuning is implemented.

The DTO correctly documents that SavePath must be provided when SaveModel is true, but this invariant is not validated in the controller. When the fine-tuning feature moves beyond the 501 Not Implemented status, add a validation check: if request.SaveModel is true, ensure request.SavePath is not null or whitespace.

src/AiDotNet.Serving/Services/ModelStartupService.cs (2)

78-97: Consider propagating cancellation token to LoadModelAsync.

The cancellation check at line 80 prevents starting new model loads, but once LoadModelAsync begins (line 88), the operation cannot be cancelled. For large models or slow I/O, consider adding a CancellationToken parameter to LoadModelAsync and passing it through to enable mid-operation cancellation.

Apply this diff to enable cancellation during model loading:

-    private async Task LoadModelAsync(StartupModel modelConfig)
+    private async Task LoadModelAsync(StartupModel modelConfig, CancellationToken cancellationToken)
     {
         // ... validation code ...
         
         // Load model based on numeric type
         // Using Task.Run to avoid blocking the startup thread for file I/O
-        await Task.Run(() =>
+        await Task.Run(() =>
         {
             switch (modelConfig.NumericType)
             {
                 case NumericType.Float:
                     LoadTypedModel<float>(modelConfig.Name, modelPath);
                     break;
                 // ... other cases ...
             }
-        });
+        }, cancellationToken);

And update the call site:

-                await LoadModelAsync(modelConfig);
+                await LoadModelAsync(modelConfig, cancellationToken);

---

188-194: Consider validating metadata dimensions more strictly.

The code defaults to inputDim=1 when FeatureCount=0 (line 189) and outputDim=1 when not found in metadata (lines 192-194). While these defaults enable the service to start, they may hide model configuration issues.

Consider logging warnings when defaults are used:

 var metadata = modelResult.GetModelMetadata();
 var inputDim = metadata.FeatureCount > 0 ? metadata.FeatureCount : 1;
+if (metadata.FeatureCount == 0)
+{
+    _logger.LogWarning("Model '{Name}' has FeatureCount=0, defaulting to inputDim=1", name);
+}
+
 var outputDim = metadata.Properties.TryGetValue("OutputDimension", out var outputDimValue) && outputDimValue is int dim
     ? dim
     : 1;
+if (!metadata.Properties.ContainsKey("OutputDimension"))
+{
+    _logger.LogWarning("Model '{Name}' missing OutputDimension metadata, defaulting to outputDim=1", name);
+}

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e223be7 and 5004768.

📒 Files selected for processing (13)

• src/AiDotNet.Serving/Batching/BatchingStrategyBase.cs (1 hunks)
• src/AiDotNet.Serving/Batching/ContinuousBatchingStrategy.cs (1 hunks)
• src/AiDotNet.Serving/Configuration/ServingOptions.cs (4 hunks)
• src/AiDotNet.Serving/Controllers/InferenceController.cs (2 hunks)
• src/AiDotNet.Serving/Controllers/ModelsController.cs (3 hunks)
• src/AiDotNet.Serving/Models/PredictionRequest.cs (1 hunks)
• src/AiDotNet.Serving/Program.cs (1 hunks)
• src/AiDotNet.Serving/Services/ContinuousBatchingRequestBatcher.cs (1 hunks)
• src/AiDotNet.Serving/Services/ModelStartupService.cs (1 hunks)
• src/AiDotNet.Serving/Services/RequestBatcher.cs (4 hunks)
• src/AiDotNet.Serving/Services/RequestBatcherBase.cs (1 hunks)
• src/AiDotNet.csproj (3 hunks)
• src/Configuration/InferenceOptimizationConfig.cs (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (10)

src/AiDotNet.Serving/Program.cs (1)

src/AiDotNet.Serving/Services/ModelStartupService.cs (2)

• ModelStartupService (39-252)
• ModelStartupService (51-59)

src/AiDotNet.Serving/Services/RequestBatcherBase.cs (3)

src/AiDotNet.Serving/Services/RequestBatcher.cs (4)

• Task (142-197)
• Dictionary (507-531)
• Dictionary (536-552)
• SetException (497-502)

src/AiDotNet.Serving/Batching/BatchingStrategyBase.cs (1)

• Dictionary (135-150)

src/AiDotNet.Serving/Batching/ContinuousBatchingStrategy.cs (1)

• Dictionary (154-165)

src/AiDotNet.Serving/Batching/ContinuousBatchingStrategy.cs (3)

src/AiDotNet.Serving/Batching/BatchingStrategyBase.cs (5)

• BatchingStrategyBase (26-159)
• ShouldProcessBatch (66-66)
• GetOptimalBatchSize (74-74)
• UpdatePerformanceFeedback (81-96)
• Dictionary (135-150)

src/AiDotNet.Serving/Services/RequestBatcher.cs (2)

• Dictionary (507-531)
• Dictionary (536-552)

src/AiDotNet.Serving/Services/ContinuousBatchingRequestBatcher.cs (2)

• Dictionary (153-172)
• Dictionary (178-188)

src/AiDotNet.Serving/Controllers/ModelsController.cs (4)

src/AiDotNet.Serving/Models/ModelInfo.cs (2)

• ModelInfo (6-37)
• LoadModelResponse (65-81)

src/AiDotNet.Serving/Services/ModelRepository.cs (3)

• ModelInfo (96-104)
• ModelInfo (119-136)
• LoadModel (25-46)

src/AiDotNet.Serving/Models/IServableModel.cs (2)

• Matrix (25-25)
• Vector (17-17)

src/AiDotNet.Serving/Models/ServableModelWrapper.cs (5)

• Matrix (109-137)
• Vector (96-106)
• ServableModelWrapper (11-138)
• ServableModelWrapper (27-39)
• ServableModelWrapper (47-84)

src/AiDotNet.Serving/Services/ModelStartupService.cs (3)

src/AiDotNet.Serving/Configuration/ServingOptions.cs (2)

• ServingOptions (58-170)
• StartupModel (175-193)

src/AiDotNet.Serving/Controllers/ModelsController.cs (1)

• NumericType (279-292)

src/Models/Results/PredictionModelResult.cs (1)

• LoadFromFile (1367-1381)

src/AiDotNet.Serving/Services/ContinuousBatchingRequestBatcher.cs (10)

src/AiDotNet.Serving/Services/RequestBatcherBase.cs (11)

• RequestBatcherBase (30-206)
• RequestBatcherBase (83-91)
• Task (101-101)
• SetException (167-170)
• IncrementRequestCount (145-148)
• Dictionary (107-119)
• Dictionary (125-125)
• RecordBatch (132-140)
• DisposeManagedResources (202-205)
• Dispose (175-179)
• Dispose (185-197)

src/AiDotNet.Serving/Services/ModelStartupService.cs (3)

• Task (65-107)
• Task (113-117)
• Task (122-170)

src/AiDotNet.Serving/Configuration/ServingOptions.cs (1)

• ServingOptions (58-170)

src/LinearAlgebra/ConfusionMatrix.cs (1)

• Increment (296-311)

src/AiDotNet.Serving/Controllers/ModelsController.cs (1)

• NumericType (279-292)

src/AiDotNet.Serving/Batching/BatchingStrategyBase.cs (1)

• Dictionary (135-150)

src/AiDotNet.Serving/Batching/ContinuousBatchingStrategy.cs (1)

• Dictionary (154-165)

src/MixedPrecision/MixedPrecisionTrainingLoop.cs (1)

• GetStatistics (180-187)

src/Diagnostics/Profiler.cs (1)

• Stop (364-377)

src/AiDotNet.Serving/Services/ModelRepository.cs (1)

• ModelRepository (10-148)

src/AiDotNet.Serving/Batching/BatchingStrategyBase.cs (4)

src/AiDotNet.Serving/Services/RequestBatcher.cs (3)

• IBatchingStrategy (93-118)
• Dictionary (507-531)
• Dictionary (536-552)

src/AiDotNet.Serving/Batching/ContinuousBatchingStrategy.cs (4)

• ShouldProcessBatch (72-87)
• GetOptimalBatchSize (96-103)
• UpdatePerformanceFeedback (110-134)
• Dictionary (154-165)

src/Polyfills/PriorityQueuePolyfill.cs (1)

• Enqueue (75-79)

src/AiDotNet.Serving/Services/RequestBatcherBase.cs (2)

• Dictionary (107-119)
• Dictionary (125-125)

src/AiDotNet.Serving/Services/RequestBatcher.cs (7)

src/AiDotNet.Serving/Batching/TimeoutBatchingStrategy.cs (2)

• TimeoutBatchingStrategy (6-38)
• TimeoutBatchingStrategy (16-20)

src/AiDotNet.Serving/Batching/SizeBatchingStrategy.cs (2)

• SizeBatchingStrategy (6-39)
• SizeBatchingStrategy (16-20)

src/AiDotNet.Serving/Batching/BucketBatchingStrategy.cs (2)

• BucketBatchingStrategy (7-70)
• BucketBatchingStrategy (19-25)

src/AiDotNet.Serving/Batching/ContinuousBatchingStrategy.cs (2)

• ContinuousBatchingStrategy (28-166)
• ContinuousBatchingStrategy (45-56)

src/AiDotNet.Serving/Batching/AdaptiveBatchingStrategy.cs (2)

• AdaptiveBatchingStrategy (7-98)
• AdaptiveBatchingStrategy (31-45)

src/AiDotNet.Serving/Padding/BucketPaddingStrategy.cs (2)

• BucketPaddingStrategy (9-111)
• BucketPaddingStrategy (20-26)

src/AiDotNet.Serving/Padding/MinimalPaddingStrategy.cs (1)

• MinimalPaddingStrategy (9-80)

src/AiDotNet.Serving/Controllers/InferenceController.cs (1)

src/AiDotNet.Serving/Models/PredictionRequest.cs (4)

• SpeculativeDecodingResponse (117-153)
• SpeculativeDecodingRequest (61-112)
• LoRAFineTuneResponse (247-294)
• LoRAFineTuneRequest (169-242)

src/AiDotNet.Serving/Configuration/ServingOptions.cs (1)

src/AiDotNet.Serving/Controllers/ModelsController.cs (1)

• NumericType (279-292)

⏰ 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). (1)

• GitHub Check: Build All Frameworks

🔇 Additional comments (36)

src/AiDotNet.csproj (5)

70-73: ✓ Past issue resolved: System.Net.Http now correctly scoped to net471 only.

The fix properly addresses the previous concern by making the System.Net.Http 4.3.4 reference conditional—it now only applies to the net471 target, leaving net8.0 unaffected (where HttpClient is built-in).

---

35-35: Add InternalsVisibleTo for AiDotNet.Serving to enable internal API sharing.

This addition allows the serving project to access internal types from the main AiDotNet project, which is appropriate for an internal architectural boundary.

---

98-101: Exclude LanguageFeaturePolyfills.cs to avoid duplication with AiDotNet.Tensors.

Polyfills are now provided by the Tensors project, so excluding them from the main project prevents duplicate definitions and potential conflicts.

---

103-106: ✓ Cross-platform ProjectReference path corrected and framework compatibility confirmed.

The path has been updated from Windows-style backslash to POSIX-style forward slash (AiDotNet.Tensors/AiDotNet.Tensors.csproj), ensuring cross-platform compatibility. The AiDotNet.Tensors project targets net8.0;net471;net462, which fully supports the main project's target frameworks (net8.0;net471), confirming compatible framework configurations for the project reference.

---

108-130: All referenced global using namespaces exist in the codebase with no conflicts detected.

All seven namespaces referenced in the global usings are properly defined: AiDotNet.Tensors.LinearAlgebra, AiDotNet.Tensors.Engines, AiDotNet.Tensors.Interfaces, AiDotNet.Tensors.NumericOperations, AiDotNet.Tensors.Helpers, AiDotNet.Autodiff, and AiDotNet.Helpers each have multiple files implementing their respective functionality. No duplicate namespace definitions or naming conflicts were detected. The global usings are well-justified for these core namespaces.

src/Configuration/InferenceOptimizationConfig.cs (1)

282-303: LGTM!

The enum definitions are clear, well-documented, and provide appropriate options for cache eviction policies and draft model types.

src/AiDotNet.Serving/Batching/BatchingStrategyBase.cs (1)

1-159: LGTM - well-structured base class with clear abstractions.

The design provides a solid foundation for batching strategies with shared latency tracking, statistics, and helper utilities. Thread-safety is properly implemented, and the abstract interface is clean.

src/AiDotNet.Serving/Configuration/ServingOptions.cs (3)

3-52: Good use of strongly-typed enums.

Replacing string-based configuration with enums (BatchingStrategyType, PaddingStrategyType, NumericType) improves type safety and eliminates magic string comparisons. The documentation for each enum member is helpful.

---

86-97: LGTM - adaptive batching configuration.

The new AdaptiveBatchSize flag and enum-based BatchingStrategy property provide a clean configuration interface that aligns with the CreateBatchingStrategy switch in RequestBatcher.cs.

---

175-193: LGTM - StartupModel configuration.

The StartupModel class correctly uses the new NumericType enum with a sensible default. This integrates well with ModelStartupService which switches on this enum to load typed models.

src/AiDotNet.Serving/Program.cs (1)

35-37: LGTM - correct registration of startup service.

The ModelStartupService is appropriately registered after its dependencies (IModelRepository). As an IHostedService, it will execute StartAsync during application startup, loading configured models from ServingOptions.StartupModels.

src/AiDotNet.Serving/Services/RequestBatcher.cs (4)

95-117: LGTM - enum-based strategy selection.

The switch expression correctly maps BatchingStrategyType enum values to their corresponding strategy implementations. The Continuous case properly passes AdaptiveBatchSize to enable/disable adaptive concurrency.

---

125-131: LGTM - padding strategy selection.

Clean migration from string matching to enum-based selection with appropriate default fallback.

---

267-284: Null checks after TryDequeue are defensive but acceptable.

For ConcurrentQueue<T>.TryDequeue, when it returns true, the out parameter is guaranteed non-null for reference types. However, if PriorityRequestQueue<T>.TryDequeue has different semantics, these checks provide a safety net. The added null guards won't hurt performance meaningfully.

---

3-3: Namespace migration to AiDotNet.Tensors.LinearAlgebra is correctly implemented.

The import aligns with the PR's tensor/linear algebra migration. Vector<T> and Matrix<T> are actively used throughout this file (lines 142, 144, 395, 400, 486, 488) and are compatible with the new namespace. No lingering references to the old AiDotNet.LinearAlgebra namespace exist in the serving project's C# files.

src/AiDotNet.Serving/Controllers/InferenceController.cs (3)

3-3: Namespace migration consistent with other files.

---

260-339: Stub endpoint with thorough validation and informative 501 response.

The GenerateWithSpeculativeDecoding endpoint validates input before returning 501, which provides better error messages for invalid requests. The 501 response body documents the current status and programmatic alternatives clearly.

---

362-493: LGTM - well-documented stub for future LoRA fine-tuning.

The endpoint clearly documents the current limitations and points users to programmatic alternatives. Input validation ensures the API contract is testable.

src/AiDotNet.Serving/Models/PredictionRequest.cs (3)

46-112: LGTM! Well-structured speculative decoding request model.

The request model has sensible defaults and thorough documentation. The parameters align with standard speculative decoding implementations.

---

117-153: LGTM! Response model covers key metrics.

The response includes all essential metrics for evaluating speculative decoding performance (acceptance rate, token counts, processing time).

---

247-294: LGTM! Comprehensive fine-tuning response model.

The response captures all relevant training outcomes including loss history, parameter counts, and timing.

src/AiDotNet.Serving/Controllers/ModelsController.cs (4)

6-7: LGTM! Required imports for typed model support.

The new using directives correctly import types needed for the model loading infrastructure.

---

133-154: LGTM! Clean numeric type dispatching with proper error handling.

The switch expression correctly routes to typed loaders, with Double as the sensible fallback. The inner try-catch provides user-friendly error messages without leaking internal details.

---

275-292: LGTM! Robust string-to-enum parsing.

Handles null/whitespace input gracefully and supports common aliases ("single" for float). Case-insensitive matching is appropriate for API inputs.

---

302-307: The parameterless constructor pattern is correct and intentional. The internal parameterless constructor is documented for deserialization scenarios, and LoadFromFile properly initializes the object by reading bytes from the file and calling Deserialize. This pattern is consistently used throughout the codebase (DeserializeModel, ModelStartupService, etc.).

However, remove the misleading comment about InternalsVisibleTo. The constructor is internal and accessible directly within the same assembly; no InternalsVisibleTo attribute is needed or present in the codebase.

src/AiDotNet.Serving/Services/RequestBatcherBase.cs (4)

83-91: LGTM! Constructor validates dependencies properly.

All dependencies are validated with null checks and ArgumentNullException.

---

107-119: LGTM! Thread-safe statistics retrieval.

Statistics are read under lock, ensuring consistent snapshot of related metrics.

---

156-170: LGTM! Static helpers for TaskCompletionSource operations.

Using TrySetResult and TrySetException is the correct pattern to handle potential race conditions on completion.

---

175-206: LGTM! Standard dispose pattern implementation.

The dispose pattern correctly uses GC.SuppressFinalize and provides extensibility via DisposeManagedResources.

src/AiDotNet.Serving/Services/ContinuousBatchingRequestBatcher.cs (1)

345-375: LGTM! Adaptive concurrency with hysteresis.

The 0.8x and 1.5x thresholds create a deadband that prevents oscillation. The algorithm is sound for gradual concurrency adjustment.

src/AiDotNet.Serving/Batching/ContinuousBatchingStrategy.cs (3)

28-56: LGTM! Clean constructor with sensible defaults.

Starting at half capacity (line 55) is a reasonable warm-up strategy. The configuration parameters cover the essential knobs for continuous batching.

---

96-103: LGTM! Thread-safe optimal batch size calculation.

Correctly acquires SyncLock before reading _currentOptimalConcurrency.

---

110-134: Per-request latency calculation may be skewed for batch sizes of 0.

Line 120 checks batchSize > 0 before dividing, which is correct. However, the semantic meaning of "per-request latency" for a single-item batch equals the total latency, which may not reflect true per-request overhead. This is acceptable for adaptive control but worth noting in comments.

src/AiDotNet.Serving/Services/ModelStartupService.cs (3)

51-59: LGTM: Proper dependency injection and validation.

The constructor correctly validates all injected dependencies and follows ASP.NET Core patterns.

---

113-117: LGTM: Appropriate StopAsync implementation.

No cleanup is needed since loaded models remain in the repository for serving.

---

134-139: > Likely an incorrect or invalid review comment.

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (1)

src/AiDotNet.Serving/Services/ModelStartupService.cs (1)

211-224: Batch prediction addresses previous feedback.

The implementation now passes the entire batch to modelResult.Predict(inputs) instead of row-by-row processing. Note the same single-output limitation as mentioned for ModelsController.LoadTypedModel<T>.

🧹 Nitpick comments (4)

src/AiDotNet.Serving/Services/ContinuousBatchingRequestBatcher.cs (2)

305-331: ProcessTypedRequest signature is misleading.

The method signature returns Task and is awaited (lines 258, 262, 266), but the implementation is entirely synchronous and returns Task.CompletedTask. This can mislead readers into thinking async I/O is happening.

Consider either:

• Removing the async pattern and returning Task.CompletedTask immediately after setting the result inline
• Or documenting that this method is synchronous despite the Task return type

Example refactor:

-private Task ProcessTypedRequest<T>(ContinuousRequest request, CancellationToken cancellationToken)
+private void ProcessTypedRequestSync<T>(ContinuousRequest request)
 {
     var model = ModelRepository.GetModel<T>(request.ModelName);
     if (model == null)
     {
         SetRequestException(request, new InvalidOperationException(
             $"Model '{request.ModelName}' not found or wrong numeric type"));
-        return Task.CompletedTask;
+        return;
     }

     try
     {
         var input = (Vector<T>)request.Input;
         var result = model.Predict(input);

         if (request.CompletionSource is TaskCompletionSource<Vector<T>> tcs)
         {
             tcs.TrySetResult(result);
         }
     }
     catch (Exception ex)
     {
         SetRequestException(request, ex);
     }
-
-    return Task.CompletedTask;
 }

Then update the calls in ProcessRequestAsync to remove await.

---

385-405: Consider implementing IAsyncDisposable for non-blocking disposal.

The current implementation uses Task.WhenAny(...).GetAwaiter().GetResult(), which blocks synchronously during disposal. While the use of ConfigureAwait(false) in ProcessingLoop mitigates deadlock risk, a fully async disposal pattern would be cleaner and more robust.

A past review comment recommended this same improvement. If the project targets .NET 6+, consider implementing IAsyncDisposable with DisposeAsync() to avoid any synchronous blocking:

-public class ContinuousBatchingRequestBatcher : RequestBatcherBase
+public class ContinuousBatchingRequestBatcher : RequestBatcherBase, IAsyncDisposable
 {
+    public async ValueTask DisposeAsync()
+    {
+        if (!Disposed)
+        {
+            _cts.Cancel();
+            
+            try
+            {
+                await _processingLoop.WaitAsync(TimeSpan.FromSeconds(5));
+            }
+            catch (TimeoutException)
+            {
+                Logger.LogWarning("Processing loop did not complete within timeout during disposal");
+            }
+            catch (OperationCanceledException)
+            {
+                // Expected
+            }
+            
+            _cts.Dispose();
+            
+            // Fail remaining requests
+            while (_requestQueue.TryDequeue(out var request))
+            {
+                SetRequestException(request, new OperationCanceledException("Batcher is shutting down"));
+            }
+            
+            foreach (var request in _runningRequests.Values)
+            {
+                SetRequestException(request, new OperationCanceledException("Batcher is shutting down"));
+            }
+            
+            Disposed = true;
+        }
+        
+        GC.SuppressFinalize(this);
+    }
 }

src/AiDotNet.Serving/Controllers/ModelsController.cs (2)

333-346: Batch function only populates first output column.

The predictBatchFunc assigns predictions only to column 0 (results[i, 0] = predictions[i]). If outputDim > 1 (multi-output models), remaining columns will contain default values. This may be intentional for regression models returning one value per sample, but consider adding a comment clarifying this limitation or handling multi-output cases.

---

302-376: Consider extracting shared model loading logic.

This LoadTypedModel<T> implementation is nearly identical to the one in ModelStartupService.cs. Consider extracting the common logic into a shared service or helper class to reduce duplication and ensure consistent behavior across both code paths.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5004768 and 40510d4.

📒 Files selected for processing (4)

• src/AiDotNet.Serving/Controllers/ModelsController.cs (3 hunks)
• src/AiDotNet.Serving/Services/ContinuousBatchingRequestBatcher.cs (1 hunks)
• src/AiDotNet.Serving/Services/ModelStartupService.cs (1 hunks)
• src/Configuration/InferenceOptimizationConfig.cs (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (3)

src/AiDotNet.Serving/Controllers/ModelsController.cs (7)

src/AiDotNet.Serving/Models/ModelInfo.cs (2)

• ModelInfo (6-37)
• LoadModelResponse (65-81)

src/AiDotNet.Serving/Services/IModelRepository.cs (2)

• ModelInfo (46-46)
• LoadModel (18-18)

src/AiDotNet.Serving/Services/ModelRepository.cs (3)

• ModelInfo (96-104)
• ModelInfo (119-136)
• LoadModel (25-46)

src/AiDotNet.Serving/Controllers/InferenceController.cs (2)

• T (201-204)
• Vector (175-183)

src/LoRA/Adapters/LoRAXSAdapter.cs (2)

• Matrix (808-831)
• Vector (570-573)

src/AiDotNet.Serving/Models/ServableModelWrapper.cs (5)

• Matrix (109-137)
• Vector (96-106)
• ServableModelWrapper (11-138)
• ServableModelWrapper (27-39)
• ServableModelWrapper (47-84)

src/Models/Results/PredictionModelResult.cs (1)

• LoadFromFile (1367-1381)

src/AiDotNet.Serving/Services/ContinuousBatchingRequestBatcher.cs (3)

src/AiDotNet.Serving/Services/RequestBatcherBase.cs (11)

• RequestBatcherBase (30-206)
• RequestBatcherBase (83-91)
• Task (101-101)
• SetException (167-170)
• IncrementRequestCount (145-148)
• Dictionary (107-119)
• Dictionary (125-125)
• RecordBatch (132-140)
• DisposeManagedResources (202-205)
• Dispose (175-179)
• Dispose (185-197)

src/AiDotNet.Serving/Configuration/ServingOptions.cs (1)

• ServingOptions (58-170)

src/AiDotNet.Serving/Services/ModelRepository.cs (1)

• ModelRepository (10-148)

src/AiDotNet.Serving/Services/ModelStartupService.cs (3)

src/AiDotNet.Serving/Configuration/ServingOptions.cs (2)

• ServingOptions (58-170)
• StartupModel (175-193)

src/AiDotNet.Serving/Services/RequestBatcherBase.cs (1)

• Task (101-101)

src/Models/Results/PredictionModelResult.cs (1)

• LoadFromFile (1367-1381)

⏰ 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). (1)

• GitHub Check: Build All Frameworks

🔇 Additional comments (10)

src/AiDotNet.Serving/Services/ContinuousBatchingRequestBatcher.cs (1)

46-435: Well-structured continuous batching implementation.

The implementation correctly handles:

• Concurrent request queuing with backpressure
• Continuous processing with configurable concurrency limits
• Adaptive concurrency based on latency feedback
• Proper thread-safety with concurrent collections
• Comprehensive metrics and statistics
• Graceful shutdown with request cancellation

The design aligns well with continuous batching principles and should deliver the expected throughput and latency benefits.

src/AiDotNet.Serving/Controllers/ModelsController.cs (3)

6-7: LGTM!

The new using directives correctly import the required types for typed model loading.

---

133-163: LGTM!

The typed model loading implementation correctly dispatches to the appropriate generic method based on the requested numeric type. The error handling appropriately wraps loading exceptions and returns structured error responses.

---

275-292: LGTM!

The ParseNumericType method cleanly handles string parsing with case-insensitive matching and appropriate default behavior.

src/AiDotNet.Serving/Services/ModelStartupService.cs (2)

39-59: LGTM!

The class structure and constructor correctly implement the hosted service pattern with proper dependency injection and null validation.

---

65-107: LGTM!

The StartAsync implementation correctly handles cancellation, aggregates errors without failing fast, and provides clear logging of loading outcomes.

src/Configuration/InferenceOptimizationConfig.cs (4)

32-68: LGTM!

The class structure with static presets provides a clean API. The Default and HighPerformance presets offer sensible configurations with clear documentation of what each enables.

---

69-198: LGTM!

The KV cache and batching settings have sensible defaults and comprehensive documentation explaining the purpose and trade-offs of each option.

---

217-254: Validation logic addresses previous feedback.

The Validate() method comprehensively checks all numeric constraints. The exception messages helpfully include the invalid values for easier debugging.

---

258-361: LGTM!

The speculative decoding settings and supporting enums are well-documented with clear explanations of trade-offs. Defaulting speculative decoding to disabled is appropriate since it requires specific model support.

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (3)

src/AiDotNet.Serving/Services/ModelStartupService.cs (1)

65-107: LGTM - Resilient startup loading.

The error handling correctly isolates individual model loading failures so one bad configuration doesn't block the entire application. The logging provides good visibility into what succeeded and failed.

Consider whether StartupModels failures should optionally throw (e.g., via a FailOnStartupModelError option) for environments where partial model availability is unacceptable.

src/AiDotNet.Serving/Services/ContinuousBatchingRequestBatcher.cs (2)

251-333: Consider making ProcessTypedRequest synchronous for clarity.

ProcessTypedRequest is declared as async Task but always returns Task.CompletedTask, making it effectively synchronous. The await calls on lines 260, 264, and 268 complete immediately. This design blocks thread pool threads during model.Predict(input) (line 320), which is acceptable for CPU-bound predictions but the async signature may be misleading.

Apply this diff to make the synchronous nature explicit:

-    private Task ProcessTypedRequest<T>(ContinuousRequest request, CancellationToken cancellationToken)
+    private void ProcessTypedRequest<T>(ContinuousRequest request, CancellationToken cancellationToken)
     {
         var model = ModelRepository.GetModel<T>(request.ModelName);
         if (model == null)
         {
             SetRequestException(request, new InvalidOperationException(
                 $"Model '{request.ModelName}' not found or wrong numeric type"));
-            return Task.CompletedTask;
+            return;
         }

         try
         {
             var input = (Vector<T>)request.Input;
             var result = model.Predict(input);

             if (request.CompletionSource is TaskCompletionSource<Vector<T>> tcs)
             {
                 tcs.TrySetResult(result);
             }
         }
         catch (Exception ex)
         {
             SetRequestException(request, ex);
         }
-
-        return Task.CompletedTask;
     }

And update the call sites:

             // Process based on numeric type
             if (request.NumericType == "Double")
             {
-                await ProcessTypedRequest<double>(request, cancellationToken);
+                ProcessTypedRequest<double>(request, cancellationToken);
             }
             else if (request.NumericType == "Single")
             {
-                await ProcessTypedRequest<float>(request, cancellationToken);
+                ProcessTypedRequest<float>(request, cancellationToken);
             }
             else if (request.NumericType == "Decimal")
             {
-                await ProcessTypedRequest<decimal>(request, cancellationToken);
+                ProcessTypedRequest<decimal>(request, cancellationToken);
             }

Alternatively, if you plan to make Predict async in the future, document why the method signature is async.

---

338-343: Consider alternatives to reflection for performance.

SetRequestException uses reflection to invoke TrySetException, which incurs runtime overhead on every request completion. While the impact is likely small for typical workloads, consider using a typed base interface or helper pattern if this becomes a performance bottleneck in high-throughput scenarios.

For example, you could introduce a non-generic wrapper:

private interface ICompletionSource
{
    void TrySetException(Exception exception);
}

private class TypedCompletionSource<T> : ICompletionSource
{
    private readonly TaskCompletionSource<Vector<T>> _tcs;
    
    public TypedCompletionSource(TaskCompletionSource<Vector<T>> tcs) => _tcs = tcs;
    
    public void TrySetException(Exception exception) => _tcs.TrySetException(exception);
    
    public void TrySetResult(Vector<T> result) => _tcs.TrySetResult(result);
}

Then store ICompletionSource instead of object in ContinuousRequest.CompletionSource. This eliminates reflection at the cost of slightly more complex wrapping.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 40510d4 and a0ca73f.

📒 Files selected for processing (2)

• src/AiDotNet.Serving/Services/ContinuousBatchingRequestBatcher.cs (1 hunks)
• src/AiDotNet.Serving/Services/ModelStartupService.cs (1 hunks)

⏰ 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). (1)

• GitHub Check: Build All Frameworks

🔇 Additional comments (12)

src/AiDotNet.Serving/Services/ModelStartupService.cs (4)

51-59: LGTM!

Constructor properly validates all dependencies with ArgumentNullException and correctly unwraps IOptions<ServingOptions>.

---

113-117: LGTM!

Appropriate implementation - model lifecycle management is correctly delegated to IModelRepository.

---

141-155: Path traversal protection correctly implemented.

This addresses the previous review concern. The implementation properly:

• Canonicalizes both the model directory and resolved path
• Ensures the root path ends with a separator to prevent prefix matching attacks (e.g., /models vs /models-backup)
• Uses StartsWith comparison on canonicalized paths

---

243-259: LGTM!

The ServableModelWrapper creation and repository registration with duplicate-name conflict handling is well implemented.

src/AiDotNet.Serving/Services/ContinuousBatchingRequestBatcher.cs (8)

48-78: LGTM!

The field declarations and properties are well-structured. The use of concurrent collections for the queue and running requests is appropriate, and the _latencyHistory queue is correctly protected by _concurrencyLock.

---

123-150: LGTM!

The backpressure handling is well-implemented, and the documentation clearly explains that priority is captured for metadata but not used for scheduling in continuous batching mode. This addresses the concerns from previous reviews.

---

196-218: LGTM!

The processing loop correctly handles cancellation and exceptions, with proper use of ConfigureAwait(false) throughout and appropriate error backoff.

---

223-246: LGTM!

The scheduling logic correctly fills available slots up to the concurrency limit. The fire-and-forget pattern on line 244 is appropriate here since ProcessRequestAsync has comprehensive exception handling and the design intentionally allows tasks to run independently.

---

348-378: LGTM!

The adaptive concurrency algorithm is well-implemented with appropriate locking, history tracking, and threshold-based adjustments. The logic correctly increases concurrency when latency is low and decreases it when latency exceeds the target.

---

383-422: LGTM - Deadlock mitigation implemented.

The disposal logic uses Task.WhenAny with a timeout (lines 391-392) to avoid indefinite blocking, which effectively addresses the deadlock concern raised in previous reviews. While GetAwaiter().GetResult() still blocks synchronously, the timeout ensures the disposal completes within 5 seconds even if the processing loop is unresponsive. This is a reasonable pattern given the IDisposable constraint.

The cleanup of queued and running requests (lines 411-419) ensures graceful shutdown with appropriate error signaling.

---

427-437: LGTM!

The ContinuousRequest class is a well-structured internal DTO for request metadata. The use of object for Input and CompletionSource appropriately handles multiple generic types, and all necessary metadata is captured.

---

196-218: Add ConfigureAwait(false) for consistency.

Line 206 correctly uses .ConfigureAwait(false), but line 215's error backoff delay does not. While this code runs via Task.Run() (which has no synchronization context), adding .ConfigureAwait(false) maintains consistency and follows best practices.

Apply this diff:

-                await Task.Delay(100, cancellationToken).ConfigureAwait(false); // Back off on errors
+                await Task.Delay(100, cancellationToken).ConfigureAwait(false); // Back off on errors

Actually, I see line 215 already has ConfigureAwait(false) in the code. Let me re-check...

Looking at line 215 in the provided code:

await Task.Delay(100, cancellationToken).ConfigureAwait(false); // Back off on errors

This already includes .ConfigureAwait(false). No change needed.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (2)

src/AiDotNet.Serving/Services/ContinuousBatchingRequestBatcher.cs (2)

94-96: Validation workaround still present.

The constructor silently corrects invalid BatchingWindowMs values rather than failing fast. A past review comment recommended adding [Range(1, int.MaxValue)] validation to the ServingOptions.BatchingWindowMs property to ensure consistency across all batching strategies and prevent invalid configurations from propagating.

---

394-407: Synchronous blocking in disposal can still deadlock.

Despite using Task.WhenAny, the call to .GetAwaiter().GetResult() at line 397 still blocks synchronously and can deadlock if called from a synchronization context (e.g., ASP.NET request thread).

A past review comment (not marked as addressed) recommended implementing IAsyncDisposable or ensuring the processing loop uses ConfigureAwait(false) for all awaits, then awaiting disposal asynchronously.

🧹 Nitpick comments (2)

src/AiDotNet.Serving/Controllers/ModelsController.cs (1)

294-398: Consider extracting shared loading logic to reduce duplication.

The LoadTypedModel<T> method is nearly identical to the one in ModelStartupService.cs (lines 196-282). Consider extracting this common logic into a shared helper or service to improve maintainability and ensure consistent behavior.

For example, create a shared ModelLoaderService that both the controller and startup service can inject and use:

public class ModelLoaderService
{
    public ServableModelWrapper<T> LoadTypedModel<T>(string name, string path, ILogger logger)
    {
        // Common loading logic here
    }
}

src/AiDotNet.Serving/Services/ContinuousBatchingRequestBatcher.cs (1)

263-274: Consider type-safe routing instead of string comparison.

Using string comparisons for type routing (request.NumericType == "Double") is fragile and case-sensitive. If the type name representation changes or namespacing affects typeof(T).Name, this could fail silently at runtime.

Option 1: Store the Type object instead of the type name string in ContinuousRequest:

 private class ContinuousRequest
 {
     public long RequestId { get; set; }
     public string ModelName { get; set; } = string.Empty;
-    public string NumericType { get; set; } = string.Empty;
+    public Type NumericType { get; set; } = null!;
     public object Input { get; set; } = null!;
     public object CompletionSource { get; set; } = null!;
     public RequestPriority Priority { get; set; } = RequestPriority.Normal;
     public DateTime EnqueueTime { get; set; }
 }

Then update QueueRequest to store the type:

 var request = new ContinuousRequest
 {
     RequestId = Interlocked.Increment(ref _requestIdCounter),
     ModelName = modelName,
-    NumericType = typeof(T).Name,
+    NumericType = typeof(T),
     Input = input,
     CompletionSource = tcs,
     Priority = priority,
     EnqueueTime = DateTime.UtcNow
 };

And update ProcessRequestAsync to use type comparison:

-if (request.NumericType == "Double")
+if (request.NumericType == typeof(double))
 {
     await ProcessTypedRequest<double>(request, cancellationToken);
 }
-else if (request.NumericType == "Single")
+else if (request.NumericType == typeof(float))
 {
     await ProcessTypedRequest<float>(request, cancellationToken);
 }
-else if (request.NumericType == "Decimal")
+else if (request.NumericType == typeof(decimal))
 {
     await ProcessTypedRequest<decimal>(request, cancellationToken);
 }
 else
 {
-    SetRequestException(request, new NotSupportedException($"Numeric type '{request.NumericType}' is not supported"));
+    SetRequestException(request, new NotSupportedException($"Numeric type '{request.NumericType.Name}' is not supported"));
 }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a0ca73f and 359d0a3.

📒 Files selected for processing (3)

• src/AiDotNet.Serving/Controllers/ModelsController.cs (3 hunks)
• src/AiDotNet.Serving/Services/ContinuousBatchingRequestBatcher.cs (1 hunks)
• src/AiDotNet.Serving/Services/ModelStartupService.cs (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

src/AiDotNet.Serving/Controllers/ModelsController.cs (5)

src/AiDotNet.Serving/Models/ModelInfo.cs (2)

• ModelInfo (6-37)
• LoadModelResponse (65-81)

src/AiDotNet.Serving/Services/ModelRepository.cs (3)

• ModelInfo (96-104)
• ModelInfo (119-136)
• LoadModel (25-46)

src/AiDotNet.Serving/Services/ModelStartupService.cs (1)

• LoadTypedModel (196-282)

src/AiDotNet.Serving/Controllers/InferenceController.cs (2)

• T (201-204)
• Vector (175-183)

src/AiDotNet.Serving/Models/ServableModelWrapper.cs (5)

• Matrix (109-137)
• Vector (96-106)
• ServableModelWrapper (11-138)
• ServableModelWrapper (27-39)
• ServableModelWrapper (47-84)

src/AiDotNet.Serving/Services/ModelStartupService.cs (2)

src/AiDotNet.Serving/Configuration/ServingOptions.cs (2)

• ServingOptions (58-170)
• StartupModel (175-193)

src/Models/Results/PredictionModelResult.cs (1)

• LoadFromFile (1367-1381)

⏰ 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). (1)

• GitHub Check: Build All Frameworks

🔇 Additional comments (13)

src/AiDotNet.Serving/Controllers/ModelsController.cs (5)

133-154: LGTM! Clean type-safe loading dispatch.

The numeric type parsing and switch-based dispatch to typed loading methods is well-structured. Error handling properly logs context and returns appropriate HTTP responses.

---

275-292: LGTM! Sensible numeric type parsing.

The case-insensitive string-to-enum mapping with a reasonable default (Double) is appropriate.

---

313-328: Correctly handles varied numeric types from deserialization.

The use of Convert.ToInt32 with try-catch appropriately addresses the previous concern about type pattern matching failing for JSON-deserialized numeric values (which may be long, double, or JsonElement).

---

330-338: Multi-output limitation is now explicitly enforced.

The check correctly ensures outputDim = 1 and logs a clear warning when multi-output models are detected. This addresses the previous concern about incomplete multi-output handling.

---

355-368: Batch prediction correctly delegates to model's native batch capability.

The code appropriately calls modelResult.Predict(inputs) once with the entire batch, which was the improvement requested in the previous review. The defensive loop condition (i < predictions.Length && i < inputs.Rows) adds robustness in case of prediction/input count mismatches.

src/AiDotNet.Serving/Services/ModelStartupService.cs (5)

141-155: Path traversal protection properly implemented.

The code correctly validates that the resolved model path stays within the configured ModelDirectory by:

1. Canonicalizing both the models root and the target path
2. Ensuring the root ends with a directory separator to prevent prefix attacks
3. Rejecting paths that resolve outside the allowed directory

This addresses the previous review concern about missing path traversal protection.

---

210-222: Correctly handles varied numeric types from metadata.

The Convert.ToInt32 approach with try-catch properly handles various numeric types that may result from JSON deserialization (e.g., long, double, JsonElement), addressing the previous type casting concern.

---

226-232: Multi-output models are explicitly handled.

The code enforces the single-output constraint (outputDim = 1) and logs an appropriate warning when multi-output models are detected, addressing the previous concern about incomplete multi-output support.

---

249-262: Batch prediction efficiently uses single model call.

The batch prediction function correctly delegates to modelResult.Predict(inputs) with the entire batch, which addresses the previous review comment about row-by-row processing overhead.

---

168-183: LGTM! Appropriate async handling for file I/O.

Using Task.Run to offload the synchronous file loading from the startup thread is appropriate. The numeric type dispatch is clean and comprehensive.

src/AiDotNet.Serving/Services/ContinuousBatchingRequestBatcher.cs (3)

128-155: LGTM!

The method correctly implements backpressure checking, uses TaskCreationOptions.RunContinuationsAsynchronously to prevent synchronization context issues, and clearly documents that priority is stored for metadata but not used for FIFO scheduling.

---

201-223: LGTM!

The processing loop correctly handles cancellation, uses ConfigureAwait(false) to avoid synchronization context capture, and implements appropriate backoff on errors.

---

353-383: LGTM!

The adaptive concurrency algorithm is well-designed with appropriate locking, bounded history tracking, and sensible threshold values for scaling decisions.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

src/AiDotNet.Serving/Services/ContinuousBatchingRequestBatcher.cs (3)

143-149: Use TrySetException for consistency and safety.

SetException throws if the task is already completed. While unlikely here, using TrySetException aligns with the pattern used elsewhere in this class and is more defensive.

         if (Options.MaxQueueSize > 0 && _requestQueue.Count >= Options.MaxQueueSize)
         {
-            tcs.SetException(new InvalidOperationException("Request queue is full. Please try again later."));
+            tcs.TrySetException(new InvalidOperationException("Request queue is full. Please try again later."));
             Logger.LogWarning("Request rejected due to backpressure. Queue size: {QueueSize}", _requestQueue.Count);
             return tcs.Task;
         }

---

312-344: Consider checking cancellationToken before processing.

The cancellationToken parameter is accepted but never used. At minimum, check for cancellation before invoking the model to avoid starting work that's already cancelled.

 private Task ProcessTypedRequest<T>(ContinuousRequest request, CancellationToken cancellationToken)
 {
+    if (cancellationToken.IsCancellationRequested)
+    {
+        if (request.CompletionSource is TaskCompletionSource<Vector<T>> cancelledTcs)
+        {
+            cancelledTcs.TrySetCanceled(cancellationToken);
+        }
+        return Task.CompletedTask;
+    }
+
     var model = ModelRepository.GetModel<T>(request.ModelName);

---

350-375: Add logging for unknown NumericType values.

If NumericType doesn't match known types, the exception is silently dropped, making debugging difficult. Add a default case with logging.

             case "Decimal":
                 if (request.CompletionSource is TaskCompletionSource<Vector<decimal>> decimalTcs)
                 {
                     decimalTcs.TrySetException(exception);
                 }
                 break;
+            default:
+                // Log unexpected type - this indicates a bug in type handling
+                Debug.Fail($"Unexpected NumericType: {request.NumericType}");
+                break;
         }
     }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 359d0a3 and e8eb585.

📒 Files selected for processing (1)

• src/AiDotNet.Serving/Services/ContinuousBatchingRequestBatcher.cs (1 hunks)

⏰ 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). (1)

• GitHub Check: Build All Frameworks

🔇 Additional comments (7)

src/AiDotNet.Serving/Services/ContinuousBatchingRequestBatcher.cs (7)

46-67: LGTM!

Well-structured field declarations with appropriate use of concurrent collections for thread-safe access, and standard Queue<double> for latency history protected by _concurrencyLock.

---

91-106: LGTM!

Defensive validation for BatchingWindowMs with sensible defaults addresses the concern about invalid configuration values. Starting concurrency at half capacity is a reasonable strategy for adaptive scaling.

---

198-251: LGTM!

The processing loop has proper cancellation handling, error back-off, and the fire-and-forget pattern at line 249 is appropriate since ProcessRequestAsync handles exceptions internally.

---

256-307: LGTM!

Good use of finally block for cleanup, metrics recording, and adaptive concurrency updates regardless of success or failure. The stopwatch-based latency tracking is appropriate.

---

377-410: LGTM!

The adaptive concurrency algorithm is well-designed with conservative scaling (±1), reasonable thresholds (80%/150% of target latency), and proper synchronization via _concurrencyLock.

---

415-454: Improved disposal pattern, but consider IAsyncDisposable for full async support.

The Task.WhenAny approach is an improvement over direct Wait(), reducing deadlock risk by ensuring the blocking call is brief. However, .GetAwaiter().GetResult() still blocks synchronously. For ASP.NET Core scenarios with synchronization contexts, implementing IAsyncDisposable would be ideal. This is acceptable for now but worth considering for a follow-up.

---

459-468: LGTM!

The internal ContinuousRequest class appropriately uses type erasure to support multiple numeric types. The null-forgiving operator is acceptable here since instantiation is controlled within QueueRequest.