This repository was archived by the owner on Sep 30, 2024. It is now read-only.
pass ModelConfigInfo down to client.Get() for self-hosted-models#63739
Merged
Conversation
Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
emidoots
commented
Jul 9, 2024
| // Using the new "modelConfiguration" site config | ||
| // TODO(slimsag): self-hosted-models: this logic only handles Cody Enterprise with Self-hosted models | ||
| // Only in this case do we have modelConfigInfo != nil | ||
| return nil, errors.Newf("TODO: implement self-hosted-models") |
Member
Author
There was a problem hiding this comment.
This is all in service of having modelConfigInfo right here.
chrsmith
approved these changes
Jul 9, 2024
chrsmith
left a comment
Contributor
There was a problem hiding this comment.
I have suggestions for how we can rewrite the logic within getModel to be clearer. But given that I'll rewrite the function entirely in XX hours, I wouldn't worry too much about it. :D
| if modelConfig := conf.Get().SiteConfig().ModelConfiguration; modelConfig != nil { | ||
| // Using new "modelConfiguration" site config. | ||
| // TODO(slimsag): self-hosted-models: currently this logic only handles Cody Enterprise with Self-hosted models | ||
| if err := modelconfig.ValidateModelRef(modelconfigtypes.ModelRef(requestParams.Model)); err != nil { |
Contributor
There was a problem hiding this comment.
This code seems a little hard to follow since you're essentially adding fallback options in case there is an error. So what do you think about inverting the error condition so this is a little easier to follow, e.g.:
// WDYT of using 'modelconfigSDK' rather than 'modelconfigtypes'?
import modelconfigSDK ".../types"
// Check if the user supplied a valid ModelRef.
if requestParams.Model != "" {
mref := modelconfigSDK.ModelRef(requestParams.Model)
if err := modelconfig.ValidateModelRef(mref); err == nil {
return requestParams.Model, nil
}
}
// Fallback to using the default models as per the configuration.
modelConfig, err := frontendmodelconfig.Get().Get()
if err != nil {
return "", err
}
return string(modelConfig.DefaultModels.CodeCompletion), nil| modelConfig, err := modelconfig.Get().Get() | ||
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| return |
Contributor
There was a problem hiding this comment.
For both of these 500 return paths, we use logger.Error(...). Since otherwise it'll be a pain to debug or troubleshoot any issues users run into.
added 5 commits
July 9, 2024 14:55
Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR has a single goal: pass a new
ModelConfigInfotype, which has theProviderandModelwe should use to serve a completions request, down intoclient.Get()This PR explicitly only handles the case we care about for Self-hosted models, and all pieces of logic that I expect will be replaced/superseded by your work @chrsmith are annotated with
// TODO(slimsag): self-hosted-models:comments so we can easily find and remove them when your work is ready.Every location I have modified has been carefully wrapped in an if statement like
if conf.Get().SiteConfig().ModelConfiguration != nilto ensure that this change only affects people who set the new"modelConfiguration"site config property.Test plan
"modelConfiguration"and removed"completions"in my dev instances' site config.TODOerror back:Changelog
N/A