-
Notifications
You must be signed in to change notification settings - Fork 40
FEATURE: new endpoint for directly accessing a persona #876
Changes from all commits
f683d69
e450111
621157c
c8b6a72
0ec222f
bd1420d
d9d1c90
8f7dbc3
0a2bd14
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,8 +74,205 @@ def destroy | |
| end | ||
| end | ||
|
|
||
| class << self | ||
| POOL_SIZE = 10 | ||
| def thread_pool | ||
| @thread_pool ||= | ||
| Concurrent::CachedThreadPool.new(min_threads: 0, max_threads: POOL_SIZE, idletime: 30) | ||
| end | ||
|
|
||
| def schedule_block(&block) | ||
| # think about a better way to handle cross thread connections | ||
| if Rails.env.test? | ||
| block.call | ||
| return | ||
| end | ||
|
|
||
| db = RailsMultisite::ConnectionManagement.current_db | ||
| thread_pool.post do | ||
| begin | ||
| RailsMultisite::ConnectionManagement.with_connection(db) { block.call } | ||
| rescue StandardError => e | ||
| Discourse.warn_exception(e, message: "Discourse AI: Unable to stream reply") | ||
| end | ||
| end | ||
| end | ||
| end | ||
|
|
||
| CRLF = "\r\n" | ||
|
|
||
| def stream_reply | ||
martin-brennan marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see Keegan mentioned a similar thing, but this is an extremely fat controller action, I really don't feel like we should be doing this very often, if ever. It doesn't feel very maintainable or easy to read, and could easily expand even further in size. Much of this is a perfect case for using a service.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reasons why it's perfect for a service:
Not saying the reply streaming stuff should be in one, but everything before it easily could be
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have been waiting a bit on service docs and service stability, stuff has been changing recently |
||
| persona = | ||
| AiPersona.find_by(name: params[:persona_name]) || | ||
| AiPersona.find_by(id: params[:persona_id]) | ||
| return render_json_error(I18n.t("discourse_ai.errors.persona_not_found")) if persona.nil? | ||
|
|
||
| return render_json_error(I18n.t("discourse_ai.errors.persona_disabled")) if !persona.enabled | ||
|
|
||
| if persona.default_llm.blank? | ||
| return render_json_error(I18n.t("discourse_ai.errors.no_default_llm")) | ||
| end | ||
|
|
||
| if params[:query].blank? | ||
| return render_json_error(I18n.t("discourse_ai.errors.no_query_specified")) | ||
| end | ||
|
|
||
| if !persona.user_id | ||
| return render_json_error(I18n.t("discourse_ai.errors.no_user_for_persona")) | ||
| end | ||
|
|
||
| if !params[:username] && !params[:user_unique_id] | ||
| return render_json_error(I18n.t("discourse_ai.errors.no_user_specified")) | ||
| end | ||
|
|
||
| user = nil | ||
|
|
||
| if params[:username] | ||
| user = User.find_by_username(params[:username]) | ||
| return render_json_error(I18n.t("discourse_ai.errors.user_not_found")) if user.nil? | ||
| elsif params[:user_unique_id] | ||
| user = stage_user | ||
| end | ||
|
|
||
| raise Discourse::NotFound if user.nil? | ||
|
|
||
| topic_id = params[:topic_id].to_i | ||
| topic = nil | ||
| post = nil | ||
|
|
||
| if topic_id > 0 | ||
| topic = Topic.find(topic_id) | ||
|
|
||
| raise Discourse::NotFound if topic.nil? | ||
|
|
||
| if topic.topic_allowed_users.where(user_id: user.id).empty? | ||
| return render_json_error(I18n.t("discourse_ai.errors.user_not_allowed")) | ||
| end | ||
|
|
||
| post = | ||
| PostCreator.create!( | ||
| user, | ||
| topic_id: topic_id, | ||
| raw: params[:query], | ||
| skip_validations: true, | ||
| ) | ||
| else | ||
| post = | ||
| PostCreator.create!( | ||
| user, | ||
| title: I18n.t("discourse_ai.ai_bot.default_pm_prefix"), | ||
| raw: params[:query], | ||
| archetype: Archetype.private_message, | ||
| target_usernames: "#{user.username},#{persona.user.username}", | ||
| skip_validations: true, | ||
| ) | ||
|
|
||
| topic = post.topic | ||
| end | ||
|
|
||
| hijack = request.env["rack.hijack"] | ||
| io = hijack.call | ||
|
|
||
| user = current_user | ||
|
|
||
| self.class.queue_streamed_reply(io, persona, user, topic, post) | ||
| end | ||
|
|
||
| private | ||
|
|
||
| AI_STREAM_CONVERSATION_UNIQUE_ID = "ai-stream-conversation-unique-id" | ||
|
|
||
| # keeping this in a static method so we don't capture ENV and other bits | ||
| # this allows us to release memory earlier | ||
| def self.queue_streamed_reply(io, persona, user, topic, post) | ||
| schedule_block do | ||
| begin | ||
| io.write "HTTP/1.1 200 OK" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess it's for a good reason (performance?), but is there any way not to do this writing of headers and data completely manually?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rails has streaming, but I don't want to stream from the foreground thread, I can make wrapper classes for this but at the end the same code will run |
||
| io.write CRLF | ||
| io.write "Content-Type: text/plain; charset=utf-8" | ||
| io.write CRLF | ||
| io.write "Transfer-Encoding: chunked" | ||
| io.write CRLF | ||
| io.write "Cache-Control: no-cache, no-store, must-revalidate" | ||
| io.write CRLF | ||
| io.write "Connection: close" | ||
| io.write CRLF | ||
| io.write "X-Accel-Buffering: no" | ||
| io.write CRLF | ||
| io.write "X-Content-Type-Options: nosniff" | ||
| io.write CRLF | ||
| io.write CRLF | ||
| io.flush | ||
|
|
||
| persona_class = | ||
| DiscourseAi::AiBot::Personas::Persona.find_by(id: persona.id, user: user) | ||
| bot = DiscourseAi::AiBot::Bot.as(persona.user, persona: persona_class.new) | ||
|
|
||
| data = | ||
| { topic_id: topic.id, bot_user_id: persona.user.id, persona_id: persona.id }.to_json + | ||
| "\n\n" | ||
|
|
||
| io.write data.bytesize.to_s(16) | ||
| io.write CRLF | ||
| io.write data | ||
| io.write CRLF | ||
|
|
||
| DiscourseAi::AiBot::Playground | ||
| .new(bot) | ||
| .reply_to(post) do |partial| | ||
| next if partial.length == 0 | ||
|
|
||
| data = { partial: partial }.to_json + "\n\n" | ||
|
|
||
| data.force_encoding("UTF-8") | ||
|
|
||
| io.write data.bytesize.to_s(16) | ||
| io.write CRLF | ||
| io.write data | ||
| io.write CRLF | ||
| io.flush | ||
| end | ||
|
|
||
| io.write "0" | ||
| io.write CRLF | ||
| io.write CRLF | ||
|
|
||
| io.flush | ||
| io.done | ||
| rescue StandardError => e | ||
| # make it a tiny bit easier to debug in dev, this is tricky | ||
| # multi-threaded code that exhibits various limitations in rails | ||
| p e if Rails.env.development? | ||
| Discourse.warn_exception(e, message: "Discourse AI: Unable to stream reply") | ||
| ensure | ||
| io.close | ||
| end | ||
| end | ||
| end | ||
|
|
||
| def stage_user | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another thing that could be in a service, or at least a reusable class in core. Nothing except the custom field is AI-specific here |
||
| unique_id = params[:user_unique_id].to_s | ||
| field = UserCustomField.find_by(name: AI_STREAM_CONVERSATION_UNIQUE_ID, value: unique_id) | ||
|
|
||
| if field | ||
| field.user | ||
| else | ||
| preferred_username = params[:preferred_username] | ||
| username = UserNameSuggester.suggest(preferred_username || unique_id) | ||
|
|
||
| user = | ||
| User.new( | ||
| username: username, | ||
| email: "#{SecureRandom.hex}@invalid.com", | ||
| staged: true, | ||
| active: false, | ||
| ) | ||
| user.custom_fields[AI_STREAM_CONVERSATION_UNIQUE_ID] = unique_id | ||
| user.save! | ||
| user | ||
| end | ||
| end | ||
|
|
||
| def find_ai_persona | ||
| @ai_persona = AiPersona.find(params[:id]) | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| # frozen_string_literal: true | ||
| class AddUniqueAiStreamConversationUserIdIndex < ActiveRecord::Migration[7.1] | ||
| def change | ||
| add_index :user_custom_fields, | ||
| [:value], | ||
| unique: true, | ||
| where: "name = 'ai-stream-conversation-unique-id'" | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like stuff that should be generic/in core/an
ActiveSupport::Concernthat we could use elsewhere too?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will move it out to a service at some point definitely do not want a concern here , this is a very sharp knife I only want people that absolutely understand repercussions to use it.