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

Stampy chat module #325

Merged
merged 6 commits into from
Nov 12, 2023
Merged

Stampy chat module #325

merged 6 commits into from
Nov 12, 2023

Conversation

mruwnik
Copy link
Collaborator

@mruwnik mruwnik commented Oct 31, 2023

A bunch of small refactors + little bug fixes, and a new module that uses the https://chat.stampy.ai chat bot

query = message.content
nlp = top_nlp_search(query)
if nlp.get('score', 0) > STAMPY_ANSWER_MIN_SCORE and nlp.get('status') == 'Live on site':
return Response(confidence=5, text=f'Check out {nlp.get("url")} ({nlp.get("title")})')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this will also get picked up by the semanticsearch module, which has a lower threshold (0.5) and higher confidence (8)

if nlp.get('score', 0) > STAMPY_ANSWER_MIN_SCORE and nlp.get('status') == 'Live on site':
return Response(confidence=5, text=f'Check out {nlp.get("url")} ({nlp.get("title")})')
if nlp.get('score', 0) > STAMPY_CHAT_MIN_SCORE:
return Response(confidence=6, callback=self.query, args=[query, history, message])
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

is this the right confidence?

NLP_SEARCH_ENDPOINT = "https://stampy-nlp-t6p37v2uia-uw.a.run.app/"

STAMPY_ANSWER_MIN_SCORE = 0.75
STAMPY_CHAT_MIN_SCORE = 0.4
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

NLP must return something with at least this score for the module to do anything. Should filter out most messages, but might need to be fiddled with a bit. Or maybe it would be worth there also being an explicit way of triggering this module with a specific phrase or something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suspect the STAMPY_CHAT_MIN_SCORE = 0.4 can actually be an even lower value (0.2?) but you'll want to experiment.

utils = Utilities.get_instance()


LOG_MAX_MESSAGES = 15 # don't store more than X messages back
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

these are Discord messages, counted per channel

yield chunk


def filter_citations(text, citations):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the chatbot returns a whole bunch of potential citations, but not all of them will be referenced in the text. This will remove the unused ones

return items[0]


def chunk_text(text: str, chunk_limit=2000, delimiter='.'):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Discord has a limit of 2000 characters per message (or at least other places in the code claim this), so this function will split the LLM's answer into smaller chunks, splitting them on full stops in order to make sure that sentences don't get chopped up. Though maybe newlines would be better, so it doesn't split on decimal points?

STAMPY_CHAT_MIN_SCORE = 0.4


def stream_lines(stream: Iterable):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The chatbot server returns messages as server-sent events, so these 2 functions basically transform a requests stream into a generator of js objects ready for using

@mruwnik mruwnik requested review from ProducerMatt, tayler6000 and Aprillion and removed request for ProducerMatt October 31, 2023 14:40
Copy link
Collaborator

@ccstan99 ccstan99 left a comment

Choose a reason for hiding this comment

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

The default for NLP search is to only return live on site questions. But if we're using it as a proxy to identify whether a question is alignment related, might want to add the showLive=0 which will use the larger set of unpublished questions. Just make sure to check it's published before giving the link to the user.

NLP_SEARCH_ENDPOINT = "https://stampy-nlp-t6p37v2uia-uw.a.run.app/"

STAMPY_ANSWER_MIN_SCORE = 0.75
STAMPY_CHAT_MIN_SCORE = 0.4
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suspect the STAMPY_CHAT_MIN_SCORE = 0.4 can actually be an even lower value (0.2?) but you'll want to experiment.

DATA_HEADER = 'data: '

STAMPY_CHAT_ENDPOINT = "https://chat.stampy.ai:8443/chat"
NLP_SEARCH_ENDPOINT = "https://stampy-nlp-t6p37v2uia-uw.a.run.app/"
Copy link
Collaborator

Choose a reason for hiding this comment

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

NLP_SEARCH_ENDPOINT = "https://stampy-nlp-t6p37v2uia-uw.a.run.app/"
Should we set it to "https://nlp.stampy.ai/" instead? If we add an additional service in the Europe region and add a load balancer later, it won't be tied to this specific service which is in us-west1 region.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch - updated

@ProducerMatt
Copy link
Member

ProducerMatt commented Oct 31, 2023

If the new chat and ChatGPT are both enabled, what kind of message will get picked up by the new one?

@ProducerMatt
Copy link
Member

I'm trying to submit changes but for some reason my local copy isn't matching what's on GitHub 🤔 I'm probably doing something wrong

@ProducerMatt
Copy link
Member

ProducerMatt commented Oct 31, 2023

Btw you've done a very good job compressing my code, thank you :)



def top_nlp_search(query: str) -> Dict[str, Any]:
resp = requests.get(NLP_SEARCH_ENDPOINT + '/api/search', params={'query': query, 'status': 'all'})
Copy link
Collaborator Author

@mruwnik mruwnik Nov 1, 2023

Choose a reason for hiding this comment

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

@ccstan99 this 'status': 'all' does the same as showLive=0

@mruwnik
Copy link
Collaborator Author

mruwnik commented Nov 1, 2023

if both are enabled. whichever one claims to have a higher confidece will be chosen. If they both have the same confidence, then I believe it's undefined which will be chosen. In practice this means that the new chat will win, as it has higher confidence.
The way this works is that all enabled modules get to answer all messages - they return a Response(text, confidence) instance, where text is either something to display or a callback. Then all the messages are sorted by confidence, and whichever has the highest confidence is picked. If it has a text message, then it returns that then and there, but if it's a callback it will execute it and add the result to the list of messages. This happens in a loop. So generally the callback will return something with higher confidence if it's a valid answer, as that will then certainly be returned (if the previously picked message had confidence n and the new message n+m, then the new message is guaranteed to have the higest confidecce). And if the callback couldn't come up with anything decent, it can return a response with a very low confidence - this will result in a different module having a chance to reply

@Aprillion Aprillion removed their request for review November 1, 2023 09:15
@mruwnik mruwnik requested review from Aprillion and removed request for Aprillion November 1, 2023 11:02
@mruwnik
Copy link
Collaborator Author

mruwnik commented Nov 4, 2023

ping

@ProducerMatt
Copy link
Member

@mruwnik seen this? #325 (comment)

@mruwnik
Copy link
Collaborator Author

mruwnik commented Nov 4, 2023

it looks like something went wrong with that comment. Could you repost it, or plop it in discord?

@ProducerMatt
Copy link
Member

@mruwnik weird. Config.py line 229, you added a default None for the private channel, and i was arguing it should be a required parameter, else the only choice is to silently suppress error logging.

@mruwnik
Copy link
Collaborator Author

mruwnik commented Nov 4, 2023

ah, that. Could you update the README to explain how to set it? I set the default, as otherwise I couldn't get it to even start, and didn't know what to put there

@mruwnik
Copy link
Collaborator Author

mruwnik commented Nov 9, 2023

ping

@ProducerMatt
Copy link
Member

@mruwnik Sorry. Added the note in README and made it required again

@ProducerMatt
Copy link
Member

I think it's ready for merge?

Copy link
Member

@ProducerMatt ProducerMatt left a comment

Choose a reason for hiding this comment

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

Seems ok from my limited view

config.py Outdated
@@ -222,7 +226,7 @@ def getenv_unique_set(var_name: str, default: T = frozenset()) -> Union[frozense
bot_dev_roles = getenv_unique_set("BOT_DEV_ROLES", frozenset())
bot_dev_ids = getenv_unique_set("BOT_DEV_IDS", frozenset())
bot_control_channel_ids = getenv_unique_set("BOT_CONTROL_CHANNEL_IDS", frozenset())
bot_private_channel_id = getenv("BOT_PRIVATE_CHANNEL_ID")
bot_private_channel_id = getenv("BOT_PRIVATE_CHANNEL_ID", None)
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be required (no default). Else the bot can't communicate in private with admins.

@@ -44,12 +44,11 @@ def get_stampy_modules() -> dict[str, Module]:
loaded_module_filenames = set()

# filenames of modules that were skipped because not enabled
skipped_module_filenames = set(ALL_STAMPY_MODULES - enabled_modules)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a Python expert so correct me if I'm wrong. I think you should use FrozenSet anywhere a set won't be modified, because it'll be faster?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

meh. Depends a lot on your use case. Either way, it'll be negligible here, with network issues taking most of the time

@mruwnik mruwnik merged commit 84cfd1c into master Nov 12, 2023
2 checks passed
@mruwnik mruwnik deleted the stampy_chat_module branch November 12, 2023 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants