Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

add new "modelConfiguration" schema to site config#63654

Merged
emidoots merged 12 commits into
mainfrom
sg/site-config
Jul 6, 2024
Merged

add new "modelConfiguration" schema to site config#63654
emidoots merged 12 commits into
mainfrom
sg/site-config

Conversation

@emidoots

@emidoots emidoots commented Jul 4, 2024

Copy link
Copy Markdown
Member

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 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
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 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.

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.

Stephen Gutekanst added 2 commits July 4, 2024 12:48
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>
@emidoots emidoots requested a review from chrsmith July 4, 2024 22:22
@cla-bot cla-bot Bot added the cla-signed label Jul 4, 2024
Comment thread schema/site.schema.json
},
"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"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

image

image

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chrsmith chrsmith left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from suggestions and minor nit picks, this all looks good.

Comment thread schema/site.schema.json Outdated
Comment thread schema/site.schema.json Outdated
Comment thread schema/site.schema.json Outdated
Comment thread schema/site.schema.json Outdated
Comment thread schema/site.schema.json Outdated
Comment thread schema/site.schema.json Outdated
"enum": ["experimental", "beta", "stable", "deprecated"],
"examples": [["stable"]]
},
"tier": {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Maybe we should just remove this entirely?

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"?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

Comment thread schema/site.schema.json
"!go": {
"pointer": true
},
"default": null,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

  1. Send out an email announcement encouraging on-prem customers to update their site configuration's default models.
  2. 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.

Comment thread schema/site.schema.json
Comment thread schema/site.schema.json
"properties": {
"type": {
"type": "string",
"enum": ["awsBedrock", "azureOpenAI", "anthropic", "fireworks", "google", "openai", "sourcegraph"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super minor personal preference nit pick you can safely ignore, to keep things tidy, we may want to keep this sorted alphabetically.

@emidoots emidoots Jul 6, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I list them in the order of the struct fields; also note this order is user-facing (dropdown autocomplete list order)

Comment thread schema/site.schema.json
},
"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"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Stephen Gutekanst and others added 8 commits July 5, 2024 15:37
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>
Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
@emidoots emidoots enabled auto-merge (squash) July 6, 2024 00:06
@emidoots

emidoots commented Jul 6, 2024

Copy link
Copy Markdown
Member Author

@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.

Stephen Gutekanst added 2 commits July 5, 2024 17:07
Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
@emidoots emidoots merged commit a1864da into main Jul 6, 2024
@emidoots emidoots deleted the sg/site-config branch July 6, 2024 05:03
emidoots pushed a commit that referenced this pull request Jul 8, 2024
…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>
emidoots pushed a commit that referenced this pull request Jul 9, 2024
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>
emidoots pushed a commit that referenced this pull request Jul 9, 2024
#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>
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.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants