Merged
Conversation
Reviewer's GuideThis PR refactors the chat completion path to adopt a shared IChatClient interface, extends dependency injection and kernel builder to register chat clients, enhances the orchestration framework with streaming callbacks, adds Python connector audio support, updates vector data documentation, and delivers comprehensive unit and integration tests for the Ollama chat client. Sequence diagram for orchestration streaming callback flowsequenceDiagram
actor User
participant Orchestration as Orchestration
participant AgentActor as AgentActor
participant Agent as Agent
participant Callback as StreamingResponseCallback
User->>Orchestration: InvokeAsync(..., StreamingResponseCallback)
Orchestration->>AgentActor: InvokeAsync(...)
AgentActor->>Agent: InvokeStreamingAsync(..., OnIntermediateMessage)
Agent-->>AgentActor: StreamingChatMessageContent
AgentActor->>Callback: StreamingResponseCallback(streamedResponse, isFinal)
Callback-->>User: (streamed message chunk)
Note over AgentActor,Callback: Callback is invoked for each streamed message
AgentActor-->>Orchestration: Final ChatMessageContent
Orchestration-->>User: OrchestrationResult
Entity relationship diagram for VectorData.Abstractions documentation updateerDiagram
VECTORSTORE ||--o{ VECTORSTORECOLLECTION : contains
VECTORSTORECOLLECTION ||--o{ VECTORRECORD : has
VECTORSTORE {
string Id
string Name
}
VECTORSTORECOLLECTION {
string Id
string Name
Schema Schema
}
VECTORRECORD {
string Id
float[] Embedding
string Metadata
}
Class diagram for new and updated Ollama Chat Client registration extensionsclassDiagram
class IServiceCollection
class IKernelBuilder
class IChatClient
class OllamaApiClient
IServiceCollection <|.. OllamaServiceCollectionExtensions
IKernelBuilder <|.. OllamaKernelBuilderExtensions
OllamaApiClient ..|> IChatClient
OllamaServiceCollectionExtensions : +AddOllamaChatClient(modelId, endpoint, serviceId)
OllamaServiceCollectionExtensions : +AddOllamaChatClient(modelId, httpClient, serviceId)
OllamaServiceCollectionExtensions : +AddOllamaChatClient(ollamaClient, serviceId)
OllamaKernelBuilderExtensions : +AddOllamaChatClient(modelId, endpoint, serviceId)
OllamaKernelBuilderExtensions : +AddOllamaChatClient(modelId, httpClient, serviceId)
OllamaKernelBuilderExtensions : +AddOllamaChatClient(ollamaClient, serviceId)
OllamaApiClient : +GetResponseAsync()
OllamaApiClient : +GetStreamingResponseAsync()
IChatClient <|.. OllamaApiClient
Class diagram for OrchestrationContext and streaming callback supportclassDiagram
class OrchestrationContext {
+string Orchestration
+TopicId Topic
+OrchestrationResponseCallback? ResponseCallback
+OrchestrationStreamingCallback? StreamingResponseCallback
+ILoggerFactory LoggerFactory
+CancellationToken Cancellation
}
class OrchestrationResponseCallback
class OrchestrationStreamingCallback
OrchestrationContext --> OrchestrationResponseCallback
OrchestrationContext --> OrchestrationStreamingCallback
Class diagram for AgentActor streaming and callback changesclassDiagram
class AgentActor {
-AgentInvokeOptions _options
+ValueTask<ChatMessageContent> InvokeAsync(IList<ChatMessageContent>, CancellationToken)
+Task InvokeAsync(IList<ChatMessageContent>, AgentInvokeOptions, CancellationToken)
+Task InvokeStreamingAsync(IList<ChatMessageContent>, AgentInvokeOptions, CancellationToken)
+AgentInvokeOptions GetInvokeOptions(Func<ChatMessageContent, Task>)
+AgentInvokeOptions CreateInvokeOptions(Func<ChatMessageContent, Task>)
}
class AgentInvokeOptions {
+Func<ChatMessageContent, Task> OnIntermediateMessage
}
AgentActor --> AgentInvokeOptions
Class diagram for HandoffActor override of streaming optionsclassDiagram
class HandoffActor {
+AgentInvokeOptions CreateInvokeOptions(Func<ChatMessageContent, Task>)
}
HandoffActor --|> AgentActor
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Bryan-Roe - I've reviewed your changes - here's some feedback:
- The C# samples use invalid list initializer syntax (e.g.,
List<ChatMessage> chatHistory = [ … ];); switch tonew List<ChatMessage> { … }to ensure the code compiles. - In the Python MCP connector
_call_toolloop, theisinstancecheck usesvalueinstead of the loop variableitem, which will bypass the intended filtering—update it to checkitem. - The three overloads in
AddOllamaChatClientshare nearly identical builder logic—consider extracting the common setup into a private helper to reduce duplication and simplify future maintenance.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The C# samples use invalid list initializer syntax (e.g., `List<ChatMessage> chatHistory = [ … ];`); switch to `new List<ChatMessage> { … }` to ensure the code compiles.
- In the Python MCP connector `_call_tool` loop, the `isinstance` check uses `value` instead of the loop variable `item`, which will bypass the intended filtering—update it to check `item`.
- The three overloads in `AddOllamaChatClient` share nearly identical builder logic—consider extracting the common setup into a private helper to reduce duplication and simplify future maintenance.
## Individual Comments
### Comment 1
<location> `dotnet/src/Agents/Orchestration/AgentActor.cs:100` </location>
<code_context>
- // The vast majority of responses will be a single message. Responses with multiple messages will have their content merged.
- ChatMessageContent response = new(firstResponse?.Message.Role ?? AuthorRole.Assistant, string.Join("\n\n", responses.Select(response => response.Message)))
+ AgentInvokeOptions options = this.GetInvokeOptions(HandleMessage);
+ if (this.Context.StreamingResponseCallback == null)
+ {
+ // No need to utilize streaming if no callback is provided
</code_context>
<issue_to_address>
Non-streaming path discards the response
Assign the result of `InvokeAsync` to the `response` variable in the non-streaming branch to ensure the method returns the correct assistant message.
</issue_to_address>
### Comment 2
<location> `python/semantic_kernel/connectors/mcp.py:904` </location>
<code_context>
+ ] = []
if isinstance(value, list):
for item in value:
- if isinstance(value, (TextContent, ImageContent, BinaryContent, ChatMessageContent)):
+ if isinstance(
+ value, (TextContent, ImageContent, BinaryContent, AudioContent, ChatMessageContent)
</code_context>
<issue_to_address>
Incorrect variable used in isinstance check
The type check should be applied to `item` within the loop, not `value`. Use `isinstance(item, ...)` to check each element's type.
</issue_to_address>
### Comment 3
<location> `dotnet/src/VectorData/VectorData.Abstractions/PACKAGE.md:11` </location>
<code_context>
+- Abstractions include:
+ - Creating, listing and deleting collections with custom schema support.
+ - Creating, retrieving, updating and deleting records.
+ - Similarty search using vector embeddings.
+ - Search using filters.
+ - Hybrid search combining vector similarity and keyword search.
</code_context>
<issue_to_address>
Typo: 'Similarty' should be 'Similarity'.
Update the bullet point to use the correct spelling: 'Similarity'.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
- Similarty search using vector embeddings.
=======
- Similarity search using vector embeddings.
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `dotnet/src/Connectors/Connectors.Ollama/Extensions/OllamaServiceCollectionExtensions.DependencyInjection.cs:28` </location>
<code_context>
+ /// <param name="endpoint">The endpoint to Ollama hosted service.</param>
+ /// <param name="serviceId">Optional service ID.</param>
+ /// <returns>The updated service collection.</returns>
+ public static IServiceCollection AddOllamaChatClient(
+ this IServiceCollection services,
+ string modelId,
</code_context>
<issue_to_address>
Consider refactoring the repeated client building and registration logic into shared private helper methods to reduce duplication across overloads.
Consider extracting the common “build & configure” logic into a single helper and a shared private registration method. This removes the duplication in each overload while keeping your three public APIs.
```csharp
// 1) common builder helper
private static IChatClient BuildChatClient(
OllamaApiClient client,
IServiceProvider sp,
ILoggerFactory? loggerFactory)
{
var builder = client.AsBuilder();
if (loggerFactory is not null)
{
builder.UseLogging(loggerFactory);
}
return builder
.UseKernelFunctionInvocation(loggerFactory)
.Build(sp);
}
// 2) shared registration impl
private static IServiceCollection AddOllamaChatClientImpl(
this IServiceCollection services,
Func<IServiceProvider, OllamaApiClient> clientFactory,
string? serviceId)
{
Verify.NotNull(services);
return services.AddKeyedSingleton<IChatClient>(
serviceId,
(sp, _) =>
{
var loggerFactory = sp.GetService<ILoggerFactory>();
var client = clientFactory(sp);
return BuildChatClient(client, sp, loggerFactory);
});
}
// 3) public overloads now delegate
public static IServiceCollection AddOllamaChatClient(
this IServiceCollection services,
string modelId,
Uri endpoint,
string? serviceId = null)
=> services.AddOllamaChatClientImpl(
sp => new OllamaApiClient(endpoint, modelId),
serviceId);
public static IServiceCollection AddOllamaChatClient(
this IServiceCollection services,
string modelId,
HttpClient? httpClient = null,
string? serviceId = null)
=> services.AddOllamaChatClientImpl(
sp =>
{
httpClient ??= HttpClientProvider.GetHttpClient(httpClient, sp);
return new OllamaApiClient(httpClient, modelId);
},
serviceId);
public static IServiceCollection AddOllamaChatClient(
this IServiceCollection services,
OllamaApiClient? ollamaClient = null,
string? serviceId = null)
=> services.AddOllamaChatClientImpl(
sp =>
{
// existing resolution logic
var client = ollamaClient
?? sp.GetKeyedService<OllamaApiClient>(serviceId)
?? sp.GetKeyedService<IOllamaApiClient>(serviceId) as OllamaApiClient
?? sp.GetService<OllamaApiClient>()
?? sp.GetRequiredService<IOllamaApiClient>() as OllamaApiClient
?? throw new InvalidOperationException($"No {nameof(IOllamaApiClient)} found");
return client;
},
serviceId);
```
Repeat the same pattern in the Embeddings region. This preserves all three overloads but centralizes the common build/configure routine.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...ectors/Connectors.Ollama/Extensions/OllamaServiceCollectionExtensions.DependencyInjection.cs
Show resolved
Hide resolved
This was referenced Jun 19, 2025
There was a problem hiding this comment.
Pull Request Overview
This pull request implements a major refactoring to unify and extend Ollama chat client support across .NET and Python SDKs, introducing new IChatClient API support, dependency injection extensions, and enhanced agent orchestration capabilities.
- Refactored .NET chat completion samples to use IChatClient with unified streaming helpers
- Added comprehensive Ollama chat client integration including DI container and kernel builder extensions
- Enhanced agent orchestration with streaming callbacks and improved test coverage across multiple agent types
Reviewed Changes
Copilot reviewed 294 out of 783 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| dotnet/src/Plugins/Plugins.UnitTests/Plugins.UnitTests.csproj | Updated package reference from Microsoft.Extensions.TimeProvider.Testing to System.Linq.AsyncEnumerable |
| dotnet/src/Plugins/Plugins.UnitTests/Memory/VolatileMemoryStoreTests.cs | Replaced .ToEnumerable() calls with async equivalents (.ToArrayAsync(), .ToListAsync()) |
| dotnet/src/Plugins/Plugins.StructuredData.EntityFramework/StructuredDataServiceExtensions.cs | Renamed SelectAsync function to Select for consistency |
| dotnet/src/Plugins/Plugins.StructuredData.EntityFramework/StructuredDataService.cs | Added conditional compilation for OData library imports (NET8+ uses OData2Linq, older versions use Community.OData.Linq) |
| dotnet/src/InternalUtilities/src/Linq/AsyncEnumerable.cs | Extended with new async enumerable methods including Cast, Reverse, FirstAsync, LastAsync, SingleAsync, ToArrayAsync, Where, and OrderBy |
| dotnet/src/IntegrationTests/Connectors/Ollama/OllamaChatClientIntegrationTests.cs | Added comprehensive integration tests for Ollama chat client including DI registration and kernel builder extensions |
| dotnet/src/Functions/Functions.OpenApi/Serialization/OpenApiTypeConverter.cs | Enhanced type conversion to support JsonElement inputs alongside existing types |
| dotnet/src/Connectors/Connectors.OpenAI/Settings/OpenAIPromptExecutionSettings.cs | Added PrepareChatHistoryForRequest method to automatically inject system and developer prompts |
Comment on lines
552
to
563
| // Inserts system and developer prompts at the beginning of the chat history if they are not already present. | ||
| if (!string.IsNullOrWhiteSpace(this.ChatDeveloperPrompt) && !chatHistory.Any(m => m.Role == AuthorRole.Developer)) | ||
| { | ||
| chatHistory.Insert(0, new ChatMessageContent(AuthorRole.Developer, this.ChatDeveloperPrompt)); | ||
| } | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(this.ChatSystemPrompt) && !chatHistory.Any(m => m.Role == AuthorRole.System)) | ||
| { | ||
| chatHistory.Insert(0, new ChatMessageContent(AuthorRole.System, this.ChatSystemPrompt)); | ||
| } | ||
|
|
||
| return chatHistory; |
There was a problem hiding this comment.
The method mutates the input ChatHistory parameter by inserting messages at index 0, which could cause unexpected side effects for callers. Consider returning a new ChatHistory instance or clearly documenting that the input parameter will be modified.
Suggested change
| // Inserts system and developer prompts at the beginning of the chat history if they are not already present. | |
| if (!string.IsNullOrWhiteSpace(this.ChatDeveloperPrompt) && !chatHistory.Any(m => m.Role == AuthorRole.Developer)) | |
| { | |
| chatHistory.Insert(0, new ChatMessageContent(AuthorRole.Developer, this.ChatDeveloperPrompt)); | |
| } | |
| if (!string.IsNullOrWhiteSpace(this.ChatSystemPrompt) && !chatHistory.Any(m => m.Role == AuthorRole.System)) | |
| { | |
| chatHistory.Insert(0, new ChatMessageContent(AuthorRole.System, this.ChatSystemPrompt)); | |
| } | |
| return chatHistory; | |
| // Create a new ChatHistory instance to avoid mutating the input parameter. | |
| var newChatHistory = new ChatHistory(); | |
| // Insert system and developer prompts at the beginning if not already present. | |
| if (!string.IsNullOrWhiteSpace(this.ChatSystemPrompt) && !chatHistory.Any(m => m.Role == AuthorRole.System)) | |
| { | |
| newChatHistory.Add(new ChatMessageContent(AuthorRole.System, this.ChatSystemPrompt)); | |
| } | |
| if (!string.IsNullOrWhiteSpace(this.ChatDeveloperPrompt) && !chatHistory.Any(m => m.Role == AuthorRole.Developer)) | |
| { | |
| newChatHistory.Add(new ChatMessageContent(AuthorRole.Developer, this.ChatDeveloperPrompt)); | |
| } | |
| // Add the rest of the original chat history messages. | |
| foreach (var message in chatHistory) | |
| { | |
| newChatHistory.Add(message); | |
| } | |
| return newChatHistory; |
Comment on lines
263
to
265
| var keys = results.Select(keySelector).ToArray(); | ||
| Array.Sort(keys, results); | ||
| foreach (var result in results) |
There was a problem hiding this comment.
The OrderBy implementation sorts arrays in-place but may not work correctly for all key types. The Array.Sort(keys, results) assumes that the key array can be sorted alongside the results array, but this will fail if TKey doesn't implement IComparable. Consider using a more robust sorting approach with key-value pairs.
Suggested change
| var keys = results.Select(keySelector).ToArray(); | |
| Array.Sort(keys, results); | |
| foreach (var result in results) | |
| foreach (var result in results.OrderBy(keySelector)) |
| { | ||
| string stringArgument => JsonValue.Create(long.Parse(stringArgument, CultureInfo.InvariantCulture)), | ||
| byte or sbyte or short or ushort or int or uint or long or ulong => JsonValue.Create(argument), | ||
| JsonElement jsonElement when jsonElement.TryGetInt64(out var intValue) => JsonValue.Create(intValue), |
There was a problem hiding this comment.
The JsonElement handling for integer type conversion may lose precision for large numbers. TryGetInt64 will fail for numbers that exceed long.MaxValue, but the original value might be a valid integer. Consider handling different numeric ranges or documenting the precision limitations.
Suggested change
| JsonElement jsonElement when jsonElement.TryGetInt64(out var intValue) => JsonValue.Create(intValue), | |
| JsonElement jsonElement when jsonElement.TryGetInt64(out var intValue) => JsonValue.Create(intValue), | |
| JsonElement jsonElement when jsonElement.ValueKind == JsonValueKind.Number && jsonElement.TryGetRawText(out var rawText) => JsonValue.Create(jsonElement.GetRawText()), |
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
1 participant
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 by Sourcery
Unify and extend Ollama chat client support across .NET and Python SDKs: refactor samples to use the new IChatClient API with streaming helpers, introduce DI and kernel builder extension methods for registering Ollama chat clients, add audio content support in the Python connector, enhance agent orchestration to expose streaming callbacks, and bolster coverage with new unit and integration tests.
New Features:
Enhancements:
Tests: