-
Notifications
You must be signed in to change notification settings - Fork 40
feat: Return aliases in /v1/models too #94
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
@mostlygeek , could you review? |
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.
ugh. i had a review I left as pending. One thing I noticed while looking at this was that aliases needed to be unique across all models. There’s no enforcement of that constraint at the moment.
@@ -37,6 +37,15 @@ type ProxyManager struct { | |||
} | |||
|
|||
func New(config *Config) *ProxyManager { | |||
// Populate aliases map if not already 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.
why duplicate this code here?
Lines 72 to 79 in b138d6c
// Populate the aliases map | |
config.aliases = make(map[string]string) | |
for modelName, modelConfig := range config.Models { | |
for _, alias := range modelConfig.Aliases { | |
config.aliases[alias] = modelName | |
} | |
} | |
} | ||
|
||
|
||
// also list all aliases from config.aliases |
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.
need tests for aliases here:
llama-swap/proxy/proxymanager_test.go
Lines 148 to 214 in b138d6c
func TestProxyManager_ListModelsHandler(t *testing.T) { | |
config := &Config{ | |
HealthCheckTimeout: 15, | |
Models: map[string]ModelConfig{ | |
"model1": getTestSimpleResponderConfig("model1"), | |
"model2": getTestSimpleResponderConfig("model2"), | |
"model3": getTestSimpleResponderConfig("model3"), | |
}, | |
} | |
proxy := New(config) | |
// Create a test request | |
req := httptest.NewRequest("GET", "/v1/models", nil) | |
req.Header.Add("Origin", "i-am-the-origin") | |
w := httptest.NewRecorder() | |
// Call the listModelsHandler | |
proxy.HandlerFunc(w, req) | |
// Check the response status code | |
assert.Equal(t, http.StatusOK, w.Code) | |
// Check for Access-Control-Allow-Origin | |
assert.Equal(t, req.Header.Get("Origin"), w.Result().Header.Get("Access-Control-Allow-Origin")) | |
// Parse the JSON response | |
var response struct { | |
Data []map[string]interface{} `json:"data"` | |
} | |
if err := json.Unmarshal(w.Body.Bytes(), &response); err != nil { | |
t.Fatalf("Failed to parse JSON response: %v", err) | |
} | |
// Check the number of models returned | |
assert.Len(t, response.Data, 3) | |
// Check the details of each model | |
expectedModels := map[string]struct{}{ | |
"model1": {}, | |
"model2": {}, | |
"model3": {}, | |
} | |
for _, model := range response.Data { | |
modelID, ok := model["id"].(string) | |
assert.True(t, ok, "model ID should be a string") | |
_, exists := expectedModels[modelID] | |
assert.True(t, exists, "unexpected model ID: %s", modelID) | |
delete(expectedModels, modelID) | |
object, ok := model["object"].(string) | |
assert.True(t, ok, "object should be a string") | |
assert.Equal(t, "model", object) | |
created, ok := model["created"].(float64) | |
assert.True(t, ok, "created should be a number") | |
assert.Greater(t, created, float64(0)) // Assuming the timestamp is positive | |
ownedBy, ok := model["owned_by"].(string) | |
assert.True(t, ok, "owned_by should be a string") | |
assert.Equal(t, "llama-swap", ownedBy) | |
} | |
// Ensure all expected models were returned | |
assert.Empty(t, expectedModels, "not all expected models were returned") | |
} |
@lee-b I'm going to add the alias checks into the configuration. |
This includes aliases in the list of models returned by the v1/models endpoint. It's needed if you want to use some tools that expect proprietary models like o1 to exist, but a good thing to have for flexibility, regardless.