FIX: correctly handle provider edit (#1125)

Prior to this commit, editing the provider wouldn't recompute the provider params. It would also not correctly recompute the "canEditURL" property.

To make possible this commit has:
- made a fix in core: https://github.com/discourse/discourse/pull/31329
- ensures the provider params are recomputed when provider is changed
- made the check on `canEditURL` based on form state and not initial model value

Tests have been added to confirm the expected behavior.
This commit is contained in:
Joffrey JAFFEUX 2025-02-13 12:03:13 +01:00 committed by GitHub
parent 35f15629fb
commit e2afbc26d3
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 53 additions and 15 deletions

View File

@ -1,7 +1,7 @@
import Component from "@glimmer/component"; import Component from "@glimmer/component";
import { cached, tracked } from "@glimmer/tracking"; import { cached, tracked } from "@glimmer/tracking";
import { concat, fn, get } from "@ember/helper"; import { concat, fn, get } from "@ember/helper";
import { action, computed } from "@ember/object"; import { action } from "@ember/object";
import { LinkTo } from "@ember/routing"; import { LinkTo } from "@ember/routing";
import { later } from "@ember/runloop"; import { later } from "@ember/runloop";
import { service } from "@ember/service"; import { service } from "@ember/service";
@ -37,8 +37,6 @@ export default class AiLlmEditorForm extends Component {
const info = this.args.llms.resultSetMeta.presets.findBy("id", id); const info = this.args.llms.resultSetMeta.presets.findBy("id", id);
const modelInfo = info.models.findBy("name", modelName); const modelInfo = info.models.findBy("name", modelName);
const params =
this.args.llms.resultSetMeta.provider_params[info.provider] ?? {};
return { return {
max_prompt_tokens: modelInfo.tokens, max_prompt_tokens: modelInfo.tokens,
@ -47,12 +45,7 @@ export default class AiLlmEditorForm extends Component {
display_name: modelInfo.display_name, display_name: modelInfo.display_name,
name: modelInfo.name, name: modelInfo.name,
provider: info.provider, provider: info.provider,
provider_params: Object.fromEntries( provider_params: this.computeProviderParams(info.provider),
Object.entries(params).map(([k, v]) => [
k,
v?.type === "enum" ? v.default : null,
])
),
}; };
} }
@ -103,11 +96,6 @@ export default class AiLlmEditorForm extends Component {
return this.testRunning || this.testResult !== null; return this.testRunning || this.testResult !== null;
} }
@computed("args.model.provider")
get canEditURL() {
return this.args.model.provider !== "aws_bedrock";
}
get modulesUsingModel() { get modulesUsingModel() {
const usedBy = this.args.model.used_by?.filter((m) => m.type !== "ai_bot"); const usedBy = this.args.model.used_by?.filter((m) => m.type !== "ai_bot");
@ -140,6 +128,21 @@ export default class AiLlmEditorForm extends Component {
return !this.args.model.isNew; return !this.args.model.isNew;
} }
computeProviderParams(provider) {
const params = this.args.llms.resultSetMeta.provider_params[provider] ?? {};
return Object.fromEntries(
Object.entries(params).map(([k, v]) => [
k,
v?.type === "enum" ? v.default : null,
])
);
}
@action
canEditURL(provider) {
return provider !== "aws_bedrock";
}
@action @action
openAddQuotaModal(addItemToCollection) { openAddQuotaModal(addItemToCollection) {
this.modal.show(AiLlmQuotaModal, { this.modal.show(AiLlmQuotaModal, {
@ -220,6 +223,12 @@ export default class AiLlmEditorForm extends Component {
} }
} }
@action
setProvider(provider, { set }) {
set("provider_params", this.computeProviderParams(provider));
set("provider", provider);
}
@action @action
delete() { delete() {
return this.dialog.confirm({ return this.dialog.confirm({
@ -287,6 +296,7 @@ export default class AiLlmEditorForm extends Component {
@disabled={{this.seeded}} @disabled={{this.seeded}}
@format="large" @format="large"
@validation="required" @validation="required"
@onSet={{this.setProvider}}
as |field| as |field|
> >
<field.Select as |select|> <field.Select as |select|>
@ -299,7 +309,7 @@ export default class AiLlmEditorForm extends Component {
</form.Field> </form.Field>
{{#unless this.seeded}} {{#unless this.seeded}}
{{#if this.canEditURL}} {{#if (this.canEditURL data.provider)}}
<form.Field <form.Field
@name="url" @name="url"
@title={{i18n "discourse_ai.llms.url"}} @title={{i18n "discourse_ai.llms.url"}}

View File

@ -71,6 +71,34 @@ RSpec.describe "Managing LLM configurations", type: :system, js: true do
expect(llm.user_id).not_to be_nil expect(llm.user_id).not_to be_nil
end end
context "when changing the provider" do
it "correctly changes the provider params" do
visit "/admin/plugins/discourse-ai/ai-llms"
find("[data-llm-id='none'] button").click()
form.field("provider").select("vllm")
expect(form).to have_field_with_name("provider_params.disable_system_prompt")
expect(form).to have_no_field_with_name("provider_params.disable_native_tools")
form.field("provider").select("open_router")
expect(form).to have_field_with_name("provider_params.disable_streaming")
expect(form).to have_field_with_name("provider_params.disable_native_tools")
end
it "updates if the url can be edited" do
visit "/admin/plugins/discourse-ai/ai-llms"
find("[data-llm-id='none'] button").click()
form.field("provider").select("vllm")
expect(form).to have_field_with_name("url")
form.field("provider").select("aws_bedrock")
expect(form).to have_no_field_with_name("url")
end
end
context "with quotas" do context "with quotas" do
fab!(:llm_model_1) { Fabricate(:llm_model, name: "claude-2") } fab!(:llm_model_1) { Fabricate(:llm_model, name: "claude-2") }
fab!(:group_1) { Fabricate(:group) } fab!(:group_1) { Fabricate(:group) }