This repository was archived by the owner on Sep 30, 2024. It is now read-only.
Refactor the 'getModel' callbacks into their own file#63359
Merged
Conversation
chrsmith
commented
Jun 20, 2024
Comment on lines
-34
to
-43
| func(_ context.Context, requestParams types.CodyCompletionRequestParameters, c *conftypes.CompletionsConfig) (string, error) { | ||
| customModel := allowedCustomModel(requestParams.Model) | ||
| if customModel != "" { | ||
| return customModel, nil | ||
| } | ||
| if requestParams.Model != "" { | ||
| return "", errors.Newf("Unsupported code completion model %q", requestParams.Model) | ||
| } | ||
| return c.CompletionModel, nil | ||
| }, |
Contributor
Author
There was a problem hiding this comment.
allowedCustomModel(string) string because as it is today, it's crazy-confusing.
allowedCustomModel(string) string was refactored into isAllowedCodeCompletionModel(string) bool. So rather than returning the input string or "", it just returns a boolean.
So previously, the (confusing) behavior was:
- If it is an allowed custom model, return the
requestParams.Modelas-is. - If it is not an allowed custom model (or not found) return "".
But that could be distilled down into:
if requestParams.Model != "" {
if isAllowedCodeCompletionModel(requestParams.Model) {
return requestParams.Model, nil
}
return "", errors.Newf("unsupported code completion model %q", requestParams.Model)
}So please double check my thinking here. But it should be identical and, IMHO, much easier to read.
Member
There was a problem hiding this comment.
Confirmed this matches my understanding as well, the behavior is identical.
emidoots
approved these changes
Jun 20, 2024
chrsmith
referenced
this pull request
Jul 12, 2024
…ore models) (#63797) This PR if what the past dozen or so [cleanup](https://github.com/sourcegraph/sourcegraph/pull/63359), [refactoring](https://github.com/sourcegraph/sourcegraph/pull/63731), and [test](https://github.com/sourcegraph/sourcegraph/pull/63761) PRs were all about: using the new `modelconfig` system for the completion APIs. This will enable users to: - Use the new site config schema for specifying LLM configuration, added in https://github.com/sourcegraph/sourcegraph/pull/63654. Sourcegraph admins who use these new site config options will be able to support many more LLM models and providers than is possible using the older "completions" site config. - For Cody Enterprise users, we no longer ignore the `CodyCompletionRequest.Model` field. And now support users specifying any LLM model (provided it is "supported" by the Sourcegraph instance). Beyond those two things, everything should continue to work like before. With any existing "completions" configuration data being converted into the `modelconfig` system (see https://github.com/sourcegraph/sourcegraph/pull/63533). ## Overview In order to understand how this all fits together, I'd suggest reviewing this PR commit-by-commit. ### [Update internal/completions to use modelconfig](https://github.com/sourcegraph/sourcegraph/commit/e6b7eb171eea6bd6a512f0e61457170a86128eae) The first change was to update the code we use to serve LLM completions. (Various implementations of the `types.CompletionsProvider` interface.) The key changes here were as follows: 1. Update the `CompletionRequest` type to include the `ModelConfigInfo` field (to make the new Provider and Model-specific configuration data available.) 2. Rename the `CompletionRequest.Model` field to `CompletionRequest.RequestedModel`. (But with a JSON annotation to maintain compatibility with existing callers.) This is to catch any bugs related to using the field directly, since that is now almost guaranteed to be a mistake. (See below.) With these changes, all of the `CompletionProvider`s were updated to reflect these changes. - Any situation where we used the `CompletionRequest.Parameters.RequestedModel` should now refer to `CompletionRequest.ModelConfigInfo.Model.ModelName`. The "model name" being the thing that should be passed to the API provider, e.g. `gpt-3.5-turbo`. - In some situations (`azureopenai`) we needed to rely on the Model ID as a more human-friendly identifier. This isn't 100% accurate, but will match the behavior we have today. A long doc comment calls out the details of what is wrong with that. - In other situations (`awsbedrock`, `azureopenai`) we read the new `modelconfig` data to configure the API provider (e.g. `Azure.UseDeprecatedAPI`), or surface model-specific metadata (e.g. AWS Provisioned Throughput ARNs). While the code is a little clunky to avoid larger refactoring, this is the heart and soul of how we will be writing new completion providers in the future. That is, taking specific configuration bags with whatever data that is required. ### [Fix bugs in modelconfig](https://github.com/sourcegraph/sourcegraph/commit/75a51d8cb520e35918bd3a67a090a36d456b1797) While we had lots of tests for converting the existing "completions" site config data into the `modelconfig.ModelConfiguration` structure, there were a couple of subtle bugs that I found while testing the larger change. The updated unit tests and comments should make that clear. ### [Update frontend/internal/httpapi/completions to use modelconfig](https://github.com/sourcegraph/sourcegraph/commit/084793e08fca51a5ab84a7d73421d575caeebaa1) The final step was to update the HTTP endpoints that serve the completion requests. There weren't any logic changes here, just refactoring how we lookup the required data. (e.g. converting the user's requested model into an actual model found in the site configuration.) We support Cody clients sending either "legacy mrefs" of the form `provider/model` like before, or the newer mref `provider::apiversion::model`. Although it will likely be a while before Cody clients are updated to only use the newer-style model references. The existing unit tests for the competitions APIs just worked, which was the plan. But for the few changes that were required I've added comments to explain the situation. ### [Fix: Support requesting models just by their ID](https://github.com/sourcegraph/sourcegraph/pull/63797/commits/99715feba614230aa84cf94aae571adb96768035) > ... We support Cody clients sending either "legacy mrefs" of the form `provider/model` like before ... Yeah, so apparently I lied 😅 . After doing more testing, the extension _also_ sends requests where the requested model is just `"model"`. (Without the provider prefix.) So that now works too. And we just blindly match "gtp-3.5-turbo" to the first mref with the matching model ID, such as "anthropic::unknown::gtp-3.5-turbo". ## Test plan Existing unit tests pass, added a few tests. And manually tested my Sg instance configured to act as both "dotcom" mode and a prototypical Cody Enterprise instance. ## Changelog Update the Cody APIs for chat or code completions to use the "new style" model configuration. This allows for great flexibility in configuring LLM providers and exposing new models, but also allows Cody Enterprise users to select different models for chats. This will warrant a longer, more detailed changelog entry for the patch release next week. As this unlocks many other exciting features.
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.
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.
Minor refactoring to the
cmd/frontend/internal/completionspackage.When we call
newCompletionsHandlerone of the parameters is a function namedgetModel. This is called to determine which LLM model should be use to respond to the incoming completion request. And we have two implementations of this function, one for code completions and another for chats.This PR just moves the logic for those two implementations into their own file (
get_model.go), along with a couple of functions for which they were the only caller.There were two minor functionality changes I made, which I'll call out in comments on this PR.
Why?
As we rework how LLM models and associated configuration flows throughout the backend, updating these functions will be a bit easier if they are pulled out like this rather than being defined inline like they are today.
Also, pretty much any place in the codebase where we have hard-coded an LLM model is "on notice" and should instead be driven entirely by some sort of global configuration file. (Since this is one of the key problems server-side LLM config is trying to solve.)
Test plan
NA, no functional changes. Relying on CI/CD and linter.
Changelog
NA