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

refactor: make non-streaming chat as /chat/request route #20

Merged
merged 12 commits into from
Mar 27, 2024

Conversation

thucpn
Copy link
Collaborator

@thucpn thucpn commented Mar 25, 2024

No description provided.

@thucpn thucpn marked this pull request as ready for review March 25, 2024 09:29

```
curl --location 'localhost:8000/api/chat/request' \
--header 'Content-Type: text/plain' \
Copy link
Collaborator

Choose a reason for hiding this comment

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

@thucpn we don't support application/json?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It should be application/json

questions.ts Outdated
@@ -370,7 +370,6 @@ export const askQuestions = async (
name: "template",
message: "Which template would you like to use?",
choices: [
{ title: "Chat without streaming", value: "simple" },
{ title: "Chat with streaming", value: "streaming" },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
{ title: "Chat with streaming", value: "streaming" },
{ title: "Chat", value: "streaming" },

e2e/basic.spec.ts Show resolved Hide resolved
result: _Message


async def preprocess_request(data: _ChatData) -> tuple:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
async def preprocess_request(data: _ChatData) -> tuple:
from typing import List, Tuple
# parses _ChatData to be used by LlamaIndex chat
async def parse_chat_data(data: _ChatData) -> Tuple[str, List[ChatMessage]]:

@@ -44,16 +42,36 @@ async def chat(
)
for m in data.messages
]
return last_message, messages
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here we're a bit in a pickle as last_message is a different message type than messages, to be consistent we could either return:

Suggested change
return last_message, messages
return last_message.content, messages

or

Suggested change
return last_message, messages
return ChatMessage(role=last_message.role,
content=last_message.content), messages

I think the first one is easier to understand

@thucpn thucpn force-pushed the refactor/remove-simple-chat branch from 61871fb to e5137d2 Compare March 27, 2024 03:03
Copy link
Collaborator

@marcusschiesser marcusschiesser left a comment

Choose a reason for hiding this comment

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

@thucpn thanks looks great

@marcusschiesser marcusschiesser merged commit b08cad0 into main Mar 27, 2024
5 checks passed
@marcusschiesser marcusschiesser deleted the refactor/remove-simple-chat branch March 27, 2024 05:59
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