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

basic react UI for debugging and testing bot. Added session_id to be … #107

Closed
wants to merge 1 commit into from

Conversation

iljamak
Copy link
Collaborator

@iljamak iljamak commented Feb 15, 2024

…able to collect history. SalesGPT API is now created at mounting time. Changed response from api at the end of conversations to make it a list of 2 values, rather than 1 (Was throwing an error)

…able to collect history. SalesGPT API is now created at mounting time. Changed response from api at the end of conversations to make it a list of 2 values, rather than 1 (Was throwing an error)
@filip-michalsky
Copy link
Owner

filip-michalsky commented Feb 16, 2024

@iljamak pls accept the invite to the salesGPT org as a collaborator, otherwise the unit tests will fail as it won't load the env variables for you. Then please re-run the checks to make sure they all pass. Actually, if you can move this to be a branch in our main repo itself as a core collaborator and submit a PR from it, that'd be great, ty! The fork + PR worflow is for new/outside collaborators.

Copy link
Owner

@filip-michalsky filip-michalsky left a comment

Choose a reason for hiding this comment

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

Great start! I think the core functionality is there, we now need to work on making it a bit more presentable and also add documentation to how to reproduce. I dropped a lot of comments around. Please let me know if some of it is unclear. Thanks!

@@ -1,2 +0,0 @@
OPENAI_API_KEY="xx"
Copy link
Owner

Choose a reason for hiding this comment

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

@iljamak pls keep this file in - its there as a placeholder to add env vars

allow_origins=["*"], # Allows all origins
allow_credentials=True,
allow_methods=["*"], # Allows all methods
allow_headers=["*"], # Allows all headers
Copy link
Owner

Choose a reason for hiding this comment

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

we will want to add this to the documentation. In general, we don't want to allow all origins. Please comment on this in the README for the API and put this as a parameter on the origins + methods. This will be then used as an environmental parameter when we dockerize this.

Copy link
Owner

Choose a reason for hiding this comment

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

in other words, ALLOW_ORIGINS=["*""] etc should all be environmental variables.

allow_credentials=True,
allow_methods=["*"], # Allows all methods
allow_headers=["*"], # Allows all headers
)
Copy link
Owner

Choose a reason for hiding this comment

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

please add a unit test and learn the

  1. Given
  2. When
  3. Then

unit test framework (work with an AI code assistant such as cursor.so on this)

@@ -19,28 +31,34 @@ async def say_hello():


class MessageList(BaseModel):
Copy link
Owner

Choose a reason for hiding this comment

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

while we are add it, the run_api.py should only contain the endpoints, it's a start, we will refactor later.

But the MessageList should not be in this file.

@@ -19,28 +31,34 @@ async def say_hello():


class MessageList(BaseModel):
session_id: str
conversation_history: List[str]
human_say: str
Copy link
Owner

Choose a reason for hiding this comment

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

let's change the turns to "human" and "{salesperson_name}" where the salesperson name is injected from the prompt template.

langchain.ipynb Show resolved Hide resolved
@@ -3,11 +3,23 @@

import uvicorn
from fastapi import FastAPI
Copy link
Owner

Choose a reason for hiding this comment

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

let's refactor the API a but for a "separation of concerns"

You can ask cursor to give you a better file structure.


@app.post("/chat")
async def chat_with_sales_agent(req: MessageList):
sales_api = SalesGPTAPI(
Copy link
Owner

Choose a reason for hiding this comment

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

Can we update this to use tools?

run_api.py Show resolved Hide resolved
run_api.py Show resolved Hide resolved
@filip-michalsky
Copy link
Owner

@iljamak closing this as migrated to the upstream repo

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.

2 participants