Initial submission #1

srtaalej · 2024-08-01T04:11:49Z

Type of change (place an x in the [ ] that applies)

New sample
New feature
Bug fix
Documentation

Summary

This PR introduces a new sample app that uses OpenAI and Anthropic APIs to generate responses to user prompts. The app showcases various interaction methods including DMs, app mentions, and a custom /ask command, while also supporting Bring Your Own Bot (BYOB) functionality and custom function implementation.

Requirements (place an x in each [ ] that applies)

I’ve checked my submission against the Samples Checklist to ensure it complies with all standards
I have ensured the changes I am contributing align with existing patterns and have tested and linted my code
I've read and agree to the Code of Conduct

WilliamBergamin

Great work 💯

left a few comments

README.md

manifest.json

requirements.txt

slack.json

state_store/create_file.py

hello-ashleyintech

looking fabulous and thank you for submitting! ❤️ I have a few requested changes I think would help improve the documentation aspect of the app.

to avoid a huge wave of comments all at once, i'm going to split reviewing into different parts, starting with the README - leaving the first round of feedback for the README. in the next pass, I will re-review the README and then review the ai directory, and so on (each pass I'll note what specific sections I'm reviewing for clarity)

README.md

.gitignore

README.md

hello-ashleyintech

🙌 phenomenal! This iteration I reviewed the README again as well as all files in ai folder, manifest.json, requirements.txt, and slack.json - I will cover /listeners next and also start testing the functionality of the app more in the next iteration! 🙌

hello-ashleyintech · 2024-08-02T15:51:29Z

ai/ai_utils/get_available_apis.py

+from ai.providers.openai import OpenAI_API
+
+"""
+New providers need to be added below


Suggested change

New providers need to be added below

New AI providers must be added below

Maybe also add a note of what the file does in this comment! Ex: This file _________. New AI providers must be added below.

ai/ai_utils/handle_response.py

ai/providers/anthropic.py

ai/providers/base_provider.py

requirements.txt

manifest.json

seratch

Great work already! Left a few comments but you don't need to resolve all of them at all! Hope some of these help you polish the details!

seratch · 2024-08-06T00:46:52Z

ai/ai_utils/handle_response.py

+        return AnthropicAPI()
+
+
+def get_ai_response(user_id: str, prompt: str, context: list = [], system_content=DEFAULT_SYSTEM_CONTENT):


Passing a mutable object as the default argument value is a common pitfall in python; you can change the argument type to Optional[List[X]] and set None as the default value instead: https://docs.quantifiedcode.com/python-anti-patterns/correctness/mutable_default_value_as_argument.html

seratch · 2024-08-06T00:48:58Z

ai/providers/anthropic.py

+            raise ValueError("Invalid model")
+        self.current_model = model_name
+
+    def get_models(self):


nice-to-have but having return more types can make the code maintenance easier

Suggested change

def get_models(self):

def get_models(self) -> dict:

seratch · 2024-08-06T00:50:21Z

ai/providers/base_provider.py

+        self.api_key: str
+
+    def set_model(self, model_name: str):
+        self.current_model = model_name


If you raise an exception here, this line of code won't be used

Suggested change

self.current_model = model_name

seratch · 2024-08-06T00:51:54Z

ai/providers/openai.py

+
+    def generate_response(self, prompt: str, system_content: str) -> str:
+        try:
+            client = openai.OpenAI(api_key=self.api_key)


you can set up this instance within the constructor and have the instance as an instance field (self.client) to eliminate a very slight overhead here

seratch · 2024-08-06T00:53:49Z

ai/providers/openai.py

+            )
+            return response.choices[0].message.content
+        except openai.APIConnectionError as e:
+            Logger.error(f"Server could not be reached: {e.__cause__}")


Printing error logs is a good idea! If this app wants to tell the end-user what's happening, this code may want to return the text to display in Slack UI rather than returning nothing.

seratch · 2024-08-06T01:10:08Z

manifest.json

+        "channel_id":{
+            "type":"slack#/types/channel_id",
+            "title":"Channel",
+            "description":"Channel that user joined",


It seems this app's bot user has to be a member of the channel, right? If yes, this app may want to have the permission to use conversations.join API. If it's not suitable for this app, the description should mention the workflow user has to manually invite the bot.

seratch · 2024-08-06T01:12:16Z

state_store/state_store.py

+from .user_identity import UserIdentity
+
+
+class StateStore:


nit: UserStateStore may sound even clearer to me.

seratch · 2024-08-06T01:13:26Z

ai/providers/base_provider.py

+# A base class for API providers, defining the interface and common properties for subclasses.
+
+
+class BaseProvider:


For consistency, having "API" in the name (e.g., BaseAPIProvider) may be clearer.

seratch · 2024-08-06T01:15:21Z

ai/ai_utils/handle_response.py

+"""
+
+
+def _get_provider(api_name: str):


