-
Notifications
You must be signed in to change notification settings - Fork 832
Scope Ollama resilience settings to Web/Program.cs and restore ServiceDefaults #6850
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…nsions.cs in ServiceDefaults
builder.Services.AddSingleton<SemanticSearch>(); | ||
#if (IsOllama) | ||
// Get the IHttpClientBuilder for chat_httpClient | ||
var chatClientBuilder = builder.Services.AddHttpClient("chat_httpClient"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how are these named http clients used by ollama?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed offline, this is not ideal as chat_httpClient
is an implementation detail, we shouldn't depend on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, it would be nice to not rely on the internal behavior of another library like this.
That said, I don't currently see another option. We need a way to get the IHttpClientBuilder
. This might be a feature request for https://github.com/CommunityToolkit/Aspire.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't currently see another option
I think the intent of #6331 was to just move the following to Web/Program.cs
Lines 33 to 42 in c4e57fb
// Turn on resilience by default | |
http.AddStandardResilienceHandler(config => | |
{ | |
// Extend the HTTP Client timeout for Ollama | |
config.AttemptTimeout.Timeout = TimeSpan.FromMinutes(3); | |
// Must be at least double the AttemptTimeout to pass options validation | |
config.CircuitBreaker.SamplingDuration = TimeSpan.FromMinutes(10); | |
config.TotalRequestTimeout.Timeout = TimeSpan.FromMinutes(10); | |
}); |
And remove that #if (IsOllama)
block, @MackinnonBuck, wouldn't that suffice? As per @jongalloway, that would narrow it down to configure only the clients in Web.csproj.
We would have two ConfigureHttpClientDefaults
calls, though, not sure if that's OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, we tried to narrow it down further checking the IHttpClientBuilder names:
builder.Services.ConfigureHttpClientDefaults(http =>
{
if (http.Name.Contains("chat") || http.Name.Contains("embeddings"))
{
#pragma warning disable EXTEXP0001 // RemoveAllResilienceHandlers is experimental
http.RemoveAllResilienceHandlers();
#pragma warning restore EXTEXP0001
// Turn on resilience by default
http.AddStandardResilienceHandler(config =>
{
// Extend the HTTP Client timeout for Ollama
config.AttemptTimeout.Timeout = TimeSpan.FromMinutes(3);
// Must be at least double the AttemptTimeout to pass options validation
config.CircuitBreaker.SamplingDuration = TimeSpan.FromMinutes(10);
config.TotalRequestTimeout.Timeout = TimeSpan.FromMinutes(10);
});
// Turn on service discovery by default
http.AddServiceDiscovery();
}
});
But this didn't work, we only observed builders with null names. We would need to dig deeper to understand what's happening, just letting you know in case it rings a bell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, my main request is that we scope to the web project as opposed to the entire Aspire solution. Could also add a comment explaining the behavior and recommending removing the block of code before deploying to production (since Ollama won't be used in production).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, got it. Thanks for the clarification.
My main suggestion would be to consider putting this logic somewhere other than Program.cs
- maybe another file defining an additional extension method that gets called from Program.cs
.
Fixes #6331
Restore standard global HTTP client resilience configuration in
ServiceDefaults/Program.cs
, removing Ollama-specific timeouts and settings. Apply extended timeouts and custom resilience handlers only tochat_httpClient
andembeddings_httpClient
inWeb/Program.cs
usingRemoveAllResilienceHandlers
andAddStandardResilienceHandler
. Scope Ollama configuration to the web project to prevent unintended global changes and align with .NET Aspire template conventions.Microsoft Reviewers: Open in CodeFlow