Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FEATURE: mentionable personas and random picker tool, context limits #466

Merged
merged 19 commits into from Feb 15, 2024

Conversation

SamSaffron
Copy link
Member

@SamSaffron SamSaffron commented Feb 9, 2024

This rather large PR adds support for the following:

image

image

  1. Personas are now optionally mentionable, meaning that you can mention them either from public topics or PMs
    1. Mentioning from PMs helps "switch" persona mid conversation, meaning if you want to look up sites setting you can invoke the site setting bot, or if you want to generate an image you can invoke dall e
    2. Mentioning outside of PMs allows you to inject a bot reply in a topic trivially
  2. We also add the support for max_context_posts this allow you to limit the amount of context you feed in, which can help control costs

Random picker lets llms pick

1. Random numbers from min to max
2. An element in a string list


The tool allows the LLM to make multiple choices at once (like roll a bunch of dice)
@SamSaffron SamSaffron changed the title chat play FEATURE: mentionable personas and random picker tool (WIP) Feb 9, 2024
Ensure PMs use the LLM defined in the OP
(streaming is somewhat expensive, lets keep it on PMs for now)
the amount of context the llm gets for completions

Very important for mentionables cause we often do not want to feed
in the entire backlog
@SamSaffron SamSaffron changed the title FEATURE: mentionable personas and random picker tool (WIP) FEATURE: mentionable personas and random picker tool, context limits Feb 13, 2024
@SamSaffron SamSaffron marked this pull request as ready for review February 13, 2024 06:34
Copy link
Contributor

@jjaffeux jjaffeux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't see anything block-worthy 👍 Might be good to wait for @martin-brennan if he has time.

Copy link
Contributor

@martin-brennan martin-brennan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a UI perspective, these list items with "(not configured)" are a bit confusing. What happens if I choose one? Is there some validation to prevent me from doing it? Would be nice if select kit supports it (not sure if it does) to disable these items in the list at the least:

image

Also, when I toggled on Mentionable then didn't choose the "Default Language Model", I got a "Persona saved!" toast, but the mentionable toggle silently went back to false. IMO we should show a validation error if the user toggles on Mentionable but doesn't choose a "Default Language Model".

Also, even if I choose a language model, saving the page always toggles Mentionable back to false, so there is a bug here somewhere.

The actual mentioning and switching worked nicely :) The "let's welcome them to the community" message will not display with bot users, so if we change to using a bot that would be good. Also the list of avatars in the top left of the PM doesn't change until the page refreshes, not a huge deal:

image

app/models/ai_persona.rb Outdated Show resolved Hide resolved
@@ -108,6 +108,16 @@ en:
ai_persona:
name: Name
description: Description
no_llm_selected: "No language model selected"
max_context_posts: "Max Context Posts"
max_context_posts_help: "The maximum number of posts to use as context for the AI when responding to a user. (empty for default)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be helpful to mention the default is 40, the only problem then is that you have to keep the translation and the server const in sync :D

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might be able to pass it down via the Site serializer and then use that to pass to the translation/the placeholder here:

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm tricky... yeah maybe this should be a site setting? I can see people who want to save money set it lower ... bot probably operates fine at 10 context and you would save a big pile of money, especially considering giant convos @tgxworld tends to have :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah site setting could be fine as well, at least as the default, because then you can use that on the client too for the input and translation.

app/models/ai_persona.rb Show resolved Hide resolved
assets/javascripts/discourse/admin/models/ai-persona.js Outdated Show resolved Hide resolved
end

post_types = [Post.types[:regular]]
post_types << Post.types[:whisper] if post.post_type == Post.types[:whisper]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
post_types << Post.types[:whisper] if post.post_type == Post.types[:whisper]
post_types << Post.types[:whisper] if post.whisper?

lib/ai_bot/playground.rb Outdated Show resolved Hide resolved
lib/ai_bot/tools/random_picker.rb Outdated Show resolved Hide resolved
spec/requests/admin/ai_personas_controller_spec.rb Outdated Show resolved Hide resolved
SamSaffron and others added 4 commits February 15, 2024 15:04
Co-authored-by: Martin Brennan <martin@discourse.org>
Co-authored-by: Martin Brennan <martin@discourse.org>
Co-authored-by: Martin Brennan <martin@discourse.org>
- fix, always use "bot" ids for auto created persona users
- dev improve routes so they are ai-personas and not ai_personas
@SamSaffron
Copy link
Member Author

Also, when I toggled on Mentionable then didn't choose the "Default Language Model", I got a "Persona saved!" toast, but the mentionable toggle silently went back to false. IMO we should show a validation error if the user toggles on Mentionable but doesn't choose a "Default Language Model".

was a bug... all sorted

Co-authored-by: Martin Brennan <martin@discourse.org>
# find the first id smaller than FIRST_USER_ID that is not taken
id = nil

id = DB.query_single(<<~SQL, FIRST_PERSONA_USER_ID, FIRST_PERSONA_USER_ID - 200).first
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we don't worry about this for now...but what if > 200 persona users are made 🤔 Might be good to make a site-wide persona count limit to deal with this. Anyway, not holding up this current PR 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh there is a fallback afterwards just in case... we take minimum ... very unlikely to hit it, but it is handled.


id = DB.query_single(<<~SQL, FIRST_PERSONA_USER_ID, FIRST_PERSONA_USER_ID - 200).first
WITH seq AS (
SELECT generate_series(?, ?, -1) AS id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nice, TIL

@SamSaffron SamSaffron merged commit 3a8d95f into main Feb 15, 2024
5 checks passed
@SamSaffron SamSaffron deleted the chat-play branch February 15, 2024 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants