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

don't return the full chat object on the prepare and answer endpoints of the REST API #242

Closed
peachkeel opened this issue Dec 8, 2023 · 3 comments · Fixed by #249
Closed

Comments

@peachkeel
Copy link
Contributor

peachkeel commented Dec 8, 2023

Feature description

I've noticed that the /chats/{id}/answer endpoint returns both the latest message and the full chat:

ragna/ragna/_api/core.py

Lines 210 to 227 in 1e256e9

@app.post("/chats/{id}/answer")
async def answer(
user: UserDependency, id: uuid.UUID, prompt: str
) -> schemas.MessageOutput:
with get_session() as session:
chat = database.get_chat(session, user=user, id=id)
chat.messages.append(
schemas.Message(content=prompt, role=ragna.core.MessageRole.USER)
)
core_chat = schema_to_core_chat(session, user=user, chat=chat)
answer = schemas.Message.from_core(await core_chat.answer(prompt))
chat.messages.append(answer)
database.update_chat(session, user=user, chat=chat)
return schemas.MessageOutput(message=answer, chat=chat)

I thought that maybe the UI needed all that state about the chat for some distributed system reason, but it looks like it does not:

answer = await self.api_wrapper.answer(self.current_chat["id"], contents)
self.current_chat["messages"].append(answer["message"])
yield {
"user": "Ragna",
"avatar": "🤖",
"value": answer["message"]["content"],
}

Assuming there is a need to maintain API backward compatibility, could we add a /chats/{id}/quick_answer endpoint to Ragna that only returns a schemas.Message object as opposed to a heavyweight schemas.MessageOutput object?

Value and/or benefit

Every call to /chats/{id}/answer appends another message to the chat. This means that each time the endpoint is called, the amount of data that needs to be sent over the wire increases. Over time, this O(m) growth makes getting answers more and more sluggish. Ideally, the /chats/{id}/answer endpoint would be O(1) from a messages in the chat perspective. The proposed /chats/{id}/quick_answer endpoint would act in that constant time/space manner.

Anything else?

No response

@peachkeel peachkeel added the type: enhancement 💅 New feature or request label Dec 8, 2023
@peachkeel peachkeel changed the title "Quick answer" API endpoint? "Quick answer" API endpoint Dec 8, 2023
@pmeier
Copy link
Member

pmeier commented Dec 9, 2023

Thanks @peachkeel for opening this. I agree and had the task of opening an issue about this in my backlog. I'm hitting the same in #185 (comment) in a worse way. TBH, this was a premature optimization. We should remove it.

The only upside for client in always returning the chat is that they don't have to track it. But this is only one API call away, so I guess this ok. Maybe later there is an actual use case, but we don't need to care right now.

@pmeier pmeier changed the title "Quick answer" API endpoint don't return the full chat object on the prepare and answer endpoints of the REST API Dec 9, 2023
@peachkeel
Copy link
Contributor Author

@pmeier,

You're welcome regarding opening the issue. And thank you for getting v0.1.2 out and being such an active maintainer!

One thing I want to bring up related to this issue is that the Source class has a content attribute:

class Source(pydantic.BaseModel):
"""Data class for sources stored inside a source storage.
Attributes:
id: Unique ID of the source.
document: Document this source belongs to.
location: Location of the source inside the document.
content: Content of the source.
num_tokens: Number of tokens of the content.
"""

The corresponding schemas.Source class does not have this attribute:

class Source(BaseModel):
# See orm.Source on why this is not a UUID
id: str
document: Document
location: str

Thus, schemas.Message objects coming from the /chats/{id}/answer endpoint are missing some relevant info about the contexts used to generate answers.

Maybe this problem should be a separate issue, but I wanted to bring it up here since we're talking about changing the API.

@pmeier
Copy link
Member

pmeier commented Dec 12, 2023

The corresponding schemas.Source class does not have this attribute:

This is intentional and warrants a separate issue. TL;DR: when "live answering" we have access to the source content and thus could just send it back to the client. However, this doesn't work when we are recreating the chat from our state database unless we also store the source content inside the database.

See #248.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants