Use the GitHub models catalog endpoint for listing models #72
Conversation
09f5dd6 to
91fa594
Compare
91fa594 to
1cc232e
Compare
1cc232e to
1d5ff3f
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR transitions the model listing functionality from Azure AI Foundry's model catalog to GitHub's native models catalog endpoint. The change uses GitHub's own models API (https://models.github.ai/catalog) for listing models while maintaining Azure AI Foundry for detailed model information.
- Updates API client to use GitHub Models catalog endpoint instead of Azure AI Foundry for model listing
- Refactors model data structures to align with GitHub's API response format
- Removes deprecated model identifier formatting utility and updates all references to use GitHub's model IDs directly
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/azuremodels/types.go | Updates type definitions to match GitHub Models API response format |
| internal/azuremodels/azure_client.go | Changes ListModels to use GitHub Models catalog endpoint with GET request |
| internal/azuremodels/model_summary.go | Updates ModelSummary struct and removes deprecated FormatIdentifier function |
| internal/azuremodels/model_details.go | Removes FormatIdentifier function and related imports |
| cmd/view/view.go | Updates to use model.ID directly instead of FormatIdentifier |
| cmd/run/run.go | Updates model selection and validation to use GitHub model IDs |
| cmd/list/list.go | Updates model listing display to use GitHub model IDs |
| Test files | Updates test cases to use new GitHub model ID format and structure |
| idI := strings.ToLower(fmt.Sprintf("%s/%s", models[i].Publisher, models[i].Name)) | ||
| idJ := strings.ToLower(fmt.Sprintf("%s/%s", models[j].Publisher, models[j].Name)) |
There was a problem hiding this comment.
[nitpick] The sorting logic duplicates the model ID formatting that was previously handled by FormatIdentifier. Consider using the existing model.ID field or extracting this formatting logic into a helper function to avoid duplication.
| idI := strings.ToLower(fmt.Sprintf("%s/%s", models[i].Publisher, models[i].Name)) | |
| idJ := strings.ToLower(fmt.Sprintf("%s/%s", models[j].Publisher, models[j].Name)) | |
| idI := FormatIdentifier(models[i].Publisher, models[i].Name) | |
| idJ := FormatIdentifier(models[j].Publisher, models[j].Name) |
Copilot uses AI. Check for mistakes.
| idI := FormatIdentifier(models[i].Publisher, models[i].Name) | ||
| idJ := FormatIdentifier(models[j].Publisher, models[j].Name) | ||
| idI := strings.ToLower(fmt.Sprintf("%s/%s", models[i].Publisher, models[i].Name)) | ||
| idJ := strings.ToLower(fmt.Sprintf("%s/%s", models[j].Publisher, models[j].Name)) |
There was a problem hiding this comment.
[nitpick] The sorting logic duplicates the model ID formatting that was previously handled by FormatIdentifier. Consider using the existing model.ID field or extracting this formatting logic into a helper function to avoid duplication.
Copilot uses AI. Check for mistakes.
| @@ -500,7 +500,8 @@ func (h *runCommandHandler) getModelNameFromArgs(models []*azuremodels.ModelSumm | |||
| if !model.IsChatModel() { | |||
| continue | |||
| } | |||
| prompt.Options = append(prompt.Options, azuremodels.FormatIdentifier(model.Publisher, model.Name)) | |||
|
|
|||
| prompt.Options = append(prompt.Options, model.ID) | |||
There was a problem hiding this comment.
It's nice being able to use a simple ID property and not have to format an identifier from other bits.
| @@ -14,14 +14,13 @@ func TestView(t *testing.T) { | |||
| t.Run("NewViewCommand happy path", func(t *testing.T) { | |||
| client := azuremodels.NewMockClient() | |||
| modelSummary := &azuremodels.ModelSummary{ | |||
| ID: "test-id-1", | |||
| ID: "openai/test-model-1", | |||
There was a problem hiding this comment.
I thought I remembered talk recently of moving away from requiring the publisher name in model identifiers, something from @sgoedecke. Did that happen? Should we also have a test that just has the model name for its ID if so?
|
|
||
| modelKey, err := modelkey.ParseModelKey(catalogModel.ID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("parsing model key %q: %w", catalogModel.ID, err) |
There was a problem hiding this comment.
Ahh you calling it a "model key" here when printing out the ID makes me think that we will indeed have the publisher + model name combined in the field, so probably don't need such a test as I suggested above. 👍🏻
With this change we'll
models viewcommand