nit: these strings are actually names of the service providers, not the API names (I mean "Chat Completions" is an API name). renaming the argument to provider_name: str and the method to _resolve_api_provider() may be an alternative way to name them.

seratch · 2024-08-06T01:16:00Z

state_store/user_identity.py

+
+class UserIdentity(TypedDict):
+    user_id: str
+    api: str


if this can be either "openai" or "anthoropic", it sounds like "provider"

hello-ashleyintech

when trying to run the slash command or dm bolty, I keep getting the following error: missing 1 required positional argument: 'msg' 😢 do you know what could be causing this?

README.md

hello-ashleyintech · 2024-08-06T14:09:49Z

README.md

@@ -19,18 +29,20 @@ Before you can run the app, you'll need to store some environment variables.
 2. Click ***Basic Information** from the left hand menu and follow the steps in the App-Level Tokens section to create an app-level token with the `connections:write` scope. Copy this token. You will store this in your environment as `SLACK_APP_TOKEN`.

 ```zsh
-# Replace with your app token and bot token
+# Replace with your app token, bot token, and the token for whichever API(s) you plan on using


specify how to configure these env vars - by running in terminal

hello-ashleyintech · 2024-08-06T15:01:43Z

ai/__init__.py

this file is empty, do we need it?

hello-ashleyintech

mentioning the bot results in the following error: ERROR:app.py:app_mentioned_callback:parse_conversation() takes 1 positional argument but 2 were given

hello-ashleyintech · 2024-08-06T15:10:05Z

listeners/commands/ask_command.py

+from ai.ai_utils.handle_response import get_ai_response
+
+
+def ask_callback(ack: Ack, command, say: Say, logger: Logger, context: BoltContext):


let's add a comment for this callback detailing what feature it's connected to and what it does!

hello-ashleyintech · 2024-08-06T15:10:13Z

listeners/events/app_home_opened.py



-def app_home_opened_callback(client, event, logger: Logger):
-    # ignore the app_home_opened event for anything but the Home tab
+def app_home_opened_callback(event, logger: Logger, client: WebClient, context: BoltContext):


let's add a comment for this callback detailing what feature it's connected to and what it does!

srtaalej · 2024-08-07T01:00:28Z

@hello-ashleyintech

when trying to run the slash command or dm bolty, I keep getting the following error: missing 1 required positional argument: 'msg' 😢 do you know what could be causing this?

Did it create a user file for you? It usually means it cant find your provider selection.

srtaalej · 2024-08-07T01:03:34Z

mentioning the bot results in the following error: ERROR:app.py:app_mentioned_callback:parse_conversation() takes 1 positional argument but 2 were given

i removed bot_id from the parameters for parse_conversation(). my bad, i missed that!

hello-ashleyintech · 2024-08-07T15:02:08Z

Did it create a user file for you? It usually means it cant find your provider selection.

It did! 😲 maybe it's having trouble reading it?

hello-ashleyintech · 2024-08-07T21:44:41Z

unblocked on my end - errors were being thrown because i didn't have credits on my account 😢 got some working API keys for each service and it's working now, will test more tomorrow and leave an updated review ❤️

zimeg

Big fan of the magic with Bolty ⚡ 🤖

The code is looking great and the feature set is so solid. Approving it now but I think some of the comments are worth addressing before merging- also feel free to ping me for more review! 🔍

Most of the comments I left are around file structure and naming which isn't too important but I think a few changes might make it easier to start changing things as an exploring developer. This will be quite a nice sample to explore 😄 🚀

zimeg · 2024-08-08T01:13:57Z

manifest.json

Nit: Can this file be linted for indentation- https://jsonlint.com? 🙏

Edit- I'm a fan of 2 or 4 spaces but can never decide myself lol. I would just avoid 3 or 5 or 1 or even 11 with all importance.

its black's formatting. should the readme instructions be black *.py instead of black .?

zimeg · 2024-08-08T01:16:46Z

slack.json

This might also have a extra space indentation 👾

#1 (comment)

zimeg · 2024-08-08T01:18:55Z

README.md

+* `handle_response.py`: Processes responses from API providers.
+<a name="byo-llm"></a>
+#### `ai/providers`
+This module contains classes for communicating with different API providers, such as [Anthropic](https://www.anthropic.com/) and [OpenAI](https://openai.com/). To add your own LLM, create a new class for it using the `base_provider.py` as an example, then update `get_available_apis.py` and `handle_response.py` to include and utilize your new class for API communication.



A section on state_store would also be nice to have here 📚 👀

zimeg · 2024-08-08T01:32:18Z

state_store/state_store.py

Can the get_user_state and set_user_state methods be moved into this class?

set_user_state creates a FileStateStore object which inherits from UserStateStore though

zimeg · 2024-08-08T01:36:29Z

listeners/events/app_home_opened.py

+        # add an empty option if the user has no previously selected model.
+        options.append(
+            {
+                "text": {"type": "plain_text", "text": " ", "emoji": True},


Could we change this the text to "None selected" or something similar? Blank might be confusing at first glance 👁️‍🗨️

how about Select a provider

zimeg · 2024-08-08T01:56:11Z

ai/ai_utils/handle_response.py

+def _get_provider(provider_name: str):
+    if provider_name.lower() == "openai":
+        return OpenAI_API()
+    if provider_name.lower() == "anthropic":
+        return AnthropicAPI()


Also not sure if this is possible, but it'd be neat if this could be from __new__ in an ai/providers/__init__.py file

zimeg · 2024-08-08T01:58:39Z

.gitignore

This could be replaced with the following in a data/.gitignore instead 👀

* !.gitignore

zimeg · 2024-08-08T01:59:04Z

.gitignore

+data/*
+!data/.gitkeep
+.slack/


We'll just want to recommend ignoring

.slack/apps.dev.json

zimeg · 2024-08-08T02:03:11Z

listeners/listener_utils/listener_constants.py

I also think these utils can be moved to the top level listeners for easier editing. IMO these could even be one conversation.py file but this can also be ignored altogether!

zimeg · 2024-08-08T02:03:57Z

manifest.json

+    "oauth_config": {
+        "scopes": {
+            "bot": [
+              "commands",


Nit but it's nice to alphabetize these lists for fast scanning

…e_store

seratch · 2024-08-08T05:38:58Z

listeners/listener_utils/parse_conversation.py

+# and formats it as a string with user IDs and their messages.
+# Used in `app_mentioned_callback`, `dm_sent_callback`,
+# and `handle_summary_function_callback`.
+def parse_conversation(conversation: str) -> Optional[List[dict]]:


conversation's type hint looks incorrect

FYI: Running https://github.com/google/pytype may help you detect this type of error

…_mentioned

README.md

hello-ashleyintech · 2024-08-08T20:57:03Z

README.md


 # Change into this project directory
-cd bolt-python-starter-template
+cd bolt-python-ai-chatbot

 # Setup your python virtual environment
 python3 -m venv .venv


maybe we can add a note here after the setup instructions to also make sure you select a provider from the app home before you try mentioning the bot, using slash commands, or dming them?

with the new error handling the bot tells them directly when they interact with it

hello-ashleyintech

LGTM, excellent work! This is such a great addition to our collection of samples! 🙌

fixed

srtaalej added 3 commits July 31, 2024 15:29

ale-initial

4c70a58

models-dict-refactor

c5260ab

black

3c14460

srtaalej requested a review from hello-ashleyintech August 1, 2024 04:13

hello-ashleyintech requested review from misscoded, zimeg, WilliamBergamin and mwbrooks August 1, 2024 13:51

WilliamBergamin previously requested changes Aug 1, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

manifest.json Outdated Show resolved Hide resolved

requirements.txt Outdated Show resolved Hide resolved

slack.json Outdated Show resolved Hide resolved

state_store/create_file.py Outdated Show resolved Hide resolved

hello-ashleyintech requested changes Aug 1, 2024

View reviewed changes

srtaalej added 2 commits August 1, 2024 11:03

clean-up

e002c20

state-store bullet

f33c02e

hello-ashleyintech requested review from hello-ashleyintech and WilliamBergamin August 1, 2024 18:57

srtaalej added 2 commits August 1, 2024 15:42

slack.json

3c1a7a4

del api name

d74e37d

hello-ashleyintech requested changes Aug 2, 2024

View reviewed changes

feedback 2

3f8b047

srtaalej requested a review from hello-ashleyintech August 2, 2024 17:07

srtaalej added 6 commits August 5, 2024 11:26

conversation context

02ca237

typo readme

10d1bf2

data .gitignore

25acdcb

data/.gitkeep

aa27d7f

event payload missing view

5ff1313

axe view_id

396e6a6

seratch reviewed Aug 6, 2024

View reviewed changes

hello-ashleyintech requested changes Aug 6, 2024

View reviewed changes

hello-ashleyintech reviewed Aug 6, 2024

View reviewed changes

srtaalej added 3 commits August 6, 2024 19:37

chore: implement feedback across various files

c0f1018

fix: - /ask + /ask-bolty

d29e7ed

fix: update chat with error message

18cdcaf

hello-ashleyintech self-requested a review August 7, 2024 21:44

zimeg approved these changes Aug 8, 2024

View reviewed changes

srtaalej added 3 commits August 7, 2024 21:09

chore: implementing feedback (.gitignore, readme, manifest.json, stat…

0f6045f

…e_store

refactor: _get_providers now accessed from ai/providers/__init__,py

130c46e

chore: implementing feedback on readme, ai, and listeners

af85791

seratch reviewed Aug 8, 2024

View reviewed changes

srtaalej added 3 commits August 8, 2024 11:29

chore; implementing feedback on parse_conversations, dm_sent, and app…

a001315

…_mentioned

black

649cf7d

refactor: ai_utils functions moved into providers/__init__.py

8adc78c

hello-ashleyintech reviewed Aug 8, 2024

View reviewed changes

README.md Show resolved Hide resolved

fix: context cant be None in get_provider_response

0e4fbc0

hello-ashleyintech reviewed Aug 8, 2024

View reviewed changes

srtaalej added 3 commits August 8, 2024 14:01

fix: verbose error handling

e018b0f

feat: /ask-bolty posts ephemeral message with prompt quote

3357cdc

fix: initial_option in app_home error

93e12eb

hello-ashleyintech self-requested a review August 9, 2024 17:08

srtaalej added 2 commits August 9, 2024 11:38

fix: openai self.client moved to generate_response

cc5fa62

fix: black

93b6079

hello-ashleyintech approved these changes Aug 9, 2024

View reviewed changes

srtaalej merged commit 4488e57 into main Aug 9, 2024
3 checks passed

hello-ashleyintech deleted the initial-submission branch August 9, 2024 18:53

	New providers need to be added below
	New AI providers must be added below

		return AnthropicAPI()


		def get_ai_response(user_id: str, prompt: str, context: list = [], system_content=DEFAULT_SYSTEM_CONTENT):

		# A base class for API providers, defining the interface and common properties for subclasses.


		class BaseProvider:

		from ai.ai_utils.handle_response import get_ai_response


		def ask_callback(ack: Ack, command, say: Say, logger: Logger, context: BoltContext):

Initial submission #1

Initial submission #1

Conversation

srtaalej commented Aug 1, 2024

Type of change (place an x in the [ ] that applies)

Summary

Requirements (place an x in each [ ] that applies)

WilliamBergamin left a comment

Choose a reason for hiding this comment

hello-ashleyintech left a comment

Choose a reason for hiding this comment

hello-ashleyintech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seratch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hello-ashleyintech left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hello-ashleyintech left a comment • edited by srtaalej Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srtaalej commented Aug 7, 2024 • edited Loading

srtaalej commented Aug 7, 2024

hello-ashleyintech commented Aug 7, 2024

hello-ashleyintech commented Aug 7, 2024

zimeg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srtaalej Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srtaalej Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hello-ashleyintech left a comment

Choose a reason for hiding this comment

hello-ashleyintech left a comment •

edited

Loading

hello-ashleyintech left a comment •

edited by srtaalej

Loading

srtaalej commented Aug 7, 2024 •

edited

Loading

srtaalej Aug 8, 2024 •

edited

Loading

srtaalej Aug 8, 2024 •

edited

Loading