-
Notifications
You must be signed in to change notification settings - Fork 9
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
Stampy chat module #325
Conversation
f2f66c5
to
bec8544
Compare
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")})') |
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.
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]) |
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.
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 |
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.
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?
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 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 |
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.
these are Discord messages, counted per channel
yield chunk | ||
|
||
|
||
def filter_citations(text, citations): |
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.
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='.'): |
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.
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): |
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.
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
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.
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 |
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 suspect the STAMPY_CHAT_MIN_SCORE = 0.4
can actually be an even lower value (0.2?) but you'll want to experiment.
modules/stampy_chat.py
Outdated
DATA_HEADER = 'data: ' | ||
|
||
STAMPY_CHAT_ENDPOINT = "https://chat.stampy.ai:8443/chat" | ||
NLP_SEARCH_ENDPOINT = "https://stampy-nlp-t6p37v2uia-uw.a.run.app/" |
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.
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.
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.
good catch - updated
If the new chat and ChatGPT are both enabled, what kind of message will get picked up by the new one? |
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 |
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'}) |
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.
@ccstan99 this 'status': 'all'
does the same as showLive=0
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. |
0532fb8
to
170e261
Compare
ping |
@mruwnik seen this? #325 (comment) |
it looks like something went wrong with that comment. Could you repost it, or plop it in discord? |
@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. |
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 |
ping |
@mruwnik Sorry. Added the note in README and made it required again |
I think it's ready for merge? |
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.
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) |
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 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) |
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'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?
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.
meh. Depends a lot on your use case. Either way, it'll be negligible here, with network issues taking most of the time
A bunch of small refactors + little bug fixes, and a new module that uses the https://chat.stampy.ai chat bot