Refactor/model configuration #119
Conversation
srebrek
force-pushed
the
refactor/model-configuration
branch
from
5f4b42e to
c1f0045
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the model configuration system to address issues #113 and #96 by introducing a type-safe, capability-based model architecture. The changes replace the string-based model selection with concrete model classes and interfaces.
Changes:
- Introduced abstract
AIModelbase class withLocalModelandCloudModelsubclasses, plus capability interfaces (IReasoningModel,IVisionModel) - Added
ModelRegistryto replace the deprecatedKnownModelsclass with thread-safe registration and lookup - Created concrete model classes for 30+ local models and 12+ cloud models with proper metadata
- Updated
Chatentity withModelId(string) andModelInstance(AIModel) properties for backward compatibility - Refactored all service layers, contexts, examples, and tests to use the new type-safe model system
Reviewed changes
Copilot reviewed 58 out of 58 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
src/MaIN.Domain/Models/Abstract/AIModel.cs |
Defines base model classes and generic variants for runtime model registration |
src/MaIN.Domain/Models/Abstract/IModelCapabilities.cs |
Defines capability interfaces for vision, reasoning, embedding, and TTS |
src/MaIN.Domain/Models/Abstract/ModelRegistry.cs |
Thread-safe registry for model registration and lookup with reflection-based initialization |
src/MaIN.Domain/Models/Concrete/LocalModels.cs |
30+ concrete local model definitions with download URLs and metadata |
src/MaIN.Domain/Models/Concrete/CloudModels.cs |
12+ cloud model definitions for OpenAI, Anthropic, Gemini, xAI, GroqCloud, DeepSeek, Ollama |
src/MaIN.Domain/Entities/Chat.cs |
Added ModelInstance property with lazy loading from ModelId |
src/MaIN.Services/Services/LLMService/LLMService.cs |
Updated to use LocalModel with type checking and interface-based capabilities |
src/MaIN.Core/Hub/Contexts/ChatContext.cs |
Added type-safe WithModel overloads accepting AIModel or generic type parameters |
src/MaIN.Core/Hub/Contexts/ModelContext.cs |
Updated download and registry operations to work with new model system |
Examples/**/*.cs |
Updated 15+ examples to use strongly-typed model classes |
| Test files | Updated unit and integration tests to use new model IDs and types |
Comments suppressed due to low confidence (2)
src/MaIN.Domain/Exceptions/Models/MissingModelIdException.cs:8
- The PublicErrorMessage still says "Model name cannot be null or empty" but should be updated to say "Model id cannot be null or empty" to match the renamed exception class and parameter.
src/MaIN.InferPage/Components/Pages/Home.razor:215 - Call to obsolete method WithModel.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (chat.ModelId == ImageGenService.LocalImageModels.FLUX) | ||
| { | ||
| chat.Visual = true; // TODO: add IImageGenModel interface and check for that instead |
There was a problem hiding this comment.
The comment says this is a TODO to add IImageGenModel interface but the code is checking for a hardcoded model ID. This approach defeats the purpose of the interface-based design. The correct fix would be to check if chat.ModelInstance implements IImageGenModel interface once that interface is added, making this type-safe and extensible.
src/MaIN.Domain/Exceptions/Models/MissingModelInstanceException.cs
Outdated
Show resolved
Hide resolved
|
|
||
| if (!_models.TryAdd(normalizedId, model)) | ||
| { | ||
| throw new InvalidOperationException($"Model with ID '{model.Id}' is already registered."); |
There was a problem hiding this comment.
Suggestion: maybe it's worth creating a custom exception ModelAlreadyRegisteredException here?
|
|
||
| if (string.IsNullOrWhiteSpace(model.Id)) | ||
| { | ||
| throw new ArgumentException("Model ID cannot be null or empty.", nameof(model)); |
There was a problem hiding this comment.
I suggest use existing custom exception MissingModelIdException
| { | ||
| if (string.IsNullOrWhiteSpace(id)) | ||
| { | ||
| throw new ArgumentException("Model ID cannot be null or empty.", nameof(id)); |
There was a problem hiding this comment.
I suggest use existing custom exception MissingModelIdException
| @@ -0,0 +1,56 @@ | |||
| using MaIN.Domain.Models; | |||
There was a problem hiding this comment.
Unused using directive - can be removed
| @@ -0,0 +1,183 @@ | |||
| using MaIN.Domain.Exceptions; | |||
There was a problem hiding this comment.
Unused using directive - can be removed
| "GPT-4o Mini", | ||
| 128000, | ||
| "Fast and affordable OpenAI model for everyday tasks") | ||
| { |
There was a problem hiding this comment.
Minor changes: I think you can remove this empty body (curly brackets). Seems to be unnecessary. It applies to all models from Cloud Models.cs and Local Models.cs
|
@srebrek Overall, great job. I left a few comments, but they were mostly minor things. I just wonder if keeping the old approach in all Agent Examples or MCP Examples was intentional. I mean, there's still no "new type-safe model system". For example, in |
Yes, agents should also be changed but I dont want to make this PR too bloated. In the next PR I will thoroughly analyze agents and make them use the new approach. |
…ration #119 - Make TTS models implement ITTSModel - Fix and add tests - Cleanup
- register test model before each test
2 participants
Closes #113
Closes #96
Add Concrete classes for every supported model. Models can be either Local or Cloud and can implement various capability interfaces such as IReasoning or IVision... (more will be added soon). It is a foundation for safer runtime behavior eg. text to text model to generate an image. It is recommended to create concrete classes for not supported models by yourself and use generic types only for runtime cases eg. dynamically load modal as a plugin.
Tested on local examples and gemini examples.
Will add ImagaGeneration and ToolCalling interfaces in the future as this PR is already big.
Tested with examples for local models and gemini.