-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
refactor: ListModels Filtering Upgrade #2773
Changes from 25 commits
d170bb7
9012344
d5d6837
688cddc
d9443cf
41c1ecc
0ced357
d2699d2
380282d
865aad4
fd92e81
7115941
bbd56bd
aace492
ffe1006
cffb148
845bb83
46b651b
ca462c8
39ab1fd
474a417
7d731f5
794c359
b23f0c0
1d279ff
c6ca55f
5c1b603
f3e9759
579ee54
a56dda7
6182737
a9e97ae
569f03b
6357b41
a0f7888
f48f62f
0ad9b72
bec39f2
a16790a
7cb71c7
f2c4fb4
66f7826
6c8c00f
5ad8943
9cb2d0c
2e8c010
5c22b95
8aa637f
595bc8f
67d6bf2
d68dcca
320ab30
f9dd9d2
1b50daa
0e77ac2
898b6be
258c9bd
204d6a1
0b43fad
64d2f63
f68d54e
e67fcce
7589128
45c809c
d4224e3
d750dc6
68a1006
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
package config | ||
|
||
import "regexp" | ||
|
||
type BackendConfigFilterFn func(string, *BackendConfig) bool | ||
|
||
func NoFilterFn(_ string, _ *BackendConfig) bool { return true } | ||
|
||
func BuildNameFilterFn(filter string) (BackendConfigFilterFn, error) { | ||
if filter == "" { | ||
return NoFilterFn, nil | ||
} | ||
rxp, err := regexp.Compile(filter) | ||
if err != nil { | ||
return nil, err | ||
} | ||
return func(name string, config *BackendConfig) bool { | ||
if config != nil { | ||
return rxp.MatchString(config.Name) | ||
} | ||
return rxp.MatchString(name) | ||
}, nil | ||
} | ||
|
||
func BuildUsecaseFilterFn(usecases BackendConfigUsecases) BackendConfigFilterFn { | ||
if usecases == FLAG_ANY { | ||
return NoFilterFn | ||
} | ||
return func(name string, config *BackendConfig) bool { | ||
if config == nil { | ||
return false // TODO: Potentially make this a param, for now, no known usecase to include | ||
} | ||
return config.HasUsecases(usecases) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,12 +21,12 @@ func ModelFromContext(ctx *fiber.Ctx, cl *config.BackendConfigLoader, loader *mo | |
} | ||
|
||
// Set model from bearer token, if available | ||
bearer := strings.TrimLeft(ctx.Get("authorization"), "Bearer ") | ||
bearer := strings.TrimLeft(ctx.Get("authorization"), "Bear ") // Reduced duplicate characters of Bearer | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this breaking? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting enough it is not. The static analysis tooling we've been experimenting with pointed this out to me and I verified it in the golang source: https://github.com/golang/go/blob/master/src/strings/strings.go TrimLeft (and its many friends) iterate through the string a single time, checking every character. If that character is contained in the "cutset" string even once, it will be removed. No need for duplicated characters - they will be scanned M*N every time its called, without benefit. This example is B e a r |
||
bearerExists := bearer != "" && loader.ExistsInModelPath(bearer) | ||
|
||
// If no model was specified, take the first available | ||
if modelInput == "" && !bearerExists && firstModel { | ||
models, _ := services.ListModels(cl, loader, "", true) | ||
models, _ := services.ListModels(cl, loader, config.NoFilterFn, services.SKIP_IF_CONFIGURED) | ||
if len(models) > 0 { | ||
modelInput = models[0] | ||
log.Debug().Msgf("No model specified, using: %s", modelInput) | ||
|
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.
My two cents on this - I would expect a "category" field on the backend config at this point. Hooking into the backend name can be brittle, mainly because a user might expand backends with --external-backends. Maybe we can mix (detection) and explicit configuration via backend config?
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.
As I mentioned in that comment, I'm significantly concerned with the maintenance burden of our yaml config backlog [to either initially create it, or update if new backends are created].
I see a few paths forward: