Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<%=
render(
Primer::OpenProject::DangerDialog.new(
title: t("admin.llm_models.destroy.title"),
form_arguments:,
test_selector: TEST_SELECTOR
)
) do |dialog|
dialog.with_confirmation_message do |message|
message.with_heading(tag: :h2) { t("admin.llm_models.destroy.heading", model: llm_model.external_id) }
message.with_description_content(
if bound_features.any?
t("admin.llm_models.destroy.description_bound", features: bound_features.to_sentence)
else
t("admin.llm_models.destroy.description")
end
)
end
end
%>
57 changes: 57 additions & 0 deletions app/components/llm_connections/delete_model_dialog_component.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
# frozen_string_literal: true

#-- copyright
# OpenProject is an open source project management software.
# Copyright (C) the OpenProject GmbH
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License version 3.
#
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
# Copyright (C) 2006-2013 Jean-Philippe Lang
# Copyright (C) 2010-2013 the ChiliProject Team
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License
# as published by the Free Software Foundation; either version 2
# of the License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See COPYRIGHT and LICENSE files for more details.
#++

module LlmConnections
class DeleteModelDialogComponent < ApplicationComponent
include OpTurbo::Streamable
include OpPrimer::ComponentHelpers

TEST_SELECTOR = "llm-model--delete-dialog"

alias_method :llm_model, :model

def form_arguments
{ action: url_helpers.llm_model_path(llm_model), method: :delete }
end

# Named so the message says what is actually at stake. The connection
# defaults count as bindings here -- deleting their model breaks every
# feature that inherits them.
def bound_features
affected_defaults
end

def affected_defaults
%i[default_chat_model_id default_embedding_model_id]
.select { |attribute| llm_model.llm_connection.public_send(attribute) == llm_model.external_id }
.map { |attribute| LlmConnection.human_attribute_name(attribute) }
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,17 @@
data: filter_input_data_attributes
) %>

<% subheader.with_action_button(
scheme: :primary,
label: t("admin.llm_connections.show.add_model_submit"),

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.

🤖 #Style ℹ️ This key names a link, not a submit, and duplicates create_submit

Two separate points on the same key, both worth a look alongside the suggestion already left on create_submit:

  1. add_model_submit labels this sub-header link, while the form's actual submit button uses admin.llm_models.form.create_submit. Both currently read "Add model", so the same text appears on two different controls and the _submit suffix sits on the one that is not a submit. Would it be clearer to rename this one (for example add_model) and leave create_submit naming the action the form performs?

  2. On the label itself: the closest analogues in the repo use a bare noun for a primary sub-header button with leading_icon: :plus - + Portfolio in portfolios/index_sub_header_component.html.erb and + Item in admin/custom_fields/hierarchy/items_component.html.erb. The other convention is a bare verb, + Add, used by the projects index. Since the icon already supplies the plus, the literal "+" would not be needed in the string either.

Separately, admin.llm_connections.show.add_model_description looks like a second ghost key in this area - unreferenced since it was added, and superseded by admin.llm_models.new.description.

mobile_label: t("admin.llm_connections.show.add_model_submit"),
mobile_icon: :plus,
leading_icon: :plus,
tag: :a,
href: helpers.new_llm_model_path,
test_selector: "llm-model--add-button"
) { t("admin.llm_connections.show.add_model_submit") } %>

<% subheader.with_action_button(
scheme: :default,
label: t("admin.llm_connections.show.refresh_models"),
Expand Down
20 changes: 20 additions & 0 deletions app/components/llm_connections/models_row_component.rb
Original file line number Diff line number Diff line change
Expand Up @@ -73,5 +73,25 @@ def source_label

%i[secondary source_discovered]
end

def button_links
llm_model.manual? ? [edit_link, delete_link] : [edit_link]
end

def edit_link
link_to(helpers.op_icon("icon-edit"),
url_helpers.edit_llm_model_path(llm_model),
data: { test_selector: "llm-model--edit-#{llm_model.id}" },
title: I18n.t(:button_edit))
end

# Opens a DangerDialog rather than a browser confirm, so the message can say
# which features are bound to the model.
def delete_link
link_to(helpers.op_icon("icon-delete"),
url_helpers.delete_dialog_llm_model_path(llm_model),
data: { controller: "async-dialog", test_selector: "llm-model--delete-#{llm_model.id}" },
title: I18n.t(:button_delete))
end
end
end
3 changes: 3 additions & 0 deletions app/components/llm_connections/models_table_component.rb
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ def initial_sort = %i[identifier asc]

def has_footer? = false

# Without this the row's button_links are never rendered.
def has_actions? = true

def mobile_title = I18n.t("admin.llm_connections.show.models_heading")

# The row class is otherwise derived by convention as LlmConnections::RowComponent.
Expand Down
183 changes: 183 additions & 0 deletions app/controllers/admin/llm_models_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,183 @@
# frozen_string_literal: true

#-- copyright
# OpenProject is an open source project management software.
# Copyright (C) the OpenProject GmbH
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License version 3.
#
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
# Copyright (C) 2006-2013 Jean-Philippe Lang
# Copyright (C) 2010-2013 the ChiliProject Team
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License
# as published by the Free Software Foundation; either version 2
# of the License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See COPYRIGHT and LICENSE files for more details.
#++

module Admin
# Models an administrator enters by hand.
#
# Necessary because not every OpenAI-compatible server exposes a model list.
# A gateway may route /v1/chat/completions and nothing else, in which case the
# operator knows the model name and OpenProject cannot discover it.
class LlmModelsController < ApplicationController
include OpTurbo::ComponentStream

layout "admin"
menu_item :llm_connection

before_action :require_feature
before_action :require_admin
before_action :set_connection

def new
@llm_model = @connection.models.new
end

def edit

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.

Please rescue & add a spec for an invalid PATCH request.

@llm_model = @connection.models.find(params.expect(:id))
@verdicts = @connection.capability_verdicts.for_model(@llm_model.external_id).index_by(&:capability)
end

def create
llm_model = @connection.models.new(llm_model_params.except(*capability_param_names).merge(manual: true))

if save_with_capabilities(llm_model)
flash[:notice] = t(".success", model: llm_model.external_id)
redirect_to llm_connection_path, status: :see_other
else
# Re-rendered rather than redirected so the Primer form shows the error
# inline against the field that caused it.
@llm_model = llm_model
render :new, status: :unprocessable_entity
end
end

def update
@llm_model = @connection.models.find(params.expect(:id))

if update_with_capabilities(@llm_model)
flash[:notice] = t(".success", model: @llm_model.external_id)
redirect_to llm_connection_path, status: :see_other
else
# Re-rendered rather than redirected so the Primer form shows the error
# inline against the field that caused it, e.g. a rename that collides
# with an existing model id.
@verdicts = @connection.capability_verdicts.for_model(@llm_model.external_id_was).index_by(&:capability)
render :edit, status: :unprocessable_entity
end
end

def delete_dialog
llm_model = @connection.models.manual.find(params.expect(:id))

respond_with_dialog LlmConnections::DeleteModelDialogComponent.new(llm_model)
end

def destroy
llm_model = @connection.models.manual.find(params.expect(:id))
destroy_with_verdicts(llm_model)

flash[:notice] = t(".success", model: llm_model.external_id)
redirect_to llm_connection_path, status: :see_other
end

private

def set_connection
@connection = LlmConnection.instance
end

# Verdicts are keyed by the identifier string, not by foreign key, so they
# would silently apply to a future model re-added under the same name.
def destroy_with_verdicts(llm_model)

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.

🤖 #Logic 🚨 Deleting a model leaves the connection defaults pointing at it

cascade_rename! goes to real trouble to keep references intact on a rename: it moves the verdicts and rewrites default_chat_model_id / default_embedding_model_id. This path deletes the verdicts but leaves the defaults untouched.

Confirmed on this branch: set default_chat_model_id to a manual model, delete it, and the column still holds the deleted identifier.

The dialog does warn through affected_defaults, so this may be a deliberate "the administrator was told". If so, could we say that in a comment? As it stands one method treats the defaults as references to maintain and the other does not, and it is not obvious which behaviour is intended.

ActiveRecord::Base.transaction do
llm_model.destroy!
@connection.capability_verdicts.for_model(llm_model.external_id).delete_all
end
end

# The flag gates the endpoints, not only the menu entry: an unfinished page
# must not accept writes just because somebody knows the URL.
def require_feature
render_404 unless OpenProject::FeatureDecisions.llm_connection_active?
end

def update_with_capabilities(llm_model)
attributes = llm_model_params.except(*capability_param_names)
# A discovered model is named by the server; only a hand-entered one may be
# renamed here, and everything referencing the old name follows it.
attributes = attributes.except(:external_id) unless llm_model.manual?

saved = false
ActiveRecord::Base.transaction do
previous_external_id = llm_model.external_id
llm_model.assign_attributes(attributes)
raise ActiveRecord::Rollback unless llm_model.save

llm_model.cascade_rename!(previous_external_id)
apply_capabilities(llm_model)
saved = true
end
saved
end

# external_id is accepted on create, and on update for manually added models.
def llm_model_params
params.expect(llm_model: [:external_id, :display_name, :admin_context_window, *capability_param_names])
end

def capability_param_names
Llm::Capabilities::ALL.map { |capability| :"capability_#{capability}" }
end

def save_with_capabilities(llm_model)
ActiveRecord::Base.transaction do
llm_model.save!
apply_capabilities(llm_model)
end

true
rescue ActiveRecord::RecordInvalid
false
end

# Stored as admin-sourced verdicts, which survive re-detection: an
# administrator knows things about their deployment that neither a published
# registry nor a probe can determine.
def apply_capabilities(llm_model)
submitted = llm_model_params

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.

🤖 #Design ℹ️ apply_capabilities re-parses the request parameters

Both callers have already built this hash before calling in. Could apply_capabilities take it as an argument instead of running params.expect a second time per request?


Llm::Capabilities::ALL.each do |capability|
assert(llm_model.external_id, capability, submitted[:"capability_#{capability}"].presence)
end
end

def assert(model_id, capability, state)
verdict = @connection.capability_verdicts
.find_or_initialize_by(model_id:, capability: capability.to_s)

if state.blank?
# "Not specified" clears an assertion rather than recording ignorance as
# fact; detection may fill it in later.
verdict.destroy! if verdict.persisted? && verdict.source_admin?
else
verdict.update!(state:, source: "admin", checked_at: Time.current)
end
end
end
end
Loading
Loading