Surface modelconfig validation errors#64201
Conversation
|
|
||
| // Register a new site configuration validator, to surface any errors | ||
| // for admin-supplied changes to LLM model configuration. | ||
| conf.ContributeValidator(func(q conftypes.SiteConfigQuerier) conf.Problems { |
There was a problem hiding this comment.
FWIW I think this is the right place to register such a validator, not in the conf package :)
| conf.ContributeValidator(func(q conftypes.SiteConfigQuerier) conf.Problems { | ||
| newSiteConfig := q.SiteConfig() | ||
| if newSiteConfig.ModelConfiguration == nil { | ||
| return nil // No modelconfig to validate. |
There was a problem hiding this comment.
I think this case means that if someone is not using the new "modelConfiguration" site config field, but instead is using the legacy "completions" configuration field.. and it has issues - or we have issues in our conversion logic somehow - that error would go silently unreported, right? I think this also might not report a failure to build the site config m.builder.build(), in that same case?
Otherwise this logic seems sound: try to apply the new config, if there's an error then we surface it.
I think this case should be replaced by something closer to the init code above calling maybeGetSiteModelConfiguration
There was a problem hiding this comment.
Yes, good catch! I'll update this and do some sanity testing. (That is, always trying to do the error checking, in case you are using the "completions" configuration block.)
There was a problem hiding this comment.
But specifically, the applyNewSiteConfig function calls both maybeGetSiteModelConfiguration and builder.build(). So if there is any problem with the site config, or building the site config, it will surface as a site configuration error.
emidoots
left a comment
There was a problem hiding this comment.
left feedback but generally lgtm, definitely an improvement!
The
frontend/internal/modelconfigpackage exposes a service where any place in the codebase you can fetch the "currently supported LLM models". And that data will automatically be updated whenever the site configuration changes, and later, as new models are released on Cody Gateway.However, the data structure is kinda complicated and requires careful configuration. e.g. to make sure that every model has a valid ModelReference, and that that ModelReference needs to refer to a ProviderID that is known, etc.
Previously, we were just silently ignoring any configuration errors. So if the site configuration data lead to an error when rendering/building the Sourcegraph instance's model configuration the site configuration data changes silently be ignored. This makes it much more difficult to test, troubleshoot, and debug modelconfig issues.
This PR just adds a new site configuration validator from within
frontend/internal/modelconfig/init.go. And just tries to apply any new site configuration changes to the currentmodelconfig::builderand returns any errors it sees. This isn't super fancy, but does make it clear when there is a site configuration problem.The reason I registered the validator within the
frontend/internal/modelconfigpackage instead of the more typicalinternal/conf, was to avoid introducing and needing to work around a circular dependency. (frontend/internal/modelconfigimports a few things, which in-turn importinternal/conf, similar to how we had to lazily register the "modelconfig GraphQL resolver".Screenshots of this in-action
(If no models have the 'chat' capability, we cannot find a default model to use.)
Test plan
Tested manually. We already have lots of tests for various validation errors in
builder_test.goandinternal/modelconfig/validation_test.go.Changelog
NA