From 20efc9285e7760f4c46b0e3b0da9141b5f4b630c Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Wed, 7 Aug 2024 16:08:56 -0300 Subject: [PATCH] FIX: Correctly save provider-specific params for new models. (#744) Creating a new model, either manually or from presets, doesn't initialize the `provider_params` object, meaning their custom params won't persist. Additionally, this change adds some validations for Bedrock params, which are mandatory, and a clear message when a completion fails because we cannot build the URL. --- ...dmin-plugins-show-discourse-ai-llms-new.js | 1 + app/models/llm_model.rb | 13 ++++++ .../components/ai-llm-editor-form.gjs | 8 ---- config/locales/server.en.yml | 4 ++ ...07150605_add_default_to_provider_params.rb | 6 +++ lib/completions/endpoints/aws_bedrock.rb | 4 ++ .../requests/admin/ai_llms_controller_spec.rb | 40 +++++++++++++++++++ 7 files changed, 68 insertions(+), 8 deletions(-) create mode 100644 db/migrate/20240807150605_add_default_to_provider_params.rb diff --git a/admin/assets/javascripts/discourse/routes/admin-plugins-show-discourse-ai-llms-new.js b/admin/assets/javascripts/discourse/routes/admin-plugins-show-discourse-ai-llms-new.js index aafc69f2..b4e8a795 100644 --- a/admin/assets/javascripts/discourse/routes/admin-plugins-show-discourse-ai-llms-new.js +++ b/admin/assets/javascripts/discourse/routes/admin-plugins-show-discourse-ai-llms-new.js @@ -3,6 +3,7 @@ import DiscourseRoute from "discourse/routes/discourse"; export default DiscourseRoute.extend({ async model() { const record = this.store.createRecord("ai-llm"); + record.provider_params = {}; return record; }, diff --git a/app/models/llm_model.rb b/app/models/llm_model.rb index d3e92854..4219537b 100644 --- a/app/models/llm_model.rb +++ b/app/models/llm_model.rb @@ -12,6 +12,7 @@ class LlmModel < ActiveRecord::Base validates :url, presence: true, unless: -> { provider == BEDROCK_PROVIDER_NAME } validates_presence_of :name, :api_key validates :max_prompt_tokens, numericality: { greater_than: 0 } + validate :required_provider_params def self.provider_params { @@ -79,6 +80,18 @@ class LlmModel < ActiveRecord::Base def lookup_custom_param(key) provider_params&.dig(key) end + + private + + def required_provider_params + return if provider != BEDROCK_PROVIDER_NAME + + %w[access_key_id region].each do |field| + if lookup_custom_param(field).blank? + errors.add(:base, I18n.t("discourse_ai.llm_models.missing_provider_param", param: field)) + end + end + end end # == Schema Information diff --git a/assets/javascripts/discourse/components/ai-llm-editor-form.gjs b/assets/javascripts/discourse/components/ai-llm-editor-form.gjs index 172ba0c7..b5d25ba2 100644 --- a/assets/javascripts/discourse/components/ai-llm-editor-form.gjs +++ b/assets/javascripts/discourse/components/ai-llm-editor-form.gjs @@ -30,14 +30,6 @@ export default class AiLlmEditorForm extends Component { @tracked testError = null; @tracked apiKeySecret = true; - didReceiveAttrs() { - super.didReceiveAttrs(...arguments); - - if (!this.args.model.provider_params) { - this.populateProviderParams(this.args.model.provider); - } - } - get selectedProviders() { const t = (provName) => { return I18n.t(`discourse_ai.llms.providers.${provName}`); diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 06ddc53d..e06a963b 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -326,3 +326,7 @@ en: hint: one: "Make sure the `%{settings}` setting was configured." other: "Make sure the settings of the provider you want were configured. Options are: %{settings}" + + llm_models: + missing_provider_param: "%{param} can't be blank" + bedrock_invalid_url: "Please complete all the fields to contact this model." diff --git a/db/migrate/20240807150605_add_default_to_provider_params.rb b/db/migrate/20240807150605_add_default_to_provider_params.rb new file mode 100644 index 00000000..4363d0bb --- /dev/null +++ b/db/migrate/20240807150605_add_default_to_provider_params.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true +class AddDefaultToProviderParams < ActiveRecord::Migration[7.1] + def change + change_column_default :llm_models, :provider_params, from: nil, to: {} + end +end diff --git a/lib/completions/endpoints/aws_bedrock.rb b/lib/completions/endpoints/aws_bedrock.rb index 0b34affe..409c856b 100644 --- a/lib/completions/endpoints/aws_bedrock.rb +++ b/lib/completions/endpoints/aws_bedrock.rb @@ -66,6 +66,10 @@ module DiscourseAi llm_model.name end + if region.blank? || bedrock_model_id.blank? + raise CompletionFailed.new(I18n.t("discourse_ai.llm_models.bedrock_invalid_url")) + end + api_url = "https://bedrock-runtime.#{region}.amazonaws.com/model/#{bedrock_model_id}/invoke" diff --git a/spec/requests/admin/ai_llms_controller_spec.rb b/spec/requests/admin/ai_llms_controller_spec.rb index 77d0b7ae..f848570d 100644 --- a/spec/requests/admin/ai_llms_controller_spec.rb +++ b/spec/requests/admin/ai_llms_controller_spec.rb @@ -97,6 +97,46 @@ RSpec.describe DiscourseAi::Admin::AiLlmsController do expect(created_model).to be_nil end end + + context "with provider-specific params" do + it "doesn't create a model if a Bedrock param is missing" do + post "/admin/plugins/discourse-ai/ai-llms.json", + params: { + ai_llm: + valid_attrs.merge( + provider: "aws_bedrock", + provider_params: { + region: "us-east-1", + }, + ), + } + + created_model = LlmModel.last + + expect(response.status).to eq(422) + expect(created_model).to be_nil + end + + it "creates the model if all required provider params are present" do + post "/admin/plugins/discourse-ai/ai-llms.json", + params: { + ai_llm: + valid_attrs.merge( + provider: "aws_bedrock", + provider_params: { + region: "us-east-1", + access_key_id: "test", + }, + ), + } + + created_model = LlmModel.last + + expect(response.status).to eq(201) + expect(created_model.lookup_custom_param("region")).to eq("us-east-1") + expect(created_model.lookup_custom_param("access_key_id")).to eq("test") + end + end end describe "PUT #update" do