fix: add SetModel method to IOptimizer to fix NullReferenceException in InitializeRandomSolution - #823
Conversation
…e in initializerandomsolution The optimizer's _model field was null when InitializeRandomSolution accessed model.ParameterCount because the model was never set on the optimizer after configuration. Changes: - Add SetModel(IFullModel) method to IOptimizer interface - Implement SetModel in OptimizerBase to set the _model field - Implement SetModel in ShardedOptimizerBase to delegate to wrapped optimizer - Call SetModel in AiModelBuilder before Optimize() in all training paths: - Regular training path (line 2107) - Hyperparameter optimization trial (line 1940) - Knowledge distillation fallback (line 5269) - Deep ensemble member optimizer (line 6793) This fixes Issue #225: [Fix] Kill Time Series Stubs & Implement Real Training by ensuring the model is always available to the optimizer before training. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds IOptimizer.SetModel and implementations in OptimizerBase and ShardedOptimizerBase; updates AiModelBuilder to call SetModel after Reset and before Optimize across training, KD-fallback, ensemble, and deep-ensemble paths; adds no-op SetModel stubs in tests; fixes ARIMA Predict index/length handling; adds .coderabbit.yaml and a small playground rename. Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as AiModelBuilder
participant Optim as IOptimizer
participant Model as IFullModel
participant Wrapped as Sharded/WrappedOptimizer
Builder->>Optim: Reset()
Note right of Optim: internal state cleared
Builder->>Optim: SetModel(Model)
Note right of Optim: optimizer bound to model
Builder->>Optim: Optimize(...)
Optim->>Model: Read/Modify parameters
%% Sharded/wrapped delegation
Builder->>Wrapped: SetModel(Model)
Wrapped->>Optim: WrappedOptimizer.SetModel(Model)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR addresses a NullReferenceException in OptimizerBase.InitializeRandomSolution by ensuring optimizers have their target model assigned before optimization runs, primarily through a new SetModel(...) API on IOptimizer and call sites in AiModelBuilder.
Changes:
- Added
SetModel(IFullModel<...> model)toIOptimizerand implemented it inOptimizerBase. - Implemented
SetModelinShardedOptimizerBaseby delegating to the wrapped optimizer. - Updated
AiModelBuildertraining flows to callSetModel(...)beforeOptimize().
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/Optimizers/OptimizerBase.cs | Adds SetModel(...) implementation to assign the internal _model. |
| src/Interfaces/IOptimizer.cs | Extends the optimizer public interface with a new SetModel(...) method. |
| src/DistributedTraining/ShardedOptimizerBase.cs | Delegates SetModel(...) to the wrapped optimizer for sharded/distributed cases. |
| src/AiModelBuilder.cs | Ensures the model is set on optimizers before optimization across several training paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/AiModelBuilder.cs`:
- Around line 5268-5270: The optimizer's internal state must be cleared before
running the fallback training to avoid carrying over partial KD updates: before
calling optimizer.SetModel(studentModel) / optimizer.Optimize(...) reset the
optimizer state (e.g., call optimizer.ResetState() or reinitialize a fresh
optimizer instance and assign it to the same variable) so Optimize runs from a
clean state; if ResetState() does not exist, construct a new optimizer of the
same type (or use a provided factory/CloneWithoutState) and then call
SetModel(studentModel) and Optimize with OptimizerHelper<T, TInput,
TOutput>.CreateOptimizationInputData(...).
🧹 Nitpick comments (1)
src/Optimizers/OptimizerBase.cs (1)
1220-1245: Consider adding a guard clause for_modelat the method entry.
InitializeRandomSolution(TInput)accesses_model!with the null-forgiving operator at lines 1245, 1325, 1337, and 1354. If a derived class or a new code path calls this beforeSetModel, the original NRE will resurface with no clear message. A single guard at the top converts a cryptic NRE into an actionableInvalidOperationException.🛡️ Proposed guard clause
protected virtual IFullModel<T, TInput, TOutput> InitializeRandomSolution(TInput trainingData) { if (trainingData == null) throw new ArgumentNullException(nameof(trainingData)); + if (_model == null) + throw new InvalidOperationException( + "Model has not been set. Call SetModel() before optimization."); // Compute lower and upper bounds from the training data
- Use _arCoefficients.Length instead of LagOrder for lastObservedValues to ensure dot product vectors have matching lengths - Guard lastObservedValues[0] access when P=0 (pure MA model) - Guard lastErrors[0] access when Q=0 (pure AR model) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add SetModel implementation to all test optimizer classes: - FederatedDeterministicDeltaOptimizer - FederatedNoOpOptimizer - SinglePassTestOptimizer - ResetTrackingOptimizer - MockOptimizer - PassthroughOptimizer - SingleStepTrainOptimizer - DeterministicNeuralNetworkParameterOptimizer - Address PR review comments: - Improve SetModel comments in AiModelBuilder to clarify purpose - Add Reset() before SetModel in KD fallback path for consistency - Make SetModel virtual in OptimizerBase with OnModelChanged hook - Add breaking change documentation to IOptimizer.SetModel Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Reorder HPO objective function: apply trial hyperparameters BEFORE Reset() so adaptive state is initialized from new hyperparameters - Improve SetModel comment wording to not imply Reset() clears the model - Update OnModelChanged signature to pass oldModel and newModel parameters allowing derived classes to react to model changes Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Configure CodeRabbit code reviews to enforce: - Facade pattern: users should only use AiModelBuilder and AiModelResult - Access modifier guidelines: prefer internal over public to hide IP - Review checklist for maintaining clean public API surface Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The matern-kernel playground example used 'length' but the constructor parameter is named 'lengthScale', causing compilation test failure. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Adds a protected RequireModel() helper that returns the model or throws InvalidOperationException with a clear message pointing to SetModel(). Replaces all _model! usages with RequireModel() for better diagnostics. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Optimizers/OptimizerBase.cs (1)
608-615:⚠️ Potential issue | 🟡 Minor
Model!.DeepCopy()still uses the null-forgiving operator — missedRequireModel()replacement.This call site can produce the same opaque
NullReferenceExceptionthe PR aims to eliminate. Updating it toRequireModel().DeepCopy()would be consistent with the changes elsewhere in the file.Proposed fix
- var solution = Model!.DeepCopy(); + var solution = RequireModel().DeepCopy();
🧹 Nitpick comments (2)
src/Optimizers/OptimizerBase.cs (2)
151-161:SetModelexpands the public API surface on optimizer types. As per the coding guidelines, users should interact throughAiModelBuilder, and optimizer internals should preferinternalaccess. SinceSetModelis driven by theIOptimizerinterface contract, consider using explicit interface implementation to keep the method off the optimizer's public type surface:void IOptimizer<T, TInput, TOutput>.SetModel(IFullModel<T, TInput, TOutput> model) { ... }This would still allow
AiModelBuilderto call it through theIOptimizerreference while not adding a new public method visible to users who might hold a concrete optimizer type. This is a minor concern given that optimizers are typically accessed via the interface already. As per coding guidelines: "Preferinternaloverpublicfor all classes, methods, and properties unless they are part of the facade API."
136-175: Well-designed lifecycle — consider sealingSetModelto protect the invariant.
RequireModel()and theOnModelChangedhook are clean additions. One architectural concern:SetModelispublic virtual, which means a derived optimizer can override it and forget to callbase.SetModel(...), leaving_modelsilentlynull. SinceOnModelChangedalready provides the extensibility hook, removingvirtualfromSetModel(making it sealed for override) would close this gap while keeping the same extension surface.Proposed diff
- public virtual void SetModel(IFullModel<T, TInput, TOutput> model) + public void SetModel(IFullModel<T, TInput, TOutput> model)If
IOptimizer.SetModelmust remain overridable for distributed wrappers likeShardedOptimizerBase, consider the Template Method pattern: keep the assignment + hook in a non-virtual public method, and only exposeOnModelChangedfor override.#!/bin/bash # Check if any derived class overrides SetModel (would break if we remove virtual) rg -n --type=cs 'override\s+.*\bSetModel\b' -C 2
|




Summary
NullReferenceExceptionatOptimizerBase.InitializeRandomSolutionline 1239 where_model.ParameterCountwas accessed but_modelwas nullRoot Cause
The optimizer's
_modelfield was never being set after configuration. The model was only passed during optimizer construction, but many constructors passednullfor the model parameter. WhenOptimize()was called,InitializeRandomSolution()would try to access_model.ParameterCountand throw aNullReferenceException.Changes
SetModel(IFullModel<T, TInput, TOutput> model)methodSetModelto set the_modelfieldSetModelto delegate to wrapped optimizerSetModel()calls beforeOptimize()in all 4 training paths:Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores