Conversation
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.
| public string MMProjectPath => "mmproj-model-f16.gguf"; | ||
| } | ||
|
|
||
| public sealed record Llava16Mistral_7b() : LocalModel( | ||
| "llava-1.6-mistral-7b", | ||
| "llava-1.6-mistral-7b.gguf", | ||
| new Uri("https://huggingface.co/cjpais/llava-1.6-mistral-7b-gguf/resolve/main/llava-v1.6-mistral-7b.Q3_K_XS.gguf?download=true"), | ||
| "LLaVA 1.6 Mistral 7B", | ||
| 4096, | ||
| "Vision-language model for image analysis, OCR, and visual Q&A"), IVisionModel | ||
| { | ||
| public string MMProjectPath => "mmproj-model-f16.gguf"; |
There was a problem hiding this comment.
Both Llava_7b and Llava16Mistral_7b use the same MMProjectPath "mmproj-model-f16.gguf". This will cause conflicts if both models are used, as they likely require different projection files. Each model should have its own unique MMProject file name.
| return _models.TryRemove(NormalizeId(id), out _); | ||
| } | ||
|
|
||
| private static string NormalizeId(string id) => id.Trim(); |
There was a problem hiding this comment.
The NormalizeId function only trims whitespace but doesn't perform case normalization, even though the ConcurrentDictionary is initialized with StringComparer.OrdinalIgnoreCase. This is inconsistent. Either remove the case-insensitive comparer or make NormalizeId also lowercase the ID for consistency.
| } | ||
| catch (Exception ex) | ||
| { | ||
| Console.WriteLine($"Warning: Could not register model type {type.Name}: {ex.Message}"); |
There was a problem hiding this comment.
Exception messages are written to Console.WriteLine during initialization. This may not be appropriate for all environments (e.g., ASP.NET applications, libraries). Consider using a proper logging framework or making the warning mechanism configurable/injectable.
| public AIModel? ModelInstance | ||
| { | ||
| get => _modelInstance ??= ModelRegistry.GetById(ModelId); | ||
| set => (_modelInstance, ModelId) = (value, value?.Id ?? ModelId); |
There was a problem hiding this comment.
The setter has a potential issue: when setting _modelInstance to null, it will keep the current ModelId instead of updating it. This creates an inconsistent state where ModelInstance is null but ModelId retains its old value. Consider: value is not null, set both _modelInstance and ModelId; if value is null, decide whether to clear ModelId or leave it.
| // Get built-in Gemini 2.5 Flash model | ||
| var model = AIHub.Model().GetModel(new Gemini2_5Flash().Id); | ||
|
|
||
| // Or create the model manually if not available in the hub | ||
| var customModel = new GenericCloudModel( | ||
| "gemini-2.5-flash", | ||
| BackendType.Gemini | ||
| ); | ||
|
|
||
| await AIHub.Chat() | ||
| .WithModel("gemini-2.5-flash") | ||
| .WithModel(customModel) |
There was a problem hiding this comment.
The example shows creating a GenericCloudModel but the model is retrieved from the registry and not used. Either use the retrieved model variable or remove it. The comment suggests getting from the hub, but then a custom model is created and used instead. This could be confusing for users following the example.
| // ===== Ollama Models ===== | ||
|
|
||
| public sealed record OllamaGemma3_4b() : CloudModel( | ||
| "gemma3:4b", |
There was a problem hiding this comment.
The model ID "gemma3-4b" doesn't match the cloud provider's expected format. According to the Gemini example using "gemini-2.5-flash", Gemini models likely use the "gemini-" prefix. However, this is registered as BackendType.Ollama, so it should use Ollama's format which is typically "model:size" (e.g., "gemma3:4b"). The ID should be consistent with how Ollama references models.
| 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.
| 4096, | ||
| "Vision-language model for image analysis, OCR, and visual Q&A"), IVisionModel | ||
| { | ||
| public string MMProjectPath => "mmproj-model-f16.gguf"; |
There was a problem hiding this comment.
The MMProjectPath is specified as a relative filename "mmproj-model-f16.gguf", but in the usage in LLMService.cs line 233, it's combined with modelsPath using Path.Combine(modelsPath, visionModel.MMProjectPath). This assumes the mmproj file is in the same directory as the models. Consider documenting this assumption or making the path more explicit.
| var x = model.GetModel("llama3.2:3b"); | ||
| await model.DownloadAsync(x.Name); | ||
| // Get models using ModelRegistry | ||
| var gemma = modelContext.GetModel("gemma3-4b"); |
| await model.DownloadAsync(x.Name); | ||
| // Get models using ModelRegistry | ||
| var gemma = modelContext.GetModel("gemma3-4b"); | ||
| var llama = modelContext.GetModel("llama3.2-3b"); |
| @@ -1,12 +1,19 @@ | |||
| using MaIN.Core; | |||
| using MaIN.Core.Hub; | |||
| using MaIN.Domain.Models.Abstract; | |||
There was a problem hiding this comment.
Unused using directive - can be removed
There was a problem hiding this comment.
Please take a look at the unit tests in this class.
CompleteAsync_ShouldCallChatService
CompleteAsync_ShouldCallChatService
failed but they worked until now.
| return this; | ||
| } | ||
|
|
||
| public IAgentConfigurationBuilder WithCustomModel(string model, string path, string? mmProject = null) |
There was a problem hiding this comment.
According to the ChatContext maybe try add sth like [Obsolete("Use WithModel<TModel>() instead.")]. Moreover in the AgentContext.cs (and interface IAgentBuilderEntryPoint) exists old method WithModel(string model). If there are no problems, maybe try changing it to a newer generic method.
| @@ -0,0 +1,5 @@ | |||
| namespace MaIN.Domain.Exceptions; | |||
|
|
|||
| public class ModelException(string message) : Exception(message) | |||
There was a problem hiding this comment.
It seems to be unused. Remove it please
| public class MissingModelInstanceException() | ||
| : MaINCustomException("Model instance cannot be null.") | ||
| { | ||
| public override string PublicErrorMessage => "Model instance cannot be null."; |
There was a problem hiding this comment.
Message defined inside constructor is the same so to avoid repetition you can use public override string PublicErrorMessage => Message;
|
|
||
| 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
| return localModel; | ||
| } | ||
|
|
||
| throw new InvalidOperationException($"Model '{modelId}' is not a local model. LLMService only supports local models."); |
There was a problem hiding this comment.
Suggestion: maybe it's worth creating a custom exception here?
| } | ||
|
|
||
| [Obsolete("Use ModelRegistry instead. This class will be removed in future versions.")] | ||
| public static class KnownModels |
There was a problem hiding this comment.
I see that there are still some uages for this class, check if it can't be chenaged with a new one - ModelRegistry
| await AIHub.Chat() | ||
| .WithModel<Gemma2_2b>() | ||
| .WithMessage("Generate a 4 sentence poem.") | ||
| .Speak(new TextToSpeechParams("kokoro:82m", voice, true)) |
There was a problem hiding this comment.
Should this model kokoro:82m still be specified manually here? I see that there is a type-safe definition of this model in LocalModels.cs
| string? SystemMessage = null) | ||
| { | ||
| /// <summary> Internal Id. For cloud models it is the cloud Id. </summary> | ||
| public virtual string Id { get; } = Id; |
There was a problem hiding this comment.
Should these properties (all properties in this record) be virtual? Are you assuming that in some derived record you will need to override the base class's properties?
| string FileName, | ||
| Uri? DownloadUrl = null, | ||
| string? Name = null, | ||
| uint MaxContextWindowSize = 128000, |
There was a problem hiding this comment.
This value of 'max context window size' (128000) is used in several places in this file, and I think it's also used in other places in the solution. Currently it can be perceived as a magic number that, if you need to edit it, you'll have to manually change it in multiple places. Maybe it's worth defining this value in a static object? This is just a suggestion, think about it; I'm not saying you have to implement it right away :)
| } | ||
|
|
||
| /// <summary> Generic class for runtime defined local models with reasoning capability. </summary> | ||
| public record GenericReasoningModel( |
There was a problem hiding this comment.
Was the omission of the word Local in the name of this record (and the ones below) intentional? You defined GenericLocalModel above, so maybe it's better to stick to the convention and explicitly write Local in the names everywhere, or omit the word everywhere?
| @@ -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
| /// <summary> | ||
| /// Registers a custom model at runtime (e.g., dynamically loaded GGUF file). | ||
| /// </summary> | ||
| public static void Register(AIModel model) |
There was a problem hiding this comment.
If you don't plan to use this method outside of this class, maybe it's worth changing its modifier to private? Currently, this method is only used in the 'Initialize' method.
| } | ||
|
|
||
| var availableIds = string.Join(", ", _models.Keys.Take(10)); | ||
| throw new KeyNotFoundException($"Model with ID '{id}' not found. Available models (first 10): {availableIds}"); |
There was a problem hiding this comment.
Is it crucial to only list the first 10 models here? Perhaps it's better to skip this or suggest using the GetAll method or the documentation to check which models are available? Besides, it might be a good candidate for a custom exception.
| "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 |
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.