support assign job name #135
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds support for assigning and tracking job names in model requests, enhances token-based authentication with time-based caching, and refactors the model listing logic for better code organization. The changes enable users to specify models using either modelName or jobName@modelName format, while introducing a 10-minute cache for token-to-model mappings to reduce redundant REST server requests.
Key Changes:
- Introduced token caching with
tokenUpdatedTimemap and 10-minute refresh interval to improve efficiency - Extended
BaseSpecstruct withJobNameandUserNamefields for enhanced metadata tracking - Refactored model listing to support both
modelNameandjobName@modelNamemapping formats
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
src/model-proxy/src/types/config_types.go |
Added JobName and UserName fields to BaseSpec struct for storing job metadata |
src/model-proxy/src/proxy/proxy.go |
Extracted /v1/models endpoint logic into listAllModels method for better code organization |
src/model-proxy/src/proxy/model_server.go |
Renamed function to ListJobModelsMapping, added ModelEndpoint struct, implemented dual mapping for both modelName and jobName@modelName formats |
src/model-proxy/src/proxy/authenticator.go |
Added tokenUpdatedTime map and RefreshIntervalSeconds constant for time-based cache management of token-to-model mappings |
Comments suppressed due to low confidence (1)
src/model-proxy/src/proxy/authenticator.go:36
- The
tokenUpdatedTimeandtokenToJobModelsmaps are not initialized in the constructor. WhiletokenUpdatedTimeis used inUpdateTokenModelsandAuthenticateReq, it will cause a nil pointer panic when first accessed since the map is never initialized. TheUpdateTokenModelsmethod writes totokenUpdatedTime[token]without checking if the map is nil first.
func NewRestServerAuthenticator() *RestServerAuthenticator {
return &RestServerAuthenticator{
tokenToModels: make(map[string]map[string][]*types.BaseSpec),
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // rest-server token => model names => model service list | ||
| tokenToModels map[string]map[string][]*types.BaseSpec | ||
| tokenToModels map[string]map[string][]*types.BaseSpec | ||
| tokenToJobModels map[string]map[string][]*types.BaseSpec |
There was a problem hiding this comment.
The tokenToJobModels field is added to the struct but is never used anywhere in the codebase. This appears to be dead code that should either be removed or implemented if it was intended for future use.
| type modelMapping struct { | ||
| type ModelEndpoint struct { | ||
| modelName string | ||
| jobID string |
There was a problem hiding this comment.
The jobID field is added to the ModelEndpoint struct but is never populated or used. The jobId is available in the goroutine context but is not assigned to result.jobID when creating the struct on line 294. This unused field adds unnecessary complexity.
| // If the token is not found or the token info is older than RefreshIntervalSeconds, refresh it | ||
| if !ok || !timeOk || time.Now().Unix()-tokenLastUpdated > RefreshIntervalSeconds { | ||
| // request to RestServer to get the models | ||
| log.Printf("[-] Error: token %s not found in the authenticator\n", obfuscateToken(token)) |
There was a problem hiding this comment.
The log message says "Error: token %s not found" but this is misleading because the condition on line 61 also triggers when the token exists but the cached data is stale (older than RefreshIntervalSeconds). This message should be updated to reflect that it could be either a missing token or a cache refresh, not just an error condition.
hippogr
left a comment
hippogr
left a comment
There was a problem hiding this comment.
only one minor comment there, so I would like to approve it first.
| // If the token is not found or the token info is older than RefreshIntervalSeconds, refresh it | ||
|
|
||
| if !ok || !timeOk || time.Now().Unix()-tokenLastUpdated > RefreshIntervalSeconds { | ||
| // request to RestServer to get the models |
There was a problem hiding this comment.
why do we need to do the judgement !timeOk || time.Now().Unix()-tokenLastUpdated > RefreshIntervalSeconds twice? only for log?
This pull request introduces improvements to the model proxy authentication and model listing logic, focusing on more robust token management, improved model-to-job mapping, and code refactoring for clarity and maintainability. The most significant changes are grouped below:
Authentication and Token Management Improvements:
tokenUpdatedTimemap and aRefreshIntervalSecondsconstant to theRestServerAuthenticatorto cache and periodically refresh token-to-model mappings, reducing redundant requests and improving efficiency. The cache is refreshed if the mapping is older than 10 minutes or missing. [1] [2] [3]Model Mapping and Struct Enhancements:
GetJobModelsMappingtoListJobModelsMapping, improved concurrency handling, and ensured that bothmodelNameandjobName@modelNameare mapped to their respective model services. This allows users to specify the job name in the model field if desired. [1] [2] [3]BaseSpecstruct to includeJobNameandUserNamefields, providing more detailed metadata for each model service.Proxy Handler Refactoring:
/v1/modelsendpoint handling by extracting the logic into a newlistAllModelsmethod, improving code organization and readability. [1] [2] [3]These changes collectively improve the efficiency, maintainability, and flexibility of the model proxy service.