-
Notifications
You must be signed in to change notification settings - Fork 709
Adapt OpenAI health check based on endpoint configuration #11763
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
Adapt OpenAI health check based on endpoint configuration #11763
Conversation
…ints Co-authored-by: davidfowl <[email protected]>
|
@copilot Instead of dynamically adding and removing health checks, we should add a single healthcheck where the implementation change to either use the OpenAIModelHealthCheck or StatusPageHealthCheck. There are 3 cases:
|
Co-authored-by: davidfowl <[email protected]>
Implemented the adaptive health check approach as suggested. The new
This avoids dynamically adding/removing health checks. Commit: 4cae8ed |
|
WithHeathCheck should set state on OpenAIResource that changes the health check to use the OpenAIModelHealthCheck. There should be 3 methods on OpenAIHealthCheck for each of the cases:
|
…source Co-authored-by: davidfowl <[email protected]>
Refactored as requested:
Commit: 1399972 |
…Check Co-authored-by: davidfowl <[email protected]>
| /// <summary> | ||
| /// Gets or sets the connection string provider for model health check. | ||
| /// </summary> | ||
| internal Func<ValueTask<string?>>? ModelConnectionString { get; set; } |
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.
This isn't required. Remove it.
| _result = HealthCheckResult.Healthy("Custom OpenAI endpoint configured"); | ||
| return _result.Value; | ||
| // Case 2: Custom endpoint with model health check - use model health check | ||
| if (resource.UseModelHealthCheck && resource.ModelConnectionString is not null) |
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.
| if (resource.UseModelHealthCheck && resource.ModelConnectionString is not null) | |
| if (resource.UseModelHealthCheck) |
|
|
||
| try | ||
| { | ||
| var builder = new DbConnectionStringBuilder() { ConnectionString = await connectionString().ConfigureAwait(false) }; |
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.
| var builder = new DbConnectionStringBuilder() { ConnectionString = await connectionString().ConfigureAwait(false) }; | |
| var builder = new DbConnectionStringBuilder() { ConnectionString = await resource.ConnectionStringExpression.GetValueAsync(cancellationToken).ConfigureAwait(false) }; |
| var connectionString = resource.ModelConnectionString; | ||
|
|
||
| if (connectionString is null) | ||
| { | ||
| _result = HealthCheckResult.Unhealthy("Model connection string not available"); | ||
| return _result.Value; | ||
| } |
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.
| var connectionString = resource.ModelConnectionString; | |
| if (connectionString is null) | |
| { | |
| _result = HealthCheckResult.Unhealthy("Model connection string not available"); | |
| return _result.Value; | |
| } |
|
@copilot I messed up and gave the wrong instruction. We don't need to change the health check on the OpenAIModelResource so revert all of those changes. We just want to change the top level health check on the OpenAIResource to make it support 2 cases:
Delete the existing health checks and make a small modification to StatusPageHealthCheck to support these 2 cases. Rename StatusPageHealthCheck to something more appropriate. |
Co-authored-by: davidfowl <[email protected]>
Simplified as requested:
Commit: b00e489 |
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.
Pull Request Overview
This PR adapts the OpenAI health check to use different logic based on endpoint configuration, fixing an issue where custom OpenAI-compatible endpoints would incorrectly attempt to check status.openai.com instead of the configured endpoint.
Key changes:
- Renamed
StatusPageHealthChecktoOpenAIHealthCheckwith adaptive behavior - Added endpoint-aware health checking logic that uses StatusPage for default endpoints and returns healthy for custom endpoints
- Updated health check registration to pass the OpenAI resource instance instead of hardcoded status URL
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Aspire.Hosting.OpenAI/OpenAIHealthCheck.cs |
Renamed from StatusPageHealthCheck, added adaptive logic to check StatusPage for default endpoint or return healthy for custom endpoints |
src/Aspire.Hosting.OpenAI/OpenAIExtensions.cs |
Updated health check registration to use OpenAIHealthCheck with resource instance instead of AddStatusPageCheck |
Co-authored-by: Copilot <[email protected]>
|
Should we ask it to make the Uris (status and default) static const? |
|
Hey @copilot, can you make the DefaultEndpoint and Status |
Co-authored-by: sebastienros <[email protected]>
Made both Uri instances static readonly fields with proper naming convention (s_ prefix):
This avoids creating Uri instances on every health check invocation. Commit: 8050e74 |
| TimeSpan? timeout = null, | ||
| HealthStatus? failureStatus = null, | ||
| IEnumerable<string>? tags = null) | ||
| private static Task<HealthCheckResult> CheckEndpointHealthAsync() |
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.
What's the point of this method?
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.
@copilot just inline this code.
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.
Inlined the method - now directly returns HealthCheckResult.Healthy("Custom OpenAI endpoint configured") in the CheckHealthAsync method instead of calling a separate wrapper method.
Commit: c8a3897
|
@copilot - Are there any tests that can be written for this change? |
Co-authored-by: davidfowl <[email protected]>
Yes, tests could be added to verify the adaptive behavior of
The existing tests verify the health check annotation is registered, but don't test the actual health check logic itself. Adding unit tests for |
|
This works. Tested various configurations and found another bug: Tried this with ollama and this configuration: var model = builder.AddOpenAI("oai")
.WithEndpoint("http://localhost:11434/v1")
.WithApiKey(builder.AddParameter("key", "", secret: true))
.AddModel("model", "qwen3:4b")
.WithHealthCheck();Ths isn't related to the PR but empty string keys don't work. |
|
/backport to release/9.5 |
|
Started backporting to release/9.5: https://github.com/dotnet/aspire/actions/runs/18184151426 |
* Initial plan * Fix OpenAI health check to not use status.openai.com for custom endpoints Co-authored-by: davidfowl <[email protected]> * Implement adaptive OpenAI health check based on endpoint configuration Co-authored-by: davidfowl <[email protected]> * Refactor to use adaptive health check with state tracking on OpenAIResource Co-authored-by: davidfowl <[email protected]> * Remove StatusPageHealthCheck.cs as it's been merged into OpenAIHealthCheck Co-authored-by: davidfowl <[email protected]> * Simplify to use adaptive OpenAIHealthCheck with 2 cases Co-authored-by: davidfowl <[email protected]> * Remove StatusPage health check logic, return healthy for all cases Co-authored-by: davidfowl <[email protected]> * Restore OpenAIHealthCheck with CheckStatusPageAsync and CheckEndpointHealthAsync methods Co-authored-by: davidfowl <[email protected]> * Apply suggestions from code review Co-authored-by: Copilot <[email protected]> * Make DefaultEndpoint and StatusPage Uri instances static readonly fields Co-authored-by: sebastienros <[email protected]> * Inline CheckEndpointHealthAsync method Co-authored-by: davidfowl <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: davidfowl <[email protected]> Co-authored-by: David Fowler <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: sebastienros <[email protected]>
Fix OpenAI Health Check Custom Endpoint Issue
Changes Made
Inlined the
CheckEndpointHealthAsync()method to simplify the code:CheckEndpointHealthAsync()method - The method was just a simple wrapperHealthCheckResult.Healthy("Custom OpenAI endpoint configured")in theCheckHealthAsyncmethodThis simplification:
Implementation Details
CheckStatusPageAsync, otherwise returns healthyBehavior
No functional changes - the health check still works exactly the same:
Default OpenAI endpoint:
Custom endpoint:
Fixes #11762
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.