-
Notifications
You must be signed in to change notification settings - Fork 707
Add GitHub Models integration #10170
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
Conversation
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 introduces a new hosting integration for GitHub Models by providing a resource type, builder extensions, tests, documentation, and solution updates.
- Added
GitHubModelsResourceand connection‐string support - Provided extension methods
AddGitHubModel,WithEndpoint, andWithApiKey - Included unit tests, README docs, and solution/project registrations
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Aspire.Hosting.GitHub.Models/GitHubModelsResource.cs | Defines the GitHubModelsResource class with connection string |
| src/Aspire.Hosting.GitHub.Models/GitHubModelsExtensions.cs | Adds extension methods on IDistributedApplicationBuilder |
| tests/Aspire.Hosting.GitHub.Models.Tests/GitHubModelsExtensionTests.cs | Adds unit tests for the new extension methods |
| src/Aspire.Hosting.GitHub.Models/README.md | Documents installation and usage examples |
| Aspire.slnx | Registers new projects and playground folders |
Comments suppressed due to low confidence (2)
src/Aspire.Hosting.GitHub.Models/GitHubModelsExtensions.cs:21
- Public API methods lack
<remarks>and<code>examples in their XML comments. Please add<remarks>to explain scenarios and<code>snippets showing typical usage.
public static IResourceBuilder<GitHubModelsResource> AddGitHubModel(this IDistributedApplicationBuilder builder, [ResourceName] string name, string model)
tests/Aspire.Hosting.GitHub.Models.Tests/GitHubModelsExtensionTests.cs:28
- Consider adding tests to verify the argument guards for
AddGitHubModel,WithEndpoint, andWithApiKey, ensuring they throw the appropriate exceptions on null or empty inputs.
var customEndpoint = "https://custom.endpoint.com";
|
Great to see this finally coming in. Probably one of the easiest ways to get started with an LLM in Aspire. I've got a few suggestions for improvements. The first is that we should consider scanning the environment for the GITHUB_TOKEN. If you are running in a Codespaces and GitHub Actions (I think) this variable is available. We might want to consider using the new dashboard message bar APIs to prompt for consent before using that token since there is usage fees attached. Alternatively have an extension like WithCodespaceToken or something like that. |
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 agree with @mitchdenny on it using the GITHUB_TOKEN environment variable convention.
playground/GitHubModelsEndToEnd/GitHubModelsEndToEnd.WebStory/Properties/launchSettings.json
Outdated
Show resolved
Hide resolved
# Conflicts: # Aspire.slnx
...ground/GitHubModelsEndToEnd/GitHubModelsEndToEnd.AppHost/GitHubModelsEndToEnd.AppHost.csproj
Outdated
Show resolved
Hide resolved
|
Couldn't find a way to create a health check that would take the key into account. There is no documented endpoint that doesn't consume tokens. I also want to try embedding models and add support if possible. |
|
@DamianEdwards tku made the external resource “running” yes? Parameters are now “active”, do we just. Make everything running ? |
|
Can't wait to try this one out!! |
Yes I made the |
That's also something that bugged me. |
|
@sebastienros can you share a video of the end to end? |
| { | ||
| var builder = new DbConnectionStringBuilder() { ConnectionString = await connectionString().ConfigureAwait(false) }; | ||
|
|
||
| using var request = new HttpRequestMessage(HttpMethod.Post, new Uri($"{builder["Endpoint"]}/chat/completions")); |
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.
new UriBuilder(builder["Endpoint"]) { Path = "/chat/completions" }.Build()?
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.
With more !.ToString()!, it becomes quite ugly to read. Do you care much?
|
/backport to release/9.4 |
|
Started backporting to release/9.4: https://github.com/dotnet/aspire/actions/runs/16205012651 |
|
@eerhardt backporting to "release/9.4" failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: Add GitHub Models integration
.git/rebase-apply/patch:262: trailing whitespace.
.git/rebase-apply/patch:764: trailing whitespace.
.git/rebase-apply/patch:766: trailing whitespace.
.git/rebase-apply/patch:776: trailing whitespace.
.git/rebase-apply/patch:780: trailing whitespace.
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Using index info to reconstruct a base tree...
M Aspire.slnx
Falling back to patching base and 3-way merge...
Auto-merging Aspire.slnx
CONFLICT (content): Merge conflict in Aspire.slnx
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 Add GitHub Models integration
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
* Add GitHub Models integration * Fix parameters and doc * Add package logo * Support GITHUB_TOKEN env * Fix test * Rename to GithubModel * Remove iisexpress in launch * Update playground/GitHubModelsEndToEnd/GitHubModelsEndToEnd.AppHost/GitHubModelsEndToEnd.AppHost.csproj Co-authored-by: Eric Erhardt <[email protected]> * Define running state on github models * Add organization * Add tests * Add health checks * Add manifest files * Make health checks opt-in * Feedback * Fix test --------- Co-authored-by: Mitch Denny <[email protected]> Co-authored-by: Eric Erhardt <[email protected]>
* Add GitHub Models integration (#10170) * Add GitHub Models integration * Fix parameters and doc * Add package logo * Support GITHUB_TOKEN env * Fix test * Rename to GithubModel * Remove iisexpress in launch * Update playground/GitHubModelsEndToEnd/GitHubModelsEndToEnd.AppHost/GitHubModelsEndToEnd.AppHost.csproj Co-authored-by: Eric Erhardt <[email protected]> * Define running state on github models * Add organization * Add tests * Add health checks * Add manifest files * Make health checks opt-in * Feedback * Fix test --------- Co-authored-by: Mitch Denny <[email protected]> Co-authored-by: Eric Erhardt <[email protected]> * Apply suggestions from code review Co-authored-by: Dan Moseley <[email protected]> Co-authored-by: Copilot <[email protected]> --------- Co-authored-by: Sébastien Ros <[email protected]> Co-authored-by: Mitch Denny <[email protected]> Co-authored-by: Dan Moseley <[email protected]> Co-authored-by: Copilot <[email protected]>
backport typos Co-authored-by: Eric Erhardt <[email protected]>
Description
Adds a new hosting integration for GitHub Models.
Fixes #9569 #7649
Checklist
<remarks />and<code />elements on your triple slash comments?