feat: add YAML configuration system for AiModelBuilder (#283) - #841
Merged
Merged
Conversation
Add optional YAML-based configuration so users can define training recipes in config files instead of writing C# for every setting. YAML sets base defaults; fluent .Configure*() calls override afterwards. New files: - YamlModelConfig: root POCO with 18 config sections - YamlConfigLoader: YAML file/string loading with validation - YamlConfigApplier: maps YAML sections to builder Configure* calls Modified files: - AiModelBuilder: added constructor accepting config file path - OptimizerFactory: added parameterless CreateOptimizer using reflection to create optimizers without requiring a model upfront - AiDotNet.csproj: added YamlDotNet 16.3.0 dependency 43 integration tests covering deserialization, error handling, builder integration, optimizer factory, and full recipe end-to-end scenarios. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds YAML-driven configuration support (YamlDotNet dependency), YAML POCOs, loader and applier bridging YAML → AiModelBuilder, new training surface (Trainer, training POCOs, factories), CSV data loader, RAG lazy-loading changes, multiple NN/JIT tweaks, many tests and an example YAML. All changes are additive. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant AiBuilder as AiModelBuilder
participant YamlLoader as YamlConfigLoader
participant YamlApplier as YamlConfigApplier
participant Factory as Factories
User->>AiBuilder: new AiModelBuilder(configPath)
AiBuilder->>YamlLoader: LoadFromFile(configPath)
YamlLoader-->>AiBuilder: YamlModelConfig
AiBuilder->>YamlApplier: YamlConfigApplier.Apply(config, builder)
YamlApplier->>Factory: CreateModel/CreateOptimizer/CreateDataset/CreateLoss(...)
Factory-->>YamlApplier: instances/options
YamlApplier->>AiBuilder: builder.Configure*(...) calls
AiBuilder-->>User: configured builder/trainer ready
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an optional YAML-based configuration path for AiModelBuilder to allow defining “training recipes” in version-controlled YAML, then applying them as base defaults before fluent overrides.
Changes:
- Introduces YAML POCO root model (
YamlModelConfig) plus loader (YamlConfigLoader) and builder-mapper (YamlConfigApplier). - Adds
AiModelBuilder(string configFilePath)constructor to load/apply YAML defaults, plus an explicit parameterless constructor. - Updates
OptimizerFactoryto support a parameterlessCreateOptimizer(OptimizerType)via reflection and fixes generic type handling in the options overload; adds integration tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/AiDotNet.csproj |
Adds YamlDotNet dependency needed for YAML parsing. |
src/Configuration/YamlModelConfig.cs |
Defines the root YAML-deserializable config shape (optimizer/time-series selectors + POCO config sections). |
src/Configuration/YamlConfigLoader.cs |
Loads YAML from file/string into YamlModelConfig with camelCase mapping + unknown-property tolerance. |
src/Configuration/YamlConfigApplier.cs |
Applies YAML sections onto AiModelBuilder via corresponding Configure* methods and enum parsing. |
src/AiModelBuilder.cs |
Adds YAML constructor that loads/applies config before any fluent overrides. |
src/Factories/OptimizerFactory.cs |
Adds parameterless optimizer creation and corrects generic type instantiation behavior. |
tests/AiDotNet.Tests/IntegrationTests/Configuration/YamlConfigTests.cs |
Integration tests covering deserialization, error handling, applier behavior, and optimizer factory creation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/AiModelBuilder.cs`:
- Around line 286-295: The AiModelBuilder constructor currently calls
YamlConfigLoader.LoadFromFile without verifying the config path; update the
constructor to normalize the path (use Path.GetFullPath or equivalent) and check
existence with File.Exists before loading, and if the file is missing throw a
FileNotFoundException referencing the normalized path; then pass the normalized
path to YamlConfigLoader.LoadFromFile and continue to call YamlConfigApplier<T,
TInput, TOutput>.Apply(config, this) as before.
In `@tests/AiDotNet.Tests/IntegrationTests/Configuration/YamlConfigTests.cs`:
- Around line 630-657: The tests
Apply_WithAdamOptimizer_CreatesOptimizerOnBuilder and
Apply_WithGradientDescentOptimizer_CreatesOptimizerOnBuilder only verify Apply
doesn't throw; update them to assert the optimizer was actually configured on
the builder by (a) adding/using a test-only accessor on AiModelBuilder like
GetConfiguredOptimizerType() or GetOptimizer() and assert it equals
"Adam"/"GradientDescent", or (b) perform a minimal end-to-end action with the
builder that behaves differently depending on optimizer type and assert the
expected behavior; call YamlConfigApplier<double, Matrix<double>,
Vector<double>>.Apply(config, builder) then assert the configured optimizer via
the chosen getter or e2e check.
- Around line 312-335: The test Constructor_WithYamlFile_AppliesConfiguration
only asserts the AiModelBuilder<double, Matrix<double>,
Vector<double>>(tempFile) is not null but doesn't verify the YAML settings were
applied; update the test to either (A) assert the actual configuration values
after construction by reading builder's exposed properties or methods (e.g.,
builder.Caching.Enabled, builder.Caching.MaxCacheSize,
builder.JitCompilation.Enabled, builder.JitCompilation.ThrowOnFailure) to match
the YAML, or (B) if the builder intentionally doesn't expose state, rename the
test to Constructor_WithYamlFile_DoesNotThrow and add a separate integration
test that exercises behavior impacted by Caching/JitCompilation to verify the
settings are effective.
- Around line 753-788: The test
Constructor_WithFullYamlRecipe_AppliesAllSections currently only asserts the
builder is not null; update it to verify that all YAML sections were applied by
asserting builder state or calls: either (a) expose read-only properties on
AiModelBuilder (e.g., Optimizer, CachingConfig, JitCompilationEnabled,
InferenceOptimizations, InterpretabilityConfig, MemoryManagement) and assert
their expected values after constructing with the temp YAML, or (b) replace
AiModelBuilder with a test double/spy that records
ConfigureOptimizer/ConfigureCaching/ConfigureJitCompilation/ConfigureInferenceOptimizations/ConfigureInterpretability/ConfigureMemoryManagement
calls and assert each was invoked, or (c) perform a minimal behavior check
(e.g., a quick build/train step) that would differ if those sections were not
applied; pick one approach and add assertions in the test to validate each of
the six sections.
- Around line 337-363: The test Constructor_WithYamlFile_FluentOverridesWork
currently only asserts builder != null and must verify that fluent
ConfigureCaching overrides the YAML values; update the test to assert the
effective caching settings are the fluent ones by inspecting the builder's
applied config (use AiModelBuilder<T,U,V>.ConfigureCaching/CacheConfig
properties or whatever exposed accessor exists—e.g., an AppliedCacheConfig,
GetCacheConfig(), or after Build() inspect the model's CacheConfig) and assert
MaxCacheSize == 100 and Enabled == false; if no accessor exists, add a minimal
read-only accessor on AiModelBuilder (e.g., AppliedCacheConfig or
GetCacheConfig) so the test can check the final values.
- Fix XML docs: "loads and validates" -> "loads and deserializes" in YamlConfigLoader - Add path normalization and existence check in AiModelBuilder YAML constructor - Add internal config accessors on AiModelBuilder for test verification - Add meaningful assertions to all tests that previously only checked NotNull - Verify YAML values, fluent overrides, optimizer creation, and full recipe sections are actually applied to the builder Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
75 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/Configuration/YamlConfigLoader.cs`:
- Line 21: The YamlConfigLoader class is a plumbing/helper type and should not
be part of the public API; change its declaration from public to internal (i.e.,
make the class internal instead of public) unless YamlConfigLoader is referenced
from any public constructors or public method signatures—if it is referenced
publicly, remove or refactor those public usages to keep the class internal.
Locate the class declaration named YamlConfigLoader and update its accessibility
accordingly, and run a quick compile/check for any API-exposed references to
resolve them.
- Around line 59-66: Remove the .IgnoreUnmatchedProperties() call from the
DeserializerBuilder chain so unknown YAML keys cause an exception, then wrap the
call to deserializer.Deserialize<YamlModelConfig>(yamlContent) in a try-catch
that catches Exception (or YamlDotNet-specific exceptions), logs/throws a new
exception with clear context ("Failed to deserialize YAML into YamlModelConfig"
plus the original exception) and do not silently return a default; if
deserialization returns null, throw a descriptive exception instead of returning
new YamlModelConfig() so misconfiguration fails fast.
In `@tests/AiDotNet.Tests/IntegrationTests/Configuration/YamlConfigTests.cs`:
- Around line 475-488: The test Apply_WithValidTimeSeriesModel_DoesNotThrow
currently only ensures no exception; update it to assert the time-series model
was applied by inspecting the AiModelBuilder instance after calling
YamlConfigApplier<double, Matrix<double>, Vector<double>>.Apply(config,
builder). Specifically, after Apply returns, query the builder (AiModelBuilder)
for the configured time-series model settings (e.g., a property or method that
exposes the model type or configured model instance) and assert that the model
type equals "ARIMA" (or that the builder's time-series model instance is
non-null and of the expected ARIMA type). Ensure the assertion uses the same
test method name Apply_WithValidTimeSeriesModel_DoesNotThrow and fails if the
builder was not configured.
- Around line 529-536: The test
OptimizerFactory_CreateOptimizer_Adam_CreatesInstance only asserts non-null;
change it to verify the optimizer is an Adam instance by constructing via
OptimizerFactory<double, Matrix<double>,
Vector<double>>.CreateOptimizer(OptimizerType.Adam) and assert the returned
object's concrete type or distinguishing properties (e.g., type name, an IsAdam
property, or specific learning-rate/defaults) match the Adam optimizer
implementation; update the Assert to use Assert.IsType<TAdam>(...) or
Assert.Equal(expectedProperty, actualProperty) against the optimizer instance
accordingly.
#283) Add the remaining 85% of the YAML training recipe system: - LossType enum (37 values matching all loss function classes) - TrainingRecipeConfig POCOs (model, dataset, optimizer, loss, trainer) - LossFunctionFactory with parameterized creation for all 33 factory-creatable losses - ModelFactory delegating to TimeSeriesModelFactory with reflection-based param mapping - DatasetFactory and CsvDataLoader for CSV-based supervised learning data - ITrainer interface and Trainer class with full training loop - TrainingResult with epoch losses, duration, and completion tracking - Generic YamlConfigLoader.LoadFromFile<T>/LoadFromString<T> overloads - Example YAML config at examples/configs/simple-training.yaml - 5 test files covering factories, config deserialization, and end-to-end training Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ptimizer config (#283) - Trainer now creates optimizer from config via OptimizerFactory and calls SetModel() - Added Optimizer property and early stopping check in training loop - Added seed support for reproducible training runs - Fixed null-forgiving operator usage in CsvDataLoader (replaced with proper null checks) - Added Params dictionary to OptimizerConfig for consistency with ModelConfig/LossFunctionConfig - Added 7 new tests: optimizer wiring, invalid optimizer name, seed reproducibility, default loss function Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add 39 comprehensive integration tests for the training recipe system covering model factories, loss functions, optimizers, CSV loading, YAML end-to-end pipelines, and learning rate propagation. Fix 8 pre-existing test failures: - ParameterAnalyzer: handle null/empty groups gracefully - GraphAttentionLayer: fix bias tensor shape mismatch in forward pass - LoRALayer: use ArgumentException for rank validation - ONNXSentenceTransformer: lazy model loading with fallback embeddings - SentenceTransformersFineTuner: defer ONNX model load to first use Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…embeddings - Revert LoRALayer to throw ArgumentOutOfRangeException (not ArgumentException) for rank validation, fix matching assertion in LoRAValidationTests - Fix LoRA training workflow test: use small dimensions and gradient clipping to prevent NaN/Infinity from non-deterministic DenseLayer weight init - BatchNormalizationLayer: gracefully handle backward/update calls when forward hasn't been called yet (return gradient unchanged, skip parameter update) instead of throwing, fixing intermittent DPCTGAN test failures - ONNXSentenceTransformer: use case-insensitive hash for fallback embeddings to match expected behavior of real sentence transformer models Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tion
- IRGraph.Validate() no longer modifies TensorShapes as a side effect,
making it a true read-only validation method
- TensorJsonConverter now rejects empty shape arrays with clear error
message ("must have at least one dimension")
- Update multi-output JitCompiler test to pre-populate shapes instead
of relying on Validate() side effects
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/RetrievalAugmentedGeneration/Embeddings/SentenceTransformersFineTuner.cs (2)
197-197:⚠️ Potential issue | 🔴 CriticalBLOCKING: Console.WriteLine is not production-ready logging.
Per coding guidelines: "Non-production patterns: Using
Console.WriteLinefor logging instead of proper logging abstractions" must be flagged as blocking.Use a proper logging abstraction (e.g.,
ILogger<T>from Microsoft.Extensions.Logging) that can be configured, filtered, and integrated with observability tooling.🔧 Proposed fix: Inject ILogger or use a logging abstraction
+using Microsoft.Extensions.Logging; + public class SentenceTransformersFineTuner<T> : EmbeddingModelBase<T> { + private readonly ILogger<SentenceTransformersFineTuner<T>>? _logger; + public SentenceTransformersFineTuner( string baseModelPath, string outputModelPath, int epochs, T learningRate, - int dimension) + int dimension, + ILogger<SentenceTransformersFineTuner<T>>? logger = null) { + _logger = logger; // ... existing constructor body }Then replace:
-Console.WriteLine($"Fine-tuning model on {pairs.Count} training pairs for {_epochs} epochs..."); +_logger?.LogInformation("Fine-tuning model on {Count} training pairs for {Epochs} epochs", pairs.Count, _epochs);Also applies to: 244-244
179-244:⚠️ Potential issue | 🔴 CriticalBLOCKING: FineTune method is a simulation/stub, not a production implementation.
The comments explicitly state: "Simulate fine-tuning process" and "In production, this would use actual neural network training...". The method creates an "adjustment layer" that doesn't actually modify model weights or produce a fine-tuned ONNX model.
Per coding guidelines: "Stubs/Placeholders" and "Simplified implementations: Code that takes shortcuts" must be flagged as blocking. The docstring promises fine-tuning but the implementation only caches adjusted embeddings for exact text matches—a fundamentally different behavior.
Production-ready code should either:
- Implement actual fine-tuning with gradient descent (using the optimizers in
src/Optimizers/)- Be clearly marked as
[Obsolete]or namedSimulateFineTuneif this is intentional scaffolding- Throw
NotSupportedExceptionwith a message explaining real fine-tuning isn't yet implementedtests/AiDotNet.Tests/UnitTests/LoRA/LoRAValidationTests.cs (4)
131-143:⚠️ Potential issue | 🟠 MajorBLOCKING: Trivial assertion—test should verify actual behavior.
This test only asserts
Assert.NotNull(adapter), which provides minimal value. If the constructor completes without throwing,adapterwill never be null. The test should verify that thepruningIntervalwas actually set correctly.Proposed fix to strengthen assertions
// Assert - Assert.NotNull(adapter); + Assert.NotNull(adapter); + Assert.Equal(100, adapter.PruningInterval); + Assert.Equal(4, adapter.Rank);As per coding guidelines: "Trivial assertions: Tests that only check
Assert.NotNullwhen they should verify actual behavior/values" must be flagged as blocking issues.
245-260:⚠️ Potential issue | 🟠 MajorBLOCKING: Trivial assertion masks incomplete verification.
The comment says "should not throw" but merely surviving the call is implicit—if it threw, the test would fail regardless of assertions.
Assert.NotNull(merged)adds no meaningful verification. The test should verify the merged layer's dimensions and potentially weights.Proposed fix to add meaningful assertions
// Act & Assert - should not throw var merged = adapter.MergeToOriginalLayer(); Assert.NotNull(merged); + // Verify merged layer has correct dimensions + Assert.Equal(inputSize, merged.InputSize); + Assert.Equal(outputSize, merged.OutputSize);As per coding guidelines: Tests with only
Assert.NotNullwhen they should verify actual behavior/values are blocking issues.
262-292:⚠️ Potential issue | 🟠 MajorBLOCKING: Same trivial assertion pattern in DoRAAdapter and QLoRAAdapter merge tests.
Both
DoRAAdapter_MergeToOriginalLayer_DoesNotThrow(line 275) andQLoRAAdapter_MergeToOriginalLayer_DoesNotThrow(line 291) suffer from the same issue—Assert.NotNull(merged)is a trivial assertion that doesn't verify the merge operation produced correct results.Add assertions to verify:
- Merged layer dimensions match expected
inputSizeandoutputSize- Weight matrices have valid values (non-NaN, non-Infinite)
As per coding guidelines: Trivial assertions that only check
Assert.NotNullare blocking test quality issues.
294-320:⚠️ Potential issue | 🟠 MajorBLOCKING: VeRAAdapter merge test has same trivial assertion issue.
While the test does properly initialize and clean up shared matrices (good!), the actual assertion at line 313 is only
Assert.NotNull(merged). Given VeRA's unique shared matrix architecture, this test should verify:
- Merged layer dimensions are correct
- The shared matrix state is properly utilized during merge
Proposed fix
// Act & Assert - should not throw var merged = adapter.MergeToOriginalLayer(); Assert.NotNull(merged); + Assert.Equal(inputSize, merged.InputSize); + Assert.Equal(outputSize, merged.OutputSize);As per coding guidelines: "Tests that only check
Assert.NotNullwhen they should verify actual behavior/values" must be flagged.
🤖 Fix all issues with AI agents
In `@src/NeuralNetworks/Layers/BatchNormalizationLayer.cs`:
- Around line 989-990: The Update method currently returns silently when
_gammaGradient or _betaGradient is null, but the XML docs promise an
InvalidOperationException if Update is called before Backward; change the
early-return into a fail-fast throw (InvalidOperationException) inside the
Update method, checking _gammaGradient and _betaGradient and throwing with a
clear message like "Update called before Backward: gamma/beta gradients are
null" so callers can detect the misuse immediately (refer to the Update method
and the _gammaGradient/_betaGradient fields to locate the check).
- Around line 766-767: The autodiff backward method currently returns
outputGradient when _lastInput is null, but the XML docs (and BackwardManual)
promise an InvalidOperationException; in the autodiff/backward method (the
method implementing backward propagation for automatic differentiation — e.g.,
Backward/BackwardAutomatic in BatchNormalizationLayer) replace the silent
pass-through branch "if (_lastInput == null) return outputGradient;" with a
fail-fast throw: throw new InvalidOperationException("Forward must be called
before Backward on BatchNormalizationLayer"); ensure the change matches the
behavior and message style used by BackwardManual and adjust unit tests/comments
if any reference the old behavior.
- Around line 635-636: The implementation currently returns outputGradient when
Backward is called without prior Forward, but the XML docs promise an
InvalidOperationException; revert to fail-fast by replacing the silent early
return in BatchNormalizationLayer.Backward (the block that checks _lastInput,
_lastMean, _lastVariance and currently returns outputGradient) with throwing an
InvalidOperationException (with a clear message like "Backward called before
Forward: missing _lastInput/_lastMean/_lastVariance") so runtime behavior
matches the documented contract and surfaces pipeline bugs immediately.
In `@src/NeuralNetworks/Layers/GraphAttentionLayer.cs`:
- Around line 511-524: The triple-nested loop that builds biasExpanded defeats
GPU broadcasting — replace the manual copy with an Engine-backed broadcast:
reshape _bias to [1,1,_outputFeatures] (or [1,_outputFeatures] if consistent
with sparse path) and use Engine.TensorAdd(avgOverHeads, biasReshaped) instead
of creating biasExpanded and copying values; keep references to avgOverHeads,
_bias, biasExpanded (remove), and Engine.TensorAdd when making the change.
In `@src/RetrievalAugmentedGeneration/Embeddings/ONNXSentenceTransformer.cs`:
- Around line 91-96: The code currently returns synthetic embeddings when the
ONNX model file doesn't exist (the if (!File.Exists(_modelPath)) block calling
GenerateFallbackEmbedding), which silently degrades production behavior; remove
this fallback so failures surface, i.e., delete the File.Exists check and the
GenerateFallbackEmbedding method and let EnsureModelLoaded/EmbedCore propagate
the missing-model error; for tests, inject a test double or override EmbedCore
in a test subclass rather than keeping GenerateFallbackEmbedding in production
code.
In `@src/Serialization/TensorJsonConverter.cs`:
- Around line 126-133: The ReadJson change now rejects empty tensor shapes but
WriteJson still emits them, breaking scalar round‑trips; update
TensorJsonConverter.WriteJson to enforce the same rule by validating
tensor.Shape (or Shape.Length) and throwing a JsonSerializationException with a
matching message (e.g., "Tensor 'shape' must have at least one dimension.") when
shape is empty, and update any tests or callers that serialize scalar/legacy
tensors to handle or migrate them; keep all logic in the TensorJsonConverter
class and mirror the same validation used in ReadJson.
In `@src/Training/Factories/ModelFactory.cs`:
- Around line 130-144: The loop in ModelFactory that checks "property" and the
try/catch around ConvertValue silently ignores unknown keys and conversion
errors; update the logic in the method that iterates kvp (and uses property,
ConvertValue, property.SetValue, options) to emit warnings when property is null
or !property.CanWrite and when the catch for
InvalidCastException/FormatException/OverflowException is hit; optionally add a
configurable strict mode flag (e.g., throw on invalid parameter when
strict=true) or collect warnings into a result/Diagnostics object and return it,
but at minimum call the existing logger to log a warning with the parameter name
(kvp.Key) and the exception/message instead of silently continuing.
In `@src/Training/Trainer.cs`:
- Around line 225-229: The Run method is blocking on an async call via
_csvLoader.LoadAsync().GetAwaiter().GetResult(), which risks deadlocks; change
Run to be async Task (or Task<(FeaturesType, LabelsType)>) and await
_csvLoader.LoadAsync() instead, or add a synchronous Load() on CsvDataLoader and
call that from Run; alternatively ensure LoadAsync() uses ConfigureAwait(false)
internally and document the risk—update references to Run and
CsvDataLoader.LoadAsync/Load accordingly to preserve signatures and call sites
(identify Run, _csvLoader, CsvDataLoader, LoadAsync when making the change).
- Around line 159-167: Replace the Console.WriteLine calls in Trainer (the block
referencing _enableLogging, Config.Model?.Name,
_optimizer/Config.Optimizer?.Name, and _lossFunction.GetType().Name) with calls
to a logging abstraction: add an ILogger<Trainer> (or a configurable
Action<string> delegate) as a dependency on the Trainer class (inject via the
constructor or property), store it as a private field (e.g., _logger), and
change the Console.WriteLine lines to use _logger.LogInformation (or invoke the
delegate) while preserving the same message text and the existing _enableLogging
gating; ensure the new dependency is optional or provide a null/NoOp logger so
behavior remains safe for existing callers.
In `@tests/AiDotNet.Tests/IntegrationTests/LoRA/LoRAIntegrationTests.cs`:
- Around line 455-510: The test currently can silently pass when loss becomes
NaN/Infinity because the loop breaks and finalLoss remains default; update the
LoRAIntegrationTests ReducesLoss test to immediately fail when loss is
non‑finite (e.g., assert/throw inside the loop when double.IsNaN(loss) ||
double.IsInfinity(loss)), record initialLoss at epoch 0 and finalLoss at last
completed epoch, and replace the weak "finite" assertions with a meaningful
reduction check (e.g., Assert.True(finalLoss < initialLoss, with a small
tolerance) referencing the variables initialLoss and finalLoss and the training
loop that calls adapter.Forward, adapter.Backward and adapter.UpdateParameters).
In
`@tests/AiDotNet.Tests/IntegrationTests/Training/TrainingRecipeIntegrationTests.cs`:
- Around line 462-489: The test
Trainer_WithVariousOptimizers_AllCreateSuccessfully currently only checks
NotNull on trainer.Optimizer; change it to assert the concrete optimizer type
for each input name by mapping optimizerName to its expected implementation
(e.g., "Adam" => AdamOptimizer, "GradientDescent" => GradientDescentOptimizer,
etc.), instantiate Trainer<double> with TrainingRecipeConfig/OptimizerConfig as
before, then use an assertion that checks the concrete type of trainer.Optimizer
(Assert.IsType or Assert.IsAssignableFrom) against the expected optimizer class
for each case to ensure each name creates the correct optimizer implementation.
- Around line 233-301: The tests for Focal and ElasticNet currently only assert
non-negative outputs so they won't catch ignored parameters; update
LossFunctionFactory_FocalLoss_DifferentGamma_ProduceDifferentLosses to assert
that loss1 and loss5 produced by CalculateLoss are different (e.g.,
Assert.NotEqual(loss1, loss5) or Assert.True(loss1 != loss5)), and update
LossFunctionFactory_ElasticNet_DifferentL1Ratios_ProduceDifferentLosses to
assert that lossL1 and lossL2 are different (e.g., Assert.NotEqual(lossL1,
lossL2)); keep or optionally retain the non-negative assertions but ensure the
new assertions verify the parameter effect on CalculateLoss from the
LossFunctionFactory creations.
- Around line 177-201: The tests ModelFactory_VAR_CreatesSuccessfully and
ModelFactory_ARMA_CreatesSuccessfully currently accept known broken behavior;
remove the placeholder comments and update the tests to assert actual prediction
correctness: after creating the model via ModelFactory<double, Matrix<double>,
Vector<double>>.Create(config) (where config.Name is "VAR" or "ARMA"), call the
model's Predict method on a small deterministic input and assert the returned
Vector<double> has the expected length and values (or at minimum the expected
length) instead of only checking NotNull/IsAssignableFrom; alternatively, if the
models are still broken, fix the model implementations (the classes returned by
ModelFactory for "VAR" and "ARMA") so that their Predict methods return
correctly sized results, and then add the prediction assertions to the tests
referencing ITimeSeriesModel<double>.Predict and ModelConfig to validate correct
behavior.
- Around line 27-120: The tests currently only assert NotNull but should verify
that ModelFactory<double,Matrix<double>,Vector<double>>.Create applied the
params from ModelConfig.Params to the created model; update each
"*_AppliesParameters" test to cast the returned model to the concrete
model/options type (or access its Options property) — e.g., for ARIMA cast to
the ARIMA model or ARIMAOptions and assert that p,d,q,learningRate,fitIntercept
equal the values in the config; similarly for SARIMA assert seasonalPeriod and
P/D/Q and for ExponentialSmoothing assert seasonalPeriod and includeTrend — use
the ModelConfig.Params keys and the model's public properties/options to compare
for equality.
In `@tests/AiDotNet.Tests/UnitTests/Training/DatasetFactoryTests.cs`:
- Around line 36-53: The test Create_WithParams_AppliesParametersToOptions
currently only asserts NotNull so it doesn't verify that DatasetConfig values
are applied; update the test to call DatasetFactory<T>.Create(config) and then
inspect the returned CsvDataLoader<double> (or its exposed
Options/Configuration) to assert specific properties from DatasetConfig (e.g.,
Path == "test.csv", HasHeader == true, BatchSize == 64 and any model-specific
setting like lagOrder == 3 if relevant). If CsvDataLoader<double> does not
expose options, modify the factory or loader to expose a read-only
Options/Config property (or provide a method to retrieve applied settings) so
the test can assert those values instead of just NotNull.
- Around line 55-73: The test Create_WithAliasParams_ResolvesCorrectly currently
only asserts NotNull and doesn't verify that the alias "p" was mapped to
LagOrder; update the test to assert the actual resolved mapping by checking the
resolved object's property (e.g., ResolvedParams.LagOrder or the Params
dictionary entry for "p") equals the expected value used in the fixture, and
replace the trivial Assert.NotNull with Assert.Equal(expectedLagOrder,
resolved.LagOrder) (or equivalent check against Params["p"]) so the alias
resolution is explicitly validated.
In `@tests/AiDotNet.Tests/UnitTests/Training/LossFunctionFactoryTests.cs`:
- Around line 56-97: Update each test to assert that the custom parameter was
actually applied: after calling
LossFunctionFactory<double>.Create(LossType.Huber, parameters) cast the returned
lossFunction to the concrete type (e.g., HuberLoss<double>) and assert its Delta
property equals 2.5; do the same for Focal (cast to FocalLoss<double> and assert
Gamma == 3.0 and Alpha == 0.5) and Quantile (cast to QuantileLoss<double> and
assert Quantile == 0.9). If those concrete types do not expose properties,
instead verify behavior changed by computing loss on the same sample inputs with
two different parameter sets and assert the results differ in the expected
direction (e.g., larger delta reduces sensitivity), referencing
LossFunctionFactory<double>.Create and the concrete loss classes (HuberLoss,
FocalLoss, QuantileLoss) to locate the code.
In `@tests/AiDotNet.Tests/UnitTests/Training/ModelFactoryTests.cs`:
- Around line 35-53: The test Create_WithParams_AppliesParametersToOptions
currently only asserts the model is non-null but should verify that the Params
dictionary (e.g., "lagOrder": 3) was actually applied to the created model;
update the test to retrieve the model's options (via a public Options/Settings
property if available on the returned model type) or use reflection to inspect
the concrete model instance returned by ModelFactory<double, Matrix<double>,
Vector<double>>.Create(ModelConfig) and assert that the option/field
corresponding to "lagOrder" equals 3; reference ModelConfig.Params,
ModelFactory.Create, and the test method
Create_WithParams_AppliesParametersToOptions when implementing the change.
- Around line 55-73: The test Create_WithAliasParams_ResolvesCorrectly currently
only checks the model is non-null; update it to assert that the alias "p" in the
ModelConfig.Params was applied by inspecting the created model's configuration
or property that stores lag order (e.g., check the model's LagOrder or
equivalent property on the returned instance from ModelFactory<double,
Matrix<double>, Vector<double>>.Create(config)) and assert it equals 2; use
ModelConfig, Params, "p", ModelFactory.Create and the model's
LagOrder/parameters accessor to locate and verify the resolved value.
- Around line 128-146: The test Create_WithSeasonalPeriodParam_Works only
verifies the model is non-null but doesn't assert that the seasonalPeriod
parameter was applied; update the test to cast the returned model from
ModelFactory<double, Matrix<double>, Vector<double>>.Create(config) to the
concrete exponential smoothing model type (e.g., ExponentialSmoothingModel or
the actual class returned by ModelFactory for "ExponentialSmoothing") and add an
assertion that the model's seasonal period property (e.g., SeasonalPeriod,
SeasonalPeriodLength, or equivalent) equals 12, ensuring the ModelConfig.Params
"seasonalPeriod" was propagated to the model.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- BatchNormalizationLayer: throw InvalidOperationException instead of silently passing through when forward hasn't been called - Trainer: replace Console.WriteLine with configurable LogAction property, make _csvLoader readonly, simplify loss function creation - ModelFactory: add Debug.WriteLine warnings for unknown params and conversion failures - TensorJsonConverter: handle empty shape as scalar tensor [1] - ONNXSentenceTransformer: throw FileNotFoundException instead of generating fallback embeddings when model file is missing - ParameterAnalyzer: throw ArgumentNullException for null groups input - Tests: add behavioral verification to factory tests, update tests to expect FileNotFoundException for missing model files, add type assertions and parameter comparison tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/RetrievalAugmentedGeneration/Embeddings/ONNXSentenceTransformer.cs`:
- Around line 22-23: Make the lazy initialization of the private fields _session
and _tokenizer in ONNXSentenceTransformer thread-safe by ensuring both are
published atomically: add a private readonly init lock (e.g., _initLock) and
perform a double-checked lock around the existing initialization logic so that
concurrent callers cannot see _session set while _tokenizer is still null or
perform duplicate inits; alternatively, replace the manual init with a single
Lazy<(InferenceSession, ITokenizer)> or a Lazy wrapper that constructs both
together and exposes them atomically to the rest of the class. Ensure all code
paths that used to set or check _session/_tokenizer use the new locked/owned
initialization routine so both objects are guaranteed non-null once published.
In `@src/Serialization/TensorJsonConverter.cs`:
- Around line 126-131: The WriteJson method in TensorJsonConverter currently
emits the tensor's raw Shape (possibly an empty array for scalars) which breaks
round-trip symmetry with the ReadJson normalization; update WriteJson to
normalize scalar shapes before serializing (e.g., if tensor.Shape.Length == 0
treat shape as new[] { 1 }) so the JSON output uses [1] for scalars just like
ReadJson produces, ensuring consistent serialize→deserialize behavior; modify
the shape-emission logic in WriteJson in the TensorJsonConverter class
accordingly.
In `@src/Training/Factories/ModelFactory.cs`:
- Around line 180-226: ConvertValue currently misses enum handling; add support
in the ConvertValue method (after nullable unwrapping and before the final
Convert.ChangeType fallback) by checking if targetType.IsEnum and then: if the
incoming value is a string attempt Enum.TryParse(targetType, valueString, true,
out parsed) and return the parsed enum; if the incoming value is numeric convert
it to the enum via Enum.ToObject(targetType, numericValue) and return that; this
ensures string names like "Additive" and numeric enum values both convert
correctly before falling back to ChangeType.
In
`@tests/AiDotNet.Tests/UnitTests/RAG/Embeddings/SentenceTransformersFineTunerTests.cs`:
- Line 4: Tests in SentenceTransformersFineTunerTests.cs use hardcoded filenames
("base-model-path.onnx", "output-model-path.onnx") which makes them flaky;
replace those literals with a deterministic helper that returns
guaranteed-missing temp paths (e.g., GetMissingModelPath()). Update every
missing-model test case (the blocks referenced around lines 65-78, 113-132,
202-216, 219-232, 254-270, 273-298) to call GetMissingModelPath() for both base
and output paths, and add a private static string GetMissingModelPath() helper
in the SentenceTransformersFineTunerTests class that generates a unique path in
Path.GetTempPath() (e.g., Path.Combine(Path.GetTempPath(), Guid.NewGuid() +
".onnx")) so tests no longer rely on the working directory.
In `@tests/AiDotNet.Tests/UnitTests/Training/LossFunctionFactoryTests.cs`:
- Around line 177-201: The test
Create_ElasticNetWithCustomParams_ProducesDifferentLoss currently does not
verify that different l1Ratio settings produce different loss values; update the
test to assert that lossL1 and lossL2 differ (e.g., use Assert.NotEqual or an
assertion with a small epsilon via Math.Abs) after calculating lossL1 and lossL2
so the ElasticNetLoss parameterization applied by
LossFunctionFactory<double>.Create is actually validated.
- Around line 80-104: The test
Create_FocalWithCustomParams_ProducesDifferentLoss currently only checks
non-negativity and types but not that different gamma values change output;
update the test to assert that the two computed losses differ by adding an
assertion like Assert.NotEqual(loss1, loss5) after computing loss1 and loss5
(the objects are created via LossFunctionFactory<double>.Create and losses
computed with CalculateLoss on the resulting FocalLoss<double> instances) so the
test verifies parameter impact.
- Make YamlConfigLoader internal with strict YAML validation - Add RunAsync to ITrainer and Trainer with ConfigureAwait(false) - Thread-safe lazy loading for ONNXSentenceTransformer (double-checked locking) - CsvDataLoader: async file loading, RFC 4180 quoted field parsing - Decouple Trainer from ITimeSeriesModel to IFullModel for broader model support - OptimizerFactory: use default/value-type params instead of all-null arguments - GraphAttentionLayer: use Engine broadcasting instead of manual loops for bias - ModelFactory: add enum string conversion support for YAML configs - TensorJsonConverter: normalize scalar shapes on write for round-trip consistency - Improve test assertions: verify model params, optimizer types, loss behavior - Use deterministic temp paths in SentenceTransformersFineTunerTests - Fix example YAML path to use placeholder instead of hardcoded data path Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 60 out of 60 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…rator
Implements a new mechanism for registering types in the YAML configuration
system without requiring Configure methods on AiModelBuilder. The generator
now discovers interfaces/classes marked with [YamlConfigurable("SectionName")]
and automatically finds and registers their implementations.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…istration Marks 89 interfaces across src/Interfaces/, src/Clustering/Interfaces/, src/DriftDetection/, src/LearningRateSchedulers/, and src/Scoring/ with the [YamlConfigurable] attribute. This enables the source generator to discover and register their implementations without needing Configure methods on AiModelBuilder. Registry grows from 3,231 to 4,636 types across 177 sections. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ce, video, pointcloud) Marks domain-specific interfaces with [YamlConfigurable] for YAML registry discovery: 11 Document interfaces, 8 Finance interfaces, 2 Video interfaces, and 3 PointCloud interfaces. Registry grows from 4,636 to 4,755 types across 196 sections. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ries Add attribute-based YAML registration to interfaces in: - PhysicsInformed (12 interfaces: PDESpecification, BoundaryCondition, etc.) - NeuralRadianceFields (1: RadianceField) - UncertaintyQuantification (2: BayesianLayer, UncertaintyEstimator) - ProgramSynthesis (3: ProgramSynthesizer, CodeModel, ProgramExecutionEngine) - CurriculumLearning (8: CurriculumScheduler, DifficultyEstimator, etc.) - ContinualLearning (6: ContinualLearner, ContinualLearningStrategy, etc.) - ActiveLearning (15: ActiveLearner, QueryStrategy, StoppingCriterion, etc.) Registry now at 4,814 types / 229 sections (up from 4,755 / 196). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Attributed: AdversarialAttack, AdversarialDefense, CertifiedDefense, DataTransformer, FineTuning, KnowledgeDistillationTrainer, OutlierRemoval, PipelineStep, ConvolutionalNetwork, TransformerNetwork. Registry: 4,818 types / 231 sections. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Attributed: AutoMLModel, Postprocessor, PredictiveModel, ContextCompressor, DocumentStore, GraphStore, HomomorphicEncryptionProvider, FederatedHeterogeneityCorrection, FederatedServerOptimizer, GradientBasedOptimizer, ContextFlow, Parameterizable. Registry: 5,471 types / 241 sections (up from 4,818 / 231). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Attributed: CompressionMetadata, EpisodicDataLoader, EpisodicDataset, ExperimentRun, FederatedClientDataLoader, GradientCache, GradientComputable, GraphDataLoader, InputGradientComputable, InputOutputDataLoader, IntermediateActivationStrategy, JitCompilable, MetaLearnerOptions, MetaLearningTask, ModelCache, PruningMask, RLDataLoader, SecondOrderGradientComputable, StreamingDataLoader, WeightLoadable, FeatureImportance, WeightedSampler. Registry: 8,052 types / 261 sections (up from 5,471 / 241). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 275 out of 275 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add [YamlConfigurable] attributes to interfaces in ActiveLearning, AdversarialRobustness, Augmentation, Clustering, ContinualLearning, CurriculumLearning, Deployment, Diffusion, DistributedTraining, Distributions, Evaluation, GaussianProcesses, Initialization, Interpretability, KnowledgeDistillation, MetaLearning, Preprocessing, ReinforcementLearning, RAG, SelfSupervisedLearning, and TransferLearning. Reverted 4 InferenceOptimization interfaces with struct constraints incompatible with the source generator. Registry: 8,261 types / 291 sections (up from 8,052 / 261). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…er model interfaces Attribute sub-interfaces in IMetric.cs (ProbabilisticClassificationMetric, RegressionMetric, RankingMetric, TimeSeriesMetric), IStatisticalTest.cs (PairedTest, StatisticalTest, MultipleComparisonTest, ClassifierComparisonTest), plus IChain, ITeacherModel, and ITransform. Registry: 8,304 types / 300 sections (up from 8,261 / 291). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rfaces Attribute IOracle, IStreamingEventDetectionSession, IStreamingTranscriptionSession, IStreamingSynthesisSession, and IAutoregressiveMultimodalModel. Completes Phase 4 attribute coverage for all actionable interfaces in the codebase. Registry: 8,304 types / 300 sections. All remaining 24 unattributed generic interfaces are already covered by Configure methods. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The YamlConfigurableAttribute.cs was never committed to git, causing CI builds to fail with CS0234 errors on both net10.0 and net471 since the attribute class didn't exist in the checkout. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Feb 15, 2026
Merged
1 of 2 tasks
3 tasks
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AiModelBuilderso power users can define training recipes in version-controlled YAML files instead of writing C# for every setting.Configure*()calls can override afterwardsUsage
Changes
src/AiDotNet.csprojsrc/Configuration/YamlModelConfig.cssrc/Configuration/YamlConfigLoader.cssrc/Configuration/YamlConfigApplier.cssrc/AiModelBuilder.cssrc/Factories/OptimizerFactory.cstests/.../YamlConfigTests.csKey design decisions
CreateOptimizer(OptimizerType)to use reflection (all optimizer ctors need model+options, not just options)Test plan
🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests