add new "modelConfiguration" schema to site config#63654
Conversation
Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
User-facing This PR adds a new top-level field to the site configuration `"modelConfiguration"` which is intended to replace the old `"completions"` field. For now, this is 100% opt-in (beta feature) and customers should only use this new field if they have talked with us to confirm it should work in their case. Today, this configuration is unused, but in future PRs it will actually be used (planned for the 5.5.0 release.) Additionally, `"modelConfiguration"` acts as a feature-flag. If it is set and not `null`, then Cody will enable this whole new model configuration system end-to-end which involves many new components: * Clients will make use of a new `/.api/modelconfig/supported-models.json` endpoint to query which models the server has available. * Cody will respect this new site configuration, discovering new models from Sourcegraph's servers without an upgrade by default, etc. * Clients will enable the "select an LLM model" dropdown menu for enterprise customers. * The old `"completions"` configuration, if present, will be ignored if `"modelConfiguration"` is set. Implementation notes This schema mirrors [the new model configuration schema](https://github.com/sourcegraph/sourcegraph/tree/main/internal/modelconfig/types) that Chris and myself have been working on tirelessly to enable a myriad of use-cases in the near future, including enabling customers to get support for new models without upgrading Sourcegraph, enabling multiple models / dropdown model selector in enterprise, improved self-hosted model support, and more. The translation is pretty much 1:1, with a few notable aspects: * I broke out [`GenericProviderConfig`](https://github.com/sourcegraph/sourcegraph/blob/main/internal/modelconfig/types/configuration.go#L49-L73) into distinct types for each provider. * I represent `ClientSideProviderConfig` and `ClientSideModelConfig` _objects_ (specify multiple fields) * I represent `ServerSideProviderConfig` and `ServerSideModelConfig` as _tagged unions_ / _discriminated unions_, i.e. where you must write a `{"type": "foo"}` field as part of the object. Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
| }, | ||
| "doNotUseThisProperty": { | ||
| "description": "Do not use/set this property, it is useless. go-jsonschema has an issue where it does not support writing a tagged union unless it can find a field each of the union types do NOT have in common; this type merely exists to provide a field that is not in common with all other oneOf types. https://sourcegraph.com/github.com/sourcegraph/go-jsonschema/-/blob/compiler/generator_tagged_union_type.go?L47-49", | ||
| "type": "string" |
There was a problem hiding this comment.
Explanation:
go-jsonschema allows us to represent tagged/discriminant unions with the extension:
"!go": {
"pointer": true,
"taggedUnionType": true
},
For example ServerSideProviderConfig, a user would write it like this in the site config:
In the generated Go data types, this gets translated into a struct where only one field can be non-nil:
type ServerSideProviderConfig struct {
AwsBedrock *ServerSideProviderConfigAWSBedrock
AzureOpenAI *ServerSideProviderConfigAzureOpenAI
Anthropic *ServerSideProviderConfigAnthropicProvider
Fireworks *ServerSideProviderConfigFireworksProvider
Google *ServerSideProviderConfigGoogleProvider
Openai *ServerSideProviderConfigOpenAIProvider
Sourcegraph *ServerSideProviderConfigSourcegraphProvider
Unused *DoNotUsePhonyDiscriminantType
}
However, the way go-jsonschema detects which field to use for the "type" in the screenshots above is rather rough: it looks at the shape of each oneOf JSON object, i.e. ServerSideProviderConfigAWSBedrock, ServerSideProviderConfigAzureOpenAI, etc. and finds which fields they have in common. Then it says the field they all have in common is the discriminant field.
What this means is that if all objects have the same field, e.g. all of these have both a type string and an endpoint string, then it doesn't know which one to use and fails to generate the Go code with an error multiple discriminant properties found for !go.taggedUnionType extension: ["type", "endpoint"]
To workaround this poor logic, I added this phony type which has a field (doNotUseThisProperty) which no other object has in common, which lets go-jsonschema find that "type" is the right field.
This is dumb. This is silly. This is annoying. But it works for now.
Finally, just to note, this phony type is only observable to go-jsonschema, users can't see this type of specify it because it is not part of the "type" enum... so it really has no user-facing impact.
Later on, I may investigate how to fix this properly in go-jsonschema - but for now this works.
There was a problem hiding this comment.
It's kinda lame, but at the same time, it looks like you did the due diligence in confirming that this is literally the best we can do.
Could you please file a Linear ticket in the server-side model config project (here) so we can track actually fixing this?
Even if the specific issues is buried deep in the bowels of a 3rd party library that the maintainers do not wish to change, at least we will have a link that internal folks can go to to see what investigation has been done so far.
There was a problem hiding this comment.
chrsmith
left a comment
There was a problem hiding this comment.
Aside from suggestions and minor nit picks, this all looks good.
| "enum": ["experimental", "beta", "stable", "deprecated"], | ||
| "examples": [["stable"]] | ||
| }, | ||
| "tier": { |
There was a problem hiding this comment.
This is something that doesn't really make sense for Cody Enterprise, and even for Cody Pro / dotcom, we don't plan on storing the canonical "list of Cody Free vs. Cody Free models" in the dotcom instance's site configuration.
So we might be able to remove this entirely from the site configuration data, and instead just figure out how to properly set this field on the backend? (And for the majority of cases, just default to "enterprise"?
| "!go": { | ||
| "pointer": true | ||
| }, | ||
| "default": null, |
There was a problem hiding this comment.
We do have the ability to infer defaults for this... but as per those Slack threads about "fast chat" and its unique requirements, maybe we should make this required? That would also require that admins set their models explicitly. WDYT?
There was a problem hiding this comment.
This is a really, really big decision.
If we default null here, it means that for the 90% of customers who use Cody gateway.. we can choose default models for them. i.e. this being set to null implies 'Sourcegraph chooses the best models for me'
If we require admins specify this field, it means that in order to setup/configure Cody for the first time, you must specify this. Additionally, when we discover e.g. starcoder3 works better than starcoder2 on Cody Gateway and want all customers to upgrade to it, we must..
- Send out an email announcement encouraging on-prem customers to update their site configuration's default models.
- Apply the configuration change for default models on all Cloud instances.
IMO we should default null. If you explicitly set default models, then we won't choose for you. Otherwise you'll be given the best experience we can give at that time.
| "properties": { | ||
| "type": { | ||
| "type": "string", | ||
| "enum": ["awsBedrock", "azureOpenAI", "anthropic", "fireworks", "google", "openai", "sourcegraph"] |
There was a problem hiding this comment.
Super minor personal preference nit pick you can safely ignore, to keep things tidy, we may want to keep this sorted alphabetically.
There was a problem hiding this comment.
I list them in the order of the struct fields; also note this order is user-facing (dropdown autocomplete list order)
| }, | ||
| "doNotUseThisProperty": { | ||
| "description": "Do not use/set this property, it is useless. go-jsonschema has an issue where it does not support writing a tagged union unless it can find a field each of the union types do NOT have in common; this type merely exists to provide a field that is not in common with all other oneOf types. https://sourcegraph.com/github.com/sourcegraph/go-jsonschema/-/blob/compiler/generator_tagged_union_type.go?L47-49", | ||
| "type": "string" |
There was a problem hiding this comment.
It's kinda lame, but at the same time, it looks like you did the due diligence in confirming that this is literally the best we can do.
Could you please file a Linear ticket in the server-side model config project (here) so we can track actually fixing this?
Even if the specific issues is buried deep in the bowels of a 3rd party library that the maintainers do not wish to change, at least we will have a link that internal folks can go to to see what investigation has been done so far.
Co-authored-by: Chris Smith <chrsmith@users.noreply.github.com>
Co-authored-by: Chris Smith <chrsmith@users.noreply.github.com>
Co-authored-by: Chris Smith <chrsmith@users.noreply.github.com>
Co-authored-by: Chris Smith <chrsmith@users.noreply.github.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>
|
@chrsmith I set this to auto-merge, but let's continue discussing https://github.com/sourcegraph/sourcegraph/pull/63654#discussion_r1667209844 - we can change this on Monday if needed. |
Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
…models API, initial self-hosted model config (#63697) These commits do a few things: --- 46b1303e62ea7e01ba6a441cc55bbe4c166ef5ce corrects a few minor mistakes with the new site config which I introduced in #63654 - namely fixing `examples` entries and nullability in a few cases. Nothing controversial here, just bug fixes. --- 750b61e7dfa661338c9b40042087aed8e795f900 makes it so that the `/.api/client-config` endpoint returns `"modelsAPIEnabled": true,` if `"modelConfiguration"` is set in the site config. For context, `"modelConfiguration"` is a new site config field, which is not used anywhere before this PR, and has this description: > BETA FEATURE, only enable if you know what you are doing. If set, Cody will use the new model configuration system and ignore the old 'completions' site configuration entirely. I will send a change to the client logic next so that it uses this `modelsAPIEnabled` field instead of the client-side feature flag `dev.useServerDefinedModels`. --- Finally, f52fba342dd2e62a606b885802f7f6bc37f4f4ac and bde67d57c39f4566dc9287f8793cb5ffd25955b3 make a few site config changes that @chrsmith and I discussed to enable Self-hosted models support. Specifically, it makes it possible to specify the following configuration in the site config: ``` // Setting this field means we are opting into the new Cody model configuration system which is in beta. "modelConfiguration": { // Disable use of Sourcegraph's servers for model discovery "sourcegraph": null, // Configure the OpenAI-compatible API endpoints that Cody should use to provide // mistral and bigcode (starcoder) models. "providerOverrides": [ { "displayName": "Mistral", "id": "mistral", "serverSideConfig": { "type": "openaicompatible", "endpoint": "...", "accessToken": "...", }, }, { "displayName": "Bigcode", "id": "bigcode", "serverSideConfig": { "type": "openaicompatible", "endpoint": "...", "accessToken": "...", }, }, ], // Configure which exact mistral and starcoder models we want available "modelOverridesRecommendedSettings": [ "bigcode::v1::starcoder2-7b", "mistral::v1::mixtral-8x7b-instruct" ], // Configure which models Cody will use by default "defaultModels": { "chat": "mistral::v1::mixtral-8x7b-instruct", "fastChat": "mistral::v1::mixtral-8x7b-instruct", "codeCompletion": "bigcode::v1::starcoder2-7b", } } ``` Currently this site config is not actually used, so configuring Sourcegraph like this should not be done today, but this will be in a future PR by me. @chrsmith one divergence from what we discussed.. me and you had planned to support this: ``` "modelOverrides": [ { "bigcode::v1::starcoder2-7b"": { "useRecommendSettings": true, }, "mistral::v1::mixtral-8x22b-instruct": { "useRecommendSettings": true, }, } ], ``` However, being able to specify `"useRecommendSettings": true,` inside of a `ModelOverride` in the site configuration means that all other `ModelOverride` fields (the ones we are accepting as recommended settings) must be optional, which seems quite bad and opens up a number of misconfiguration possibilities. Instead, I opted to introduce a new top-level field for model overrides _with recommended settings_, so the above becomes this instead: ``` "modelOverridesRecommendedSettings": [ "bigcode::v1::starcoder2-7b", "mistral::v1::mixtral-8x7b-instruct" ], ``` This has the added benefit of making it impossible to set both `"useRecommendSettings": true,` and other fields. I will make it a site config error (prevents admins from saving configuration) to specify the same model in both `modelOverrides` and `modelOverridesRecommendedSettings` in a future PR. --- ## Test plan Doesn't affect users yet. Careful review. ## Changelog N/A --------- Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
This causes the `modelconfig` package to actually begin converting the new `"modelConfiguration"` site config which was introduced in #63654 into the internal data types we use for model configuration. This is all pretty straightforward / lame type conversion code, but carefully written to preserve all the semantics we care about. Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
#63706) This causes the `modelconfig` package to actually begin converting the new `"modelConfiguration"` site config which was introduced in #63654 into the internal data types we use for model configuration. This is all pretty straightforward / lame type conversion code, but carefully written to preserve all the semantics we care about. ## Test plan Written very carefully, and confirmed that this query: ``` curl -H "Authorization: token $(cat ~/local-token)" https://sourcegraph.test:3443/.api/modelconfig/supported-models.json ``` returns what I would expect with this site configuration: <details> <summary>site config</summary> ``` // Setting this field means we are opting into the new Cody model configuration system which is in beta. "modelConfiguration": { // Disable use of Sourcegraph's servers for model discovery "sourcegraph": null, // Configure the OpenAI-compatible API endpoints that Cody should use to provide // mistral and bigcode (starcoder) models. "providerOverrides": [ { "displayName": "Mistral", "id": "mistral", "serverSideConfig": { "type": "openaicompatible", "endpoint": "...", "accessToken": "...", }, }, { "displayName": "Bigcode", "id": "bigcode", "serverSideConfig": { "type": "openaicompatible", "endpoint": "...", "accessToken": "...", }, }, ], // Configure which exact mistral and starcoder models we want available "modelOverridesRecommendedSettings": [ "bigcode::v1::starcoder2-7b", "mistral::v1::mixtral-8x7b-instruct" ], // Configure which models Cody will use by default "defaultModels": { "chat": "mistral::v1::mixtral-8x7b-instruct", "fastChat": "mistral::v1::mixtral-8x7b-instruct", "codeCompletion": "bigcode::v1::starcoder2-7b", } } ``` </details> We could certainly use more unit tests here to prevent potential future regressions, I can add those in a future PR. ## Changelog Has no effect on users unless they opt into the early-access `"modelConfiguration"` site config feature. --------- Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com> Co-authored-by: Chris Smith <chrsmith@users.noreply.github.com>
…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.
User-facing impacts
This PR adds a new top-level field to the site configuration
"modelConfiguration"which is intended to replace the old
"completions"field. For now, this is 100%opt-in (beta feature) and customers should only use this new field if they have
talked with us to confirm it should work in their case.
Today, this configuration is unused, but in future PRs it will actually be used (planned for the 5.5.0 release.)
Additionally,
"modelConfiguration"acts as a feature-flag. If it is setand not
null, then Cody will enable this whole new model configuration systemend-to-end which involves many new components:
/.api/modelconfig/supported-models.jsonendpoint to query which models the server has available."completions"configuration, if present, will be ignored if"modelConfiguration"is set.Implementation notes
This schema mirrors the new model configuration schema
that Chris and myself have been working on tirelessly to enable a myriad
of use-cases in the near future, including enabling customers to get
support for new models without upgrading Sourcegraph, enabling multiple
models / dropdown model selector in enterprise, improved self-hosted model
support, and more.
The translation is pretty much 1:1, with a few notable aspects:
GenericProviderConfiginto distinct types for each provider.ClientSideProviderConfigandClientSideModelConfigobjects (specify multiple fields)ServerSideProviderConfigandServerSideModelConfigas tagged unions / discriminated unions, i.e. where you must write a{"type": "foo"}field as part of the object.My next PR will be to handle conversion from the site config types to the
modelconfig/types.Test plan
No impact to product behavior yet, nothing to test.
Changelog
Changelog entry will come later with proper docs link, when we are ready for customers to use this.